Re: [PATCH RFC -next v2 11/41] md/md-bitmap: simplify md_bitmap_create() + md_bitmap_load()

[Yu Kuai <yukuai1@xxxxxxxxxxxxxxx>]

Hi,

在 2024/08/19 16:10, Su Yue 写道:
> On Wed 14 Aug 2024 at 15:10, Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote:
>
> > From: Yu Kuai <yukuai3@xxxxxxxxxx>
> >
> > Other than internal api get_bitmap_from_slot(), all other places will
> > set returned bitmap to mddev->bitmap. So move the setting of
> > mddev->bitmap into md_bitmap_create() to simplify code.
> >
> > Signed-off-by: Yu Kuai <yukuai3@xxxxxxxxxx>
> > ---
> >  drivers/md/md-bitmap.c | 23 +++++++++++++++--------
> >  drivers/md/md-bitmap.h |  2 +-
> >  drivers/md/md.c        | 30 +++++++++---------------------
> >  3 files changed, 25 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> > index eed3b930ade4..75e58da9a1a5 100644
> > --- a/drivers/md/md-bitmap.c
> > +++ b/drivers/md/md-bitmap.c
> > @@ -1879,7 +1879,7 @@ void md_bitmap_destroy(struct mddev *mddev)
> >   * if this returns an error, bitmap_destroy must be called to   do 
> > clean up
> >   * once mddev->bitmap is set
> >   */
> > -struct bitmap *md_bitmap_create(struct mddev *mddev, int slot)
> > +static struct bitmap *bitmap_create(struct mddev *mddev, int slot)
> >  {
> >      struct bitmap *bitmap;
> >      sector_t blocks = mddev->resync_max_sectors;
> > @@ -1966,6 +1966,17 @@ struct bitmap *md_bitmap_create(struct mddev 
> > *mddev, int slot)
> >      return ERR_PTR(err);
> >  }
> >
> > +int md_bitmap_create(struct mddev *mddev, int slot)
> NIT: We have two functions named md_bitmap_create() now. The static
> one will be renamed to __md_bitmap_create in next patch. Better to rename
> in this patch.

The static is renamed to bitmap_create() in this patch. And in the next
patch, the static is renamed to __bitmap_create() while the exported one
is renamed to bitmap_create().

I'll rename the static one to __bitmap_create() directly.

Thanks!
Kuai
> --
> Su
>
> > +{
> > +    struct bitmap *bitmap = bitmap_create(mddev, slot);
> > +
> > +    if (IS_ERR(bitmap))
> > +        return PTR_ERR(bitmap);
> > +
> > +    mddev->bitmap = bitmap;
> > +    return 0;
> > +}
> > +
> >  int md_bitmap_load(struct mddev *mddev)
> >  {
> >      int err = 0;
> > @@ -2030,7 +2041,7 @@ struct bitmap *get_bitmap_from_slot(struct mddev 
> > *mddev, int slot)
> >      int rv = 0;
> >      struct bitmap *bitmap;
> >
> > -    bitmap = md_bitmap_create(mddev, slot);
> > +    bitmap = bitmap_create(mddev, slot);
> >      if (IS_ERR(bitmap)) {
> >          rv = PTR_ERR(bitmap);
> >          return ERR_PTR(rv);
> > @@ -2381,7 +2392,6 @@ location_store(struct mddev *mddev, const char 
> > *buf, size_t len)
> >      } else {
> >          /* No bitmap, OK to set a location */
> >          long long offset;
> > -        struct bitmap *bitmap;
> >
> >          if (strncmp(buf, "none", 4) == 0)
> >              /* nothing to be done */;
> > @@ -2408,13 +2418,10 @@ location_store(struct mddev *mddev, const char 
> > *buf, size_t len)
> >              }
> >
> >              mddev->bitmap_info.offset = offset;
> > -            bitmap = md_bitmap_create(mddev, -1);
> > -            if (IS_ERR(bitmap)) {
> > -                rv = PTR_ERR(bitmap);
> > +            rv = md_bitmap_create(mddev, -1);
> > +            if (rv)
> >                  goto out;
> > -            }
> >
> > -            mddev->bitmap = bitmap;
> >              rv = md_bitmap_load(mddev);
> >              if (rv) {
> >                  mddev->bitmap_info.offset = 0;
> > diff --git a/drivers/md/md-bitmap.h b/drivers/md/md-bitmap.h
> > index a8a5d4804174..e187f9099f2e 100644
> > --- a/drivers/md/md-bitmap.h
> > +++ b/drivers/md/md-bitmap.h
> > @@ -252,7 +252,7 @@ struct bitmap_operations {
> >  void mddev_set_bitmap_ops(struct mddev *mddev);
> >
> >  /* these are used only by md/bitmap */
> > -struct bitmap *md_bitmap_create(struct mddev *mddev, int slot);
> > +int md_bitmap_create(struct mddev *mddev, int slot);
> >  int md_bitmap_load(struct mddev *mddev);
> >  void md_bitmap_flush(struct mddev *mddev);
> >  void md_bitmap_destroy(struct mddev *mddev);
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index f67f2540fd6c..6e130f6c2abd 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -6211,16 +6211,10 @@ int md_run(struct mddev *mddev)
> >      }
> >      if (err == 0 && pers->sync_request &&
> >          (mddev->bitmap_info.file || mddev->bitmap_info.offset)) {
> > -        struct bitmap *bitmap;
> > -
> > -        bitmap = md_bitmap_create(mddev, -1);
> > -        if (IS_ERR(bitmap)) {
> > -            err = PTR_ERR(bitmap);
> > +        err = md_bitmap_create(mddev, -1);
> > +        if (err)
> >              pr_warn("%s: failed to create bitmap (%d)\n",
> >                  mdname(mddev), err);
> > -        } else
> > -            mddev->bitmap = bitmap;
> > -
> >      }
> >      if (err)
> >          goto bitmap_abort;
> > @@ -7275,14 +7269,10 @@ static int set_bitmap_file(struct mddev 
> > *mddev, int fd)
> >      err = 0;
> >      if (mddev->pers) {
> >          if (fd >= 0) {
> > -            struct bitmap *bitmap;
> > -
> > -            bitmap = md_bitmap_create(mddev, -1);
> > -            if (!IS_ERR(bitmap)) {
> > -                mddev->bitmap = bitmap;
> > +            err = md_bitmap_create(mddev, -1);
> > +            if (!err)
> >                  err = md_bitmap_load(mddev);
> > -            } else
> > -                err = PTR_ERR(bitmap);
> > +
> >              if (err) {
> >                  md_bitmap_destroy(mddev);
> >                  fd = -1;
> > @@ -7291,6 +7281,7 @@ static int set_bitmap_file(struct mddev *mddev, 
> > int fd)
> >              md_bitmap_destroy(mddev);
> >          }
> >      }
> > +
> >      if (fd < 0) {
> >          struct file *f = mddev->bitmap_info.file;
> >          if (f) {
> > @@ -7559,7 +7550,6 @@ static int update_array_info(struct mddev 
> > *mddev, mdu_array_info_t *info)
> >              goto err;
> >          }
> >          if (info->state & (1<<MD_SB_BITMAP_PRESENT)) {
> > -            struct bitmap *bitmap;
> >              /* add the bitmap */
> >              if (mddev->bitmap) {
> >                  rv = -EEXIST;
> > @@ -7573,12 +7563,10 @@ static int update_array_info(struct mddev 
> > *mddev, mdu_array_info_t *info)
> >                  mddev->bitmap_info.default_offset;
> >              mddev->bitmap_info.space =
> >                  mddev->bitmap_info.default_space;
> > -            bitmap = md_bitmap_create(mddev, -1);
> > -            if (!IS_ERR(bitmap)) {
> > -                mddev->bitmap = bitmap;
> > +            rv = md_bitmap_create(mddev, -1);
> > +            if (!rv)
> >                  rv = md_bitmap_load(mddev);
> > -            } else
> > -                rv = PTR_ERR(bitmap);
> > +
> >              if (rv)
> >                  md_bitmap_destroy(mddev);
> >          } else {
> .