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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <naohiro.aota@xxxxxxx>
> > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
> > 
> > <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



[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux