Re: [PATCH v2 13/14] fs: xfs: Validate atomic writes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 06/03/2024 21:22, Dave Chinner wrote:
On Mon, Mar 04, 2024 at 01:04:27PM +0000, John Garry wrote:
Validate that an atomic write adheres to length/offset rules. Since we
require extent alignment for atomic writes, this effectively also enforces
that the BIO which iomap produces is aligned.

Signed-off-by: John Garry<john.g.garry@xxxxxxxxxx>
---
  fs/xfs/xfs_file.c | 11 ++++++++++-
  1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index d0bd9d5f596c..cecc5428fd7c 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -709,11 +709,20 @@ xfs_file_dio_write(
  	struct kiocb		*iocb,
  	struct iov_iter		*from)
  {
-	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
+	struct inode		*inode = file_inode(iocb->ki_filp);
+	struct xfs_inode	*ip = XFS_I(inode);
  	struct xfs_mount	*mp = ip->i_mount;
  	struct xfs_buftarg      *target = xfs_inode_buftarg(ip);
  	size_t			count = iov_iter_count(from);
+ if (iocb->ki_flags & IOCB_ATOMIC) {
+		if (!generic_atomic_write_valid(iocb->ki_pos, from,
+			i_blocksize(inode),
a.k.a. mp->m_bsize. If you use that here, then the need for the VFS
inode goes away, too.

ok


+			XFS_FSB_TO_B(mp, xfs_get_extsz(ip)))) {
+			return -EINVAL;
+		}
+	}
Also, I think the checks are the wrong way around here. We can only
do IOCB_ATOMIC IO on force aligned/atomic write inodes, so shouldn't
we be checking that first,

We are checking that, but not here.

In 14/14, we only set FMODE_CAN_ATOMIC_WRITE for when xfs_inode_has_atomicwrites() is true, and only when FMODE_CAN_ATOMIC_WRITE is set can we get this far.

I don't see a point in duplicating this xfs_inode_has_atomicwrites() check, so I will make the commit message clearer on this - ok? Or add a comment.

then basing the rest of the checks on the
assumption that atomic writes are allowed and have been set up
correctly on the inode? i.e.

	if (iocb->ki_flags & IOCB_ATOMIC) {
		if (!xfs_inode_has_atomicwrites(ip))
			return -EINVAL;
		if (!generic_atomic_write_valid(iocb->ki_pos, from,
				mp->m_bsize, ip->i_extsize))
			return -EINVAL;
	}

because xfs_inode_has_atomicwrites() implies ip->i_extsize has been
set to the required atomic IO size?

I was not too comfortable using ip->i_extsize, as this can be set without forcealign being set. I know that we would not get this far without forcealign (being set).

Having said that, I don't like all the xfs_get_extsz() calls, so something better is required. Let me know you you think.

Thanks,
John





[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux