| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b7fc02d2-7643-4bf1-1b15-c1ecdf883c87@huaweicloud.com>
Date: Tue, 23 Sep 2025 21:36:59 +0800
From: Li Nan <linan666@...weicloud.com>
To: Xiao Ni <xni@...hat.com>, linan666@...weicloud.com
Cc: corbet@....net, song@...nel.org, yukuai3@...wei.com, hare@...e.de,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-raid@...r.kernel.org, martin.petersen@...cle.com,
yangerkun@...wei.com, yi.zhang@...wei.com
Subject: Re: [PATCH v5 2/2] md: allow configuring logical block size
在 2025/9/23 19:36, Xiao Ni 写道:
> Hi Li Nan
>
> On Thu, Sep 18, 2025 at 8:08 PM <linan666@...weicloud.com> wrote:
>>
>> From: Li Nan <linan122@...wei.com>
>>
>> Previously, raid array used the maximum logical block size (LBS)
>> of all member disks. Adding a larger LBS disk at runtime could
>> unexpectedly increase RAID's LBS, risking corruption of existing
>> partitions. This can be reproduced by:
>>
>> ```
>> # LBS of sd[de] is 512 bytes, sdf is 4096 bytes.
>> mdadm -CRq /dev/md0 -l1 -n3 /dev/sd[de] missing --assume-clean
>>
>> # LBS is 512
>> cat /sys/block/md0/queue/logical_block_size
>>
>> # create partition md0p1
>> parted -s /dev/md0 mklabel gpt mkpart primary 1MiB 100%
>> lsblk | grep md0p1
>>
>> # LBS becomes 4096 after adding sdf
>> mdadm --add -q /dev/md0 /dev/sdf
>> cat /sys/block/md0/queue/logical_block_size
>>
>> # partition lost
>> partprobe /dev/md0
>> lsblk | grep md0p1
>> ```
>
> Thanks for the reproducer. I can reproduce it myself.
>
>>
>> Simply restricting larger-LBS disks is inflexible. In some scenarios,
>> only disks with 512 bytes LBS are available currently, but later, disks
>> with 4KB LBS may be added to the array.
>
> If we add a disk with 4KB LBS and configure it to 4KB by the sysfs
> interface, how can we make the partition table readable and avoid the
> problem mentioned above?
>
Thanks for your review.
The main cause of partition loss is LBS changes. Therefore, we should
specify a 4K LBS at creation time, instead of modifying LBS after the RAID
is already in use. For example:
mdadm -C --logical-block-size=4096 ...
In this way, even if all underlying disks are 512-byte, the RAID will be
created with a 4096 LBS. Adding 4096-byte disks later will not cause any
issues.
>>
>> Making LBS configurable is the best way to solve this scenario.
>> After this patch, the raid will:
>> - store LBS in disk metadata
>> - add a read-write sysfs 'mdX/logical_block_size'
>>
>> Future mdadm should support setting LBS via metadata field during RAID
>> creation and the new sysfs. Though the kernel allows runtime LBS changes,
>> users should avoid modifying it after creating partitions or filesystems
>> to prevent compatibility issues.
>>
>> Only 1.x metadata supports configurable LBS. 0.90 metadata inits all
>> fields to default values at auto-detect. Supporting 0.90 would require
>> more extensive changes and no such use case has been observed.
>>
>> Note that many RAID paths rely on PAGE_SIZE alignment, including for
>> metadata I/O. A larger LBS than PAGE_SIZE will result in metadata
>> read/write failures. So this config should be prevented.
>>
>> Signed-off-by: Li Nan <linan122@...wei.com>
>> ---
>> Documentation/admin-guide/md.rst | 7 +++
>> drivers/md/md.h | 1 +
>> include/uapi/linux/raid/md_p.h | 3 +-
>> drivers/md/md-linear.c | 1 +
>> drivers/md/md.c | 75 ++++++++++++++++++++++++++++++++
>> drivers/md/raid0.c | 1 +
>> drivers/md/raid1.c | 1 +
>> drivers/md/raid10.c | 1 +
>> drivers/md/raid5.c | 1 +
>> 9 files changed, 90 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/admin-guide/md.rst b/Documentation/admin-guide/md.rst
>> index 1c2eacc94758..f5c81fad034a 100644
>> --- a/Documentation/admin-guide/md.rst
>> +++ b/Documentation/admin-guide/md.rst
>> @@ -238,6 +238,13 @@ All md devices contain:
>> the number of devices in a raid4/5/6, or to support external
>> metadata formats which mandate such clipping.
>>
>> + logical_block_size
>> + Configures the array's logical block size in bytes. This attribute
>> + is only supported for RAID1, RAID5, RAID10 with 1.x meta. The value
>
> s/RAID5/RAID456/g
>
I will fix it later. Thanks.
>> + should be written before starting the array. The final array LBS
>> + will use the max value between this configuration and all rdev's LBS.
>> + Note that LBS cannot exceed PAGE_SIZE.
>> +
>> reshape_position
>> This is either ``none`` or a sector number within the devices of
>> the array where ``reshape`` is up to. If this is set, the three
>> diff --git a/drivers/md/md.h b/drivers/md/md.h
>> index afb25f727409..b0147b98c8d3 100644
>> --- a/drivers/md/md.h
>> +++ b/drivers/md/md.h
>> @@ -432,6 +432,7 @@ struct mddev {
>> sector_t array_sectors; /* exported array size */
>> int external_size; /* size managed
>> * externally */
>> + unsigned int logical_block_size;
>> __u64 events;
>> /* If the last 'event' was simply a clean->dirty transition, and
>> * we didn't write it to the spares, then it is safe and simple
>> diff --git a/include/uapi/linux/raid/md_p.h b/include/uapi/linux/raid/md_p.h
>> index ac74133a4768..310068bb2a1d 100644
>> --- a/include/uapi/linux/raid/md_p.h
>> +++ b/include/uapi/linux/raid/md_p.h
>> @@ -291,7 +291,8 @@ struct mdp_superblock_1 {
>> __le64 resync_offset; /* data before this offset (from data_offset) known to be in sync */
>> __le32 sb_csum; /* checksum up to devs[max_dev] */
>> __le32 max_dev; /* size of devs[] array to consider */
>> - __u8 pad3[64-32]; /* set to 0 when writing */
>> + __le32 logical_block_size; /* same as q->limits->logical_block_size */
>> + __u8 pad3[64-36]; /* set to 0 when writing */
>>
>> /* device state information. Indexed by dev_number.
>> * 2 bytes per device
>> diff --git a/drivers/md/md-linear.c b/drivers/md/md-linear.c
>> index 5d9b08115375..da8babb8da59 100644
>> --- a/drivers/md/md-linear.c
>> +++ b/drivers/md/md-linear.c
>> @@ -72,6 +72,7 @@ static int linear_set_limits(struct mddev *mddev)
>>
>> md_init_stacking_limits(&lim);
>> lim.max_hw_sectors = mddev->chunk_sectors;
>> + lim.logical_block_size = mddev->logical_block_size;
>> lim.max_write_zeroes_sectors = mddev->chunk_sectors;
>> lim.io_min = mddev->chunk_sectors << 9;
>> err = mddev_stack_rdev_limits(mddev, &lim, MDDEV_STACK_INTEGRITY);
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 40f56183c744..e0184942c8ec 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -1963,6 +1963,7 @@ static int super_1_validate(struct mddev *mddev, struct md_rdev *freshest, struc
>> mddev->layout = le32_to_cpu(sb->layout);
>> mddev->raid_disks = le32_to_cpu(sb->raid_disks);
>> mddev->dev_sectors = le64_to_cpu(sb->size);
>> + mddev->logical_block_size = le32_to_cpu(sb->logical_block_size);
>> mddev->events = ev1;
>> mddev->bitmap_info.offset = 0;
>> mddev->bitmap_info.space = 0;
>> @@ -2172,6 +2173,7 @@ static void super_1_sync(struct mddev *mddev, struct md_rdev *rdev)
>> sb->chunksize = cpu_to_le32(mddev->chunk_sectors);
>> sb->level = cpu_to_le32(mddev->level);
>> sb->layout = cpu_to_le32(mddev->layout);
>> + sb->logical_block_size = cpu_to_le32(mddev->logical_block_size);
>> if (test_bit(FailFast, &rdev->flags))
>> sb->devflags |= FailFast1;
>> else
>> @@ -5900,6 +5902,66 @@ static struct md_sysfs_entry md_serialize_policy =
>> __ATTR(serialize_policy, S_IRUGO | S_IWUSR, serialize_policy_show,
>> serialize_policy_store);
>>
>> +static int mddev_set_logical_block_size(struct mddev *mddev,
>> + unsigned int lbs)
>> +{
>> + int err = 0;
>> + struct queue_limits lim;
>> +
>> + if (queue_logical_block_size(mddev->gendisk->queue) >= lbs) {
>> + pr_err("%s: incompatible logical_block_size %u, can not set\n",
>> + mdname(mddev), lbs);
>
> Is it better to print the mddev's LBS and give the message "it can't
> set lbs smaller than mddev logical block size"?
>
I agree. Let me improve this.
>> + return -EINVAL;
>> + }
>> +
>> + lim = queue_limits_start_update(mddev->gendisk->queue);
>> + lim.logical_block_size = lbs;
>> + pr_info("%s: logical_block_size is changed, data may be lost\n",
>> + mdname(mddev));
>> + err = queue_limits_commit_update(mddev->gendisk->queue, &lim);
>> + if (err)
>> + return err;
>> +
>> + mddev->logical_block_size = lbs;
>> + return 0;
>> +}
>> +
>> +static ssize_t
>> +lbs_show(struct mddev *mddev, char *page)
>> +{
>> + return sprintf(page, "%u\n", mddev->logical_block_size);
>> +}
>> +
>> +static ssize_t
>> +lbs_store(struct mddev *mddev, const char *buf, size_t len)
>> +{
>> + unsigned int lbs;
>> + int err = -EBUSY;
>> +
>> + /* Only 1.x meta supports configurable LBS */
>> + if (mddev->major_version == 0)
>> + return -EINVAL;
>
> It looks like it should check raid level here as doc mentioned above, right?
Yeah, kuai suggests supporting this feature only in 1.x meta.
--
Thanks,
Nan
Powered by blists - more mailing lists