Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230628143201.1522227-1-jbaron@akamai.com> <CAPhsuW6-vrNh1MSCznEk_AmahBw9iq+jyG=Kmyi0N5m-T2dByA@mail.gmail.com>
In-Reply-To: <CAPhsuW6-vrNh1MSCznEk_AmahBw9iq+jyG=Kmyi0N5m-T2dByA@mail.gmail.com>
From:   Song Liu <song@kernel.org>
Date:   Wed, 28 Jun 2023 15:55:49 -0700
Message-ID: <CAPhsuW4yXpWMy5Pa20S69t5xUocpkOkWjWW=ZR3a4uttTn9pHg@mail.gmail.com>
Subject: Re: [PATCH] md/raid0: drop discard support past the first zone for
 the original layout (stable fix)
To:     Jason Baron <jbaron@akamai.com>
Cc:     linux-kernel@vger.kernel.org, linux-raid@vger.kernel.org,
        NeilBrown <neilb@suse.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Jun 28, 2023 at 3:43=E2=80=AFPM Song Liu <song@kernel.org> wrote:
>
> Hi Jason,
>
> Thanks for the fix!
>
> On Wed, Jun 28, 2023 at 7:32=E2=80=AFAM Jason Baron <jbaron@akamai.com> w=
rote:
> >
> > We've found that using raid0 with the 'original' layout and discard
> > enabled with different disk sizes (such that at least two zones are
> > created) can result in data corruption. This is due to the fact that
> > the discard handling in 'raid0_handle_discard()' assumes the 'alternate=
'
> > layout. We've seen this corruption using ext4 but other filesystems are
> > likely susceptible as well.
> >
> > More specifically, while multiple zones are necessary to create the
> > corruption, the corruption may not occur with multiple zones if they
> > layout in such a way the layout matches what the 'alternate' layout
> > would have produced. Thus, not all raid0 devices with the 'original'
> > layout, different size disks and discard enabled will encounter this
> > corruption.
> >
> > The 3.14 kernel inadvertently changed the raid0 disk layout for differe=
nt
> > size disks. Thus, running a pre-3.14 kernel and post-3.14 kernel on the
> > same raid0 array could corrupt data. This lead to the creation of the
> > 'original' layout (to match the pre-3.14 layout) and the 'alternate' la=
yout
> > (to match the post 3.14 layout) in the 5.4 kernel time frame and an opt=
ion
> > to tell the kernel which layout to use (since it couldn't be autodetect=
ed).
> > However, when the 'original' layout was added back to 5.4 discard suppo=
rt
> > for the 'original' layout was not added leading this issue.
> >
> > I've been able to reliably reproduce the corruption with the following
> > test case:
> >
> > 1. create raid0 array with different size disks using original layout
> > 2. mkfs
> > 3. mount -o discard
> > 4. create lots of files
> > 5. remove 1/2 the files
> > 6. fstrim -a (or just the mount point for the raid0 array)
> > 7. umount
> > 8. fsck -fn /dev/md0 (spews all sorts of corruptions)
> >
> > The fix here is a minimal fix intended for stable trees which doesn't d=
o
> > discard if we have the original layout and we are not in first zone or
> > if the i/o crosses zones (we either do the entire discard or none of it=
).
> > The proper fix to actually perform discard to all zones for the origina=
l
> > layout will land in upstream versions. We have implemented the minimal
> > fix here for stable branches to reduce risk.
> >
> > I've verified the change using the reproducer mentioned above. Typicall=
y,
> > the corruption is seen after less than 3 iterations, while the patch ha=
s
> > run 500+ iterations.
> >
> > Cc: NeilBrown <neilb@suse.de>
> > Cc: Song Liu <song@kernel.org>
> > Fixes: c84a1372df92 ("md/raid0: avoid RAID0 data corruption due to layo=
ut confusion.")
> > Signed-off-by: Jason Baron <jbaron@akamai.com>
> > ---
> >  drivers/md/raid0.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
> > index f8ee9a95e25d..2713a4acb44f 100644
> > --- a/drivers/md/raid0.c
> > +++ b/drivers/md/raid0.c
> > @@ -444,10 +444,25 @@ static void raid0_handle_discard(struct mddev *md=
dev, struct bio *bio)
> >         sector_t end_disk_offset;
> >         unsigned int end_disk_index;
> >         unsigned int disk;
> > +       bool bio_zone_overlap =3D false;
> >
> >         zone =3D find_zone(conf, &start);
> > +       if (bio_end_sector(bio) > zone->zone_end)
> > +               bio_zone_overlap =3D true;
> > +
> > +       /* The discard code below doesn't properly support the original
> > +        * layout for the zones above the first one. We are adding
> > +        * proper support in later kernel versions but have decided
> > +        * that dropping discard support here is the lower risk option
> > +        * to avoid data corruption for stable versions.
> > +        */
> > +       if ((conf->layout =3D=3D RAID0_ORIG_LAYOUT) &&
> > +           ((zone !=3D conf->strip_zone) || (bio_zone_overlap))) {
> > +               bio_endio(bio);
> > +               return;
> > +       }
>
> For bio_zone_overlap case, I think we can still do the split below and
> send discard to the first zone?

Actually, on a second (third?) thought, I think we can just ship the other
version (20230623180523.1901230-1-jbaron@akamai.com) to stable
kernel. It is very safe. So we don't need more work on this version.
Sorry for the confusion.

Thanks again for the fix!
Song