* [PATCH V4 0/2] cleanup and bugfix for xfs tests related to btree format
@ 2022-12-07 9:31 Ziyang Zhang
2022-12-07 9:31 ` [PATCH V4 1/2] common/xfs: Add a helper to export inode core size Ziyang Zhang
2022-12-07 9:31 ` [PATCH V4 2/2] common/populate: Ensure that S_IFDIR.FMT_BTREE is in btree format Ziyang Zhang
0 siblings, 2 replies; 6+ messages in thread
From: Ziyang Zhang @ 2022-12-07 9:31 UTC (permalink / raw)
To: fstests, linux-xfs; +Cc: djwong, hsiangkao, allison.henderson, Ziyang Zhang
The first patch add a helper in common/xfs to export the inode core size
which is needed by some xfs test cases.
The second patch ensure that S_IFDIR.FMT_BTREE is in btree format while
populating dir.
V4:
- let the new helper function accept the "missing" parameter
- make _xfs_inode_core_bytes echo 176 or 96 so tests can work correctly on
both v4 and v5
V3:
- refactor xfs tests cases using inode core size
V2:
- take Darrick's advice to cleanup code
Ziyang Zhang (2):
common/xfs: Add a helper to export inode core size
common/populate: Ensure that S_IFDIR.FMT_BTREE is in btree format
common/populate | 34 +++++++++++++++++++++++++++++++++-
common/xfs | 24 ++++++++++++++++++++++++
tests/xfs/335 | 3 ++-
tests/xfs/336 | 3 ++-
tests/xfs/337 | 3 ++-
tests/xfs/341 | 3 ++-
tests/xfs/342 | 3 ++-
7 files changed, 67 insertions(+), 6 deletions(-)
--
2.18.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V4 1/2] common/xfs: Add a helper to export inode core size
2022-12-07 9:31 [PATCH V4 0/2] cleanup and bugfix for xfs tests related to btree format Ziyang Zhang
@ 2022-12-07 9:31 ` Ziyang Zhang
2022-12-07 18:06 ` Zorro Lang
2022-12-07 9:31 ` [PATCH V4 2/2] common/populate: Ensure that S_IFDIR.FMT_BTREE is in btree format Ziyang Zhang
1 sibling, 1 reply; 6+ messages in thread
From: Ziyang Zhang @ 2022-12-07 9:31 UTC (permalink / raw)
To: fstests, linux-xfs; +Cc: djwong, hsiangkao, allison.henderson, Ziyang Zhang
Some xfs test cases need the number of bytes reserved for only the inode
record, excluding the immediate fork areas. Now the value is hard-coded
and it is not a good chioce. Add a helper in common/xfs to export the
inode core size.
Signed-off-by: Ziyang Zhang <ZiyangZhang@linux.alibaba.com>
---
common/xfs | 15 +++++++++++++++
tests/xfs/335 | 3 ++-
tests/xfs/336 | 3 ++-
tests/xfs/337 | 3 ++-
tests/xfs/341 | 3 ++-
tests/xfs/342 | 3 ++-
6 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/common/xfs b/common/xfs
index 8ac1964e..5074c350 100644
--- a/common/xfs
+++ b/common/xfs
@@ -1486,3 +1486,18 @@ _require_xfsrestore_xflag()
$XFSRESTORE_PROG -h 2>&1 | grep -q -e '-x' || \
_notrun 'xfsrestore does not support -x flag.'
}
+
+# Number of bytes reserved for only the inode record, excluding the
+# immediate fork areas.
+_xfs_inode_core_bytes()
+{
+ local dir="$1"
+
+ if _xfs_has_feature "$dir" crc; then
+ # v5 filesystems
+ echo 176
+ else
+ # v4 filesystems
+ echo 96
+ fi
+}
diff --git a/tests/xfs/335 b/tests/xfs/335
index ccc508e7..2fd9be54 100755
--- a/tests/xfs/335
+++ b/tests/xfs/335
@@ -31,7 +31,8 @@ blksz="$(_get_block_size $SCRATCH_MNT)"
echo "Create a three-level rtrmapbt"
# inode core size is at least 176 bytes; btree header is 56 bytes;
# rtrmap record is 32 bytes; and rtrmap key/pointer are 56 bytes.
-i_ptrs=$(( (isize - 176) / 56 ))
+i_core_size="$(_xfs_inode_core_bytes $SCRATCH_MNT)"
+i_ptrs=$(( (isize - i_core_size) / 56 ))
bt_ptrs=$(( (blksz - 56) / 56 ))
bt_recs=$(( (blksz - 56) / 32 ))
diff --git a/tests/xfs/336 b/tests/xfs/336
index b1de8e5f..085b43ae 100755
--- a/tests/xfs/336
+++ b/tests/xfs/336
@@ -42,7 +42,8 @@ rm -rf $metadump_file
echo "Create a three-level rtrmapbt"
# inode core size is at least 176 bytes; btree header is 56 bytes;
# rtrmap record is 32 bytes; and rtrmap key/pointer are 56 bytes.
-i_ptrs=$(( (isize - 176) / 56 ))
+i_core_size="$(_xfs_inode_core_bytes $SCRATCH_MNT)"
+i_ptrs=$(( (isize - i_core_size) / 56 ))
bt_ptrs=$(( (blksz - 56) / 56 ))
bt_recs=$(( (blksz - 56) / 32 ))
diff --git a/tests/xfs/337 b/tests/xfs/337
index a2515e36..5d6b9964 100755
--- a/tests/xfs/337
+++ b/tests/xfs/337
@@ -33,7 +33,8 @@ blksz="$(_get_block_size $SCRATCH_MNT)"
# inode core size is at least 176 bytes; btree header is 56 bytes;
# rtrmap record is 32 bytes; and rtrmap key/pointer are 56 bytes.
-i_ptrs=$(( (isize - 176) / 56 ))
+i_core_size="$(_xfs_inode_core_bytes $SCRATCH_MNT)"
+i_ptrs=$(( (isize - i_core_size) / 56 ))
bt_ptrs=$(( (blksz - 56) / 56 ))
bt_recs=$(( (blksz - 56) / 32 ))
diff --git a/tests/xfs/341 b/tests/xfs/341
index f026aa37..c70dec3c 100755
--- a/tests/xfs/341
+++ b/tests/xfs/341
@@ -33,7 +33,8 @@ rtextsz_blks=$((rtextsz / blksz))
# inode core size is at least 176 bytes; btree header is 56 bytes;
# rtrmap record is 32 bytes; and rtrmap key/pointer are 56 bytes.
-i_ptrs=$(( (isize - 176) / 56 ))
+i_core_size="$(_xfs_inode_core_bytes $SCRATCH_MNT)"
+i_ptrs=$(( (isize - i_core_size) / 56 ))
bt_recs=$(( (blksz - 56) / 32 ))
blocks=$((i_ptrs * bt_recs + 1))
diff --git a/tests/xfs/342 b/tests/xfs/342
index 1ae414eb..4855627f 100755
--- a/tests/xfs/342
+++ b/tests/xfs/342
@@ -30,7 +30,8 @@ blksz="$(_get_block_size $SCRATCH_MNT)"
# inode core size is at least 176 bytes; btree header is 56 bytes;
# rtrmap record is 32 bytes; and rtrmap key/pointer are 56 bytes.
-i_ptrs=$(( (isize - 176) / 56 ))
+i_core_size="$(_xfs_inode_core_bytes $SCRATCH_MNT)"
+i_ptrs=$(( (isize - i_core_size) / 56 ))
bt_recs=$(( (blksz - 56) / 32 ))
blocks=$((i_ptrs * bt_recs + 1))
--
2.18.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH V4 2/2] common/populate: Ensure that S_IFDIR.FMT_BTREE is in btree format
2022-12-07 9:31 [PATCH V4 0/2] cleanup and bugfix for xfs tests related to btree format Ziyang Zhang
2022-12-07 9:31 ` [PATCH V4 1/2] common/xfs: Add a helper to export inode core size Ziyang Zhang
@ 2022-12-07 9:31 ` Ziyang Zhang
2022-12-07 18:28 ` Zorro Lang
1 sibling, 1 reply; 6+ messages in thread
From: Ziyang Zhang @ 2022-12-07 9:31 UTC (permalink / raw)
To: fstests, linux-xfs; +Cc: djwong, hsiangkao, allison.henderson, Ziyang Zhang
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>
---
common/populate | 34 +++++++++++++++++++++++++++++++++-
common/xfs | 9 +++++++++
2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/common/populate b/common/populate
index 6e004997..95cf56de 100644
--- a/common/populate
+++ b/common/populate
@@ -71,6 +71,37 @@ __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 missing="$3"
+ 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}"
+ nr=0
+ while true; do
+ creat=mkdir
+ test "$((nr % 20))" -eq 0 && creat=touch
+ $creat "${name}/$(printf "%.08d" "$nr")"
+ if [ "$((nr % 40))" -eq 0 ]; then
+ nextents="$(_xfs_get_fsxattr nextents $name)"
+ [ $nextents -gt $max_nextents ] && break
+ fi
+ nr=$((nr+1))
+ done
+
+ test -z "${missing}" && return
+ seq 1 2 "${nr}" | while read d; do
+ rm -rf "${name}/$(printf "%.08d" "$d")"
+ done
+}
+
# Add a bunch of attrs to a file
__populate_create_attr() {
name="$1"
@@ -176,6 +207,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 +258,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" true
# Symlinks
# - FMT_LOCAL
diff --git a/common/xfs b/common/xfs
index 5074c350..3bfe8566 100644
--- a/common/xfs
+++ b/common/xfs
@@ -1487,6 +1487,15 @@ _require_xfsrestore_xflag()
_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()
--
2.18.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V4 1/2] common/xfs: Add a helper to export inode core size
2022-12-07 9:31 ` [PATCH V4 1/2] common/xfs: Add a helper to export inode core size Ziyang Zhang
@ 2022-12-07 18:06 ` Zorro Lang
0 siblings, 0 replies; 6+ messages in thread
From: Zorro Lang @ 2022-12-07 18:06 UTC (permalink / raw)
To: Ziyang Zhang; +Cc: fstests, linux-xfs, djwong, hsiangkao, allison.henderson
On Wed, Dec 07, 2022 at 05:31:46PM +0800, Ziyang Zhang wrote:
> Some xfs test cases need the number of bytes reserved for only the inode
> record, excluding the immediate fork areas. Now the value is hard-coded
> and it is not a good chioce. Add a helper in common/xfs to export the
> inode core size.
>
> Signed-off-by: Ziyang Zhang <ZiyangZhang@linux.alibaba.com>
> ---
Darrick has given RVB to this patch, if you didn't change anything from V3
ou can keep it:
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> common/xfs | 15 +++++++++++++++
> tests/xfs/335 | 3 ++-
> tests/xfs/336 | 3 ++-
> tests/xfs/337 | 3 ++-
> tests/xfs/341 | 3 ++-
> tests/xfs/342 | 3 ++-
> 6 files changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/common/xfs b/common/xfs
> index 8ac1964e..5074c350 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -1486,3 +1486,18 @@ _require_xfsrestore_xflag()
> $XFSRESTORE_PROG -h 2>&1 | grep -q -e '-x' || \
> _notrun 'xfsrestore does not support -x flag.'
> }
> +
> +# Number of bytes reserved for only the inode record, excluding the
> +# immediate fork areas.
> +_xfs_inode_core_bytes()
> +{
> + local dir="$1"
> +
> + if _xfs_has_feature "$dir" crc; then
> + # v5 filesystems
> + echo 176
> + else
> + # v4 filesystems
> + echo 96
> + fi
> +}
> diff --git a/tests/xfs/335 b/tests/xfs/335
> index ccc508e7..2fd9be54 100755
> --- a/tests/xfs/335
> +++ b/tests/xfs/335
> @@ -31,7 +31,8 @@ blksz="$(_get_block_size $SCRATCH_MNT)"
> echo "Create a three-level rtrmapbt"
> # inode core size is at least 176 bytes; btree header is 56 bytes;
> # rtrmap record is 32 bytes; and rtrmap key/pointer are 56 bytes.
> -i_ptrs=$(( (isize - 176) / 56 ))
> +i_core_size="$(_xfs_inode_core_bytes $SCRATCH_MNT)"
> +i_ptrs=$(( (isize - i_core_size) / 56 ))
> bt_ptrs=$(( (blksz - 56) / 56 ))
> bt_recs=$(( (blksz - 56) / 32 ))
>
> diff --git a/tests/xfs/336 b/tests/xfs/336
> index b1de8e5f..085b43ae 100755
> --- a/tests/xfs/336
> +++ b/tests/xfs/336
> @@ -42,7 +42,8 @@ rm -rf $metadump_file
> echo "Create a three-level rtrmapbt"
> # inode core size is at least 176 bytes; btree header is 56 bytes;
> # rtrmap record is 32 bytes; and rtrmap key/pointer are 56 bytes.
> -i_ptrs=$(( (isize - 176) / 56 ))
> +i_core_size="$(_xfs_inode_core_bytes $SCRATCH_MNT)"
> +i_ptrs=$(( (isize - i_core_size) / 56 ))
> bt_ptrs=$(( (blksz - 56) / 56 ))
> bt_recs=$(( (blksz - 56) / 32 ))
>
> diff --git a/tests/xfs/337 b/tests/xfs/337
> index a2515e36..5d6b9964 100755
> --- a/tests/xfs/337
> +++ b/tests/xfs/337
> @@ -33,7 +33,8 @@ blksz="$(_get_block_size $SCRATCH_MNT)"
>
> # inode core size is at least 176 bytes; btree header is 56 bytes;
> # rtrmap record is 32 bytes; and rtrmap key/pointer are 56 bytes.
> -i_ptrs=$(( (isize - 176) / 56 ))
> +i_core_size="$(_xfs_inode_core_bytes $SCRATCH_MNT)"
> +i_ptrs=$(( (isize - i_core_size) / 56 ))
> bt_ptrs=$(( (blksz - 56) / 56 ))
> bt_recs=$(( (blksz - 56) / 32 ))
>
> diff --git a/tests/xfs/341 b/tests/xfs/341
> index f026aa37..c70dec3c 100755
> --- a/tests/xfs/341
> +++ b/tests/xfs/341
> @@ -33,7 +33,8 @@ rtextsz_blks=$((rtextsz / blksz))
>
> # inode core size is at least 176 bytes; btree header is 56 bytes;
> # rtrmap record is 32 bytes; and rtrmap key/pointer are 56 bytes.
> -i_ptrs=$(( (isize - 176) / 56 ))
> +i_core_size="$(_xfs_inode_core_bytes $SCRATCH_MNT)"
> +i_ptrs=$(( (isize - i_core_size) / 56 ))
> bt_recs=$(( (blksz - 56) / 32 ))
>
> blocks=$((i_ptrs * bt_recs + 1))
> diff --git a/tests/xfs/342 b/tests/xfs/342
> index 1ae414eb..4855627f 100755
> --- a/tests/xfs/342
> +++ b/tests/xfs/342
> @@ -30,7 +30,8 @@ blksz="$(_get_block_size $SCRATCH_MNT)"
>
> # inode core size is at least 176 bytes; btree header is 56 bytes;
> # rtrmap record is 32 bytes; and rtrmap key/pointer are 56 bytes.
> -i_ptrs=$(( (isize - 176) / 56 ))
> +i_core_size="$(_xfs_inode_core_bytes $SCRATCH_MNT)"
> +i_ptrs=$(( (isize - i_core_size) / 56 ))
> bt_recs=$(( (blksz - 56) / 32 ))
>
> blocks=$((i_ptrs * bt_recs + 1))
> --
> 2.18.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V4 2/2] common/populate: Ensure that S_IFDIR.FMT_BTREE is in btree format
2022-12-07 9:31 ` [PATCH V4 2/2] common/populate: Ensure that S_IFDIR.FMT_BTREE is in btree format Ziyang Zhang
@ 2022-12-07 18:28 ` Zorro Lang
2022-12-07 19:38 ` Darrick J. Wong
0 siblings, 1 reply; 6+ messages in thread
From: Zorro Lang @ 2022-12-07 18:28 UTC (permalink / raw)
To: Ziyang Zhang; +Cc: fstests, linux-xfs, djwong, hsiangkao, allison.henderson
On Wed, Dec 07, 2022 at 05:31:47PM +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>
> ---
> common/populate | 34 +++++++++++++++++++++++++++++++++-
> common/xfs | 9 +++++++++
> 2 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/common/populate b/common/populate
> index 6e004997..95cf56de 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -71,6 +71,37 @@ __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 missing="$3"
> + 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}"
> + nr=0
> + while true; do
> + creat=mkdir
> + test "$((nr % 20))" -eq 0 && creat=touch
> + $creat "${name}/$(printf "%.08d" "$nr")"
> + if [ "$((nr % 40))" -eq 0 ]; then
> + nextents="$(_xfs_get_fsxattr nextents $name)"
> + [ $nextents -gt $max_nextents ] && break
> + fi
> + nr=$((nr+1))
> + done
> +
> + test -z "${missing}" && return
> + seq 1 2 "${nr}" | while read d; do
> + rm -rf "${name}/$(printf "%.08d" "$d")"
> + done
Oh, you've done this change in V4, sorry I just reviewed an old version. A
little picky review points as below:
This function makes sense to me, just the "local" key word is used so randomly,
some variables have, some doesn't :)
> +}
> +
> # Add a bunch of attrs to a file
> __populate_create_attr() {
> name="$1"
> @@ -176,6 +207,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 +258,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" true
>
> # Symlinks
> # - FMT_LOCAL
> diff --git a/common/xfs b/common/xfs
> index 5074c350..3bfe8566 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -1487,6 +1487,15 @@ _require_xfsrestore_xflag()
> _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()
Generally common/xfs names this kind of helpers as _xfs_get_xxxx(), likes
_xfs_get_rtextents()
_xfs_get_rtextsize()
_xfs_get_dir_blocksize()
...
> +{
> + local mntpoint="$1"
> +
> + $XFS_INFO_PROG "$mntpoint" | grep 'meta-data=.*isize' | sed -e 's/^.*isize=\([0-9]*\).*$/\1/g'
It can be done with one pipe:
$XFS_INFO_PROG "$mntpoint" | sed -n '/meta-data=.*isize/s/^.*isize=\([0-9]*\).*$/\1/p'
With above changes you can have:
Reviewed-by: Zorro Lang <zlang@redhat.com>
> +}
> +
> # Number of bytes reserved for only the inode record, excluding the
> # immediate fork areas.
> _xfs_inode_core_bytes()
> --
> 2.18.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V4 2/2] common/populate: Ensure that S_IFDIR.FMT_BTREE is in btree format
2022-12-07 18:28 ` Zorro Lang
@ 2022-12-07 19:38 ` Darrick J. Wong
0 siblings, 0 replies; 6+ messages in thread
From: Darrick J. Wong @ 2022-12-07 19:38 UTC (permalink / raw)
To: Zorro Lang; +Cc: Ziyang Zhang, fstests, linux-xfs, hsiangkao, allison.henderson
On Thu, Dec 08, 2022 at 02:28:50AM +0800, Zorro Lang wrote:
> On Wed, Dec 07, 2022 at 05:31:47PM +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>
> > ---
> > common/populate | 34 +++++++++++++++++++++++++++++++++-
> > common/xfs | 9 +++++++++
> > 2 files changed, 42 insertions(+), 1 deletion(-)
> >
> > diff --git a/common/populate b/common/populate
> > index 6e004997..95cf56de 100644
> > --- a/common/populate
> > +++ b/common/populate
> > @@ -71,6 +71,37 @@ __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 missing="$3"
> > + 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}"
> > + nr=0
> > + while true; do
> > + creat=mkdir
> > + test "$((nr % 20))" -eq 0 && creat=touch
> > + $creat "${name}/$(printf "%.08d" "$nr")"
> > + if [ "$((nr % 40))" -eq 0 ]; then
> > + nextents="$(_xfs_get_fsxattr nextents $name)"
> > + [ $nextents -gt $max_nextents ] && break
> > + fi
> > + nr=$((nr+1))
> > + done
> > +
> > + test -z "${missing}" && return
> > + seq 1 2 "${nr}" | while read d; do
> > + rm -rf "${name}/$(printf "%.08d" "$d")"
> > + done
>
> Oh, you've done this change in V4, sorry I just reviewed an old version. A
> little picky review points as below:
>
> This function makes sense to me, just the "local" key word is used so randomly,
> some variables have, some doesn't :)
All variables inside a helper function *should* be using 'local', unless
the goal is to set variables in an ancestor scope. Note that this can
mean variables in the top level namespace, or a different function
further up in the call stack.
Unfortunately, I didn't know about this bashism when I started writing
fstests, which is why it's inconsistent all over the place. :(
This little script:
#!/bin/bash
moo=5
bar() {
moo=$((moo + 1))
}
fubar() {
bar
}
cow() {
local moo=7
bar
echo "cow $moo"
}
grud() {
local moo=11
fubar
echo "grud $moo"
}
cow
bar
echo "global $moo"
grud
echo "global $moo"
fubar
echo "global $moo"
Prints this on output:
$ ./demo.sh
global 5
cow 8
global 5
global 6
grud 12
global 6
global 7
--D
> > +}
> > +
> > # Add a bunch of attrs to a file
> > __populate_create_attr() {
> > name="$1"
> > @@ -176,6 +207,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 +258,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" true
> >
> > # Symlinks
> > # - FMT_LOCAL
> > diff --git a/common/xfs b/common/xfs
> > index 5074c350..3bfe8566 100644
> > --- a/common/xfs
> > +++ b/common/xfs
> > @@ -1487,6 +1487,15 @@ _require_xfsrestore_xflag()
> > _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()
>
> Generally common/xfs names this kind of helpers as _xfs_get_xxxx(), likes
> _xfs_get_rtextents()
> _xfs_get_rtextsize()
> _xfs_get_dir_blocksize()
> ...
>
> > +{
> > + local mntpoint="$1"
> > +
> > + $XFS_INFO_PROG "$mntpoint" | grep 'meta-data=.*isize' | sed -e 's/^.*isize=\([0-9]*\).*$/\1/g'
>
> It can be done with one pipe:
> $XFS_INFO_PROG "$mntpoint" | sed -n '/meta-data=.*isize/s/^.*isize=\([0-9]*\).*$/\1/p'
>
> With above changes you can have:
> Reviewed-by: Zorro Lang <zlang@redhat.com>
>
> > +}
> > +
> > # Number of bytes reserved for only the inode record, excluding the
> > # immediate fork areas.
> > _xfs_inode_core_bytes()
> > --
> > 2.18.4
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-12-07 19:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-07 9:31 [PATCH V4 0/2] cleanup and bugfix for xfs tests related to btree format Ziyang Zhang
2022-12-07 9:31 ` [PATCH V4 1/2] common/xfs: Add a helper to export inode core size Ziyang Zhang
2022-12-07 18:06 ` Zorro Lang
2022-12-07 9:31 ` [PATCH V4 2/2] common/populate: Ensure that S_IFDIR.FMT_BTREE is in btree format Ziyang Zhang
2022-12-07 18:28 ` Zorro Lang
2022-12-07 19:38 ` Darrick J. Wong
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.