Re: [md PATCH 00/16] hot-replace support for RAID4/5/6

[NeilBrown <neilb@suse.de>]

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

On Wed, 14 Dec 2011 23:14:04 -0800 "Williams, Dan J"
<dan.j.williams@intel.com> wrote:

> On Wed, Dec 14, 2011 at 10:18 PM, NeilBrown <neilb@suse.de> wrote:
> >> f248f8c md: create externally visible flags for supporting hot-replace.
> >>
> >> 'replaceable' just strikes me as a confusing name as all devices are
> >> nominally "replaceable", but whether you want it to be actively
> >> replaced is a different consideration.  What about "incumbent" to mark
> >> the disk as currently holding a position we want it to vacate and
> >> remove any potential confusion with 'replacement'.
> >
> > Fair point.  I had wondered if I should not have the flag and just use the
> > "write_error" flag.  However the meaning is slightly different.
> >
> > I don't really like "incumbent" as it gives no indication that there is a
> > desire to replace the device.  Maybe "want_replacement" ??
> 
> Yeah that works.  I was hung up on the previous scheme only differing
> the in "able" vs "ment" suffix, so a "want_" prefix fits the bill.

I've changed it all to "want_replacements" and agree that is it clearer.

> 
> >> 37aebb5 md/raid5: preferentially read from replacement device if possible.
> [..]
> >> Should this one liner be broken out for -stable?
> >> Do you see a particular problem that this fixes that is already possible
> > without hot-replace?
> 
> I would need to think it through a bit further, but the changelog was
> sufficiently convincing so I thought I would ask.

I'm pretty sure it isn't really needed before replacements is added but
thanks for checking.

> 
> >> Nit, not sure if it's worth fixing but this one introduces some
> >> inconsistent line wrapping around logical operators... "at the end" vs
> >> "beginning of next line"
> >>
> [..]
> >
> > Thanks.
> > I almost always prefer 'at the start' as import things should be obvious.
> > So I have updated 'want_replace'.
> 
> ...and here I've been an 'at the end' Sneetch and convinced myself
> that it's easier to read...
> 
> >>
> >> 2693b9e md/raid5: handle activation of replacement device when
> >> recovery completes.
> >>
> >> I questioned not needing a barrier in raid5_end_write_request after
> >> finding conf->disks[i].replacement == NULL until I found the note in
> >> raid5_end_read_request about the rdev being pinned until all i/o
> >> returns.  Maybe a similar note to raid5_end_write_request?
> >
> > I like adding explanatory notes ... but I'm not quite sure what you are
> > suggesting here.  Could you be a little more explicit?  Thanks.
> >
> 
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 59f5b05..8074515 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -1758,7 +1758,8 @@ static void raid5_end_write_request(struct bio
> *bi, int error)
>                                 replacement = 1;
>                         else
>                                 /* rdev was removed and 'replacement'
> -                                * replaced it.
> +                                * replaced it.  rdev is not removed
> +                                * until all requests are finished.
>                                  */
>                                 rdev = conf->disks[i].rdev;
>                         break;
> 

I've added that - thanks.


Thanks,
NeilBrown

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