Re: [PATCH 1/2] md/raid0: don't free conf on raid0_run failure

[Yu Kuai <yukuai1@huaweicloud.com>]

Hi,

在 2024/06/07 14:08, Yu Kuai 写道:
> 在 2024/06/05 1:25, Christoph Hellwig 写道:
>> The core md code calls the ->free method which already frees conf.
>>
>> Fixes: 0c031fd37f69 ("md: Move alloc/free acct bioset in to personality")
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> LGTM
> Reviewed-by: Yu Kuai <yukuai3@huawei.com>

Sorry that I just noticed that the sysfs api level_store() calls the
pers->run() as well, and it didn't handle failure by pers->free().
It's weried that the api will return success in this case, and after
this patch, it will require __md_stop() to free the conf.

Can you also fix the level_store()? By checking pers->run() and if it
failed, call pers->free() and return error.

Thanks,
Kuai

> 
>> ---
>>   drivers/md/raid0.c | 21 +++++----------------
>>   1 file changed, 5 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
>> index c5d4aeb68404c9..81c01347cd24e6 100644
>> --- a/drivers/md/raid0.c
>> +++ b/drivers/md/raid0.c
>> @@ -365,18 +365,13 @@ static sector_t raid0_size(struct mddev *mddev, 
>> sector_t sectors, int raid_disks
>>       return array_sectors;
>>   }
>> -static void free_conf(struct mddev *mddev, struct r0conf *conf)
>> -{
>> -    kfree(conf->strip_zone);
>> -    kfree(conf->devlist);
>> -    kfree(conf);
>> -}
>> -
>>   static void raid0_free(struct mddev *mddev, void *priv)
>>   {
>>       struct r0conf *conf = priv;
>> -    free_conf(mddev, conf);
>> +    kfree(conf->strip_zone);
>> +    kfree(conf->devlist);
>> +    kfree(conf);
>>   }
>>   static int raid0_set_limits(struct mddev *mddev)
>> @@ -415,7 +410,7 @@ static int raid0_run(struct mddev *mddev)
>>       if (!mddev_is_dm(mddev)) {
>>           ret = raid0_set_limits(mddev);
>>           if (ret)
>> -            goto out_free_conf;
>> +            return ret;
>>       }
>>       /* calculate array device size */
>> @@ -427,13 +422,7 @@ static int raid0_run(struct mddev *mddev)
>>       dump_zones(mddev);
>> -    ret = md_integrity_register(mddev);
>> -    if (ret)
>> -        goto out_free_conf;
>> -    return 0;
>> -out_free_conf:
>> -    free_conf(mddev, conf);
>> -    return ret;
>> +    return md_integrity_register(mddev);
>>   }
>>   /*
>>
> 
> .
>