| Message ID | 20230906093720.1070929-1-linan122@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Song Liu |
| Headers | show |
| Series | md/raid1: only update stack limits with the device in use | expand |
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 > > */ > > >
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, Kuai > > 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 > . >
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) 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 */
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(-)