Re: [PATCH v3] md: Allow setting persistent superblock version for md= command line

[Yu Kuai <yukuai1@huaweicloud.com>]

Hi,

在 2025/08/27 15:26, Yu Kuai 写道:
> Hi,
> 
> 在 2025/08/25 22:53, jeremias@jears.at 写道:
>> This allows for setting a superblock version on the kernel command 
>> line to be
>> able to assemble version >=1.0 arrays. It can optionally be set like 
>> this:
>>
>> md=vX.X,...
>>
>> This will set the version of the array before assembly so it can be 
>> assembled
>> correctly.
>>
> 
> You should explain that current autodetect is only supported for 0.90
> array.
> 
>> Also updated docs accordingly.
>>
>> v2: Use pr_warn instead of printk
>>
>> v3: Change order of options so it stays with past pattern
>>
>> Signed-off-by: Jeremias Stotter <jeremias@jears.at>
>> ---
>>   Documentation/admin-guide/md.rst |  8 +++++
>>   drivers/md/md-autodetect.c       | 59 ++++++++++++++++++++++++++++++--
>>   2 files changed, 65 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/md.rst 
>> b/Documentation/admin-guide/md.rst
>> index 4ff2cc291d18..f57ae871c997 100644
>> --- a/Documentation/admin-guide/md.rst
>> +++ b/Documentation/admin-guide/md.rst
>> @@ -23,6 +23,14 @@ or, to assemble a partitionable array::
>>
>>     md=d<md device no.>,dev0,dev1,...,devn
>>
>> +if you are using superblock versions greater than 0, use the following::
>> +
>> +  md=<md device no.>,v<superblock version no.>,dev0,dev1,...,devn
>> +
>> +for example, for a raid array with superblock version 1.2 it could 
>> look like this::
>> +
>> +  md=0,v1.2,/dev/sda1,/dev/sdb1
>> +
>>   ``md device no.``
>>   +++++++++++++++++
>>
> 
> What about md_autostart_arrays()? where 0.90 is still the only default
> choice.
>> diff --git a/drivers/md/md-autodetect.c b/drivers/md/md-autodetect.c
>> index 4b80165afd23..67d38559ad50 100644
>> --- a/drivers/md/md-autodetect.c
>> +++ b/drivers/md/md-autodetect.c
>> @@ -32,6 +32,8 @@ static struct md_setup_args {
>>       int partitioned;
>>       int level;
>>       int chunk;
>> +    int major_version;
>> +    int minor_version;
>>       char *device_names;
>>   } md_setup_args[256] __initdata;
>>
>> @@ -63,6 +65,7 @@ static int __init md_setup(char *str)
>>       char *pername = "";
>>       char *str1;
>>       int ent;
>> +    int major_i = 0, minor_i = 0;
>>
>>       if (*str == 'd') {
>>           partitioned = 1;
>> @@ -109,6 +112,49 @@ static int __init md_setup(char *str)
>>       case 0:
>>           md_setup_args[ent].level = LEVEL_NONE;
>>           pername="super-block";
>> +
>> +        if (*str == 'v') { /* Superblock version */
>> +            char *version = ++str;
>> +            char *version_end = strchr(str, ',');
>> +
>> +            if (!version_end) {
>> +                pr_warn("md: Version (%s) has been specified wrongly, 
>> no ',' found, use like this: md=<md dev. no.>,X.X,...\n",
>> +                    version);
>> +                return 0;
>> +            }
>> +            *version_end = '\0';
>> +            str = version_end + 1;
>> +
>> +            char *separator = strchr(version, '.');
>> +
>> +            if (!separator) {
>> +                pr_warn("md: Version (%s) has been specified wrongly, 
>> no '.' to separate major and minor version found, use like this: 
>> md=<md dev. no.>,vX.X,...\n",
>> +                    version);
>> +                return 0;
>> +            }
>> +            *separator = '\0';
>> +            char *minor_s = separator + 1;
>> +
>> +            int ret = kstrtoint(version, 10, &major_i);
>> +
>> +            if (ret != 0) {
>> +                pr_warn("md: Version has been specified wrongly, 
>> couldn't convert major '%s' to number, use like this: md=<md dev. 
>> no.>,vX.X,...\n",
>> +                    version);
>> +                return 0;
>> +            }
>> +            if (major_i != 0 && major_i != 1) {
>> +                pr_warn("md: Major version %d is not valid, use 0 or 
>> 1\n",
>> +                    major_i);
>> +                return 0;
>> +            }
>> +            ret = kstrtoint(minor_s, 10, &minor_i);
>> +            if (ret != 0) {
>> +                pr_warn("md: Version has been specified wrongly, 
>> couldn't convert minor '%s' to number, use like this: md=<md dev. 
>> no.>,vX.X,...\n",
>> +                    minor_s);
>> +                return 0;
>> +            }
>> +        }
>> +
>>       }
>>
>>       printk(KERN_INFO "md: Will configure md%d (%s) from %s, below.\n",
>> @@ -116,6 +162,8 @@ static int __init md_setup(char *str)
>>       md_setup_args[ent].device_names = str;
>>       md_setup_args[ent].partitioned = partitioned;
>>       md_setup_args[ent].minor = minor;
>> +    md_setup_args[ent].minor_version = minor_i;
>> +    md_setup_args[ent].major_version = major_i;
>>
>>       return 1;
>>   }
>> @@ -200,6 +248,9 @@ static void __init md_setup_drive(struct 
>> md_setup_args *args)
>>
>>       err = md_set_array_info(mddev, &ainfo);
>>
>> +    mddev->major_version = args->major_version;
>> +    mddev->minor_version = args->minor_version;
> 
> I would expect to fix md_set_array_info() to hanlde this new case, to
> make code more readable.
> 
>> +
>>       for (i = 0; i <= MD_SB_DISKS && devices[i]; i++) {
>>           struct mdu_disk_info_s dinfo = {
>>               .major    = MAJOR(devices[i]),
>> @@ -273,11 +324,15 @@ void __init md_run_setup(void)
>>   {
>>       int ent;
>>
>> +    /*
>> +     * Assemble manually defined raids first
>> +     */
>> +    for (ent = 0; ent < md_setup_ents; ent++)
>> +        md_setup_drive(&md_setup_args[ent]);
>> +

And BTW, take a closer look at autodetect code, although you set
mddev->major_version to 1, however, md_set_array_info() will set
mddev->raid_disks while mddev->pers is still NULL, hence from
md_add_new_disk(), -EINVAL will be returned directly:

md_add_new_disk
	if (!mddev->raid_disks)
		......
		return;
	if (mddev->pers)
		......
		return;

         /* otherwise, md_add_new_disk is only allowed
         ┊* for major_version==0 superblocks
         ┊*/
         if (mddev->major_version != 0) {
                 pr_warn("%s: ADD_NEW_DISK not supported\n", mdname(mddev));
                 return -EINVAL;
         }

What am I missing?

Thanks,
Kuai

> 
> You just explain what you did in comment, Why do you change the order?
> 
> Thanks,
> Kuai
> 
>>       if (raid_noautodetect)
>>           printk(KERN_INFO "md: Skipping autodetection of RAID arrays. 
>> (raid=autodetect will force)\n");
>>       else
>>           autodetect_raid();
>>
>> -    for (ent = 0; ent < md_setup_ents; ent++)
>> -        md_setup_drive(&md_setup_args[ent]);
>>   }
>>
>> .
>>
> 
> .
> 

And BTW, take a closer look at autodetect code, alouth