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

[John Garry]

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