by Shinichiro Kawasaki

[permalink] [raw]

Subject: Re: [PATCH 5/7] common: rename _filter_mkfs to _xfs_filter_mkfs

On Feb 08, 2022 / 16:53, Darrick J. Wong wrote:
> On Tue, Feb 08, 2022 at 04:43:16PM -0800, Darrick J. Wong wrote:
> > On Mon, Feb 07, 2022 at 03:55:39PM +0900, Shin'ichiro Kawasaki wrote:
> > > The helper function works only for xfs and used only for xfs except
> > > generic/204. Rename the function to clearly indicate that the function
> > > is only for xfs.
> > >
> > > Suggested-by: Naohiro Aota <[email protected]>
> > > Signed-off-by: Shin'ichiro Kawasaki <[email protected]>
> >
> > <snip the diffstat>
> >
> > > diff --git a/common/attr b/common/attr
> > > index 35682d7c..964c790a 100644
> > > --- a/common/attr
> > > +++ b/common/attr
> > > @@ -13,7 +13,7 @@ _acl_get_max()
> > > # CRC format filesystems have much larger ACL counts. The actual
> > > # number is into the thousands, but testing that meany takes too
> > > # long, so just test well past the old limit of 25.
> > > - $XFS_INFO_PROG $TEST_DIR | _filter_mkfs > /dev/null 2> $tmp.info
> > > + $XFS_INFO_PROG $TEST_DIR | _xfs_filter_mkfs > /dev/null 2> $tmp.info
> > > . $tmp.info
> > > rm $tmp.info
> > > if [ $_fs_has_crcs -eq 0 ]; then
> > > diff --git a/common/filter b/common/filter
> > > index c3db7a56..24fd0650 100644
> > > --- a/common/filter
> > > +++ b/common/filter
> > > @@ -117,7 +117,7 @@ _filter_date()
> > >
> > > # prints filtered output on stdout, values (use eval) on stderr
> > > # Non XFS filesystems always return a 4k block size and a 256 byte inode.
> > > -_filter_mkfs()
> > > +_xfs_filter_mkfs()
> > > {
> > > case $FSTYP in
> > > xfs)
> > > diff --git a/common/xfs b/common/xfs
> >
> > This renames the generic function to be "only for xfs" but it leaves the
> > non-XFS bits. Those bits are /really/ problematic (hardcoded
> > isize=256 and dbsize=4096? Seriously??) and themselves were introduced
> > in commit a4d5b247 ("xfstests: Make 204 work with different block and
> > inode sizes.") oh wow.
> >
> > I'm sorry that someone left this a mess, but let's try to make it easy
> > to clean up all the other filesystems, please. Specifically, could you
> > please:
> >
> > 1. Hoist the XFS-specific code from _filter_mkfs into a new
> > helper _xfs_filter_mkfs() in common/xfs?
>
> UGH. I pressed <Send>, not <Save>. Picking up from where I left off:
>
> 2. Make the generic _filter_mkfs function call the XFS-specific one,
> ala:
>
> _filter_mkfs()
> {
> case $FSTYP in
> xfs)
> _xfs_filter_mkfs "$@"
> ;;
> *)
> cat - >/dev/null
> perl -e 'print STDERR "dbsize=4096\nisize=256\n"'
> return ;;
> esac
> }
>
> This way you don't have to make a gigantic treewide change, and we can
> start to make this function work properly for filesystems that have the
> ability to do an offline geometry dump (aka dumpe2fs for ext*).

Thank you for the comment. My thought was a rather negative one: I assumed that
_filter_mkfs would not support other filesystems than xfs. I will add a patch
in v2 series based on your suggestion above, expecting that _filter_mkfs will
support other filesystems in the future.

--
Best Regards,
Shin'ichiro Kawasaki