Re: [PATCH RFC -next 07/26] md/md-bitmap: merge md_bitmap_update_sb() into bitmap_operations

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

On Sat, 10 Aug 2024 10:08:35 +0800
Yu Kuai <yukuai1@huaweicloud.com> wrote:

> From: Yu Kuai <yukuai3@huawei.com>
> 
> So that the implementation won't be exposed, and it'll be possible
> to invent a new bitmap by replacing bitmap_operations.

Please update commit message.

> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  drivers/md/md-bitmap.c  | 15 ++++++++-------
>  drivers/md/md-bitmap.h  | 11 ++++++++++-
>  drivers/md/md-cluster.c |  3 ++-
>  drivers/md/md.c         |  4 ++--
>  4 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> index 0ff733756043..b34f13aa2697 100644
> --- a/drivers/md/md-bitmap.c
> +++ b/drivers/md/md-bitmap.c
> @@ -472,7 +472,7 @@ static void md_bitmap_wait_writes(struct bitmap *bitmap)
>  
>  
>  /* update the event counter and sync the superblock to disk */
> -void md_bitmap_update_sb(struct bitmap *bitmap)
> +static void bitmap_update_sb(struct bitmap *bitmap)
>  {
>  	bitmap_super_t *sb;
>  
> @@ -510,7 +510,6 @@ void md_bitmap_update_sb(struct bitmap *bitmap)
>  		write_sb_page(bitmap, bitmap->storage.sb_index,
>  			      bitmap->storage.sb_page, 1);
>  }
> -EXPORT_SYMBOL(md_bitmap_update_sb);
>  
>  /* print out the bitmap file superblock */
>  static void bitmap_print_sb(struct bitmap *bitmap)
> @@ -893,7 +892,7 @@ static void md_bitmap_file_unmap(struct bitmap_storage
> *store) static void md_bitmap_file_kick(struct bitmap *bitmap)
>  {
>  	if (!test_and_set_bit(BITMAP_STALE, &bitmap->flags)) {
> -		md_bitmap_update_sb(bitmap);
> +		bitmap_update_sb(bitmap);
>  
>  		if (bitmap->storage.file) {
>  			pr_warn("%s: kicking failed bitmap file %pD4 from
> array!\n", @@ -1796,7 +1795,7 @@ static void bitmap_flush(struct mddev *mddev)
>  	md_bitmap_daemon_work(mddev);
>  	if (mddev->bitmap_info.external)
>  		md_super_wait(mddev);
> -	md_bitmap_update_sb(bitmap);
> +	bitmap_update_sb(bitmap);
>  }
>  
>  /*
> @@ -2014,7 +2013,7 @@ static int bitmap_load(struct mddev *mddev)
>  	mddev_set_timeout(mddev, mddev->bitmap_info.daemon_sleep, true);
>  	md_wakeup_thread(mddev->thread);
>  
> -	md_bitmap_update_sb(bitmap);
> +	bitmap_update_sb(bitmap);

You changed function name here and it is harmful for git blame. What is the
reason behind that? it must be described in commit message to help Song making
the decision if it is worthy merging or not.

>  
>  	if (test_bit(BITMAP_WRITE_ERROR, &bitmap->flags))
>  		err = -EIO;
> @@ -2075,7 +2074,7 @@ int md_bitmap_copy_from_slot(struct mddev *mddev, int
> slot, }
>  
>  	if (clear_bits) {
> -		md_bitmap_update_sb(bitmap);
> +		bitmap_update_sb(bitmap);
>  		/* BITMAP_PAGE_PENDING is set, but bitmap_unplug needs
>  		 * BITMAP_PAGE_DIRTY or _NEEDWRITE to write ... */
>  		for (i = 0; i < bitmap->storage.file_pages; i++)
> @@ -2568,7 +2567,7 @@ backlog_store(struct mddev *mddev, const char *buf,
> size_t len) mddev_create_serial_pool(mddev, rdev);
>  	}
>  	if (old_mwb != backlog)
> -		md_bitmap_update_sb(mddev->bitmap);
> +		bitmap_update_sb(mddev->bitmap);
>  
>  	mddev_unlock_and_resume(mddev);
>  	return len;
> @@ -2712,6 +2711,8 @@ static struct bitmap_operations bitmap_ops = {
>  	.load			= bitmap_load,
>  	.destroy		= bitmap_destroy,
>  	.flush			= bitmap_flush,
> +
> +	.update_sb		= bitmap_update_sb,
>  };
>  
>  void mddev_set_bitmap_ops(struct mddev *mddev)
> diff --git a/drivers/md/md-bitmap.h b/drivers/md/md-bitmap.h
> index 935c5dc45b89..29c217630ae5 100644
> --- a/drivers/md/md-bitmap.h
> +++ b/drivers/md/md-bitmap.h
> @@ -239,6 +239,8 @@ struct bitmap_operations {
>  	int (*load)(struct mddev *mddev);
>  	void (*destroy)(struct mddev *mddev);
>  	void (*flush)(struct mddev *mddev);
> +
> +	void (*update_sb)(struct bitmap *bitmap);
>  };
>  
>  /* the bitmap API */
> @@ -277,7 +279,14 @@ static inline void md_bitmap_flush(struct mddev *mddev)
>  	mddev->bitmap_ops->flush(mddev);
>  }
>  
> -void md_bitmap_update_sb(struct bitmap *bitmap);
> +static inline void md_bitmap_update_sb(struct mddev *mddev)
> +{
> +	if (!mddev->bitmap || !mddev->bitmap_ops->update_sb)
> +		return;

I would like to avoid dead code here. !mddev->bitmap is probably not an option
an this point in code. !mddev->bitmap_ops->update_sb i not an option because we
have only one bitmap op. Do I miss something?

I will stop here for today now to give you a chance to reply, to be sure that
we are on same page. I see that my comments are similar so it may not be worthy
to go one by one and repeat same comment. I may miss something important.

Thanks
Mariusz