Re: [RESEND PATCH V2] common/populate: Ensure that S_IFDIR.FMT_BTREE is in btree format

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Dec 06, 2022 at 11:13:10AM +0800, Ziyang Zhang wrote:
> On 2022/12/6 05:51, Darrick J. Wong wrote:
> > On Sat, Dec 03, 2022 at 12:26:57AM +0000, Allison Henderson wrote:
> >> On Fri, 2022-12-02 at 19:27 +0800, Ziyang Zhang wrote:
> >>> Sometimes "$((128 * dblksz / 40))" dirents cannot make sure that
> >>> S_IFDIR.FMT_BTREE could become btree format for its DATA fork.
> >>>
> >>> Actually we just observed it can fail after apply our inode
> >>> extent-to-btree workaround. The root cause is that the kernel may be
> >>> too good at allocating consecutive blocks so that the data fork is
> >>> still in extents format.
> >>>
> >>> Therefore instead of using a fixed number, let's make sure the number
> >>> of extents is large enough than (inode size - inode core size) /
> >>> sizeof(xfs_bmbt_rec_t).
> >>>
> >>> Suggested-by: "Darrick J. Wong" <djwong@kernel.org>
> >>> Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> >>> Signed-off-by: Ziyang Zhang <ZiyangZhang@linux.alibaba.com>
> >>
> >> New version looks much cleaner.  
> >> Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> >>
> >>> ---
> >>> V2: take Darrick's advice to cleanup code
> >>>  common/populate | 28 +++++++++++++++++++++++++++-
> >>>  common/xfs      | 17 +++++++++++++++++
> >>>  2 files changed, 44 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/common/populate b/common/populate
> >>> index 6e004997..1ca76459 100644
> >>> --- a/common/populate
> >>> +++ b/common/populate
> >>> @@ -71,6 +71,31 @@ __populate_create_dir() {
> >>>         done
> >>>  }
> >>>  
> >>> +# Create a large directory and ensure that it's a btree format
> >>> +__populate_xfs_create_btree_dir() {
> >>> +       local name="$1"
> >>> +       local isize="$2"
> >>> +       local icore_size="$(_xfs_inode_core_bytes)"
> >>> +       # We need enough extents to guarantee that the data fork is
> >>> in
> >>> +       # btree format.  Cycling the mount to use xfs_db is too slow,
> >>> so
> >>> +       # watch for when the extent count exceeds the space after the
> >>> +       # inode core.
> >>> +       local max_nextents="$(((isize - icore_size) / 16))"
> >>> +
> >>> +       mkdir -p "${name}"
> >>> +       d=0
> >>> +       while true; do
> >>> +               creat=mkdir
> >>> +               test "$((d % 20))" -eq 0 && creat=touch
> >>> +               $creat "${name}/$(printf "%.08d" "$d")"
> >>> +               if [ "$((d % 40))" -eq 0 ]; then
> >>> +                       nextents="$(_xfs_get_fsxattr nextents $name)"
> >>> +                       [ $nextents -gt $max_nextents ] && break
> >>> +               fi
> >>> +               d=$((d+1))
> >>> +       done
> >>> +}
> >>> +
> >>>  # Add a bunch of attrs to a file
> >>>  __populate_create_attr() {
> >>>         name="$1"
> >>> @@ -176,6 +201,7 @@ _scratch_xfs_populate() {
> >>>  
> >>>         blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> >>>         dblksz="$(_xfs_get_dir_blocksize "$SCRATCH_MNT")"
> >>> +       isize="$(_xfs_inode_size "$SCRATCH_MNT")"
> >>>         crc="$(_xfs_has_feature "$SCRATCH_MNT" crc -v)"
> >>>         if [ $crc -eq 1 ]; then
> >>>                 leaf_hdr_size=64
> >>> @@ -226,7 +252,7 @@ _scratch_xfs_populate() {
> >>>  
> >>>         # - BTREE
> >>>         echo "+ btree dir"
> >>> -       __populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_BTREE"
> >>> "$((128 * dblksz / 40))" true
> >>> +       __populate_xfs_create_btree_dir
> >>> "${SCRATCH_MNT}/S_IFDIR.FMT_BTREE" "$isize"
> >>>  
> >>>         # Symlinks
> >>>         # - FMT_LOCAL
> >>> diff --git a/common/xfs b/common/xfs
> >>> index 8ac1964e..0359e422 100644
> >>> --- a/common/xfs
> >>> +++ b/common/xfs
> >>> @@ -1486,3 +1486,20 @@ _require_xfsrestore_xflag()
> >>>         $XFSRESTORE_PROG -h 2>&1 | grep -q -e '-x' || \
> >>>                         _notrun 'xfsrestore does not support -x
> >>> flag.'
> >>>  }
> >>> +
> >>> +
> >>> +# Number of bytes reserved for a full inode record, which includes
> >>> the
> >>> +# immediate fork areas.
> >>> +_xfs_inode_size()
> >>> +{
> >>> +       local mntpoint="$1"
> >>> +
> >>> +       $XFS_INFO_PROG "$mntpoint" | grep 'meta-data=.*isize' | sed -
> >>> e 's/^.*isize=\([0-9]*\).*$/\1/g'
> >>> +}
> >>> +
> >>> +# Number of bytes reserved for only the inode record, excluding the
> >>> +# immediate fork areas.
> >>> +_xfs_inode_core_bytes()
> >>> +{
> >>> +       echo 176
> >>> +}
> > 
> > Please refactor all the other users:
> > 
> > $ git grep -w isize.*176
> > tests/xfs/335:34:i_ptrs=$(( (isize - 176) / 56 ))
> > tests/xfs/336:45:i_ptrs=$(( (isize - 176) / 56 ))
> > tests/xfs/337:36:i_ptrs=$(( (isize - 176) / 56 ))
> > tests/xfs/341:36:i_ptrs=$(( (isize - 176) / 48 ))
> > tests/xfs/342:33:i_ptrs=$(( (isize - 176) / 56 ))
> > 
> > (I'll test out this patch and try to do the same for ATTR.FMT_BTREE in
> > the meantime)
> 
> Hi, Darrick
> 
> Should I create a new patch to refactor these test cases
> or refactor them just in this patch?
> 
> Looks like these test cases just need _xfs_inode_core_bytes()
> and commit message of this patch is not written for them.

Depends on what Zorro says, but yes, usually we'd want to refactor the
open-coded logic into a helper in a single patch, and after that comes
the bugfix.

--D

> Regards,
> Zhang