Re: [PATCH] md/raid0: avoid RAID0 data corruption due to layout confusion.

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3549 bytes --]

On Wed, Sep 11 2019, Song Liu wrote:

> On Wed, Sep 11, 2019 at 12:10 AM NeilBrown <neilb@suse.de> wrote:
>>
>> On Tue, Sep 10 2019, Guoqing Jiang wrote:
>>
>> > On 9/10/19 5:45 PM, Song Liu wrote:
>> >>
>> >>
>> >>> On Sep 10, 2019, at 12:33 AM, NeilBrown <neilb@suse.de> wrote:
>> >>>
>> >>> On Mon, Sep 09 2019, Song Liu wrote:
>> >>>
>> >>>> Hi Neil,
>> >>>>
>> >>>>> On Sep 9, 2019, at 7:57 AM, NeilBrown <neilb@suse.de> wrote:
>> >>>>>
>> >>>>>
>> >>>>> If the drives in a RAID0 are not all the same size, the array is
>> >>>>> divided into zones.
>> >>>>> The first zone covers all drives, to the size of the smallest.
>> >>>>> The second zone covers all drives larger than the smallest, up to
>> >>>>> the size of the second smallest - etc.
>> >>>>>
>> >>>>> A change in Linux 3.14 unintentionally changed the layout for the
>> >>>>> second and subsequent zones.  All the correct data is still stored, but
>> >>>>> each chunk may be assigned to a different device than in pre-3.14 kernels.
>> >>>>> This can lead to data corruption.
>> >>>>>
>> >>>>> It is not possible to determine what layout to use - it depends which
>> >>>>> kernel the data was written by.
>> >>>>> So we add a module parameter to allow the old (0) or new (1) layout to be
>> >>>>> specified, and refused to assemble an affected array if that parameter is
>> >>>>> not set.
>> >>>>>
>> >>>>> Fixes: 20d0189b1012 ("block: Introduce new bio_split()")
>> >>>>> cc: stable@vger.kernel.org (3.14+)
>> >>>>> Signed-off-by: NeilBrown <neilb@suse.de>
>> >>>>
>> >>>> Thanks for the patches. They look great. However, I am having problem
>> >>>> apply them (not sure whether it is a problem on my side). Could you
>> >>>> please push it somewhere so I can use cherry-pick instead?
>> >>>
>> >>> I rebased them on block/for-next, fixed the problems that Guoqing found,
>> >>> and pushed them to
>> >>>   https://github.com/neilbrown/linux md/raid0
>> >>>
>> >>> NeilBrown
>> >>
>> >> Thanks Neil!
>> >
>> > Thanks for the explanation about set the flag.
>> >
>> >>
>> >> Guoqing, if this looks good, please reply with your Reviewed-by
>> >> or Acked-by.
>> >
>> > No more comments from my side, but I am not sure if it is better/possible to use one
>> > sysfs node to control the behavior instead of module parameter, then we can support
>> > different raid0 layout dynamically.
>>
>> A strength of module parameters is that you can set them in
>>   /etc/modprobe.d/00-local.conf
>> and then they are automatically set on boot.
>> For sysfs, you need some tool to set them.
>>
>> >
>> > Anyway, Acked-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
>> >
>
> I am adding the following change to the 1/2. Please let me know if it doesn't
> make sense.

I don't object, through with the current code it is impossible for that
warning to fire.
Code might change in the future though, and it's better to be safe than
sorry.

Thanks,
NeilBrown

>
> Thanks,
> Song
>
> diff --git i/drivers/md/raid0.c w/drivers/md/raid0.c
> index a9fcff50bbfc..54d0064787a8 100644
> --- i/drivers/md/raid0.c
> +++ w/drivers/md/raid0.c
> @@ -615,6 +615,10 @@ static bool raid0_make_request(struct mddev
> *mddev, struct bio *bio)
>         case RAID0_ALT_MULTIZONE_LAYOUT:
>                 tmp_dev = map_sector(mddev, zone, sector, &sector);
>                 break;
> +       default:
> +               WARN("md/raid0:%s: Invalid layout\n", mdname(mddev));
> +               bio_io_error(bio);
> +               return true;
>         }
>
>         if (unlikely(is_mddev_broken(tmp_dev, "raid0"))) {

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]