Re: [PATCHv2 1/4] generic/468: Add another falloc test entry

[Ritesh Harjani <ritesh.list@gmail.com>]

First of all thanks Darrick for the review.
And sorry about such a long delay in getting back to this.

On 22/03/15 09:51AM, Darrick J. Wong wrote:
> On Tue, Mar 15, 2022 at 07:58:56PM +0530, Ritesh Harjani wrote:
> > Add another falloc test entry which could hit a kernel bug
> > with ext4 fast_commit feature w/o below kernel commit [1].
> >
> > <log>
> > [  410.888496][ T2743] BUG: KASAN: use-after-free in ext4_mb_mark_bb+0x26a/0x6c0
> > [  410.890432][ T2743] Read of size 8 at addr ffff888171886000 by task mount/2743
> >
> > This happens when falloc -k size is huge which spans across more than
> > 1 flex block group in ext4. This causes a bug in fast_commit replay
> > code which is fixed by kernel commit at [1].
> >
> > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=bfdc502a4a4c058bf4cbb1df0c297761d528f54d
> >
> > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> > ---
> >  tests/generic/468     | 4 ++++
> >  tests/generic/468.out | 2 ++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/tests/generic/468 b/tests/generic/468
> > index 95752d3b..cbef9746 100755
> > --- a/tests/generic/468
> > +++ b/tests/generic/468
> > @@ -34,6 +34,9 @@ _scratch_mkfs >/dev/null 2>&1
> >  _require_metadata_journaling $SCRATCH_DEV
> >  _scratch_mount
> >
> > +blocksize=4096
>
> What happens if the file blocksize isn't 4k?  Does fastcommit only
> support one block size?  I didn't think it has any such restriction?

It does support other block size too.
Now, for bs < 4096 it is still fine. Yes, it won't trigger for 64K bs.
But that is ok since anyway to trigger this issue with 64K, we will need a much
larger disk size that anyway folks doesn't run fstests with
$((32768*65536*18))


>
> > +fact=18
>
> This needs a bit more explanation -- why 18?  I think the reason is that
> you need the fallocate to cross into another flexbg, and flexbgs (by
> default) are 16bg long, right?

That is right. Sure, I will add a comment explaining this, in the next revision.

>
> If that's the case, then don't you need to detect the flexbg size so
> that this is still an effective test if someone runs fstests with
> MKFS_OPTIONS='-G 32' or something?

We can do that. But it is rare for anyone to test only with '-G 32' and not test
with a default option. 32 might still be ok, but anything bigger then that
might still require the test to not run due to requirement of disk size.

And plus I wanted to keep the test generic enough such that it covers the
regression test with default fast_commit mkfs option.

-ritesh

>
> --D
>
> > +
> >  testfile=$SCRATCH_MNT/testfile
> >
> >  # check inode metadata after shutdown
> > @@ -85,6 +88,7 @@ for i in fsync fdatasync; do
> >  	test_falloc $i "-k " 1024
> >  	test_falloc $i "-k " 4096
> >  	test_falloc $i "-k " 104857600
> > +	test_falloc $i "-k " $((32768*$blocksize*$fact))
> >  done
> >
> >  status=0
> > diff --git a/tests/generic/468.out b/tests/generic/468.out
> > index b3a28d5e..a09cedb8 100644
> > --- a/tests/generic/468.out
> > +++ b/tests/generic/468.out
> > @@ -5,9 +5,11 @@ QA output created by 468
> >  ==== falloc -k 1024 test with fsync ====
> >  ==== falloc -k 4096 test with fsync ====
> >  ==== falloc -k 104857600 test with fsync ====
> > +==== falloc -k 2415919104 test with fsync ====
> >  ==== falloc 1024 test with fdatasync ====
> >  ==== falloc 4096 test with fdatasync ====
> >  ==== falloc 104857600 test with fdatasync ====
> >  ==== falloc -k 1024 test with fdatasync ====
> >  ==== falloc -k 4096 test with fdatasync ====
> >  ==== falloc -k 104857600 test with fdatasync ====
> > +==== falloc -k 2415919104 test with fdatasync ====
> > --
> > 2.31.1
> >