On 30/09/2024 17:41, Darrick J. Wong wrote:
On Mon, Sep 30, 2024 at 12:54:37PM +0000, John Garry wrote:
Validate that an atomic write adheres to length/offset rules. Currently
we can only write a single FS block.
For an IOCB with IOCB_ATOMIC set to get as far as xfs_file_dio_write(),
FMODE_CAN_ATOMIC_WRITE will need to be set for the file; for this,
ATOMICWRITES flags would also need to be set for the inode.
Signed-off-by: John Garry <john.g.garry@xxxxxxxxxx>
---
fs/xfs/xfs_file.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 412b1d71b52b..fa6a44b88ecc 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -688,6 +688,13 @@ xfs_file_dio_write(
struct xfs_buftarg *target = xfs_inode_buftarg(ip);
size_t count = iov_iter_count(from);
+ if (iocb->ki_flags & IOCB_ATOMIC) {
+ if (count != ip->i_mount->m_sb.sb_blocksize)
+ return -EINVAL;
+ if (!generic_atomic_write_valid(iocb, from))
+ return -EINVAL;
+ }
Does xfs_file_write_iter need a catch-all so that we don't fall back to
buffered write for a directio write that returns ENOTBLK?
if (iocb->ki_flags & IOCB_DIRECT) {
/*
* Allow a directio write to fall back to a buffered
* write *only* in the case that we're doing a reflink
* CoW. In all other directio scenarios we do not
* allow an operation to fall back to buffered mode.
*/
ret = xfs_file_dio_write(iocb, from);
if (ret != -ENOTBLK || (iocb->ki_flags & IOCB_ATOMIC))
return ret;
}
IIRC iomap_dio_rw can return ENOTBLK if pagecache invalidation fails for
the region that we're trying to directio write.
I see where you are talking about. There is also a ENOTBLK from
unaligned write for CoW, but we would not see that.
But I was thinking to use a common helper to catch this, like
generic_write_checks_count() [which is called on the buffered IO path]:
----8<-----
diff --git a/fs/read_write.c b/fs/read_write.c
index 32b476bf9be0..222f25c6439c 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1774,6 +1774,10 @@ int generic_write_checks_count(struct kiocb
*iocb, loff_t *count)
if (!*count)
return 0;
+ if (iocb->ki_flags & IOCB_ATOMIC &&
+ !(iocb->ki_flags & IOCB_DIRECT))
+ return -EINVAL;
+
if (iocb->ki_flags & IOCB_APPEND)
iocb->ki_pos = i_size_read(inode);
---->8-----
But we keep the IOCB_DIRECT flag for the buffered IO fallback (so no good).
Another option would be:
----8<-----
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -679,7 +679,12 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter
*iter,
if (ret != -EAGAIN) {
trace_iomap_dio_invalidate_fail(inode, iomi.pos,
iomi.len);
- ret = -ENOTBLK;
+ if (iocb->ki_flags & IOCB_ATOMIC) {
+ if (ret == -ENOTBLK)
+ ret = -EAGAIN;
+ }else {
+ ret = -ENOTBLK;
+ }
}
goto out_free_dio;
}
---->8-----
I suggest that, as other FSes (like ext4) handle -ENOTBLK and would need
to be changed similar to XFS. But I am not sure if changing the error
code from -ENOTBLK for IOCB_ATOMIC is ok.
Let me know what you think about possible alternative solutions.
Thanks,
John
