| Message ID | 20240328070256.2918605-2-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] xfs: check if_bytes under the ilock in xfs_reflink_end_cow_extent | expand |
On Thu, Mar 28, 2024 at 08:02:51AM +0100, Christoph Hellwig wrote: > Accessing if_bytes without the ilock is racy. Move the check a little > further down into the ilock critical section. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_reflink.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c > index 7da0e8f961d351..df632790a0a51c 100644 > --- a/fs/xfs/xfs_reflink.c > +++ b/fs/xfs/xfs_reflink.c > @@ -731,12 +731,6 @@ xfs_reflink_end_cow_extent( > int nmaps; > int error; > > - /* No COW extents? That's easy! */ > - if (ifp->if_bytes == 0) { > - *offset_fsb = end_fsb; > - return 0; > - } This unlocked access was supposed to short-circuit the case where there's absolutely nothing in the cow fork at all, so that we don't have to wait for a transaction and the ILOCK. Is the unlocked access causing problems? > - > resblks = XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK); > error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, > XFS_TRANS_RESERVE, &tp); > @@ -751,6 +745,12 @@ xfs_reflink_end_cow_extent( > xfs_ilock(ip, XFS_ILOCK_EXCL); > xfs_trans_ijoin(tp, ip, 0); > > + /* No COW extents? That's easy! */ > + if (ifp->if_bytes == 0) { > + *offset_fsb = end_fsb; > + goto out_cancel; > + } This is already taken care of by the clause that comes below the end of this diff: /* * In case of racing, overlapping AIO writes no COW extents might be * left by the time I/O completes for the loser of the race. In that * case we are done. */ if (!xfs_iext_lookup_extent(ip, ifp, *offset_fsb, &icur, &got) || got.br_startoff >= end_fsb) { *offset_fsb = end_fsb; goto out_cancel; } Since xfs_iext_lookup_extent will return false if the cow fork tree is empty. That said, I think the xfs_iext_count_may_overflow stuff is misplaced -- we should be querying the cow fork extent and bouncing out early before we bother with checking/upgrading the nextents width. If xfs_iext_count_upgrade dirtied the transaction, the early bailout will cause a shutdown. (The iext upgrade only needs to happen after the bmapi_read.) --D > + > error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, > XFS_IEXT_REFLINK_END_COW_CNT); > if (error == -EFBIG) > -- > 2.39.2 > >
On Fri, Mar 29, 2024 at 09:14:29AM -0700, Darrick J. Wong wrote: > This unlocked access was supposed to short-circuit the case where > there's absolutely nothing in the cow fork at all, so that we don't have > to wait for a transaction and the ILOCK. Is the unlocked access > causing problems? I've not observeved problems. But I can't see how this access can be race free. Note that this case can only happen if we have racy direct I/O writes, so I'm not sure trying to optimize performance for it makes much sense. > > + /* No COW extents? That's easy! */ > > + if (ifp->if_bytes == 0) { > > + *offset_fsb = end_fsb; > > + goto out_cancel; > > + } > > This is already taken care of by the clause that comes below > the end of this diff: > Since xfs_iext_lookup_extent will return false if the cow fork tree is > empty. > That said, I think the xfs_iext_count_may_overflow stuff is misplaced -- > we should be querying the cow fork extent and bouncing out early before > we bother with checking/upgrading the nextents width. If > xfs_iext_count_upgrade dirtied the transaction, the early bailout will > cause a shutdown. > > (The iext upgrade only needs to happen after the bmapi_read.) Yes, I guess the right thing is to move the upgrade later and then just let xfs_iext_lookup_extent handle the no extents case.
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c index 7da0e8f961d351..df632790a0a51c 100644 --- a/fs/xfs/xfs_reflink.c +++ b/fs/xfs/xfs_reflink.c @@ -731,12 +731,6 @@ xfs_reflink_end_cow_extent( int nmaps; int error; - /* No COW extents? That's easy! */ - if (ifp->if_bytes == 0) { - *offset_fsb = end_fsb; - return 0; - } - resblks = XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK); error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, XFS_TRANS_RESERVE, &tp); @@ -751,6 +745,12 @@ xfs_reflink_end_cow_extent( xfs_ilock(ip, XFS_ILOCK_EXCL); xfs_trans_ijoin(tp, ip, 0); + /* No COW extents? That's easy! */ + if (ifp->if_bytes == 0) { + *offset_fsb = end_fsb; + goto out_cancel; + } + error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, XFS_IEXT_REFLINK_END_COW_CNT); if (error == -EFBIG)
Accessing if_bytes without the ilock is racy. Move the check a little further down into the ilock critical section. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_reflink.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)