On Wed, 14 Dec 2011 23:14:04 -0800 "Williams, Dan J" <dan.j.williams@xxxxxxxxx> wrote: > On Wed, Dec 14, 2011 at 10:18 PM, NeilBrown <neilb@xxxxxxx> 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:
signature.asc
Description: PGP signature
