xfs: fix error returns from xfs_bmapi_write
authorChristoph Hellwig <hch@lst.de>
Mon, 29 Apr 2024 06:15:21 +0000 (08:15 +0200)
committerChandan Babu R <chandanbabu@kernel.org>
Tue, 30 Apr 2024 04:15:18 +0000 (09:45 +0530)
xfs_bmapi_write can return 0 without actually returning a mapping in
mval in two different cases:

 1) when there is absolutely no space available to do an allocation
 2) when converting delalloc space, and the allocation is so small
    that it only covers parts of the delalloc extent before the
    range requested by the caller

Callers at best can handle one of these cases, but in many cases can't
cope with either one.  Switch xfs_bmapi_write to always return a
mapping or return an error code instead.  For case 1) above ENOSPC is
the obvious choice which is very much what the callers expect anyway.
For case 2) there is no really good error code, so pick a funky one
from the SysV streams portfolio.

This fixes the reproducer here:

    https://lore.kernel.org/linux-xfs/CAEJPjCvT3Uag-pMTYuigEjWZHn1sGMZ0GCjVVCv29tNHK76Cgg@mail.gmail.com0/

which uses reserved blocks to create file systems that are gravely
out of space and thus cause at least xfs_file_alloc_space to hang
and trigger the lack of ENOSPC handling in xfs_dquot_disk_alloc.

Note that this patch does not actually make any caller but
xfs_alloc_file_space deal intelligently with case 2) above.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reported-by: 刘通 <lyutoon@gmail.com>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Signed-off-by: Chandan Babu R <chandanbabu@kernel.org>
fs/xfs/libxfs/xfs_attr_remote.c
fs/xfs/libxfs/xfs_bmap.c
fs/xfs/libxfs/xfs_da_btree.c
fs/xfs/scrub/quota_repair.c
fs/xfs/scrub/rtbitmap_repair.c
fs/xfs/xfs_bmap_util.c
fs/xfs/xfs_dquot.c
fs/xfs/xfs_iomap.c
fs/xfs/xfs_reflink.c
fs/xfs/xfs_rtalloc.c

index a8de9dc1e998a338917153ce3a0b3dccbefc7779..beb0efdd8f6b8382af75d42982891e91e9b9f0de 100644 (file)
@@ -625,7 +625,6 @@ xfs_attr_rmtval_set_blk(
        if (error)
                return error;
 
-       ASSERT(nmap == 1);
        ASSERT((map->br_startblock != DELAYSTARTBLOCK) &&
               (map->br_startblock != HOLESTARTBLOCK));
 
index 49e2fcad593df39f5b1fffd96801f51d39d3d37b..14c9781ec0ce75e59c99feacca8e805954d96199 100644 (file)
@@ -4217,8 +4217,10 @@ xfs_bmapi_allocate(
        } else {
                error = xfs_bmap_alloc_userdata(bma);
        }
-       if (error || bma->blkno == NULLFSBLOCK)
+       if (error)
                return error;
+       if (bma->blkno == NULLFSBLOCK)
+               return -ENOSPC;
 
        if (bma->flags & XFS_BMAPI_ZERO) {
                error = xfs_zero_extent(bma->ip, bma->blkno, bma->length);
@@ -4397,6 +4399,15 @@ xfs_bmapi_finish(
  * extent state if necessary.  Details behaviour is controlled by the flags
  * parameter.  Only allocates blocks from a single allocation group, to avoid
  * locking problems.
+ *
+ * Returns 0 on success and places the extent mappings in mval.  nmaps is used
+ * as an input/output parameter where the caller specifies the maximum number
+ * of mappings that may be returned and xfs_bmapi_write passes back the number
+ * of mappings (including existing mappings) it found.
+ *
+ * Returns a negative error code on failure, including -ENOSPC when it could not
+ * allocate any blocks and -ENOSR when it did allocate blocks to convert a
+ * delalloc range, but those blocks were before the passed in range.
  */
 int
 xfs_bmapi_write(
@@ -4525,10 +4536,16 @@ xfs_bmapi_write(
                        ASSERT(len > 0);
                        ASSERT(bma.length > 0);
                        error = xfs_bmapi_allocate(&bma);
-                       if (error)
+                       if (error) {
+                               /*
+                                * If we already allocated space in a previous
+                                * iteration return what we go so far when
+                                * running out of space.
+                                */
+                               if (error == -ENOSPC && bma.nallocs)
+                                       break;
                                goto error0;
-                       if (bma.blkno == NULLFSBLOCK)
-                               break;
+                       }
 
                        /*
                         * If this is a CoW allocation, record the data in
@@ -4566,7 +4583,6 @@ xfs_bmapi_write(
                if (!xfs_iext_next_extent(ifp, &bma.icur, &bma.got))
                        eof = true;
        }
-       *nmap = n;
 
        error = xfs_bmap_btree_to_extents(tp, ip, bma.cur, &bma.logflags,
                        whichfork);
@@ -4577,7 +4593,22 @@ xfs_bmapi_write(
               ifp->if_nextents > XFS_IFORK_MAXEXT(ip, whichfork));
        xfs_bmapi_finish(&bma, whichfork, 0);
        xfs_bmap_validate_ret(orig_bno, orig_len, orig_flags, orig_mval,
-               orig_nmap, *nmap);
+               orig_nmap, n);
+
+       /*
+        * When converting delayed allocations, xfs_bmapi_allocate ignores
+        * the passed in bno and always converts from the start of the found
+        * delalloc extent.
+        *
+        * To avoid a successful return with *nmap set to 0, return the magic
+        * -ENOSR error code for this particular case so that the caller can
+        * handle it.
+        */
+       if (!n) {
+               ASSERT(bma.nallocs >= *nmap);
+               return -ENOSR;
+       }
+       *nmap = n;
        return 0;
 error0:
        xfs_bmapi_finish(&bma, whichfork, error);
@@ -4685,9 +4716,6 @@ xfs_bmapi_convert_one_delalloc(
        if (error)
                goto out_finish;
 
-       error = -ENOSPC;
-       if (WARN_ON_ONCE(bma.blkno == NULLFSBLOCK))
-               goto out_finish;
        if (WARN_ON_ONCE(!xfs_valid_startblock(ip, bma.got.br_startblock))) {
                xfs_bmap_mark_sick(ip, whichfork);
                error = -EFSCORRUPTED;
index b13796629e22134abbf5d76ccb79973dd77ddcb4..16a529a8878083aee350d746c2d98fd307b83cef 100644 (file)
@@ -2297,8 +2297,8 @@ xfs_da_grow_inode_int(
        struct xfs_inode        *dp = args->dp;
        int                     w = args->whichfork;
        xfs_rfsblock_t          nblks = dp->i_nblocks;
-       struct xfs_bmbt_irec    map, *mapp;
-       int                     nmap, error, got, i, mapi;
+       struct xfs_bmbt_irec    map, *mapp = &map;
+       int                     nmap, error, got, i, mapi = 1;
 
        /*
         * Find a spot in the file space to put the new block.
@@ -2314,14 +2314,7 @@ xfs_da_grow_inode_int(
        error = xfs_bmapi_write(tp, dp, *bno, count,
                        xfs_bmapi_aflag(w)|XFS_BMAPI_METADATA|XFS_BMAPI_CONTIG,
                        args->total, &map, &nmap);
-       if (error)
-               return error;
-
-       ASSERT(nmap <= 1);
-       if (nmap == 1) {
-               mapp = &map;
-               mapi = 1;
-       } else if (nmap == 0 && count > 1) {
+       if (error == -ENOSPC && count > 1) {
                xfs_fileoff_t           b;
                int                     c;
 
@@ -2339,16 +2332,13 @@ xfs_da_grow_inode_int(
                                        args->total, &mapp[mapi], &nmap);
                        if (error)
                                goto out_free_map;
-                       if (nmap < 1)
-                               break;
                        mapi += nmap;
                        b = mapp[mapi - 1].br_startoff +
                            mapp[mapi - 1].br_blockcount;
                }
-       } else {
-               mapi = 0;
-               mapp = NULL;
        }
+       if (error)
+               goto out_free_map;
 
        /*
         * Count the blocks we got, make sure it matches the total.
index 0bab4c30cb85ab8a87b7792e7daae1c48f2a75aa..90cd1512bba96103c7f7517bd98525ba26ba62cd 100644 (file)
@@ -77,8 +77,6 @@ xrep_quota_item_fill_bmap_hole(
                        irec, &nmaps);
        if (error)
                return error;
-       if (nmaps != 1)
-               return -ENOSPC;
 
        dq->q_blkno = XFS_FSB_TO_DADDR(mp, irec->br_startblock);
 
@@ -444,10 +442,6 @@ xrep_quota_data_fork(
                                        XFS_BMAPI_CONVERT, 0, &nrec, &nmap);
                        if (error)
                                goto out;
-                       if (nmap != 1) {
-                               error = -ENOSPC;
-                               goto out;
-                       }
                        ASSERT(nrec.br_startoff == irec.br_startoff);
                        ASSERT(nrec.br_blockcount == irec.br_blockcount);
 
index 46f5d5f605c915684c61ee442a0261adbac3f671..0fef98e9f83409ce9fb666806a2b9b74c8963ab8 100644 (file)
@@ -108,8 +108,6 @@ xrep_rtbitmap_data_mappings(
                                0, &map, &nmaps);
                if (error)
                        return error;
-               if (nmaps != 1)
-                       return -EFSCORRUPTED;
 
                /* Commit new extent and all deferred work. */
                error = xrep_defer_finish(sc);
index 53aa90a0ee3a857eb00090ace1947b5667e82eaf..2e6f08198c0719becb3e111eb48251760fd66f84 100644 (file)
@@ -721,33 +721,32 @@ xfs_alloc_file_space(
                if (error)
                        goto error;
 
-               error = xfs_bmapi_write(tp, ip, startoffset_fsb,
-                               allocatesize_fsb, XFS_BMAPI_PREALLOC, 0, imapp,
-                               &nimaps);
-               if (error)
-                       goto error;
-
-               ip->i_diflags |= XFS_DIFLAG_PREALLOC;
-               xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
-
-               error = xfs_trans_commit(tp);
-               xfs_iunlock(ip, XFS_ILOCK_EXCL);
-               if (error)
-                       break;
-
                /*
                 * If the allocator cannot find a single free extent large
                 * enough to cover the start block of the requested range,
-                * xfs_bmapi_write will return 0 but leave *nimaps set to 0.
+                * xfs_bmapi_write will return -ENOSR.
                 *
                 * In that case we simply need to keep looping with the same
                 * startoffset_fsb so that one of the following allocations
                 * will eventually reach the requested range.
                 */
-               if (nimaps) {
+               error = xfs_bmapi_write(tp, ip, startoffset_fsb,
+                               allocatesize_fsb, XFS_BMAPI_PREALLOC, 0, imapp,
+                               &nimaps);
+               if (error) {
+                       if (error != -ENOSR)
+                               goto error;
+                       error = 0;
+               } else {
                        startoffset_fsb += imapp->br_blockcount;
                        allocatesize_fsb -= imapp->br_blockcount;
                }
+
+               ip->i_diflags |= XFS_DIFLAG_PREALLOC;
+               xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+
+               error = xfs_trans_commit(tp);
+               xfs_iunlock(ip, XFS_ILOCK_EXCL);
        }
 
        return error;
index 13aba84bd64afbd99b4f4eff9ddfac01b7e7a960..43acb4f0d174330910982ec5a15e06977f43a8dc 100644 (file)
@@ -357,7 +357,6 @@ xfs_dquot_disk_alloc(
                goto err_cancel;
 
        ASSERT(map.br_blockcount == XFS_DQUOT_CLUSTER_SIZE_FSB);
-       ASSERT(nmaps == 1);
        ASSERT((map.br_startblock != DELAYSTARTBLOCK) &&
               (map.br_startblock != HOLESTARTBLOCK));
 
index ec43aa7cec603134b30163b5e6f93ed7f664ad90..f5d0ed45721b5c813383643b0406ade2aebed9a0 100644 (file)
@@ -322,14 +322,6 @@ xfs_iomap_write_direct(
        if (error)
                goto out_unlock;
 
-       /*
-        * Copy any maps to caller's array and return any error.
-        */
-       if (nimaps == 0) {
-               error = -ENOSPC;
-               goto out_unlock;
-       }
-
        if (unlikely(!xfs_valid_startblock(ip, imap->br_startblock))) {
                xfs_bmap_mark_sick(ip, XFS_DATA_FORK);
                error = xfs_alert_fsblock_zero(ip, imap);
index 7da0e8f961d351183eefa996e13affda1d0a635a..5ecb52a234becc5bda005f1f4cbb2c182959c257 100644 (file)
@@ -430,13 +430,6 @@ xfs_reflink_fill_cow_hole(
        if (error)
                return error;
 
-       /*
-        * Allocation succeeded but the requested range was not even partially
-        * satisfied?  Bail out!
-        */
-       if (nimaps == 0)
-               return -ENOSPC;
-
 convert:
        return xfs_reflink_convert_unwritten(ip, imap, cmap, convert_now);
 
@@ -499,13 +492,6 @@ xfs_reflink_fill_delalloc(
                error = xfs_trans_commit(tp);
                if (error)
                        return error;
-
-               /*
-                * Allocation succeeded but the requested range was not even
-                * partially satisfied?  Bail out!
-                */
-               if (nimaps == 0)
-                       return -ENOSPC;
        } while (cmap->br_startoff + cmap->br_blockcount <= imap->br_startoff);
 
        return xfs_reflink_convert_unwritten(ip, imap, cmap, convert_now);
index b476a876478d93f61eb5018143ae3053588dc25c..150f544445ca8233a23aa6eb15367fda3d7ba4f4 100644 (file)
@@ -709,8 +709,6 @@ xfs_growfs_rt_alloc(
                nmap = 1;
                error = xfs_bmapi_write(tp, ip, oblocks, nblocks - oblocks,
                                        XFS_BMAPI_METADATA, 0, &map, &nmap);
-               if (!error && nmap < 1)
-                       error = -ENOSPC;
                if (error)
                        goto out_trans_cancel;
                /*