Hi Darrick,
On Thu, Dec 01, 2022 at 07:52:44AM -0800, Darrick J. Wong wrote:
> On Thu, Dec 01, 2022 at 04:12:08PM +0800, Gao Xiang 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>
> > Cc: Ziyang Zhang <ZiyangZhang@xxxxxxxxxxxxxxxxx>
> > Signed-off-by: Gao Xiang <hsiangkao@xxxxxxxxxxxxxxxxx>
> > ---
> > common/populate | 22 +++++++++++++++++++++-
> > 1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/common/populate b/common/populate
> > index 6e004997..e179a300 100644
> > --- a/common/populate
> > +++ b/common/populate
> > @@ -71,6 +71,25 @@ __populate_create_dir() {
> > done
> > }
> >
> > +# Create a large directory and ensure that it's a btree format
> > +__populate_create_btree_dir() {
>
> Since this encodes behavior specific to xfs, this ought to be called
>
> __populate_xfs_create_btree_dir
>
> or something like that.
>
> > + name="$1"
> > + isize="$2"
>
> These ought to be local variables, e.g.
>
> local name="$1"
> local isize="$2"
>
> So that they don't pollute the global name scope. Yay bash.
>
> > +
> > + 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
> > + nexts="$($XFS_IO_PROG -c "stat" $name | grep 'fsxattr.nextents' | sed -e 's/^.*nextents = //g' -e 's/\([0-9]*\).*$/\1/g')"
>
> _xfs_get_fsxattr...
>
> > + [ "$nexts" -gt "$(((isize - 176) / 16))" ] && break
>
> Only need to calculate this once if you declare this at the top:
>
> # 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 - 176) / 16))"
>
> and then you can do:
>
> [[ $nexts -gt $max_nextents ]] && break
>
> Also not a fan of hardcoding 176 around fstests, but I don't know how
> we'd detect that at all.
>
> # Number of bytes reserved for only the inode record, excluding the
> # immediate fork areas.
> _xfs_inode_core_bytes()
> {
> echo 176
> }
>
> I guess? Or extract it from tests/xfs/122.out?
Thanks for your comments.
I guess hard-coded 176 in _xfs_inode_core_bytes() is fine for now
(It seems a bit weird to extract a number from a test expected result..)
Otherwise I agree with your comments. I will ask Ziyang to follow your
comments and send out v2.
Thanks,
Gao Xiang
