Subject:[PATCH 001:013]: md: Raid0 reshape

Subject:[PATCH 001:013]: md: Raid0 reshape

[raz ben yehuda]

md assumes that personality has all its membes of the same
size,A fact that is incorrect for raid0.

 md.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Signed-off-by: razb <raziebe@gmail.com>
---
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 0f11fd1..e14fb90 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5683,7 +5683,8 @@ static void status_resync(struct seq_file *seq, mddev_t * mddev)
 		max_sectors = mddev->resync_max_sectors;
 	else
 		max_sectors = mddev->dev_sectors;
-
+	if (mddev->level == 0)
+		max_sectors = mddev->resync_max_sectors;
 	/*
 	 * Should not happen.
 	 */
@@ -6280,7 +6281,13 @@ void md_do_sync(mddev_t *mddev)
 			    rdev->recovery_offset < j)
 				j = rdev->recovery_offset;
 	}
-
+	/*
+	* raid0 members may not be of the same size,use array_size.
+	*/
+	if (mddev->level == 0) {
+		max_sectors = mddev->array_sectors;
+		j = mddev->recovery_cp;
+	}
 	printk(KERN_INFO "md: %s of RAID array %s\n", desc, mdname(mddev));
 	printk(KERN_INFO "md: minimum _guaranteed_  speed:"
 		" %d KB/sec/disk.\n", speed_min(mddev));

Re: Subject:[PATCH 001:013]: md: Raid0 reshape

[Bill Davidsen]

raz ben yehuda wrote:
> md assumes that personality has all its membes of the same
> size,A fact that is incorrect for raid0.
>
>  md.c |   11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> Signed-off-by: razb <raziebe@gmail.com>
> ---
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 0f11fd1..e14fb90 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5683,7 +5683,8 @@ static void status_resync(struct seq_file *seq, mddev_t * mddev)
>  		max_sectors = mddev->resync_max_sectors;
>  	else
>  		max_sectors = mddev->dev_sectors;
> -
> +	if (mddev->level == 0)
> +		max_sectors = mddev->resync_max_sectors;
>  	/*
>  	 * Should not happen.
>  	 */
> @@ -6280,7 +6281,13 @@ void md_do_sync(mddev_t *mddev)
>  			    rdev->recovery_offset < j)
>  				j = rdev->recovery_offset;
>  	}
> -
> +	/*
> +	* raid0 members may not be of the same size,use array_size.
> +	*/
> +	if (mddev->level == 0) {
> +		max_sectors = mddev->array_sectors;
> +		j = mddev->recovery_cp;
> +	}
>  	printk(KERN_INFO "md: %s of RAID array %s\n", desc, mdname(mddev));
>  	printk(KERN_INFO "md: minimum _guaranteed_  speed:"
>  		" %d KB/sec/disk.\n", speed_min(mddev));
>   

If I admit I only spent about 20 minutes looking at this code will you 
explain why you use different fields of the struct to set max_sectors? I 
guess my real confusion is why resync_max_sectors would be valid, given 
that raid0 has no redundancy. Or are you using it in some obscure way 
for reshape values?  The values stored in the field don't really to be 
what you want... Yes, the reshape is new to me.

-- 
Bill Davidsen <davidsen@tmr.com>
  Obscure bug of 2004: BASH BUFFER OVERFLOW - if bash is being run by a
normal user and is setuid root, with the "vi" line edit mode selected,
and the character set is "big5," an off-by-one error occurs during
wildcard (glob) expansion.

Re: Subject:[PATCH 001:013]: md: Raid0 reshape

[Andre Noll]

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

On 00:51, raz ben yehuda wrote:
> md assumes that personality has all its membes of the same
> size,A fact that is incorrect for raid0.
> 
>  md.c |   11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> Signed-off-by: razb <raziebe@gmail.com>
> ---
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 0f11fd1..e14fb90 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5683,7 +5683,8 @@ static void status_resync(struct seq_file *seq, mddev_t * mddev)
>  		max_sectors = mddev->resync_max_sectors;
>  	else
>  		max_sectors = mddev->dev_sectors;
> -
> +	if (mddev->level == 0)
> +		max_sectors = mddev->resync_max_sectors;
>  	/*
>  	 * Should not happen.
>  	 */
> @@ -6280,7 +6281,13 @@ void md_do_sync(mddev_t *mddev)
>  			    rdev->recovery_offset < j)
>  				j = rdev->recovery_offset;
>  	}
> -
> +	/*
> +	* raid0 members may not be of the same size,use array_size.
> +	*/
> +	if (mddev->level == 0) {
> +		max_sectors = mddev->array_sectors;
> +		j = mddev->recovery_cp;
> +	}
>  	printk(KERN_INFO "md: %s of RAID array %s\n", desc, mdname(mddev));
>  	printk(KERN_INFO "md: minimum _guaranteed_  speed:"
>  		" %d KB/sec/disk.\n", speed_min(mddev));

Hm, we want to get rid of personality-dependent code in md.c, so new
code should never check mddev->level. In the first hunk I think it
would be possible to check if pers->sync_request is NULL.

Is the second hunk really necessary? AFAICS md_do_sync() won't be
called for raid0 anyway.

Regards
Andre
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Subject:[PATCH 001:013]: md: Raid0 reshape

[Raz]

Hello Andre. Please see inline.
I really tried to minimize md patch.

On Wed, Jun 17, 2009 at 11:12 AM, Andre Noll<maan@systemlinux.org> wrote:
> On 00:51, raz ben yehuda wrote:
>> md assumes that personality has all its membes of the same
>> size,A fact that is incorrect for raid0.
>>
>>  md.c |   11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> Signed-off-by: razb <raziebe@gmail.com>
>> ---
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 0f11fd1..e14fb90 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -5683,7 +5683,8 @@ static void status_resync(struct seq_file *seq, mddev_t * mddev)
>>               max_sectors = mddev->resync_max_sectors;
>>       else
>>               max_sectors = mddev->dev_sectors;
>> -
>> +     if (mddev->level == 0)
>> +             max_sectors = mddev->resync_max_sectors;
>>       /*
>>        * Should not happen.
>>        */
>> @@ -6280,7 +6281,13 @@ void md_do_sync(mddev_t *mddev)
>>                           rdev->recovery_offset < j)
>>                               j = rdev->recovery_offset;
>>       }
>> -
>> +     /*
>> +     * raid0 members may not be of the same size,use array_size.
>> +     */
>> +     if (mddev->level == 0) {
>> +             max_sectors = mddev->array_sectors;
>> +             j = mddev->recovery_cp;
>> +     }
>>       printk(KERN_INFO "md: %s of RAID array %s\n", desc, mdname(mddev));
>>       printk(KERN_INFO "md: minimum _guaranteed_  speed:"
>>               " %d KB/sec/disk.\n", speed_min(mddev));
>
> Hm, we want to get rid of personality-dependent code in md.c, so new
> code should never check mddev->level. In the first hunk I think it
> would be possible to check if pers->sync_request is NULL.
No. sync_request does exists. this is what Neil wanted me to do.
implement raid0_sync
so I will be using md for this purpose. I knew that md patch is a
problem, this is why I posted
it first.
> Is the second hunk really necessary? AFAICS md_do_sync() won't be
> called for raid0 anyway.
second hunk is necessary for resume reshape. md_do_sync is called
indirecrly by raid0d -->md_check_recovery.

> Regards
> Andre
> --
> The only person who always got his work done by Friday was Robinson Crusoe
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.1 (GNU/Linux)
>
> iD8DBQFKOKV2Wto1QDEAkw8RAtweAJ9Zub5oTgEFomNsPQUAY4onGnZzDACfdkD1
> WxZ1hdqoMz4p+BNQ8uH5+PY=
> =txka
> -----END PGP SIGNATURE-----
>
>
--
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

Re: Subject:[PATCH 001:013]: md: Raid0 reshape

[raz ben yehuda]

On Tue, 2009-06-16 at 19:24 -0400, Bill Davidsen wrote:
> raz ben yehuda wrote:
> > md assumes that personality has all its membes of the same
> > size,A fact that is incorrect for raid0.
> >
> >  md.c |   11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > Signed-off-by: razb <raziebe@gmail.com>
> > ---
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 0f11fd1..e14fb90 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -5683,7 +5683,8 @@ static void status_resync(struct seq_file *seq, mddev_t * mddev)
> >  		max_sectors = mddev->resync_max_sectors;
> >  	else
> >  		max_sectors = mddev->dev_sectors;
> > -
> > +	if (mddev->level == 0)
> > +		max_sectors = mddev->resync_max_sectors;
> >  	/*
> >  	 * Should not happen.
> >  	 */
> > @@ -6280,7 +6281,13 @@ void md_do_sync(mddev_t *mddev)
> >  			    rdev->recovery_offset < j)
> >  				j = rdev->recovery_offset;
> >  	}
> > -
> > +	/*
> > +	* raid0 members may not be of the same size,use array_size.
> > +	*/
> > +	if (mddev->level == 0) {
> > +		max_sectors = mddev->array_sectors;
> > +		j = mddev->recovery_cp;
> > +	}
> >  	printk(KERN_INFO "md: %s of RAID array %s\n", desc, mdname(mddev));
> >  	printk(KERN_INFO "md: minimum _guaranteed_  speed:"
> >  		" %d KB/sec/disk.\n", speed_min(mddev));
> >   
> 
> If I admit I only spent about 20 minutes looking at this code will you 
> explain why you use different fields of the struct to set max_sectors? I 

md_sync uses max_sectors as an ending point of the reshape process.
problem is that md assumes all raid's members are of the same size, so
it uses dev_sectors and this is not true to raid0 with multiple zones
> guess my real confusion is why resync_max_sectors would be valid, given 
> that raid0 has no redundancy. Or are you using it in some obscure way 
> for reshape values?  The values stored in the field don't really to be 
> what you want... Yes, the reshape is new to me.
It has nothing to do with redundancy, but with the state machine of md.
I agree that there should be more elegant way to fix this, but it means
bigger fixes in md, and this is something I definitely don't want
to do.

Re: Subject:[PATCH 001:013]: md: Raid0 reshape

[Bill Davidsen]

raz ben yehuda wrote:
> On Tue, 2009-06-16 at 19:24 -0400, Bill Davidsen wrote:
>   
>> raz ben yehuda wrote:
>>     
>>> md assumes that personality has all its membes of the same
>>> size,A fact that is incorrect for raid0.
>>>
>>>  md.c |   11 +++++++++--
>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> Signed-off-by: razb <raziebe@gmail.com>
>>> ---
>>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>>> index 0f11fd1..e14fb90 100644
>>> --- a/drivers/md/md.c
>>> +++ b/drivers/md/md.c
>>> @@ -5683,7 +5683,8 @@ static void status_resync(struct seq_file *seq, mddev_t * mddev)
>>>  		max_sectors = mddev->resync_max_sectors;
>>>  	else
>>>  		max_sectors = mddev->dev_sectors;
>>> -
>>> +	if (mddev->level == 0)
>>> +		max_sectors = mddev->resync_max_sectors;
>>>  	/*
>>>  	 * Should not happen.
>>>  	 */
>>> @@ -6280,7 +6281,13 @@ void md_do_sync(mddev_t *mddev)
>>>  			    rdev->recovery_offset < j)
>>>  				j = rdev->recovery_offset;
>>>  	}
>>> -
>>> +	/*
>>> +	* raid0 members may not be of the same size,use array_size.
>>> +	*/
>>> +	if (mddev->level == 0) {
>>> +		max_sectors = mddev->array_sectors;
>>> +		j = mddev->recovery_cp;
>>> +	}
>>>  	printk(KERN_INFO "md: %s of RAID array %s\n", desc, mdname(mddev));
>>>  	printk(KERN_INFO "md: minimum _guaranteed_  speed:"
>>>  		" %d KB/sec/disk.\n", speed_min(mddev));
>>>   
>>>       
>> If I admit I only spent about 20 minutes looking at this code will you 
>> explain why you use different fields of the struct to set max_sectors? I 
>>     
>
> md_sync uses max_sectors as an ending point of the reshape process.
> problem is that md assumes all raid's members are of the same size, so
> it uses dev_sectors and this is not true to raid0 with multiple zones
>   
>> guess my real confusion is why resync_max_sectors would be valid, given 
>> that raid0 has no redundancy. Or are you using it in some obscure way 
>> for reshape values?  The values stored in the field don't really to be 
>> what you want... Yes, the reshape is new to me.
>>     
> It has nothing to do with redundancy, but with the state machine of md.
> I agree that there should be more elegant way to fix this, but it means
> bigger fixes in md, and this is something I definitely don't want
> to do.
>   

There is always a trade off between cost of implementation and cost of 
maintenance. If Neil is happy with this I have no objection, it just 
seems to invite misunderstanding at some future enhancement. Perhaps a 
an additional comment is desirable, although the name of the field 
invites confusion, pity there's no reshape_* field to use, for 
readability if nothing else.

-- 
Bill Davidsen <davidsen@tmr.com>
  Obscure bug of 2004: BASH BUFFER OVERFLOW - if bash is being run by a
normal user and is setuid root, with the "vi" line edit mode selected,
and the character set is "big5," an off-by-one error occurs during
wildcard (glob) expansion.

Re: Subject:[PATCH 001:013]: md: Raid0 reshape

[Andre Noll]

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

On 11:23, Raz wrote:

> >> +     if (mddev->level == 0) {
> >> +             max_sectors = mddev->array_sectors;
> >> +             j = mddev->recovery_cp;
> >> +     }
> >>       printk(KERN_INFO "md: %s of RAID array %s\n", desc, mdname(mddev));
> >>       printk(KERN_INFO "md: minimum _guaranteed_  speed:"
> >>               " %d KB/sec/disk.\n", speed_min(mddev));
> >
> > Hm, we want to get rid of personality-dependent code in md.c, so new
> > code should never check mddev->level. In the first hunk I think it
> > would be possible to check if pers->sync_request is NULL.
> No. sync_request does exists. this is what Neil wanted me to do.
> implement raid0_sync
> so I will be using md for this purpose. I knew that md patch is a
> problem, this is why I posted
> it first.

OK, so pers->sync_request can't be used, but that was only one
possible option to replace the check mddev->level == 0 by something
more generic. BTW: The log message should describe in which case the
resync status shows an incorrect value and why one can not determine
which value to use for max_sectors by looking at mddev->recovery.

> > Is the second hunk really necessary? AFAICS md_do_sync() won't be
> > called for raid0 anyway.
> second hunk is necessary for resume reshape. md_do_sync is called
> indirecrly by raid0d -->md_check_recovery.

Yes, I didn't notice because raid0d() is introduced later in patch
10/13 of your series. When introducing new and apparently dead code
it's always nice to let people know that the new code is going to be
used by a function that will be added in a subsequent patch :)

Regards
Andre
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Subject:[PATCH 001:013]: md: Raid0 reshape

[Raz]

On Wed, Jun 17, 2009 at 7:23 PM, Andre Noll<maan@systemlinux.org> wrote:
> On 11:23, Raz wrote:
>
>> >> +     if (mddev->level == 0) {
>> >> +             max_sectors = mddev->array_sectors;
>> >> +             j = mddev->recovery_cp;
>> >> +     }
>> >>       printk(KERN_INFO "md: %s of RAID array %s\n", desc, mdname(mddev));
>> >>       printk(KERN_INFO "md: minimum _guaranteed_  speed:"
>> >>               " %d KB/sec/disk.\n", speed_min(mddev));
>> >
>> > Hm, we want to get rid of personality-dependent code in md.c, so new
>> > code should never check mddev->level. In the first hunk I think it
>> > would be possible to check if pers->sync_request is NULL.
>> No. sync_request does exists. this is what Neil wanted me to do.
>> implement raid0_sync
>> so I will be using md for this purpose. I knew that md patch is a
>> problem, this is why I posted
>> it first.
>
> OK, so pers->sync_request can't be used, but that was only one
> possible option to replace the check mddev->level == 0 by something
> more generic. BTW: The log message should describe in which case the
> resync status shows an incorrect value and why one can not determine
> which value to use for max_sectors by looking at mddev->recovery.
>
>> > Is the second hunk really necessary? AFAICS md_do_sync() won't be
>> > called for raid0 anyway.
>> second hunk is necessary for resume reshape. md_do_sync is called
>> indirecrly by raid0d -->md_check_recovery.
>
> Yes, I didn't notice because raid0d() is introduced later in patch
> 10/13 of your series. When introducing new and apparently dead code
> it's always nice to let people know that the new code is going to be
> used by a function that will be added in a subsequent patch :)
noted.
> Regards
> Andre
> --
> The only person who always got his work done by Friday was Robinson Crusoe
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.1 (GNU/Linux)
>
> iD8DBQFKORh+Wto1QDEAkw8RAm3GAJ9orjx5oVNM/bCsmWwmYtPpa5s82gCdHxzE
> 1+jPSDd77+YrehyaKDoeICE=
> =XZIE
> -----END PGP SIGNATURE-----
>
>
--
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