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.
Regards,
Zhang
