| Message ID | 20230223143939.3817-1-heming.zhao@suse.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | Grow: fix can't change bitmap type from none to clustered. | expand |
On Thu, Feb 23, 2023 at 10:39:39PM +0800, Heming Zhao wrote: > Commit a042210648ed ("disallow create or grow clustered bitmap with > writemostly set") introduced this bug. We should use 'true' logic not > '== 0' to deny setting up clustered array under WRITEMOSTLY condition. > > How to trigger > > ``` > ~/mdadm # ./mdadm -Ss && ./mdadm --zero-superblock /dev/sd{a,b} > ~/mdadm # ./mdadm -C /dev/md0 -l mirror -b clustered -e 1.2 -n 2 \ > /dev/sda /dev/sdb --assume-clean > mdadm: array /dev/md0 started. > ~/mdadm # ./mdadm --grow /dev/md0 --bitmap=none > ~/mdadm # ./mdadm --grow /dev/md0 --bitmap=clustered > mdadm: /dev/md0 disks marked write-mostly are not supported with clustered bitmap > ``` > > Signed-off-by: Heming Zhao <heming.zhao@suse.com> Acked-by: Coly Li <colyli@suse.de> > --- > Grow.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Grow.c b/Grow.c > index 8f5cf07d10d9..bb5fe45c851c 100644 > --- a/Grow.c > +++ b/Grow.c > @@ -429,7 +429,7 @@ int Grow_addbitmap(char *devname, int fd, struct context *c, struct shape *s) > dv = map_dev(disk.major, disk.minor, 1); > if (!dv) > continue; > - if (((disk.state & (1 << MD_DISK_WRITEMOSTLY)) == 0) && > + if ((disk.state & (1 << MD_DISK_WRITEMOSTLY)) && > (strcmp(s->bitmap_file, "clustered") == 0)) { > pr_err("%s disks marked write-mostly are not supported with clustered bitmap\n",devname); > free(mdi);
Dear Heming, Thank you for your patch. It’d be great, if you removed the dot/period from the end of the commit message summary. Kind regards, Paul
On 2/23/23 09:39, Heming Zhao wrote: > Commit a042210648ed ("disallow create or grow clustered bitmap with > writemostly set") introduced this bug. We should use 'true' logic not > '== 0' to deny setting up clustered array under WRITEMOSTLY condition. > > How to trigger > > ``` > ~/mdadm # ./mdadm -Ss && ./mdadm --zero-superblock /dev/sd{a,b} > ~/mdadm # ./mdadm -C /dev/md0 -l mirror -b clustered -e 1.2 -n 2 \ > /dev/sda /dev/sdb --assume-clean > mdadm: array /dev/md0 started. > ~/mdadm # ./mdadm --grow /dev/md0 --bitmap=none > ~/mdadm # ./mdadm --grow /dev/md0 --bitmap=clustered > mdadm: /dev/md0 disks marked write-mostly are not supported with clustered bitmap > ``` > > Signed-off-by: Heming Zhao <heming.zhao@suse.com> Applied! Thanks, Jes
Hello Jes, On 2/24/23 2:23 AM, Jes Sorensen wrote: > On 2/23/23 09:39, Heming Zhao wrote: >> Commit a042210648ed ("disallow create or grow clustered bitmap with >> writemostly set") introduced this bug. We should use 'true' logic not >> '== 0' to deny setting up clustered array under WRITEMOSTLY condition. >> >> How to trigger >> >> ``` >> ~/mdadm # ./mdadm -Ss && ./mdadm --zero-superblock /dev/sd{a,b} >> ~/mdadm # ./mdadm -C /dev/md0 -l mirror -b clustered -e 1.2 -n 2 \ >> /dev/sda /dev/sdb --assume-clean >> mdadm: array /dev/md0 started. >> ~/mdadm # ./mdadm --grow /dev/md0 --bitmap=none >> ~/mdadm # ./mdadm --grow /dev/md0 --bitmap=clustered >> mdadm: /dev/md0 disks marked write-mostly are not supported with clustered bitmap >> ``` >> >> Signed-off-by: Heming Zhao <heming.zhao@suse.com> > > Applied! > > Thanks, > Jes > With Paul Menzel comment, I will remove the dot/period in patch subject then send a v2. ------------------ @Nigel Croxon or other people Yesterday my brain only focused on fixing bug, no thinking the a042210648ed use case. Why or what reason makes the rule for clustered array denies to use write-mostly disk? I could image a scenario to use write-mostly disk. In cloud env, VMs have two legs, one is local shared disk for speed up IOs, another is remote shared disk (e.g: JBD) for back up. If anyone think this scenario makes sense, the best solution is to revert a042210648ed. Thanks, Heming
On 2/23/23 18:50, Heming Zhao wrote: > Hello Jes, > > On 2/24/23 2:23 AM, Jes Sorensen wrote: >> On 2/23/23 09:39, Heming Zhao wrote: >>> Commit a042210648ed ("disallow create or grow clustered bitmap with >>> writemostly set") introduced this bug. We should use 'true' logic not >>> '== 0' to deny setting up clustered array under WRITEMOSTLY condition. >>> >>> How to trigger >>> >>> ``` >>> ~/mdadm # ./mdadm -Ss && ./mdadm --zero-superblock /dev/sd{a,b} >>> ~/mdadm # ./mdadm -C /dev/md0 -l mirror -b clustered -e 1.2 -n 2 \ >>> /dev/sda /dev/sdb --assume-clean >>> mdadm: array /dev/md0 started. >>> ~/mdadm # ./mdadm --grow /dev/md0 --bitmap=none >>> ~/mdadm # ./mdadm --grow /dev/md0 --bitmap=clustered >>> mdadm: /dev/md0 disks marked write-mostly are not supported with >>> clustered bitmap >>> ``` >>> >>> Signed-off-by: Heming Zhao <heming.zhao@suse.com> >> >> Applied! >> >> Thanks, >> Jes >> > > With Paul Menzel comment, I will remove the dot/period in patch subject > then > send a v2. I already applied it. I don't think the dot is worth respinning the patch for. Thanks, Jes
diff --git a/Grow.c b/Grow.c index 8f5cf07d10d9..bb5fe45c851c 100644 --- a/Grow.c +++ b/Grow.c @@ -429,7 +429,7 @@ int Grow_addbitmap(char *devname, int fd, struct context *c, struct shape *s) dv = map_dev(disk.major, disk.minor, 1); if (!dv) continue; - if (((disk.state & (1 << MD_DISK_WRITEMOSTLY)) == 0) && + if ((disk.state & (1 << MD_DISK_WRITEMOSTLY)) && (strcmp(s->bitmap_file, "clustered") == 0)) { pr_err("%s disks marked write-mostly are not supported with clustered bitmap\n",devname); free(mdi);
Commit a042210648ed ("disallow create or grow clustered bitmap with writemostly set") introduced this bug. We should use 'true' logic not '== 0' to deny setting up clustered array under WRITEMOSTLY condition. How to trigger ``` ~/mdadm # ./mdadm -Ss && ./mdadm --zero-superblock /dev/sd{a,b} ~/mdadm # ./mdadm -C /dev/md0 -l mirror -b clustered -e 1.2 -n 2 \ /dev/sda /dev/sdb --assume-clean mdadm: array /dev/md0 started. ~/mdadm # ./mdadm --grow /dev/md0 --bitmap=none ~/mdadm # ./mdadm --grow /dev/md0 --bitmap=clustered mdadm: /dev/md0 disks marked write-mostly are not supported with clustered bitmap ``` Signed-off-by: Heming Zhao <heming.zhao@suse.com> --- Grow.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)