md/raid1: only update stack limits with the device in use

  Spare device affects array stack limits is unreasonable. For example,
create a raid1 with two 512 byte devices, the logical_block_size of array
will be 512. But after add a 4k devcie as spare, logical_block_size of
array will change as follows.

  mdadm -C /dev/md0 -n 2 -l 10 /dev/sd[ab]	//sd[ab] is 512
  //logical_block_size of md0: 512

  mdadm --add /dev/md0 /dev/sdc			//sdc is 4k
  //logical_block_size of md0: 512

  mdadm -S /dev/md0
  mdadm -A /dev/md0 /dev/sd[ab]
  //logical_block_size of md0: 4k

This will confuse users, as nothing has been changed, why did the
logical_block_size of array change?

Now, only update logical_block_size of array with the device in use.

Signed-off-by: Li Nan <linan122@huawei.com>
---
 drivers/md/raid1.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)
  

On Wed, Sep 6, 2023 at 11:30 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> Hi,
>
> 在 2023/09/06 17:37, Li Nan 写道:
> > Spare device affects array stack limits is unreasonable. For example,
> > create a raid1 with two 512 byte devices, the logical_block_size of array
> > will be 512. But after add a 4k devcie as spare, logical_block_size of
> > array will change as follows.
> >
> >    mdadm -C /dev/md0 -n 2 -l 10 /dev/sd[ab]   //sd[ab] is 512
> >    //logical_block_size of md0: 512
> >
> >    mdadm --add /dev/md0 /dev/sdc                      //sdc is 4k
> >    //logical_block_size of md0: 512
> >
> >    mdadm -S /dev/md0
> >    mdadm -A /dev/md0 /dev/sd[ab]
> >    //logical_block_size of md0: 4k
> >
> > This will confuse users, as nothing has been changed, why did the
> > logical_block_size of array change?
> >
> > Now, only update logical_block_size of array with the device in use.
> >
> > Signed-off-by: Li Nan <linan122@huawei.com>
> > ---
> >   drivers/md/raid1.c | 19 ++++++++-----------
> >   1 file changed, 8 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> > index 95504612b7e2..d75c5dd89e86 100644
> > --- a/drivers/md/raid1.c
> > +++ b/drivers/md/raid1.c
> > @@ -3140,19 +3140,16 @@ static int raid1_run(struct mddev *mddev)
>
> I'm not sure about this behaviour, 'logical_block_size' can be
> increased while adding new underlying disk, the key point is not when
> to increase 'logical_block_size'. If there is a mounted fs, or
> partition in the array, I think the array will be corrupted.

How common is such fs/partition corruption? I think some fs and partition
table can work properly with 512=>4096 change?

Thanks,
Song

>
> Perhaps once that array is started, logical_block_size should not be
> changed anymore, this will require 'logical_block_size' to be metadate
> inside raid superblock. And the array should deny any new disk with
> bigger logical_block_size.
>
> Thanks,
> Kuai
>
>
> >       if (mddev->queue)
> >               blk_queue_max_write_zeroes_sectors(mddev->queue, 0);
> >
> > -     rdev_for_each(rdev, mddev) {
> > -             if (!mddev->gendisk)
> > -                     continue;
> > -             disk_stack_limits(mddev->gendisk, rdev->bdev,
> > -                               rdev->data_offset << 9);
> > -     }
> > -
> >       mddev->degraded = 0;
> > -     for (i = 0; i < conf->raid_disks; i++)
> > -             if (conf->mirrors[i].rdev == NULL ||
> > -                 !test_bit(In_sync, &conf->mirrors[i].rdev->flags) ||
> > -                 test_bit(Faulty, &conf->mirrors[i].rdev->flags))
> > +     for (i = 0; i < conf->raid_disks; i++) {
> > +             rdev = conf->mirrors[i].rdev;
> > +             if (rdev && mddev->gendisk)
> > +                     disk_stack_limits(mddev->gendisk, rdev->bdev,
> > +                                       rdev->data_offset << 9);
> > +             if (!rdev || !test_bit(In_sync, &rdev->flags) ||
> > +                 test_bit(Faulty, &rdev->flags))
> >                       mddev->degraded++;
> > +     }
> >       /*
> >        * RAID1 needs at least one disk in active
> >        */
> >
>
  

On Sat, Sep 9, 2023 at 7:24 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> Hi,
>
> 在 2023/09/09 4:42, Song Liu 写道:
> > On Wed, Sep 6, 2023 at 11:30 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
> >>
> >> Hi,
> >>
> >> 在 2023/09/06 17:37, Li Nan 写道:
> >>> Spare device affects array stack limits is unreasonable. For example,
> >>> create a raid1 with two 512 byte devices, the logical_block_size of array
> >>> will be 512. But after add a 4k devcie as spare, logical_block_size of
> >>> array will change as follows.
> >>>
> >>>     mdadm -C /dev/md0 -n 2 -l 10 /dev/sd[ab]   //sd[ab] is 512
> >>>     //logical_block_size of md0: 512
> >>>
> >>>     mdadm --add /dev/md0 /dev/sdc                      //sdc is 4k
> >>>     //logical_block_size of md0: 512
> >>>
> >>>     mdadm -S /dev/md0
> >>>     mdadm -A /dev/md0 /dev/sd[ab]
> >>>     //logical_block_size of md0: 4k
> >>>
> >>> This will confuse users, as nothing has been changed, why did the
> >>> logical_block_size of array change?
> >>>
> >>> Now, only update logical_block_size of array with the device in use.
> >>>
> >>> Signed-off-by: Li Nan <linan122@huawei.com>
> >>> ---
> >>>    drivers/md/raid1.c | 19 ++++++++-----------
> >>>    1 file changed, 8 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> >>> index 95504612b7e2..d75c5dd89e86 100644
> >>> --- a/drivers/md/raid1.c
> >>> +++ b/drivers/md/raid1.c
> >>> @@ -3140,19 +3140,16 @@ static int raid1_run(struct mddev *mddev)
> >>
> >> I'm not sure about this behaviour, 'logical_block_size' can be
> >> increased while adding new underlying disk, the key point is not when
> >> to increase 'logical_block_size'. If there is a mounted fs, or
> >> partition in the array, I think the array will be corrupted.
> >
> > How common is such fs/partition corruption? I think some fs and partition
> > table can work properly with 512=>4096 change?
>
> For fs, that should depend on fs bs that is usually set in mkfs, if bs
> is less than 4096, then such fs can't be mounted.
>
> For partition, that is much worse, start sector and end sector will stay
> the same, while sector size is changed. And 4096 -> 512 change is the
> same.

Thanks for this information.

> >>
> >> Perhaps once that array is started, logical_block_size should not be
> >> changed anymore, this will require 'logical_block_size' to be metadate
> >> inside raid superblock. And the array should deny any new disk with
> >> bigger logical_block_size.

I really hope we can avoid adding this to the raid superblock. But I am not
sure what would be a better solution (that is also backward compatible).
Do we have real world reports of such issues?

Thanks,
Song
  

Hi,

在 2023/09/23 5:53, Song Liu 写道:
> On Sat, Sep 9, 2023 at 7:24 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>
>> Hi,
>>
>> 在 2023/09/09 4:42, Song Liu 写道:
>>> On Wed, Sep 6, 2023 at 11:30 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> 在 2023/09/06 17:37, Li Nan 写道:
>>>>> Spare device affects array stack limits is unreasonable. For example,
>>>>> create a raid1 with two 512 byte devices, the logical_block_size of array
>>>>> will be 512. But after add a 4k devcie as spare, logical_block_size of
>>>>> array will change as follows.
>>>>>
>>>>>      mdadm -C /dev/md0 -n 2 -l 10 /dev/sd[ab]   //sd[ab] is 512
>>>>>      //logical_block_size of md0: 512
>>>>>
>>>>>      mdadm --add /dev/md0 /dev/sdc                      //sdc is 4k
>>>>>      //logical_block_size of md0: 512
>>>>>
>>>>>      mdadm -S /dev/md0
>>>>>      mdadm -A /dev/md0 /dev/sd[ab]
>>>>>      //logical_block_size of md0: 4k
>>>>>
>>>>> This will confuse users, as nothing has been changed, why did the
>>>>> logical_block_size of array change?
>>>>>
>>>>> Now, only update logical_block_size of array with the device in use.
>>>>>
>>>>> Signed-off-by: Li Nan <linan122@huawei.com>
>>>>> ---
>>>>>     drivers/md/raid1.c | 19 ++++++++-----------
>>>>>     1 file changed, 8 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>>>>> index 95504612b7e2..d75c5dd89e86 100644
>>>>> --- a/drivers/md/raid1.c
>>>>> +++ b/drivers/md/raid1.c
>>>>> @@ -3140,19 +3140,16 @@ static int raid1_run(struct mddev *mddev)
>>>>
>>>> I'm not sure about this behaviour, 'logical_block_size' can be
>>>> increased while adding new underlying disk, the key point is not when
>>>> to increase 'logical_block_size'. If there is a mounted fs, or
>>>> partition in the array, I think the array will be corrupted.
>>>
>>> How common is such fs/partition corruption? I think some fs and partition
>>> table can work properly with 512=>4096 change?
>>
>> For fs, that should depend on fs bs that is usually set in mkfs, if bs
>> is less than 4096, then such fs can't be mounted.
>>
>> For partition, that is much worse, start sector and end sector will stay
>> the same, while sector size is changed. And 4096 -> 512 change is the
>> same.
> 
> Thanks for this information.
> 
>>>>
>>>> Perhaps once that array is started, logical_block_size should not be
>>>> changed anymore, this will require 'logical_block_size' to be metadate
>>>> inside raid superblock. And the array should deny any new disk with
>>>> bigger logical_block_size.
> 
> I really hope we can avoid adding this to the raid superblock. But I am not
> sure what would be a better solution (that is also backward compatible).
> Do we have real world reports of such issues?

Yes, our customer is using raid1 with one 4k disk and other 512 disk as
root device, and they reported that if 4k disk is kicked out from the
array, then system can't reboot.

And for backward compatible, I think it can be solved by continue to use
biggest block size from uderlying disks if metadata is 0.

Thanks,
Kuai


> 
> Thanks,
> Song
> .
>