Re: [f2fs-dev] [PATCH v10 0/5] add support for direct I/O with fscrypt using blk-crypto
On Thu, Jan 20, 2022 at 06:36:03PM -0800, Darrick J. Wong wrote:
> Sure. How's this? I couldn't think of a real case of directio
> requiring different alignments for pos and bytecount, so the only real
> addition here is the alignment requirements for best performance.
While I see some benefits of adding the information to a catchall like
statx we really need to be careful to not bloat the structure like
crazy.
> struct statx {
> ...
> /* 0x90 */
> __u64 stx_mnt_id;
>
> /* Memory buffer alignment required for directio, in bytes. */
> __u32 stx_dio_mem_align;
>
> /* File range alignment required for directio, in bytes. */
> __u32 stx_dio_fpos_align_min;
So this really needs a good explanation why we need both iven that we
had no real use case for this.
> /* File range alignment needed for best performance, in bytes. */
> __u32 stx_dio_fpos_align_opt;
And why we really care about this. I guess you want to allow sector
size dio in reflink setups, but discourage it. But is this really as
important?
> /* Maximum size of a directio request, in bytes. */
> __u32 stx_dio_max_iosize;
I know XFS_IOC_DIOINFO had this, but does it really make much sense?
Why do we need it for direct I/O and not buffered I/O?
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v10 1/5] fscrypt: add functions for direct I/O support
On Thu, Jan 20, 2022 at 01:04:17AM -0800, Eric Biggers wrote: > I actually had changed this from v9 because fscrypt_dio_supported() seemed > backwards, given that its purpose is to check whether DIO is unsupported, not > whether it's supported per se (and the function's comment reflected this). > What > ext4 and f2fs do is check a list of reasons why DIO would *not* be supported, > and if none apply, then it is supported. This is just one of those reasons. > > This is subjective though, so if people prefer the old way, I'll change it > back. I find non-negated API much better and would also help with undinwinding the ext4/f2fs mess. But I'm not going to block the series on such a minor detail, of course. ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v10 0/5] add support for direct I/O with fscrypt using blk-crypto
On Fri, Jan 21, 2022 at 10:57:55AM +1100, Dave Chinner wrote:
> On Thu, Jan 20, 2022 at 02:48:52PM -0800, Eric Biggers wrote:
> > On Fri, Jan 21, 2022 at 09:04:14AM +1100, Dave Chinner wrote:
> > > On Thu, Jan 20, 2022 at 01:00:27PM -0800, Darrick J. Wong wrote:
> > > > On Thu, Jan 20, 2022 at 12:39:14PM -0800, Eric Biggers wrote:
> > > > > On Thu, Jan 20, 2022 at 09:10:27AM -0800, Darrick J. Wong wrote:
> > > > > > On Thu, Jan 20, 2022 at 12:30:23AM -0800, Christoph Hellwig wrote:
> > > > > > > On Wed, Jan 19, 2022 at 11:12:10PM -0800, Eric Biggers wrote:
> > > > > > > >
> > > > > > > > Given the above, as far as I know the only remaining objection
> > > > > > > > to this
> > > > > > > > patchset would be that DIO constraints aren't sufficiently
> > > > > > > > discoverable
> > > > > > > > by userspace. Now, to put this in context, this is a
> > > > > > > > longstanding issue
> > > > > > > > with all Linux filesystems, except XFS which has
> > > > > > > > XFS_IOC_DIOINFO. It's
> > > > > > > > not specific to this feature, and it doesn't actually seem to
> > > > > > > > be too
> > > > > > > > important in practice; many other filesystem features place
> > > > > > > > constraints
> > > > > > > > on DIO, and f2fs even *only* allows fully FS block size aligned
> > > > > > > > DIO.
> > > > > > > > (And for better or worse, many systems using fscrypt already
> > > > > > > > have
> > > > > > > > out-of-tree patches that enable DIO support, and people don't
> > > > > > > > seem to
> > > > > > > > have trouble with the FS block size alignment requirement.)
> > > > > > >
> > > > > > > It might make sense to use this as an opportunity to implement
> > > > > > > XFS_IOC_DIOINFO for ext4 and f2fs.
> > > > > >
> > > > > > Hmm. A potential problem with DIOINFO is that it doesn't explicitly
> > > > > > list the /file/ position alignment requirement:
> > > > > >
> > > > > > struct dioattr {
> > > > > > __u32 d_mem; /* data buffer memory alignment
> > > > > > */
> > > > > > __u32 d_miniosz; /* min xfer size
> > > > > > */
> > > > > > __u32 d_maxiosz; /* max xfer size
> > > > > > */
> > > > > > };
> > > > >
> > > > > Well, the comment above struct dioattr says:
> > > > >
> > > > > /*
> > > > >* Direct I/O attribute record used with XFS_IOC_DIOINFO
> > > > >* d_miniosz is the min xfer size, xfer size multiple and file
> > > > > seek offset
> > > > >* alignment.
> > > > >*/
> > > > >
> > > > > So d_miniosz serves that purpose already.
> > > > >
> > > > > >
> > > > > > Since I /think/ fscrypt requires that directio writes be aligned to
> > > > > > file
> > > > > > block size, right?
> > > > >
> > > > > The file position must be a multiple of the filesystem block size,
> > > > > yes.
> > > > > Likewise for the "minimum xfer size" and "xfer size multiple", and
> > > > > the "data
> > > > > buffer memory alignment" for that matter. So I think XFS_IOC_DIOINFO
> > > > > would be
> > > > > good enough for the fscrypt direct I/O case.
> > > >
> > > > Oh, ok then. In that case, just hoist XFS_IOC_DIOINFO to the VFS and
> > > > add a couple of implementations for ext4 and f2fs, and I think that'll
> > > > be enough to get the fscrypt patchset moving again.
> > >
> > > On the contrary, I'd much prefer to see this information added to
> > > statx(). The file offset alignment info is a property of the current
> > > file (e.g. XFS can have different per-file requirements depending on
> > > whether the file data is hosted on the data or RT device, etc) and
> > > so it's not a fixed property of the filesystem.
> > >
> > > statx() was designed to be extended with per-file property
> > > information, and we already have stuff like filesystem block size in
> > > that syscall. Hence I would much prefer that we extend it with the
> > > DIO properties we need to support rather than "create" a new VFS
> > > ioctl to extract this information. We already have statx(), so let's
> > > use it for what it was intended for.
Eh, ok. Let's do that instead.
> > >
> >
> > I assumed that XFS_IOC_DIOINFO *was* per-file. XFS's *implementation* of it
> > looks at the filesystem only,
>
> You've got that wrong.
>
> case XFS_IOC_DIOINFO: {
> >> struct xfs_buftarg *target = xfs_inode_buftarg(ip);
> struct dioattr da;
>
> da.d_mem = da.d_miniosz = target->bt_logical_sectorsize;
>
> xfs_inode_buftarg() is determining which block device the inode is
> storing it's data on, so the returned dioattr values can be
> different for different inodes in the filesystem...
>
> It's always been that way since the early Irix days - XFS RT devices
> could have very different IO constraints than the data device and
> DIO had to conform to the hardware limits underlying the filesystem.
> Hence the dioattr information has -always- been per-inode
Re: [f2fs-dev] [PATCH] unicode: clean up the Kconfig symbol confusion
Christoph Hellwig writes:
> Turn the CONFIG_UNICODE symbol into a tristate that generates some always
> built in code and remove the confusing CONFIG_UNICODE_UTF8_DATA symbol.
>
> Note that a lot of the IS_ENALBED() checks could be turned from cpp
> statements into normal ifs, but this change is intended to be fairly
> mechanic, so that should be cleaned up later.
Hi,
Just a typo s/ENALBED/ENABLED/.
> Fixes: 2b3d04787012 ("unicode: Add utf8-data module")
> Reported-by: Linus Torvalds
> Signed-off-by: Christoph Hellwig
I fixed the typo and pushed the patch to a linux-next visible branch
https://git.kernel.org/pub/scm/linux/kernel/git/krisman/unicode.git/commit/?h=for-next=5298d4bfe80f6ae6ae2777bcd1357b0022d98573
I'm also sending a patch series shortly turning IS_ENABLED into part of
the code flow where possible.
Thank you,
--
Gabriel Krisman Bertazi
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v10 0/5] add support for direct I/O with fscrypt using blk-crypto
On Thu, Jan 20, 2022 at 02:48:52PM -0800, Eric Biggers wrote:
> On Fri, Jan 21, 2022 at 09:04:14AM +1100, Dave Chinner wrote:
> > On Thu, Jan 20, 2022 at 01:00:27PM -0800, Darrick J. Wong wrote:
> > > On Thu, Jan 20, 2022 at 12:39:14PM -0800, Eric Biggers wrote:
> > > > On Thu, Jan 20, 2022 at 09:10:27AM -0800, Darrick J. Wong wrote:
> > > > > On Thu, Jan 20, 2022 at 12:30:23AM -0800, Christoph Hellwig wrote:
> > > > > > On Wed, Jan 19, 2022 at 11:12:10PM -0800, Eric Biggers wrote:
> > > > > > >
> > > > > > > Given the above, as far as I know the only remaining objection to
> > > > > > > this
> > > > > > > patchset would be that DIO constraints aren't sufficiently
> > > > > > > discoverable
> > > > > > > by userspace. Now, to put this in context, this is a
> > > > > > > longstanding issue
> > > > > > > with all Linux filesystems, except XFS which has XFS_IOC_DIOINFO.
> > > > > > > It's
> > > > > > > not specific to this feature, and it doesn't actually seem to be
> > > > > > > too
> > > > > > > important in practice; many other filesystem features place
> > > > > > > constraints
> > > > > > > on DIO, and f2fs even *only* allows fully FS block size aligned
> > > > > > > DIO.
> > > > > > > (And for better or worse, many systems using fscrypt already have
> > > > > > > out-of-tree patches that enable DIO support, and people don't
> > > > > > > seem to
> > > > > > > have trouble with the FS block size alignment requirement.)
> > > > > >
> > > > > > It might make sense to use this as an opportunity to implement
> > > > > > XFS_IOC_DIOINFO for ext4 and f2fs.
> > > > >
> > > > > Hmm. A potential problem with DIOINFO is that it doesn't explicitly
> > > > > list the /file/ position alignment requirement:
> > > > >
> > > > > struct dioattr {
> > > > > __u32 d_mem; /* data buffer memory alignment
> > > > > */
> > > > > __u32 d_miniosz; /* min xfer size
> > > > > */
> > > > > __u32 d_maxiosz; /* max xfer size
> > > > > */
> > > > > };
> > > >
> > > > Well, the comment above struct dioattr says:
> > > >
> > > > /*
> > > > * Direct I/O attribute record used with XFS_IOC_DIOINFO
> > > > * d_miniosz is the min xfer size, xfer size multiple and file
> > > > seek offset
> > > > * alignment.
> > > > */
> > > >
> > > > So d_miniosz serves that purpose already.
> > > >
> > > > >
> > > > > Since I /think/ fscrypt requires that directio writes be aligned to
> > > > > file
> > > > > block size, right?
> > > >
> > > > The file position must be a multiple of the filesystem block size, yes.
> > > > Likewise for the "minimum xfer size" and "xfer size multiple", and the
> > > > "data
> > > > buffer memory alignment" for that matter. So I think XFS_IOC_DIOINFO
> > > > would be
> > > > good enough for the fscrypt direct I/O case.
> > >
> > > Oh, ok then. In that case, just hoist XFS_IOC_DIOINFO to the VFS and
> > > add a couple of implementations for ext4 and f2fs, and I think that'll
> > > be enough to get the fscrypt patchset moving again.
> >
> > On the contrary, I'd much prefer to see this information added to
> > statx(). The file offset alignment info is a property of the current
> > file (e.g. XFS can have different per-file requirements depending on
> > whether the file data is hosted on the data or RT device, etc) and
> > so it's not a fixed property of the filesystem.
> >
> > statx() was designed to be extended with per-file property
> > information, and we already have stuff like filesystem block size in
> > that syscall. Hence I would much prefer that we extend it with the
> > DIO properties we need to support rather than "create" a new VFS
> > ioctl to extract this information. We already have statx(), so let's
> > use it for what it was intended for.
> >
>
> I assumed that XFS_IOC_DIOINFO *was* per-file. XFS's *implementation* of it
> looks at the filesystem only,
You've got that wrong.
case XFS_IOC_DIOINFO: {
>> struct xfs_buftarg *target = xfs_inode_buftarg(ip);
struct dioattr da;
da.d_mem = da.d_miniosz = target->bt_logical_sectorsize;
xfs_inode_buftarg() is determining which block device the inode is
storing it's data on, so the returned dioattr values can be
different for different inodes in the filesystem...
It's always been that way since the early Irix days - XFS RT devices
could have very different IO constraints than the data device and
DIO had to conform to the hardware limits underlying the filesystem.
Hence the dioattr information has -always- been per-inode
information.
> (Per-file state is required for encrypted
> files. It's also required for other filesystem features; e.g., files that use
> compression or fs-verity don't support direct I/O at all.)
Which is exactly why is should be a property of statx(), rather than
try to re-use a ~30 year old filesystem
Re: [f2fs-dev] [PATCH v10 0/5] add support for direct I/O with fscrypt using blk-crypto
On Fri, Jan 21, 2022 at 09:04:14AM +1100, Dave Chinner wrote:
> On Thu, Jan 20, 2022 at 01:00:27PM -0800, Darrick J. Wong wrote:
> > On Thu, Jan 20, 2022 at 12:39:14PM -0800, Eric Biggers wrote:
> > > On Thu, Jan 20, 2022 at 09:10:27AM -0800, Darrick J. Wong wrote:
> > > > On Thu, Jan 20, 2022 at 12:30:23AM -0800, Christoph Hellwig wrote:
> > > > > On Wed, Jan 19, 2022 at 11:12:10PM -0800, Eric Biggers wrote:
> > > > > >
> > > > > > Given the above, as far as I know the only remaining objection to
> > > > > > this
> > > > > > patchset would be that DIO constraints aren't sufficiently
> > > > > > discoverable
> > > > > > by userspace. Now, to put this in context, this is a longstanding
> > > > > > issue
> > > > > > with all Linux filesystems, except XFS which has XFS_IOC_DIOINFO.
> > > > > > It's
> > > > > > not specific to this feature, and it doesn't actually seem to be too
> > > > > > important in practice; many other filesystem features place
> > > > > > constraints
> > > > > > on DIO, and f2fs even *only* allows fully FS block size aligned DIO.
> > > > > > (And for better or worse, many systems using fscrypt already have
> > > > > > out-of-tree patches that enable DIO support, and people don't seem
> > > > > > to
> > > > > > have trouble with the FS block size alignment requirement.)
> > > > >
> > > > > It might make sense to use this as an opportunity to implement
> > > > > XFS_IOC_DIOINFO for ext4 and f2fs.
> > > >
> > > > Hmm. A potential problem with DIOINFO is that it doesn't explicitly
> > > > list the /file/ position alignment requirement:
> > > >
> > > > struct dioattr {
> > > > __u32 d_mem; /* data buffer memory alignment
> > > > */
> > > > __u32 d_miniosz; /* min xfer size
> > > > */
> > > > __u32 d_maxiosz; /* max xfer size
> > > > */
> > > > };
> > >
> > > Well, the comment above struct dioattr says:
> > >
> > > /*
> > >* Direct I/O attribute record used with XFS_IOC_DIOINFO
> > >* d_miniosz is the min xfer size, xfer size multiple and file seek
> > > offset
> > >* alignment.
> > >*/
> > >
> > > So d_miniosz serves that purpose already.
> > >
> > > >
> > > > Since I /think/ fscrypt requires that directio writes be aligned to file
> > > > block size, right?
> > >
> > > The file position must be a multiple of the filesystem block size, yes.
> > > Likewise for the "minimum xfer size" and "xfer size multiple", and the
> > > "data
> > > buffer memory alignment" for that matter. So I think XFS_IOC_DIOINFO
> > > would be
> > > good enough for the fscrypt direct I/O case.
> >
> > Oh, ok then. In that case, just hoist XFS_IOC_DIOINFO to the VFS and
> > add a couple of implementations for ext4 and f2fs, and I think that'll
> > be enough to get the fscrypt patchset moving again.
>
> On the contrary, I'd much prefer to see this information added to
> statx(). The file offset alignment info is a property of the current
> file (e.g. XFS can have different per-file requirements depending on
> whether the file data is hosted on the data or RT device, etc) and
> so it's not a fixed property of the filesystem.
>
> statx() was designed to be extended with per-file property
> information, and we already have stuff like filesystem block size in
> that syscall. Hence I would much prefer that we extend it with the
> DIO properties we need to support rather than "create" a new VFS
> ioctl to extract this information. We already have statx(), so let's
> use it for what it was intended for.
>
I assumed that XFS_IOC_DIOINFO *was* per-file. XFS's *implementation* of it
looks at the filesystem only, but that would be the expected implementation if
the DIO constraints don't currently vary between different files in XFS.
If DIO constraints do in fact already vary between different files in XFS, is
this just a bug in the XFS implementation of XFS_IOC_DIOINFO? Or was
XFS_IOC_DIOINFO only ever intended to report per-filesystem state? If the
latter, then yes, that would mean it wouldn't really be suitable to reuse to
start reporting per-file state. (Per-file state is required for encrypted
files. It's also required for other filesystem features; e.g., files that use
compression or fs-verity don't support direct I/O at all.)
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v10 0/5] add support for direct I/O with fscrypt using blk-crypto
On Thu, Jan 20, 2022 at 01:00:27PM -0800, Darrick J. Wong wrote:
> On Thu, Jan 20, 2022 at 12:39:14PM -0800, Eric Biggers wrote:
> > On Thu, Jan 20, 2022 at 09:10:27AM -0800, Darrick J. Wong wrote:
> > > On Thu, Jan 20, 2022 at 12:30:23AM -0800, Christoph Hellwig wrote:
> > > > On Wed, Jan 19, 2022 at 11:12:10PM -0800, Eric Biggers wrote:
> > > > >
> > > > > Given the above, as far as I know the only remaining objection to this
> > > > > patchset would be that DIO constraints aren't sufficiently
> > > > > discoverable
> > > > > by userspace. Now, to put this in context, this is a longstanding
> > > > > issue
> > > > > with all Linux filesystems, except XFS which has XFS_IOC_DIOINFO.
> > > > > It's
> > > > > not specific to this feature, and it doesn't actually seem to be too
> > > > > important in practice; many other filesystem features place
> > > > > constraints
> > > > > on DIO, and f2fs even *only* allows fully FS block size aligned DIO.
> > > > > (And for better or worse, many systems using fscrypt already have
> > > > > out-of-tree patches that enable DIO support, and people don't seem to
> > > > > have trouble with the FS block size alignment requirement.)
> > > >
> > > > It might make sense to use this as an opportunity to implement
> > > > XFS_IOC_DIOINFO for ext4 and f2fs.
> > >
> > > Hmm. A potential problem with DIOINFO is that it doesn't explicitly
> > > list the /file/ position alignment requirement:
> > >
> > > struct dioattr {
> > > __u32 d_mem; /* data buffer memory alignment */
> > > __u32 d_miniosz; /* min xfer size*/
> > > __u32 d_maxiosz; /* max xfer size*/
> > > };
> >
> > Well, the comment above struct dioattr says:
> >
> > /*
> > * Direct I/O attribute record used with XFS_IOC_DIOINFO
> > * d_miniosz is the min xfer size, xfer size multiple and file seek
> > offset
> > * alignment.
> > */
> >
> > So d_miniosz serves that purpose already.
> >
> > >
> > > Since I /think/ fscrypt requires that directio writes be aligned to file
> > > block size, right?
> >
> > The file position must be a multiple of the filesystem block size, yes.
> > Likewise for the "minimum xfer size" and "xfer size multiple", and the "data
> > buffer memory alignment" for that matter. So I think XFS_IOC_DIOINFO would
> > be
> > good enough for the fscrypt direct I/O case.
>
> Oh, ok then. In that case, just hoist XFS_IOC_DIOINFO to the VFS and
> add a couple of implementations for ext4 and f2fs, and I think that'll
> be enough to get the fscrypt patchset moving again.
On the contrary, I'd much prefer to see this information added to
statx(). The file offset alignment info is a property of the current
file (e.g. XFS can have different per-file requirements depending on
whether the file data is hosted on the data or RT device, etc) and
so it's not a fixed property of the filesystem.
statx() was designed to be extended with per-file property
information, and we already have stuff like filesystem block size in
that syscall. Hence I would much prefer that we extend it with the
DIO properties we need to support rather than "create" a new VFS
ioctl to extract this information. We already have statx(), so let's
use it for what it was intended for.
> > The real question is whether there are any direct I/O implementations where
> > XFS_IOC_DIOINFO would *not* be good enough, for example due to "xfer size
> > multiple" != "file seek offset alignment" being allowed. In that case we
> > would
> > need to define a new ioctl that is more general (like the one you described
> > below) rather than simply uplifting XFS_IOC_DIOINFO.
>
> I don't think there are any currently, but if anyone ever redesigns
> DIOINFO we might as well make all those pieces explicit.
>
> > More general is nice, but it's not helpful if no one will actually use the
> > extra
> > information. So we need to figure out what is actually useful.
>
> Clearly I haven't wanted d_opt_fpos badly enough to propose
> revving the ioctl. ;)
I think the number of applications that use DIOINFO outside of
xfsprogs/xfsdump/fstests can probably be counted on one hand.
Debian code search tells me:
-qemu (under ifdef CONFIG_XFS)
-ceph 16.2 (seastar database support?)
-diod contains a copy of fsstress
-e2fsprogs contains a copy of fsstress
-openmpi (under ifdef SGIMPI)
-partclone - actually, that has a complete copy of the xfsprogs
libxfs/ iand include/ directory in it, so it's using
the old libxfs_device_alignment() call that uses
XFS_IOC_DIOINFOD, and only when builing the xfsclone
binary.
Yup, I can count them on one 6 fingered hand, and their only use is
when XFS filesystems are specifically discovered. :)
Hence I think it would be much more useful to application developers
to include the IO alignment information in statx(), not to lift an
ioctl that is pretty much unused and
Re: [f2fs-dev] [PATCH v10 0/5] add support for direct I/O with fscrypt using blk-crypto
On Thu, Jan 20, 2022 at 12:39:14PM -0800, Eric Biggers wrote:
> On Thu, Jan 20, 2022 at 09:10:27AM -0800, Darrick J. Wong wrote:
> > On Thu, Jan 20, 2022 at 12:30:23AM -0800, Christoph Hellwig wrote:
> > > On Wed, Jan 19, 2022 at 11:12:10PM -0800, Eric Biggers wrote:
> > > >
> > > > Given the above, as far as I know the only remaining objection to this
> > > > patchset would be that DIO constraints aren't sufficiently discoverable
> > > > by userspace. Now, to put this in context, this is a longstanding issue
> > > > with all Linux filesystems, except XFS which has XFS_IOC_DIOINFO. It's
> > > > not specific to this feature, and it doesn't actually seem to be too
> > > > important in practice; many other filesystem features place constraints
> > > > on DIO, and f2fs even *only* allows fully FS block size aligned DIO.
> > > > (And for better or worse, many systems using fscrypt already have
> > > > out-of-tree patches that enable DIO support, and people don't seem to
> > > > have trouble with the FS block size alignment requirement.)
> > >
> > > It might make sense to use this as an opportunity to implement
> > > XFS_IOC_DIOINFO for ext4 and f2fs.
> >
> > Hmm. A potential problem with DIOINFO is that it doesn't explicitly
> > list the /file/ position alignment requirement:
> >
> > struct dioattr {
> > __u32 d_mem; /* data buffer memory alignment */
> > __u32 d_miniosz; /* min xfer size*/
> > __u32 d_maxiosz; /* max xfer size*/
> > };
>
> Well, the comment above struct dioattr says:
>
> /*
>* Direct I/O attribute record used with XFS_IOC_DIOINFO
>* d_miniosz is the min xfer size, xfer size multiple and file seek
> offset
>* alignment.
>*/
>
> So d_miniosz serves that purpose already.
>
> >
> > Since I /think/ fscrypt requires that directio writes be aligned to file
> > block size, right?
>
> The file position must be a multiple of the filesystem block size, yes.
> Likewise for the "minimum xfer size" and "xfer size multiple", and the "data
> buffer memory alignment" for that matter. So I think XFS_IOC_DIOINFO would be
> good enough for the fscrypt direct I/O case.
Oh, ok then. In that case, just hoist XFS_IOC_DIOINFO to the VFS and
add a couple of implementations for ext4 and f2fs, and I think that'll
be enough to get the fscrypt patchset moving again.
> The real question is whether there are any direct I/O implementations where
> XFS_IOC_DIOINFO would *not* be good enough, for example due to "xfer size
> multiple" != "file seek offset alignment" being allowed. In that case we
> would
> need to define a new ioctl that is more general (like the one you described
> below) rather than simply uplifting XFS_IOC_DIOINFO.
I don't think there are any currently, but if anyone ever redesigns
DIOINFO we might as well make all those pieces explicit.
> More general is nice, but it's not helpful if no one will actually use the
> extra
> information. So we need to figure out what is actually useful.
Clearly I haven't wanted d_opt_fpos badly enough to propose
revving the ioctl. ;)
--D
>
> > How about something like this:
> >
> > struct dioattr2 {
> > __u32 d_mem; /* data buffer memory alignment */
> > __u32 d_miniosz; /* min xfer size*/
> > __u32 d_maxiosz; /* max xfer size*/
> >
> > /* file range must be aligned to this value */
> > __u32 d_min_fpos;
> >
> > /* for optimal performance, align file range to this */
> > __u32 d_opt_fpos;
> >
> > __u32 d_padding[11];
> > };
> >
>
> - Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v10 0/5] add support for direct I/O with fscrypt using blk-crypto
On Thu, Jan 20, 2022 at 09:10:27AM -0800, Darrick J. Wong wrote:
> On Thu, Jan 20, 2022 at 12:30:23AM -0800, Christoph Hellwig wrote:
> > On Wed, Jan 19, 2022 at 11:12:10PM -0800, Eric Biggers wrote:
> > >
> > > Given the above, as far as I know the only remaining objection to this
> > > patchset would be that DIO constraints aren't sufficiently discoverable
> > > by userspace. Now, to put this in context, this is a longstanding issue
> > > with all Linux filesystems, except XFS which has XFS_IOC_DIOINFO. It's
> > > not specific to this feature, and it doesn't actually seem to be too
> > > important in practice; many other filesystem features place constraints
> > > on DIO, and f2fs even *only* allows fully FS block size aligned DIO.
> > > (And for better or worse, many systems using fscrypt already have
> > > out-of-tree patches that enable DIO support, and people don't seem to
> > > have trouble with the FS block size alignment requirement.)
> >
> > It might make sense to use this as an opportunity to implement
> > XFS_IOC_DIOINFO for ext4 and f2fs.
>
> Hmm. A potential problem with DIOINFO is that it doesn't explicitly
> list the /file/ position alignment requirement:
>
> struct dioattr {
> __u32 d_mem; /* data buffer memory alignment */
> __u32 d_miniosz; /* min xfer size*/
> __u32 d_maxiosz; /* max xfer size*/
> };
Well, the comment above struct dioattr says:
/*
* Direct I/O attribute record used with XFS_IOC_DIOINFO
* d_miniosz is the min xfer size, xfer size multiple and file seek
offset
* alignment.
*/
So d_miniosz serves that purpose already.
>
> Since I /think/ fscrypt requires that directio writes be aligned to file
> block size, right?
The file position must be a multiple of the filesystem block size, yes.
Likewise for the "minimum xfer size" and "xfer size multiple", and the "data
buffer memory alignment" for that matter. So I think XFS_IOC_DIOINFO would be
good enough for the fscrypt direct I/O case.
The real question is whether there are any direct I/O implementations where
XFS_IOC_DIOINFO would *not* be good enough, for example due to "xfer size
multiple" != "file seek offset alignment" being allowed. In that case we would
need to define a new ioctl that is more general (like the one you described
below) rather than simply uplifting XFS_IOC_DIOINFO.
More general is nice, but it's not helpful if no one will actually use the extra
information. So we need to figure out what is actually useful.
> How about something like this:
>
> struct dioattr2 {
> __u32 d_mem; /* data buffer memory alignment */
> __u32 d_miniosz; /* min xfer size*/
> __u32 d_maxiosz; /* max xfer size*/
>
> /* file range must be aligned to this value */
> __u32 d_min_fpos;
>
> /* for optimal performance, align file range to this */
> __u32 d_opt_fpos;
>
> __u32 d_padding[11];
> };
>
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v10 0/5] add support for direct I/O with fscrypt using blk-crypto
On Thu, Jan 20, 2022 at 12:30:23AM -0800, Christoph Hellwig wrote:
> On Wed, Jan 19, 2022 at 11:12:10PM -0800, Eric Biggers wrote:
> >
> > Given the above, as far as I know the only remaining objection to this
> > patchset would be that DIO constraints aren't sufficiently discoverable
> > by userspace. Now, to put this in context, this is a longstanding issue
> > with all Linux filesystems, except XFS which has XFS_IOC_DIOINFO. It's
> > not specific to this feature, and it doesn't actually seem to be too
> > important in practice; many other filesystem features place constraints
> > on DIO, and f2fs even *only* allows fully FS block size aligned DIO.
> > (And for better or worse, many systems using fscrypt already have
> > out-of-tree patches that enable DIO support, and people don't seem to
> > have trouble with the FS block size alignment requirement.)
>
> It might make sense to use this as an opportunity to implement
> XFS_IOC_DIOINFO for ext4 and f2fs.
Hmm. A potential problem with DIOINFO is that it doesn't explicitly
list the /file/ position alignment requirement:
struct dioattr {
__u32 d_mem; /* data buffer memory alignment */
__u32 d_miniosz; /* min xfer size*/
__u32 d_maxiosz; /* max xfer size*/
};
Since I /think/ fscrypt requires that directio writes be aligned to file
block size, right?
> > I plan to propose a new generic ioctl to address the issue of DIO
> > constraints being insufficiently discoverable. But until then, I'm
Which is what I suspect Eric meant by this sentence. :)
> > wondering if people are willing to consider this patchset again, or
> > whether it is considered blocked by this issue alone. (And if this
> > patchset is still unacceptable, would it be acceptable with f2fs support
> > only, given that f2fs *already* only allows FS block size aligned DIO?)
>
> I think the patchset looks fine, but I'd really love to have a way for
> the alignment restrictions to be discoverable from the start.
I agree. The mechanics of the patchset look ok to me, but it's very
unfortunate that there's no way for userspace programs to ask the kernel
about the directio geometry for a file.
Ever since we added reflink to XFS I've wanted to add a way to tell
userspace that direct writes to a reflink(able) file will be much more
efficient if they can align the io request to 1 fs block instead of 1
sector.
How about something like this:
struct dioattr2 {
__u32 d_mem; /* data buffer memory alignment */
__u32 d_miniosz; /* min xfer size*/
__u32 d_maxiosz; /* max xfer size*/
/* file range must be aligned to this value */
__u32 d_min_fpos;
/* for optimal performance, align file range to this */
__u32 d_opt_fpos;
__u32 d_padding[11];
};
--D
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v10 1/5] fscrypt: add functions for direct I/O support
On Thu, Jan 20, 2022 at 12:27:45AM -0800, Christoph Hellwig wrote: > > +/** > > + * fscrypt_dio_unsupported() - check whether a DIO (direct I/O) request is > > + *unsupported due to encryption constraints > > + * @iocb: the file and position the I/O is targeting > > + * @iter: the I/O data segment(s) > > + * > > + * Return: true if DIO is unsupported > > + */ > > +bool fscrypt_dio_unsupported(struct kiocb *iocb, struct iov_iter *iter) > > I always find non-negated functions easier to follow, i.e. turn this > into fscrypt_dio_supported(). > I actually had changed this from v9 because fscrypt_dio_supported() seemed backwards, given that its purpose is to check whether DIO is unsupported, not whether it's supported per se (and the function's comment reflected this). What ext4 and f2fs do is check a list of reasons why DIO would *not* be supported, and if none apply, then it is supported. This is just one of those reasons. This is subjective though, so if people prefer the old way, I'll change it back. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v10 0/5] add support for direct I/O with fscrypt using blk-crypto
On Wed, Jan 19, 2022 at 11:12:10PM -0800, Eric Biggers wrote: > > Given the above, as far as I know the only remaining objection to this > patchset would be that DIO constraints aren't sufficiently discoverable > by userspace. Now, to put this in context, this is a longstanding issue > with all Linux filesystems, except XFS which has XFS_IOC_DIOINFO. It's > not specific to this feature, and it doesn't actually seem to be too > important in practice; many other filesystem features place constraints > on DIO, and f2fs even *only* allows fully FS block size aligned DIO. > (And for better or worse, many systems using fscrypt already have > out-of-tree patches that enable DIO support, and people don't seem to > have trouble with the FS block size alignment requirement.) It might make sense to use this as an opportunity to implement XFS_IOC_DIOINFO for ext4 and f2fs. > I plan to propose a new generic ioctl to address the issue of DIO > constraints being insufficiently discoverable. But until then, I'm > wondering if people are willing to consider this patchset again, or > whether it is considered blocked by this issue alone. (And if this > patchset is still unacceptable, would it be acceptable with f2fs support > only, given that f2fs *already* only allows FS block size aligned DIO?) I think the patchset looks fine, but I'd really love to have a way for the alignment restrictions to be discoverable from the start. ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v10 2/5] iomap: support direct I/O with fscrypt using blk-crypto
On Wed, Jan 19, 2022 at 11:12:12PM -0800, Eric Biggers wrote: > bio = bio_alloc(GFP_KERNEL, 1); > + fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits, > + GFP_KERNEL); Note that this will create a (harmless) conflict with my "improve the bio allocation interface" series. Otherwise looks good: Reviewed-by: Christoph Hellwig ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v10 1/5] fscrypt: add functions for direct I/O support
> +/** > + * fscrypt_dio_unsupported() - check whether a DIO (direct I/O) request is > + * unsupported due to encryption constraints > + * @iocb: the file and position the I/O is targeting > + * @iter: the I/O data segment(s) > + * > + * Return: true if DIO is unsupported > + */ > +bool fscrypt_dio_unsupported(struct kiocb *iocb, struct iov_iter *iter) I always find non-negated functions easier to follow, i.e. turn this into fscrypt_dio_supported(). > + /* > + * Since the granularity of encryption is filesystem blocks, the file > + * position and total I/O length must be aligned to the filesystem block > + * size -- not just to the block device's logical block size as is > + * traditionally the case for DIO on many filesystems (not including > + * f2fs, which only allows filesystem block aligned DIO anyway). I would not really mention a specific file system here. ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
