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@xxxxxxxxxx>
> >>> Signed-off-by: Gao Xiang <hsiangkao@xxxxxxxxxxxxxxxxx>
> >>> Signed-off-by: Ziyang Zhang <ZiyangZhang@xxxxxxxxxxxxxxxxx>
> >>
> >> New version looks much cleaner.
> >> Reviewed-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> >>
> >>> ---
> >>> 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
