diff mbox series

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

Message ID 20240810020854.797814-8-yukuai1@huaweicloud.com (mailing list archive)
State Superseded
Headers show
Series md/md-bitmap: introduce bitmap_operations | expand

Commit Message

Yu Kuai Aug. 10, 2024, 2:08 a.m. UTC 
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.

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(-)

Comments

Mariusz Tkaczyk Aug. 13, 2024, 7:17 a.m. UTC | #1 
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
Yu Kuai Aug. 13, 2024, 7:33 a.m. UTC | #2 
Hi,

在 2024/08/13 15:17, Mariusz Tkaczyk 写道:
> 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?

mddev->bitmap will be an option for the array with bitmap=none. However,
!mddev->bitmap_ops->update_sb is indeed not an option.

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

Yes, but for the similiar commit message, I'm not sure how to improve
this for now. We don't want a large patch to merge all the ops at once.

Thanks,
Kuai

> 
> Thanks
> Mariusz
> 
> 
> .
>
diff mbox series

Patch

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);
 
 	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;
+
+	mddev->bitmap_ops->update_sb(mddev->bitmap);
+}
+
 void md_bitmap_status(struct seq_file *seq, struct bitmap *bitmap);
 
 int  md_bitmap_setallbits(struct bitmap *bitmap);
diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
index 1d0db62f0351..79e67393fee0 100644
--- a/drivers/md/md-cluster.c
+++ b/drivers/md/md-cluster.c
@@ -1244,7 +1244,8 @@  static int cluster_check_sync_size(struct mddev *mddev)
 		bm_lockres->flags |= DLM_LKF_NOQUEUE;
 		rv = dlm_lock_sync(bm_lockres, DLM_LOCK_PW);
 		if (!rv)
-			md_bitmap_update_sb(bitmap);
+			mddev->bitmap_ops->update_sb(bitmap);
+
 		lockres_free(bm_lockres);
 
 		sb = kmap_atomic(bitmap->storage.sb_page);
diff --git a/drivers/md/md.c b/drivers/md/md.c
index f23138dd96a7..e749e24e12de 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2821,7 +2821,7 @@  void md_update_sb(struct mddev *mddev, int force_change)
 
 	mddev_add_trace_msg(mddev, "md md_update_sb");
 rewrite:
-	md_bitmap_update_sb(mddev->bitmap);
+	md_bitmap_update_sb(mddev);
 	rdev_for_each(rdev, mddev) {
 		if (rdev->sb_loaded != 1)
 			continue; /* no noise on spare devices */
@@ -9966,7 +9966,7 @@  static void check_sb_changes(struct mddev *mddev, struct md_rdev *rdev)
 		if (ret)
 			pr_info("md-cluster: resize failed\n");
 		else
-			md_bitmap_update_sb(mddev->bitmap);
+			md_bitmap_update_sb(mddev);
 	}
 
 	/* Check for change of roles in the active devices */