Re: [PATCH] ext4/054,ext4/055: don't run when using DAX

Re: [PATCH] ext4/054,ext4/055: don't run when using DAX

[Zorro Lang]

On Wed, Apr 27, 2022 at 03:44:58PM -0400, Theodore Ts'o wrote:
> On Thu, Apr 28, 2022 at 01:19:23AM +0800, Zorro Lang wrote:
> > I just noticed that _scratch_mkfs_sized() and _scratch_mkfs_blocksized() both use
> > _scratch_mkfs_xfs for XFS, I'm wondering if ext4 would like to use _scratch_mkfs_ext4()
> > or even use _scratch_mkfs() directly in these two functions. Then you can do something
> > likes:
> >   MKFS_OPTIONS="$MKFS_OPTIONS -F -O quota"
> >   _scratch_mkfs_blocksized 1024
> > or:
> >   MKFS_OPTIONS="$MKFS_OPTIONS -F -O quota" _scratch_mkfs_blocksized 1024
> 
> I'd prefer to keep changing _scratch_mkfs_sized and
> _scatch_mkfs_blocksized to use _scratch_mfks_ext4 as a separate
> commit.  It makes sense to do that, but it does mean some behavioral
> changes; specifically in the external log case,
> "_scratch_mkfs_blocksized" will now create a file system using an
> external log.  It's probably a good change, but there is some testing
> I'd like to do first before makinig that change and I don't have time
> for it.

Sure, totally agree :)

> 
> > We just provide a helper to avoid someone forget 'dax', I don't object someone would
> > like to "exclude dax" by explicit method :) So if you don't have much time to do this
> > change, you can just do what you said above, then I'll take another time/chance to
> > change _scratch_mkfs_* things.
> 
> Hmm, one thing which I noticed when searching through things.  xfs/432
> does this:
> 
> _scratch_mkfs -b size=1k -n size=64k > "$seqres.full" 2>&1
> 
> So in {gce,kvm}-xfstests we have an exclude file entry in
> .../fs/xfs/cfg/dax.exclude:
> 
> # This test formats a file system with a 1k block size, which is not
> # compatible with DAX (at least with systems with a 4k page size).
> xfs/432
> 
> ... in order to suppress a test failure.
> 
> Arguably we should add an "_exclude_scratch_mount_option dax" to this
> test, as opposed to having an explicit test exclusion in my test
> runner.  Or we figure out how to change xfs/432 to use
> _scratch_mkfs_blocksized.  So there is a lot of cleanup that can be
> done here, and I suspect we should do this work incrementally.  :-)

Thanks for finding that, yes, we can do a cleanup later, if you have
a failed testing list welcome to provide to be references :)

> 
> > Maybe we should think about let all _scratch_mkfs_*[1] helpers use _scratch_mkfs
> > consistently. But that will change and affect too many things. I don't want to break
> > fundamental code too much, might be better to let each fs help to change and test
> > that bit by bit, when they need :)
> 
> Yep.   :-)
> 
> 						- Ted
> 
> P.S.  Here's something else that should probably be moved from my test
> runner into xfstests.  Again from .../xfs/cfg/dax.exclude:
> 
> # mkfs.xfs options which now includes reflink, and reflink is not
> # compatible with DAX
> xfs/032
> xfs/205
> xfs/294

Yes, xfs reflink can't work with DAX now, I don't know if it *will*, maybe
Darrick knows more details.

> 
> Maybe _scratch_mkfs_xfs should be parsing the output of mkfs.xfs to
> see if reflink is enabled, and then automatically asserting an
> "_exclude_scratch_mount_option dax", perhaps?

Hmm... good point, does it make sense to you, Darrick?

This patch can do what we talked in last patch, and welcome later patches from
extN forks:) I can help to deal with XFS part later. I don't know if btrfs has
similar troubles, welcome patches if they need too.

The change on _scratch_mkfs_blocksized will help common part, but can' help all
situations. Maybe better to let each fs change and test their fundamental helper
changes separately, to avoid regression :)

Thanks,
Zorro

> 

Re: [PATCH] ext4/054,ext4/055: don't run when using DAX

[Dave Chinner]

On Thu, Apr 28, 2022 at 12:53:13PM +0800, Zorro Lang wrote:
> On Wed, Apr 27, 2022 at 03:44:58PM -0400, Theodore Ts'o wrote:
> > On Thu, Apr 28, 2022 at 01:19:23AM +0800, Zorro Lang wrote:
> > > I just noticed that _scratch_mkfs_sized() and _scratch_mkfs_blocksized() both use
> > > _scratch_mkfs_xfs for XFS, I'm wondering if ext4 would like to use _scratch_mkfs_ext4()
> > > or even use _scratch_mkfs() directly in these two functions. Then you can do something
> > > likes:
> > >   MKFS_OPTIONS="$MKFS_OPTIONS -F -O quota"
> > >   _scratch_mkfs_blocksized 1024
> > > or:
> > >   MKFS_OPTIONS="$MKFS_OPTIONS -F -O quota" _scratch_mkfs_blocksized 1024
> > 
> > I'd prefer to keep changing _scratch_mkfs_sized and
> > _scatch_mkfs_blocksized to use _scratch_mfks_ext4 as a separate
> > commit.  It makes sense to do that, but it does mean some behavioral
> > changes; specifically in the external log case,
> > "_scratch_mkfs_blocksized" will now create a file system using an
> > external log.  It's probably a good change, but there is some testing
> > I'd like to do first before makinig that change and I don't have time
> > for it.
> 
> Sure, totally agree :)
> 
> > 
> > > We just provide a helper to avoid someone forget 'dax', I don't object someone would
> > > like to "exclude dax" by explicit method :) So if you don't have much time to do this
> > > change, you can just do what you said above, then I'll take another time/chance to
> > > change _scratch_mkfs_* things.
> > 
> > Hmm, one thing which I noticed when searching through things.  xfs/432
> > does this:
> > 
> > _scratch_mkfs -b size=1k -n size=64k > "$seqres.full" 2>&1
> > 
> > So in {gce,kvm}-xfstests we have an exclude file entry in
> > .../fs/xfs/cfg/dax.exclude:
> > 
> > # This test formats a file system with a 1k block size, which is not
> > # compatible with DAX (at least with systems with a 4k page size).
> > xfs/432
> > 
> > ... in order to suppress a test failure.
> > 
> > Arguably we should add an "_exclude_scratch_mount_option dax" to this
> > test, as opposed to having an explicit test exclusion in my test
> > runner.  Or we figure out how to change xfs/432 to use
> > _scratch_mkfs_blocksized.  So there is a lot of cleanup that can be
> > done here, and I suspect we should do this work incrementally.  :-)
> 
> Thanks for finding that, yes, we can do a cleanup later, if you have
> a failed testing list welcome to provide to be references :)
> 
> > 
> > > Maybe we should think about let all _scratch_mkfs_*[1] helpers use _scratch_mkfs
> > > consistently. But that will change and affect too many things. I don't want to break
> > > fundamental code too much, might be better to let each fs help to change and test
> > > that bit by bit, when they need :)
> > 
> > Yep.   :-)
> > 
> > 						- Ted
> > 
> > P.S.  Here's something else that should probably be moved from my test
> > runner into xfstests.  Again from .../xfs/cfg/dax.exclude:
> > 
> > # mkfs.xfs options which now includes reflink, and reflink is not
> > # compatible with DAX
> > xfs/032
> > xfs/205
> > xfs/294
> 
> Yes, xfs reflink can't work with DAX now, I don't know if it *will*, maybe
> Darrick knows more details.

The DAX+reflink patches are almost ready to be merged - everything
has been reviewed and if I get updated patches in the next week or
two that address all the remaining concerns I'll probably merge
them.

> > Maybe _scratch_mkfs_xfs should be parsing the output of mkfs.xfs to
> > see if reflink is enabled, and then automatically asserting an
> > "_exclude_scratch_mount_option dax", perhaps?

The time to do this was about 4 years ago, not right now when we are
potentially within a couple of weeks of actually landing the support
for this functionality in the dev tree and need the fstests
infrastructure to explicitly support this configuration....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] ext4/054,ext4/055: don't run when using DAX

[Zorro Lang]

On Thu, Apr 28, 2022 at 03:58:25PM +1000, Dave Chinner wrote:
> On Thu, Apr 28, 2022 at 12:53:13PM +0800, Zorro Lang wrote:
> > On Wed, Apr 27, 2022 at 03:44:58PM -0400, Theodore Ts'o wrote:
> > > On Thu, Apr 28, 2022 at 01:19:23AM +0800, Zorro Lang wrote:
> > > > I just noticed that _scratch_mkfs_sized() and _scratch_mkfs_blocksized() both use
> > > > _scratch_mkfs_xfs for XFS, I'm wondering if ext4 would like to use _scratch_mkfs_ext4()
> > > > or even use _scratch_mkfs() directly in these two functions. Then you can do something
> > > > likes:
> > > >   MKFS_OPTIONS="$MKFS_OPTIONS -F -O quota"
> > > >   _scratch_mkfs_blocksized 1024
> > > > or:
> > > >   MKFS_OPTIONS="$MKFS_OPTIONS -F -O quota" _scratch_mkfs_blocksized 1024
> > > 
> > > I'd prefer to keep changing _scratch_mkfs_sized and
> > > _scatch_mkfs_blocksized to use _scratch_mfks_ext4 as a separate
> > > commit.  It makes sense to do that, but it does mean some behavioral
> > > changes; specifically in the external log case,
> > > "_scratch_mkfs_blocksized" will now create a file system using an
> > > external log.  It's probably a good change, but there is some testing
> > > I'd like to do first before makinig that change and I don't have time
> > > for it.
> > 
> > Sure, totally agree :)
> > 
> > > 
> > > > We just provide a helper to avoid someone forget 'dax', I don't object someone would
> > > > like to "exclude dax" by explicit method :) So if you don't have much time to do this
> > > > change, you can just do what you said above, then I'll take another time/chance to
> > > > change _scratch_mkfs_* things.
> > > 
> > > Hmm, one thing which I noticed when searching through things.  xfs/432
> > > does this:
> > > 
> > > _scratch_mkfs -b size=1k -n size=64k > "$seqres.full" 2>&1
> > > 
> > > So in {gce,kvm}-xfstests we have an exclude file entry in
> > > .../fs/xfs/cfg/dax.exclude:
> > > 
> > > # This test formats a file system with a 1k block size, which is not
> > > # compatible with DAX (at least with systems with a 4k page size).
> > > xfs/432
> > > 
> > > ... in order to suppress a test failure.
> > > 
> > > Arguably we should add an "_exclude_scratch_mount_option dax" to this
> > > test, as opposed to having an explicit test exclusion in my test
> > > runner.  Or we figure out how to change xfs/432 to use
> > > _scratch_mkfs_blocksized.  So there is a lot of cleanup that can be
> > > done here, and I suspect we should do this work incrementally.  :-)
> > 
> > Thanks for finding that, yes, we can do a cleanup later, if you have
> > a failed testing list welcome to provide to be references :)
> > 
> > > 
> > > > Maybe we should think about let all _scratch_mkfs_*[1] helpers use _scratch_mkfs
> > > > consistently. But that will change and affect too many things. I don't want to break
> > > > fundamental code too much, might be better to let each fs help to change and test
> > > > that bit by bit, when they need :)
> > > 
> > > Yep.   :-)
> > > 
> > > 						- Ted
> > > 
> > > P.S.  Here's something else that should probably be moved from my test
> > > runner into xfstests.  Again from .../xfs/cfg/dax.exclude:
> > > 
> > > # mkfs.xfs options which now includes reflink, and reflink is not
> > > # compatible with DAX
> > > xfs/032
> > > xfs/205
> > > xfs/294
> > 
> > Yes, xfs reflink can't work with DAX now, I don't know if it *will*, maybe
> > Darrick knows more details.
> 
> The DAX+reflink patches are almost ready to be merged - everything
> has been reviewed and if I get updated patches in the next week or
> two that address all the remaining concerns I'll probably merge
> them.

Thanks, good to know that. So we don't need to concern DAX+reflink test cases.

> 
> > > Maybe _scratch_mkfs_xfs should be parsing the output of mkfs.xfs to
> > > see if reflink is enabled, and then automatically asserting an
> > > "_exclude_scratch_mount_option dax", perhaps?
> 
> The time to do this was about 4 years ago, not right now when we are
> potentially within a couple of weeks of actually landing the support
> for this functionality in the dev tree and need the fstests
> infrastructure to explicitly support this configuration....

Sure, we'll give it a regression testing too, when DAX+reflink is ready.

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>