add support for direct I/O with fscrypt using blk-crypto

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.)

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?)

Quoted 4 lines

+/**
+ * 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

Quoted 5 lines

+ * @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().

Quoted 11 lines · Eric Biggers

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.

Quoted 2 lines

I plan to propose a new generic ioctl to address the issue of DIO constraints being insufficiently discoverable. But until then, I'm

Quoted 4 lines

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.

Quoted 13 lines · Christoph Hellwig

Quoted 11 lines · Eric Biggers

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.)

Quoted 2 lines

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?

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];
};

Quoted 17 lines · Darrick J. Wong

Quoted 13 lines · Christoph Hellwig

Quoted 11 lines · Eric Biggers

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.)

Quoted 2 lines

It might make sense to use this as an opportunity to implement XFS_IOC_DIOINFO for ext4 and f2fs.

Quoted 8 lines

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.

Quoted 1 line

Quoted 2 lines

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.

Quoted 17 lines

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

Quoted 41 lines · Eric Biggers

Quoted 17 lines · Darrick J. Wong

Quoted 13 lines · Christoph Hellwig

Quoted 11 lines · Eric Biggers

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.)

Quoted 2 lines

It might make sense to use this as an opportunity to implement XFS_IOC_DIOINFO for ext4 and f2fs.

Quoted 8 lines

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.

Quoted 3 lines

Since I /think/ fscrypt requires that directio writes be aligned to file block size, right?

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

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.

<nod> Clearly I haven't wanted d_opt_fpos badly enough to propose revving the ioctl. ;)

Quoted 42 lines · Darrick J. Wong

Quoted 41 lines · Eric Biggers

Quoted 17 lines · Darrick J. Wong

Quoted 13 lines · Christoph Hellwig

Quoted 11 lines · Eric Biggers

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.)

Quoted 2 lines

It might make sense to use this as an opportunity to implement XFS_IOC_DIOINFO for ext4 and f2fs.

Quoted 8 lines

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.

Quoted 3 lines

Since I /think/ fscrypt requires that directio writes be aligned to file block size, right?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 58 lines · Dave Chinner

Quoted 42 lines · Darrick J. Wong

Quoted 41 lines · Eric Biggers

Quoted 17 lines · Darrick J. Wong

Quoted 13 lines · Christoph Hellwig

Quoted 11 lines · Eric Biggers

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.)

Quoted 2 lines

It might make sense to use this as an opportunity to implement XFS_IOC_DIOINFO for ext4 and f2fs.

Quoted 8 lines

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.

Quoted 3 lines

Since I /think/ fscrypt requires that directio writes be aligned to file block size, right?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 7 lines

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,

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

         struct xfs_buftarg      *target = xfs_inode_buftarg(ip);

(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.)

Quoted 59 lines · Eric Biggers

Quoted 58 lines · Dave Chinner

Quoted 42 lines · Darrick J. Wong

Quoted 41 lines · Eric Biggers

Quoted 17 lines · Darrick J. Wong

Quoted 13 lines · Christoph Hellwig

Quoted 11 lines · Eric Biggers

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.)

Quoted 2 lines

It might make sense to use this as an opportunity to implement XFS_IOC_DIOINFO for ext4 and f2fs.

Quoted 8 lines

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.

Quoted 3 lines

Since I /think/ fscrypt requires that directio writes be aligned to file block size, right?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 4 lines

Quoted 1 line

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: {

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

         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.

Quoted 3 lines

(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 specific API from a different OS that was never intended to indicate things like "DIO not supported on this file at all"....

We've been bitten many times by this "lift a rarely used filesystem specific ioctl to the VFS because it exists" method of API promotion. It almost always ends up in us discovering further down the track that there's something wrong with the API, it doesn't quite do what we need, we have to extend it anyway, or it's just plain borken, etc. And then we have to create a new, fit for purpose API anyway, and there's two VFS APIs we have to maintain forever instead of just one...

Can we learn from past mistakes this time instead of repeating them yet again?

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

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.

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.

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;

	/* File range alignment needed for best performance, in bytes. */
	__u32	stx_dio_fpos_align_opt;

	/* Maximum size of a directio request, in bytes. */
	__u32	stx_dio_max_iosize;

On Fri, Jan 21, 2022 at 10:57:55AM +1100, Dave Chinner 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.

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;

	/* 0xa0 */

	/* File range alignment needed for best performance, in bytes. */
	__u32	stx_dio_fpos_align_opt;

	/* Maximum size of a directio request, in bytes. */
	__u32	stx_dio_max_iosize;

#define STATX_DIRECTIO 0x00001000U /* Want/got directio geometry */

How about that?

Quoted 5 lines

	/* 0xa0 */

	/* File range alignment needed for best performance, in bytes. */
	__u32	stx_dio_fpos_align_opt;

This is a common property of both DIO and buffered IO, so no need for it to be dio-only property.

	__u32	stx_offset_align_optimal;

Quoted 6 lines · Dave Chinner

Quoted 5 lines

	/* 0xa0 */

	/* File range alignment needed for best performance, in bytes. */
	__u32	stx_dio_fpos_align_opt;

Quoted 5 lines

This is a common property of both DIO and buffered IO, so no need for it to be dio-only property.

	__u32	stx_offset_align_optimal;

Looking at this more closely: will stx_offset_align_optimal actually be useful, given that st[x]_blksize already exists?

From the stat(2) and statx(2) man pages:

st_blksize
This field gives the "preferred" block size for efficient filesystem I/O.

stx_blksize
The "preferred" block size for efficient filesystem I/O. (Writ‐ ing to a file in smaller chunks may cause an inefficient read- modify-rewrite.)

File offsets aren't explicitly mentioned, but I think it's implied they should be a multiple of st[x]_blksize, just like the I/O size. Otherwise, the I/O would obviously require reading/writing partial blocks.

So, the proposed stx_offset_align_optimal field sounds like the same thing to me. Is there anything I'm misunderstanding?

Putting stx_offset_align_optimal behind the STATX_DIRECTIO flag would also be confusing if it would apply to both direct and buffered I/O.