| Message ID | 20240304130428.13026-14-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | block atomic writes for XFS | expand |
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@oracle.com> > --- > 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. > + 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, 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? Cheers, Dave.
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@oracle.com> >> --- >> 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
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), + XFS_FSB_TO_B(mp, xfs_get_extsz(ip)))) { + return -EINVAL; + } + } + /* direct I/O must be aligned to device logical sector size */ if ((iocb->ki_pos | count) & target->bt_logical_sectormask) return -EINVAL;
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@oracle.com> --- fs/xfs/xfs_file.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)