]> www.infradead.org Git - users/hch/xfs.git/commitdiff
xfs: restrict when we try to align cow fork delalloc to cowextsz hints
authorDarrick J. Wong <djwong@kernel.org>
Wed, 19 Jun 2024 17:32:44 +0000 (10:32 -0700)
committerChandan Babu R <chandanbabu@kernel.org>
Wed, 26 Jun 2024 08:59:24 +0000 (14:29 +0530)
xfs/205 produces the following failure when always_cow is enabled:

  --- a/tests/xfs/205.out 2024-02-28 16:20:24.437887970 -0800
  +++ b/tests/xfs/205.out.bad 2024-06-03 21:13:40.584000000 -0700
  @@ -1,4 +1,5 @@
   QA output created by 205
   *** one file
  +   !!! disk full (expected)
   *** one file, a few bytes at a time
   *** done

This is the result of overly aggressive attempts to align cow fork
delalloc reservations to the CoW extent size hint.  Looking at the trace
data, we're trying to append a single fsblock to the "fred" file.
Trying to create a speculative post-eof reservation fails because
there's not enough space.

We then set @prealloc_blocks to zero and try again, but the cowextsz
alignment code triggers, which expands our request for a 1-fsblock
reservation into a 39-block reservation.  There's not enough space for
that, so the whole write fails with ENOSPC even though there's
sufficient space in the filesystem to allocate the single block that we
need to land the write.

There are two things wrong here -- first, we shouldn't be attempting
speculative preallocations beyond what was requested when we're low on
space.  Second, if we've already computed a posteof preallocation, we
shouldn't bother trying to align that to the cowextsize hint.

Fix both of these problems by adding a flag that only enables the
expansion of the delalloc reservation to the cowextsize if we're doing a
non-extending write, and only if we're not doing an ENOSPC retry.  This
requires us to move the ENOSPC retry logic to xfs_bmapi_reserve_delalloc.

I probably should have caught this six years ago when 6ca30729c206d was
being reviewed, but oh well.  Update the comments to reflect what the
code does now.

Fixes: 6ca30729c206d ("xfs: bmap code cleanup")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Chandan Babu R <chandanbabu@kernel.org>
fs/xfs/libxfs/xfs_bmap.c
fs/xfs/xfs_iomap.c

index c101cf266bc4dbf605e5ebe40cfe124dfc1ab8e2..6af6f744fdd6a29a9c04a1d457bf4e55b5038e51 100644 (file)
@@ -4058,20 +4058,32 @@ xfs_bmapi_reserve_delalloc(
        xfs_extlen_t            indlen;
        uint64_t                fdblocks;
        int                     error;
-       xfs_fileoff_t           aoff = off;
+       xfs_fileoff_t           aoff;
+       bool                    use_cowextszhint =
+                                       whichfork == XFS_COW_FORK && !prealloc;
 
+retry:
        /*
         * Cap the alloc length. Keep track of prealloc so we know whether to
         * tag the inode before we return.
         */
+       aoff = off;
        alen = XFS_FILBLKS_MIN(len + prealloc, XFS_MAX_BMBT_EXTLEN);
        if (!eof)
                alen = XFS_FILBLKS_MIN(alen, got->br_startoff - aoff);
        if (prealloc && alen >= len)
                prealloc = alen - len;
 
-       /* Figure out the extent size, adjust alen */
-       if (whichfork == XFS_COW_FORK) {
+       /*
+        * If we're targetting the COW fork but aren't creating a speculative
+        * posteof preallocation, try to expand the reservation to align with
+        * the COW extent size hint if there's sufficient free space.
+        *
+        * Unlike the data fork, the CoW cancellation functions will free all
+        * the reservations at inactivation, so we don't require that every
+        * delalloc reservation have a dirty pagecache.
+        */
+       if (use_cowextszhint) {
                struct xfs_bmbt_irec    prev;
                xfs_extlen_t            extsz = xfs_get_cowextsz_hint(ip);
 
@@ -4090,7 +4102,7 @@ xfs_bmapi_reserve_delalloc(
         */
        error = xfs_quota_reserve_blkres(ip, alen);
        if (error)
-               return error;
+               goto out;
 
        /*
         * Split changing sb for alen and indlen since they could be coming
@@ -4140,6 +4152,17 @@ out_unreserve_frextents:
 out_unreserve_quota:
        if (XFS_IS_QUOTA_ON(mp))
                xfs_quota_unreserve_blkres(ip, alen);
+out:
+       if (error == -ENOSPC || error == -EDQUOT) {
+               trace_xfs_delalloc_enospc(ip, off, len);
+
+               if (prealloc || use_cowextszhint) {
+                       /* retry without any preallocation */
+                       use_cowextszhint = false;
+                       prealloc = 0;
+                       goto retry;
+               }
+       }
        return error;
 }
 
index 3783426739258c9e13b99b64a0444f636254fef9..414903885ab90571e18634b98d7551ecff270749 100644 (file)
@@ -1148,33 +1148,23 @@ xfs_buffered_write_iomap_begin(
                }
        }
 
-retry:
-       error = xfs_bmapi_reserve_delalloc(ip, allocfork, offset_fsb,
-                       end_fsb - offset_fsb, prealloc_blocks,
-                       allocfork == XFS_DATA_FORK ? &imap : &cmap,
-                       allocfork == XFS_DATA_FORK ? &icur : &ccur,
-                       allocfork == XFS_DATA_FORK ? eof : cow_eof);
-       switch (error) {
-       case 0:
-               break;
-       case -ENOSPC:
-       case -EDQUOT:
-               /* retry without any preallocation */
-               trace_xfs_delalloc_enospc(ip, offset, count);
-               if (prealloc_blocks) {
-                       prealloc_blocks = 0;
-                       goto retry;
-               }
-               fallthrough;
-       default:
-               goto out_unlock;
-       }
-
        if (allocfork == XFS_COW_FORK) {
+               error = xfs_bmapi_reserve_delalloc(ip, allocfork, offset_fsb,
+                               end_fsb - offset_fsb, prealloc_blocks, &cmap,
+                               &ccur, cow_eof);
+               if (error)
+                       goto out_unlock;
+
                trace_xfs_iomap_alloc(ip, offset, count, allocfork, &cmap);
                goto found_cow;
        }
 
+       error = xfs_bmapi_reserve_delalloc(ip, allocfork, offset_fsb,
+                       end_fsb - offset_fsb, prealloc_blocks, &imap, &icur,
+                       eof);
+       if (error)
+               goto out_unlock;
+
        /*
         * Flag newly allocated delalloc blocks with IOMAP_F_NEW so we punch
         * them out if the write happens to fail.