Re: [PATCH V13 00/16] Bail out if transaction can cause extent count to overflow

[Chandan Babu R <chandanrlinux@gmail.com>]

The changes in the patch "[PATCH V13 05/16] xfs: Check for extent overflow
when removing dir entries" are not correct. I noticed this only after sending
the patchset. The correct change should be,

       error = xfs_iext_count_may_overflow(ip, whichfork, 1);
       if (error) {
               ASSERT(S_ISDIR(VFS_I(ip)->i_mode) &&
                       whichfork == XFS_DATA_FORK);
               error = -ENOSPC;
               goto done;
       }

Hence please ignore this patchset. I will send the fixed version soon. Sorry
about the noise.

On 10 Jan 2021 at 08:59, Chandan Babu R wrote:
> XFS does not check for possible overflow of per-inode extent counter
> fields when adding extents to either data or attr fork.
>
> For e.g.
> 1. Insert 5 million xattrs (each having a value size of 255 bytes) and
>    then delete 50% of them in an alternating manner.
>
> 2. On a 4k block sized XFS filesystem instance, the above causes 98511
>    extents to be created in the attr fork of the inode.
>
>    xfsaild/loop0  2008 [003]  1475.127209: probe:xfs_inode_to_disk: (ffffffffa43fb6b0) if_nextents=98511 i_ino=131
>
> 3. The incore inode fork extent counter is a signed 32-bit
>    quantity. However, the on-disk extent counter is an unsigned 16-bit
>    quantity and hence cannot hold 98511 extents.
>
> 4. The following incorrect value is stored in the xattr extent counter,
>    # xfs_db -f -c 'inode 131' -c 'print core.naextents' /dev/loop0
>    core.naextents = -32561
>
> This patchset adds a new helper function
> (i.e. xfs_iext_count_may_overflow()) to check for overflow of the
> per-inode data and xattr extent counters and invokes it before
> starting an fs operation (e.g. creating a new directory entry). With
> this patchset applied, XFS detects counter overflows and returns with
> an error rather than causing a silent corruption.
>
> The patchset has been tested by executing xfstests with the following
> mkfs.xfs options,
> 1. -m crc=0 -b size=1k
> 2. -m crc=0 -b size=4k
> 3. -m crc=0 -b size=512
> 4. -m rmapbt=1,reflink=1 -b size=1k
> 5. -m rmapbt=1,reflink=1 -b size=4k
>
> The patches can also be obtained from
> https://github.com/chandanr/linux.git at branch xfs-reserve-extent-count-v13.
>
> I have two patches that define the newly introduced error injection
> tags in xfsprogs
> (https://lore.kernel.org/linux-xfs/20201104114900.172147-1-chandanrlinux@gmail.com/).
>
> I have also written tests
> (https://github.com/chandanr/xfstests/commits/extent-overflow-tests)
> for verifying the checks introduced in the kernel.
>
> Changelog:
> V12 -> V13:
>   1. xfs_rename():
>      - Add comment explaining why we do not check for extent count
>        overflow for the source directory entry of a rename operation.
>      - Fix grammatical nit in a comment.
>   2. xfs_bmap_del_extent_real():
>      Replace explicit checks for inode's mode and fork with an
>      assert() call since extent count overflow check here is
>      applicable only to directory entry remove/rename operation.
>
> V11 -> V12:
>   1. Rebase patches on top of Linux v5.11-rc1.
>   2. Revert back to using using a pseudo max inode extent count of 10.
>      Hence the patches
>      - [PATCH V12 05/14] xfs: Check for extent overflow when adding/removing xattrs
>      - [PATCH V12 10/14] xfs: Introduce error injection to reduce maximum
>      have been reverted back (including retaining of corresponding RVB
>      tags) to how it was under V10 of the patchset.
>
>      V11 of the patchset had increased the max pseudo extent count to
>      35 to allow for "directory entry remove" operation to always
>      succeed. However the corresponding logic was incorrect. Please
>      refer to "[PATCH V12 04/14] xfs: Check for extent overflow when
>      adding/removing dir entries" to find logic and explaination of
>      the newer logic.
>
>      "[PATCH V12 04/14] xfs: Check for extent overflow when
>      adding/removing dir entries" is the only patch yet to be reviewed.
>
> V10 -> V11:
>   1. For directory/xattr insert operations we now reserve sufficient
>      number of "extent count" so as to guarantee a future
>      directory/xattr remove operation.
>   2. The pseudo max extent count value has been increased to 35.
>
> V9 -> V10:
>   1. Pull back changes which cause xfs_bmap_compute_alignments() to
>      return "stripe alignment" into 12th patch i.e. "xfs: Compute bmap
>      extent alignments in a separate function".
>
> V8 -> V9:
>   1. Enabling XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT error tag will
>      always allocate single block sized free extents (if
>      available).
>   2. xfs_bmap_compute_alignments() now returns stripe alignment as its
>      return value.
>   3. Dropped Allison's RVB tag for "xfs: Compute bmap extent
>      alignments in a separate function" and "xfs: Introduce error
>      injection to allocate only minlen size extents for files".
>
> V7 -> V8:
>   1. Rename local variable in xfs_alloc_fix_freelist() from "i" to "stat".
>
> V6 -> V7:
>   1. Create new function xfs_bmap_exact_minlen_extent_alloc() (enabled
>      only when CONFIG_XFS_DEBUG is set to y) which issues allocation
>      requests for minlen sized extents only. In order to achieve this,
>      common code from xfs_bmap_btalloc() have been refactored into new
>      functions.
>   2. All major functions implementing logic associated with
>      XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT error tag are compiled only
>      when CONFIG_XFS_DEBUG is set to y.
>   3. Remove XFS_IEXT_REFLINK_REMAP_CNT macro and replace it with an
>      integer which holds the number of new extents to be
>      added to the data fork.
>
> V5 -> V6:
>   1. Rebased the patchset on xfs-linux/for-next branch.
>   2. Drop "xfs: Set tp->t_firstblock only once during a transaction's
>      lifetime" patch from the patchset.
>   3. Add a comment to xfs_bmap_btalloc() describing why it was chosen
>      to start "free space extent search" from AG 0 when
>      XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT is enabled and when the
>      transaction is allocating its first extent.
>   4. Fix review comments associated with coding style.
>
> V4 -> V5:
>   1. Introduce new error tag XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT to
>      let user space programs to be able to guarantee that free space
>      requests for files are satisfied by allocating minlen sized
>      extents.
>   2. Change xfs_bmap_btalloc() and xfs_alloc_vextent() to allocate
>      minlen sized extents when XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT is
>      enabled.
>   3. Introduce a new patch that causes tp->t_firstblock to be assigned
>      to a value only when its previous value is NULLFSBLOCK.
>   4. Replace the previously introduced MAXERRTAGEXTNUM (maximum inode
>      fork extent count) with the hardcoded value of 10.
>   5. xfs_bui_item_recover(): Use XFS_IEXT_ADD_NOSPLIT_CNT when mapping
>      an extent.
>   6. xfs_swap_extent_rmap(): Use xfs_bmap_is_real_extent() instead of
>      xfs_bmap_is_update_needed() to assess if the extent really needs
>      to be swapped.
>
> V3 -> V4:
>   1. Introduce new patch which lets userspace programs to test "extent
>      count overflow detection" by injecting an error tag. The new
>      error tag reduces the maximum allowed extent count to 10.
>   2. Injecting the newly defined error tag prevents
>      xfs_bmap_add_extent_hole_real() from merging a new extent with
>      its neighbours to allow writing deterministic tests for testing
>      extent count overflow for Directories, Xattr and growing realtime
>      devices. This is required because the new extent being allocated
>      can be contiguous with its neighbours (w.r.t both file and disk
>      offsets).
>   3. Injecting the newly defined error tag forces block sized extents
>      to be allocated for summary/bitmap files when growing a realtime
>      device. This is required because xfs_growfs_rt_alloc() allocates
>      as large an extent as possible for summary/bitmap files and hence
>      it would be impossible to write deterministic tests.
>   4. Rename XFS_IEXT_REMOVE_CNT to XFS_IEXT_PUNCH_HOLE_CNT to reflect
>      the actual meaning of the fs operation.
>   5. Fold XFS_IEXT_INSERT_HOLE_CNT code into that associated with
>      XFS_IEXT_PUNCH_HOLE_CNT since both perform the same job.
>   6. xfs_swap_extent_rmap(): Check for extent overflow should be made
>      on the source file only if the donor file extent has a valid
>      on-disk mapping and vice versa.
>
> V2 -> V3:
>   1. Move the definition of xfs_iext_count_may_overflow() from
>      libxfs/xfs_trans_resv.c to libxfs/xfs_inode_fork.c. Also, I tried
>      to make xfs_iext_count_may_overflow() an inline function by
>      placing the definition in libxfs/xfs_inode_fork.h. However this
>      required that the definition of 'struct xfs_inode' be available,
>      since xfs_iext_count_may_overflow() uses a 'struct xfs_inode *'
>      type variable.
>   2. Handle XFS_COW_FORK within xfs_iext_count_may_overflow() by
>      returning a success value.
>   3. Rename XFS_IEXT_ADD_CNT to XFS_IEXT_ADD_NOSPLIT_CNT. Thanks to
>      Darrick for the suggesting the new name.
>   4. Expand comments to make use of 80 columns.
>
> V1 -> V2:
>   1. Rename helper function from xfs_trans_resv_ext_cnt() to
>      xfs_iext_count_may_overflow().
>   2. Define and use macros to represent fs operations and the
>      corresponding increase in extent count.
>   3. Split the patches based on the fs operation being performed.
>
> Chandan Babu R (16):
>   xfs: Add helper for checking per-inode extent count overflow
>   xfs: Check for extent overflow when trivally adding a new extent
>   xfs: Check for extent overflow when punching a hole
>   xfs: Check for extent overflow when adding dir entries
>   xfs: Check for extent overflow when removing dir entries
>   xfs: Check for extent overflow when renaming dir entries
>   xfs: Check for extent overflow when adding/removing xattrs
>   xfs: Check for extent overflow when writing to unwritten extent
>   xfs: Check for extent overflow when moving extent from cow to data
>     fork
>   xfs: Check for extent overflow when remapping an extent
>   xfs: Check for extent overflow when swapping extents
>   xfs: Introduce error injection to reduce maximum inode fork extent
>     count
>   xfs: Remove duplicate assert statement in xfs_bmap_btalloc()
>   xfs: Compute bmap extent alignments in a separate function
>   xfs: Process allocated extent in a separate function
>   xfs: Introduce error injection to allocate only minlen size extents
>     for files
>
>  fs/xfs/libxfs/xfs_alloc.c      |  50 ++++++
>  fs/xfs/libxfs/xfs_alloc.h      |   3 +
>  fs/xfs/libxfs/xfs_attr.c       |  13 ++
>  fs/xfs/libxfs/xfs_bmap.c       | 284 ++++++++++++++++++++++++---------
>  fs/xfs/libxfs/xfs_errortag.h   |   6 +-
>  fs/xfs/libxfs/xfs_inode_fork.c |  27 ++++
>  fs/xfs/libxfs/xfs_inode_fork.h |  63 ++++++++
>  fs/xfs/xfs_bmap_item.c         |  10 ++
>  fs/xfs/xfs_bmap_util.c         |  31 ++++
>  fs/xfs/xfs_dquot.c             |   8 +-
>  fs/xfs/xfs_error.c             |   6 +
>  fs/xfs/xfs_inode.c             |  54 ++++++-
>  fs/xfs/xfs_iomap.c             |  10 ++
>  fs/xfs/xfs_reflink.c           |  16 ++
>  fs/xfs/xfs_rtalloc.c           |   5 +
>  fs/xfs/xfs_symlink.c           |   5 +
>  16 files changed, 512 insertions(+), 79 deletions(-)


--
chandan