Re: [PATCH 1/2] md/raid0: Free r0conf memory when register integrity failed

[Guoqing Jiang <guoqing.jiang@linux.dev>]

On 12/10/21 9:18 AM, Xiao Ni wrote:
> On Fri, Dec 10, 2021 at 2:02 AM Song Liu <song@kernel.org> wrote:
>> On Wed, Dec 8, 2021 at 9:55 PM Xiao Ni <xni@redhat.com> wrote:
>>> It doesn't free memory when register integrity failed. And move
>>> free conf codes into a single function.
>>>
>>> Signed-off-by: Xiao Ni <xni@redhat.com>
>>> ---
>>>   drivers/md/raid0.c | 18 +++++++++++++++---
>>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
>>> index 62c8b6adac70..3fa47df1c60e 100644
>>> --- a/drivers/md/raid0.c
>>> +++ b/drivers/md/raid0.c
>>> @@ -356,6 +356,7 @@ static sector_t raid0_size(struct mddev *mddev, sector_t sectors, int raid_disks
>>>          return array_sectors;
>>>   }
>>>
>>> +static void free_conf(struct r0conf *conf);
>>>   static void raid0_free(struct mddev *mddev, void *priv);
>>>
>>>   static int raid0_run(struct mddev *mddev)
>>> @@ -413,19 +414,30 @@ static int raid0_run(struct mddev *mddev)
>>>          dump_zones(mddev);
>>>
>>>          ret = md_integrity_register(mddev);
>>> +       if (ret)
>>> +               goto free;
>>>
>>>          return ret;
>>> +
>>> +free:
>>> +       free_conf(conf);
>> Can we just use raid0_free() here? Also, after freeing conf,
> At first I used raid0_free too. But it looks strange after adding
> acct_bioset_exit
> in raid0_free. Because if creating stripe zones failed, it doesn't
> need to free conf,
> although it doesn't have problems that passes NULL to kfree.

Maybe something like this, just FYI.

diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index 62c8b6adac70..c24bec49b36f 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -377,6 +377,12 @@ static int raid0_run(struct mddev *mddev)
                         return ret;
                 mddev->private = conf;
         }
+
+       if (acct_bioset_init(mddev)) {
+               pr_err("md/raid0:%s: alloc acct bioset failed.\n", 
mdname(mddev));
+               return -ENOMEM;
+       }
+
         conf = mddev->private;
         if (mddev->queue) {
                 struct md_rdev *rdev;
@@ -413,6 +419,11 @@ static int raid0_run(struct mddev *mddev)
         dump_zones(mddev);

         ret = md_integrity_register(mddev);
+       if (ret) {
+               raid0_free(mddev, mddev->private);
+               mddev->private = NULL;
+               acct_bioset_exit(mddev);
+       }

         return ret;
  }

Thanks,
Guoqing