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@xxxxxx>
> ---
> 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
>
>
