| Message ID | 20220427005209.4188220-1-tytso@mit.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ext4/054,ext4/055: don't run when using DAX | expand |
On Tue, Apr 26, 2022 at 08:52:09PM -0400, Theodore Ts'o wrote: > The ext4/054 and ext4/055 tests create a scratch file system with a 1k > block size. This is not compatible with mounting with the DAX option, > which requires a block size equal to the page size (which is 4k on > x86). > > Also, the ext4/054 test doesn't use the test device, so remove the > _require_test declaration. > > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > --- Hi Ted, Thanks for point out this issue. I got a little question about this patch, feel free to correct me if that's wrong. "The DAX code currently only supports files with a block size equal to your kernel's PAGE_SIZE" [1], so I suppose any cases with smaller blocksize (< pagesize) should "_exclude_scratch_mount_option dax". And if this supposition is right, we'd better do "skipping dax testing if blocksize is less than pagesize" in a common helper. Good news is we have _scratch_mkfs_blocksized. So how about do: if [ $blocksize < $pagesize ];then _exclude_scratch_mount_option dax fi in _scratch_mkfs_blocksized? then let ext4/054 and ext4/055 turn to use _scratch_mkfs_blocksized. Thanks, Zorro [1] https://www.kernel.org/doc/Documentation/filesystems/dax.txt > tests/ext4/054 | 2 +- > tests/ext4/055 | 3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/tests/ext4/054 b/tests/ext4/054 > index 9a11719f..6c722f32 100755 > --- a/tests/ext4/054 > +++ b/tests/ext4/054 > @@ -19,8 +19,8 @@ _begin_fstest auto quick dangerous_fuzzers > > # real QA test starts here > _supported_fs ext4 > -_require_test > _require_scratch_nocheck > +_exclude_scratch_mount_option dax > _require_xfs_io_command "falloc" > _require_xfs_io_command "pwrite" > _require_xfs_io_command "fsync" > diff --git a/tests/ext4/055 b/tests/ext4/055 > index 8f466f1b..1ae42b89 100755 > --- a/tests/ext4/055 > +++ b/tests/ext4/055 > @@ -17,8 +17,9 @@ > _begin_fstest auto quota > > # real QA test starts here > -_require_scratch_nocheck > _supported_fs ext4 > +_require_scratch_nocheck > +_exclude_scratch_mount_option dax > _require_user fsgqa > _require_user fsgqa2 > _require_command "$DEBUGFS_PROG" debugfs > -- > 2.31.0 >
On Wed, Apr 27, 2022 at 04:05:40PM +0800, Zorro Lang wrote: > > "The DAX code currently only supports files with a block size equal to your > kernel's PAGE_SIZE" [1], so I suppose any cases with smaller blocksize (< pagesize) > should "_exclude_scratch_mount_option dax". > > And if this supposition is right, we'd better do "skipping dax testing if blocksize > is less than pagesize" in a common helper. Good news is we have _scratch_mkfs_blocksized. > So how about do: > if [ $blocksize < $pagesize ];then > _exclude_scratch_mount_option dax > fi > in _scratch_mkfs_blocksized? then let ext4/054 and ext4/055 turn to use _scratch_mkfs_blocksized. That's a good thing to and it would work for ext4/054 and ext/022 (which already uses "_exclude_scratch_mount_option dax" and "_scratch_mkfs"). However, a number of other tests, including ext4/055 and ext4/035 need to give additional parameters to mfks, such as "-O quota" or "-E resize=262144". So _scratch_mkfs_blocksized isn't always going to work for tests in ext4/* where we often are passing ext4-specific mkfs options to mkfs.ext4. So I can add your suggestion to _scratch_mkfs_blocksized, and use that for ext4/054, but it's not going to be a solution for ext4/055. Cheers, - Ted
On Wed, Apr 27, 2022 at 10:53:24AM -0400, Theodore Ts'o wrote: > On Wed, Apr 27, 2022 at 04:05:40PM +0800, Zorro Lang wrote: > > > > "The DAX code currently only supports files with a block size equal to your > > kernel's PAGE_SIZE" [1], so I suppose any cases with smaller blocksize (< pagesize) > > should "_exclude_scratch_mount_option dax". > > > > And if this supposition is right, we'd better do "skipping dax testing if blocksize > > is less than pagesize" in a common helper. Good news is we have _scratch_mkfs_blocksized. > > So how about do: > > if [ $blocksize < $pagesize ];then > > _exclude_scratch_mount_option dax > > fi > > in _scratch_mkfs_blocksized? then let ext4/054 and ext4/055 turn to use _scratch_mkfs_blocksized. > > That's a good thing to and it would work for ext4/054 and ext/022 > (which already uses "_exclude_scratch_mount_option dax" and > "_scratch_mkfs"). However, a number of other tests, including > ext4/055 and ext4/035 need to give additional parameters to mfks, such > as "-O quota" or "-E resize=262144". > > So _scratch_mkfs_blocksized isn't always going to work for tests in > ext4/* where we often are passing ext4-specific mkfs options to > mkfs.ext4. > > So I can add your suggestion to _scratch_mkfs_blocksized, and use that > for ext4/054, but it's not going to be a solution for ext4/055. 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 Hmm... that might look a little ugly... it's doesn't matter if we write as: _exclude_scratch_mount_option dax _scratch_mkfs "-F -O quota -b 1024" 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. 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 :) Thanks, Zorro $ grep -rs _scratch_mkfs_.*() common/ common/casefold:_scratch_mkfs_casefold() common/casefold:_scratch_mkfs_casefold_strict() common/encrypt:_scratch_mkfs_encrypted() common/encrypt:_scratch_mkfs_sized_encrypted() common/encrypt:_scratch_mkfs_stable_inodes_encrypted() common/verity:_scratch_mkfs_verity() common/verity:_scratch_mkfs_encrypted_verity() common/rc:_scratch_mkfs_options() common/rc:_scratch_mkfs_sized() common/rc:_scratch_mkfs_geom() common/rc:_scratch_mkfs_blocksized() common/rc:_scratch_mkfs_richacl() > > Cheers, > > - Ted >
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. > 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. :-) > 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 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?
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 >
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.
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 >
On Tue, Apr 26, 2022 at 08:52:09PM -0400, Theodore Ts'o wrote: > The ext4/054 and ext4/055 tests create a scratch file system with a 1k > block size. This is not compatible with mounting with the DAX option, > which requires a block size equal to the page size (which is 4k on > x86). Following up on a point I made during this morning's ext4 concall -- I avoided these problems by adding a cli option to mkfs.xfs to set the DAX flag on the root directory (since it's advisory and propagates to all new children) and then updated my fstests config to use that. No more mount failures due to blocksize != pagesize! :) --D > Also, the ext4/054 test doesn't use the test device, so remove the > _require_test declaration. > > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > --- > tests/ext4/054 | 2 +- > tests/ext4/055 | 3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/tests/ext4/054 b/tests/ext4/054 > index 9a11719f..6c722f32 100755 > --- a/tests/ext4/054 > +++ b/tests/ext4/054 > @@ -19,8 +19,8 @@ _begin_fstest auto quick dangerous_fuzzers > > # real QA test starts here > _supported_fs ext4 > -_require_test > _require_scratch_nocheck > +_exclude_scratch_mount_option dax > _require_xfs_io_command "falloc" > _require_xfs_io_command "pwrite" > _require_xfs_io_command "fsync" > diff --git a/tests/ext4/055 b/tests/ext4/055 > index 8f466f1b..1ae42b89 100755 > --- a/tests/ext4/055 > +++ b/tests/ext4/055 > @@ -17,8 +17,9 @@ > _begin_fstest auto quota > > # real QA test starts here > -_require_scratch_nocheck > _supported_fs ext4 > +_require_scratch_nocheck > +_exclude_scratch_mount_option dax > _require_user fsgqa > _require_user fsgqa2 > _require_command "$DEBUGFS_PROG" debugfs > -- > 2.31.0 >
diff --git a/tests/ext4/054 b/tests/ext4/054 index 9a11719f..6c722f32 100755 --- a/tests/ext4/054 +++ b/tests/ext4/054 @@ -19,8 +19,8 @@ _begin_fstest auto quick dangerous_fuzzers # real QA test starts here _supported_fs ext4 -_require_test _require_scratch_nocheck +_exclude_scratch_mount_option dax _require_xfs_io_command "falloc" _require_xfs_io_command "pwrite" _require_xfs_io_command "fsync" diff --git a/tests/ext4/055 b/tests/ext4/055 index 8f466f1b..1ae42b89 100755 --- a/tests/ext4/055 +++ b/tests/ext4/055 @@ -17,8 +17,9 @@ _begin_fstest auto quota # real QA test starts here -_require_scratch_nocheck _supported_fs ext4 +_require_scratch_nocheck +_exclude_scratch_mount_option dax _require_user fsgqa _require_user fsgqa2 _require_command "$DEBUGFS_PROG" debugfs
The ext4/054 and ext4/055 tests create a scratch file system with a 1k block size. This is not compatible with mounting with the DAX option, which requires a block size equal to the page size (which is 4k on x86). Also, the ext4/054 test doesn't use the test device, so remove the _require_test declaration. Signed-off-by: Theodore Ts'o <tytso@mit.edu> --- tests/ext4/054 | 2 +- tests/ext4/055 | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-)