[mdadm/master,v2,4/4] mdadm: add support for new lockless bitmap

Commit Message

Yu Kuai Nov. 7, 2024, 8:13 a.m. UTC

From: Yu Kuai <[e-mail]>

A new major number 6 is used for the new bitmap.

Noted that for the kernel that doesn't support lockless bitmap, create
such array will fail:

md0: invalid bitmap file superblock: unrecognized superblock version.

Signed-off-by: Yu Kuai <[e-mail]>
---
 Create.c | 5 ++++-
 Grow.c   | 3 ++-
 bitmap.h | 1 +
 mdadm.c  | 9 ++++++++-
 mdadm.h  | 1 +
 5 files changed, 16 insertions(+), 3 deletions(-)

Comments

Mariusz Tkaczyk Nov. 7, 2024, 9:02 a.m. UTC | #1

On Thu,  7 Nov 2024 16:13:47 +0800
Yu Kuai <[e-mail]> wrote:

> +	if (strcmp(val, "lockless") == 0) {
> +		s->btype = BitmapLockless;
> +		pr_info("Experimental lockless bitmap, use at your own
> disk!\n");
> +		return MDADM_STATUS_SUCCESS;
> +	}
> +

Hi Kuai,
I'm fine with previous patches. For this one, I'm not sure If I can take it yet.
The changes you added if for are not merged, therefore merging this looks bad
from process point of view (I'm merging feature that is not available in
kernel upstream). Am I missing something?

I would like to hear Song voice on that.

IMO, you should keep it as your own customization until development of new
bitmap is done but I understand that the topic is not simple and you might want
to people to test it so having mdadm build-in is an option.

If you really want this I would need a detailed process of "way to stable" in commit
message that I can always refer to. I'm challenging something like that first
time so I hope Song can add something.

Thanks,
Mariusz

Yu Kuai Nov. 7, 2024, 11:18 a.m. UTC | #2

Hi,

在 2024/11/07 17:02, Mariusz Tkaczyk 写道:
> On Thu,  7 Nov 2024 16:13:47 +0800
> Yu Kuai <[e-mail]> wrote:
> 
>> +	if (strcmp(val, "lockless") == 0) {
>> +		s->btype = BitmapLockless;
>> +		pr_info("Experimental lockless bitmap, use at your own
>> disk!\n");
>> +		return MDADM_STATUS_SUCCESS;
>> +	}
>> +
> 
> Hi Kuai,
> I'm fine with previous patches. For this one, I'm not sure If I can take it yet.
> The changes you added if for are not merged, therefore merging this looks bad
> from process point of view (I'm merging feature that is not available in
> kernel upstream). Am I missing something?

No, you're right that kernel changes are not ready yet. We just finish
testing the demo.
> 
> I would like to hear Song voice on that.
> 
> IMO, you should keep it as your own customization until development of new
> bitmap is done but I understand that the topic is not simple and you might want
> to people to test it so having mdadm build-in is an option.

Of course, I can remove this patch in the next version.
> 
> If you really want this I would need a detailed process of "way to stable" in commit
> message that I can always refer to. I'm challenging something like that first
> time so I hope Song can add something.

Thanks,
Kuai

> 
> Thanks,
> Mariusz
> 
> .
>

Patch

diff --git a/Create.c b/Create.c
index 29858483..3637af01 100644
--- a/Create.c
+++ b/Create.c
@@ -541,6 +541,8 @@  int Create(struct supertype *st, struct mddev_ident *ident, int subdevs,
 			pr_err("At least 2 nodes are needed for cluster-md\n");
 			return 1;
 		}
+	} else if (s->btype == BitmapLockless) {
+		major_num = BITMAP_MAJOR_LOCKLESS;
 	}
 
 	memset(&info, 0, sizeof(info));
@@ -1195,7 +1197,8 @@  int Create(struct supertype *st, struct mddev_ident *ident, int subdevs,
 	 * to stop another mdadm from finding and using those devices.
 	 */
 
-	if (s->btype == BitmapInternal || s->btype == BitmapCluster) {
+	if (s->btype == BitmapInternal || s->btype == BitmapCluster ||
+	    s->btype == BitmapLockless) {
 		if (!st->ss->add_internal_bitmap) {
 			pr_err("internal bitmaps not supported with %s metadata\n",
 				st->ss->name);
diff --git a/Grow.c b/Grow.c
index fae5b8a7..95a8da5e 100644
--- a/Grow.c
+++ b/Grow.c
@@ -383,7 +383,8 @@  int Grow_addbitmap(char *devname, int fd, struct context *c, struct shape *s)
 		free(mdi);
 	}
 
-	if (s->btype == BitmapInternal || s->btype == BitmapCluster) {
+	if (s->btype == BitmapInternal || s->btype == BitmapCluster ||
+	    s->btype == BitmapLockless) {
 		int rv;
 		int d;
 		int offset_setable = 0;
diff --git a/bitmap.h b/bitmap.h
index 7b1f80f2..4b5d2d93 100644
--- a/bitmap.h
+++ b/bitmap.h
@@ -13,6 +13,7 @@ 
 #define BITMAP_MAJOR_HI 4
 #define	BITMAP_MAJOR_HOSTENDIAN 3
 #define	BITMAP_MAJOR_CLUSTERED 5
+#define	BITMAP_MAJOR_LOCKLESS 6
 
 #define BITMAP_MINOR 39
 
diff --git a/mdadm.c b/mdadm.c
index 82a48722..314a5923 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -56,6 +56,12 @@  static mdadm_status_t set_bitmap_value(struct shape *s, struct context *c, char
 		return MDADM_STATUS_SUCCESS;
 	}
 
+	if (strcmp(val, "lockless") == 0) {
+		s->btype = BitmapLockless;
+		pr_info("Experimental lockless bitmap, use at your own disk!\n");
+		return MDADM_STATUS_SUCCESS;
+	}
+
 	if (strcmp(val, "clustered") == 0) {
 		s->btype = BitmapCluster;
 		/* Set the default number of cluster nodes
@@ -1251,7 +1257,8 @@  int main(int argc, char *argv[])
 			pr_err("--bitmap is required for consistency policy: %s\n",
 			       map_num_s(consistency_policies, s.consistency_policy));
 			exit(2);
-		} else if ((s.btype == BitmapInternal || s.btype == BitmapCluster) &&
+		} else if ((s.btype == BitmapInternal || s.btype == BitmapCluster ||
+			    s.btype == BitmapLockless) &&
 			   s.consistency_policy != CONSISTENCY_POLICY_BITMAP &&
 			   s.consistency_policy != CONSISTENCY_POLICY_JOURNAL) {
 			pr_err("--bitmap is not compatible with consistency policy: %s\n",
diff --git a/mdadm.h b/mdadm.h
index af54a6ab..5e35d78c 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -607,6 +607,7 @@  enum bitmap_type {
 	BitmapNone,
 	BitmapInternal,
 	BitmapCluster,
+	BitmapLockless,
 	BitmapUnknown,
 };