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

[Dave Chinner <david@fromorbit.com>]

On Tue, Apr 05, 2022 at 04:36:03PM +0530, Ritesh Harjani wrote:
> On 22/04/04 09:28AM, Dave Chinner wrote:
> > On Thu, Mar 31, 2022 at 06:24:20PM +0530, Ritesh Harjani wrote:
> > > From: Ritesh Harjani <riteshh@linux.ibm.com>
> > >
> > > 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     | 8 ++++++++
> > >  tests/generic/468.out | 2 ++
> > >  2 files changed, 10 insertions(+)
> > >
> > > diff --git a/tests/generic/468 b/tests/generic/468
> > > index 95752d3b..5e73cff9 100755
> > > --- a/tests/generic/468
> > > +++ b/tests/generic/468
> > > @@ -34,6 +34,13 @@ _scratch_mkfs >/dev/null 2>&1
> > >  _require_metadata_journaling $SCRATCH_DEV
> > >  _scratch_mount
> > >
> > > +# blocksize and fact are used in the last case of the fsync/fdatasync test.
> > > +# This is mainly trying to test recovery operation in case where the data
> > > +# blocks written, exceeds the default flex group size (32768*4096*16) in ext4.
> > > +blocks=32768
> > > +blocksize=4096
> >
> > Block size can change based on mkfs parameters. You should extract
> > this dynamically from the filesystem the test is being run on.
> >
> 
> Yes, but we still have kept just 4096 because, anything bigger than that like
> 65536 might require a bigger disk size itself to test. The overall size
> requirement of the disk will then become ~36G (32768 * 65536 * 18)
> Hence I went ahead with 4096 which is good enough for testing.

If the test setup doesn't have a disk large enough, then the test
should be skipped. That's what '_require_scratch_size' is for.

i.e. _require_scratch_size $larger_than_ext4_fg_size

Will do that check once we've calculated the size needed.

> But sure, I will add a comment explaining why we have hardcoded it to 4096
> so that others don't get confused. Larger than this size disk anyway doesn't get
> tested much right?

You shouldn't be constricting the test based on assumptions about
test configurations. If someone decides to test 64k block size, then
they can size their devices appropriately for the configuration they
want to test.  If a 64kB block size filesystem can overrun the
on-disk structure and fail, then the test should exercise that and
fail appropriately.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com