Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 6 Apr 2022 17:26:25 +0530
From:   Ritesh Harjani <ritesh.list@gmail.com>
To:     Dave Chinner <david@fromorbit.com>
Cc:     fstests <fstests@vger.kernel.org>, linux-ext4@vger.kernel.org,
        linux-fsdevel@vger.kernel.org,
        "Darrick J . Wong" <djwong@kernel.org>,
        Ritesh Harjani <riteshh@linux.ibm.com>
Subject: Re: [PATCHv3 1/4] generic/468: Add another falloc test entry
Message-ID: <20220406115625.3a2fzn3je7s43xtt@riteshh-domain>
References: <cover.1648730443.git.ritesh.list@gmail.com>
 <75f4c780e8402a8f993cb987e85a31e4895f13de.1648730443.git.ritesh.list@gmail.com>
 <20220403232823.GS1609613@dread.disaster.area>
 <20220405110603.qqxyivpo4vzj5tlt@riteshh-domain>
 <20220406040508.GC1609613@dread.disaster.area>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220406040508.GC1609613@dread.disaster.area>
Precedence: bulk

On 22/04/06 02:05PM, Dave Chinner wrote:
> 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.

Sure.

>
> > 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.

Sure Dave. Got the point. I will try and make the changes, such that
test doesn't assume any particular user test configuration. And be generic as
much as possible so that we could hit the issue we are aiming via this test.

-ritesh