mdadm -Db switches array to write-pending?!

mdadm -Db switches array to write-pending?!

[Martin Wilck]

Hi Neil, hi all,

I discovered a weird behavior of mdadm -Db - it will switch a RAID array
from read-auto state to write-pending. For DDF (and possibly other
formats) this will cause metadata writes.

If I read the code correctly, the reason is that mdadm -Db will use the
GET_BITMAP_FILE ioctl, which calls md_allow_write(). I saw this on a
CentOS 6.3 kernel but comparing the code I didn't see a difference in
recent kernels.

I am wondering if this is intentional, and if no, if anything could be
done about it. It is certainly surprising behavior.

Regards
Martin

Re: mdadm -Db switches array to write-pending?!

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1352 bytes --]

On Wed, 10 Jul 2013 19:54:09 +0200 Martin Wilck <mwilck@arcor.de> wrote:

> Hi Neil, hi all,
> 
> I discovered a weird behavior of mdadm -Db - it will switch a RAID array
> from read-auto state to write-pending. For DDF (and possibly other
> formats) this will cause metadata writes.
> 
> If I read the code correctly, the reason is that mdadm -Db will use the
> GET_BITMAP_FILE ioctl, which calls md_allow_write(). I saw this on a
> CentOS 6.3 kernel but comparing the code I didn't see a difference in
> recent kernels.
> 
> I am wondering if this is intentional, and if no, if anything could be
> done about it. It is certainly surprising behavior.
> 
> Regards
> Martin

I hadn't thought of that consequence.
Probably the  best thing to do is:

diff --git a/drivers/md/md.c b/drivers/md/md.c
index d059af5..b19a1c8 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5629,10 +5629,7 @@ static int get_bitmap_file(struct mddev * mddev, void __user * arg)
 	char *ptr, *buf = NULL;
 	int err = -ENOMEM;
 
-	if (md_allow_write(mddev))
-		file = kmalloc(sizeof(*file), GFP_NOIO);
-	else
-		file = kmalloc(sizeof(*file), GFP_KERNEL);
+	file = kmalloc(sizeof(*file), GFP_NOIO);
 
 	if (!file)
 		goto out;


A failure here is not likely and not catastrophic.

Does it fix the situation for you?

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: mdadm -Db switches array to write-pending?!

[Martin Wilck]

On 07/15/2013 07:18 AM, NeilBrown wrote:

> Probably the  best thing to do is:
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d059af5..b19a1c8 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5629,10 +5629,7 @@ static int get_bitmap_file(struct mddev * mddev, void __user * arg)
>  	char *ptr, *buf = NULL;
>  	int err = -ENOMEM;
>  
> -	if (md_allow_write(mddev))
> -		file = kmalloc(sizeof(*file), GFP_NOIO);
> -	else
> -		file = kmalloc(sizeof(*file), GFP_KERNEL);
> +	file = kmalloc(sizeof(*file), GFP_NOIO);
>  
>  	if (!file)
>  		goto out;
> 
> 
> A failure here is not likely and not catastrophic.
> 
> Does it fix the situation for you?

Yes it does.
Martin

Re: mdadm -Db switches array to write-pending?!

[Martin Wilck]

Hi Neil,

On 07/17/2013 10:19 PM, Martin Wilck wrote:
> On 07/15/2013 07:18 AM, NeilBrown wrote:
> 
>> Probably the  best thing to do is:
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index d059af5..b19a1c8 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -5629,10 +5629,7 @@ static int get_bitmap_file(struct mddev * mddev, void __user * arg)
>>  	char *ptr, *buf = NULL;
>>  	int err = -ENOMEM;
>>  
>> -	if (md_allow_write(mddev))
>> -		file = kmalloc(sizeof(*file), GFP_NOIO);
>> -	else
>> -		file = kmalloc(sizeof(*file), GFP_KERNEL);
>> +	file = kmalloc(sizeof(*file), GFP_NOIO);
>>  
>>  	if (!file)
>>  		goto out;
>>
>>
>> A failure here is not likely and not catastrophic.
>>
>> Does it fix the situation for you?
> 
> Yes it does.
> Martin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Any particular reason why you haven't applied this patch in your git
tree http://git.neil.brown.name/?p=md.git;a=summary yet?

Martin

Re: mdadm -Db switches array to write-pending?!

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]

On Fri, 26 Jul 2013 20:01:54 +0200 Martin Wilck <mwilck@arcor.de> wrote:

> Hi Neil,
> 
> On 07/17/2013 10:19 PM, Martin Wilck wrote:
> > On 07/15/2013 07:18 AM, NeilBrown wrote:
> > 
> >> Probably the  best thing to do is:
> >>
> >> diff --git a/drivers/md/md.c b/drivers/md/md.c
> >> index d059af5..b19a1c8 100644
> >> --- a/drivers/md/md.c
> >> +++ b/drivers/md/md.c
> >> @@ -5629,10 +5629,7 @@ static int get_bitmap_file(struct mddev * mddev, void __user * arg)
> >>  	char *ptr, *buf = NULL;
> >>  	int err = -ENOMEM;
> >>  
> >> -	if (md_allow_write(mddev))
> >> -		file = kmalloc(sizeof(*file), GFP_NOIO);
> >> -	else
> >> -		file = kmalloc(sizeof(*file), GFP_KERNEL);
> >> +	file = kmalloc(sizeof(*file), GFP_NOIO);
> >>  
> >>  	if (!file)
> >>  		goto out;
> >>
> >>
> >> A failure here is not likely and not catastrophic.
> >>
> >> Does it fix the situation for you?
> > 
> > Yes it does.
> > Martin
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> Any particular reason why you haven't applied this patch in your git
> tree http://git.neil.brown.name/?p=md.git;a=summary yet?

Hi Martin,
 you need to look in the for-next branch.
http://git.neil.brown.name/?p=md.git;a=shortlog;h=refs/heads/for-next

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]