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

Su Yue
2024-08-19T08:18:12Z

On Wed 14 Aug 2024 at 15:10, Yu Kuai <yukuai1@huaweicloud.com>
wrote:

> From: Yu Kuai <yukuai3@huawei.com>
>
> 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@huawei.com>
> ---
> 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.

--
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 {