Re: [PATCH] imsm: fix: correct checking newly missing disks

[NeilBrown <neilb@suse.de>]

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

On Mon, 5 Dec 2011 18:19:09 -0800 "Williams, Dan J"
<dan.j.williams@intel.com> wrote:

> On Mon, Dec 5, 2011 at 5:10 PM, NeilBrown <neilb@suse.de> wrote:
> > On Thu, 1 Dec 2011 14:23:16 +0000 "Dorau, Lukasz" <lukasz.dorau@intel.com>
> > wrote:
> >> > > diff --git a/super-intel.c b/super-intel.c
> >> > > index 4ebee78..511a32a 100644
> >> > > --- a/super-intel.c
> >> > > +++ b/super-intel.c
> >> > > @@ -2529,13 +2529,13 @@ static void getinfo_super_imsm(struct supertype
> >> > *st, struct mdinfo *info, char *
> >> > >
> >> > >                failed = imsm_count_failed(super, dev);
> >> > >                state = imsm_check_degraded(super, dev, failed);
> >> > > -               map = get_imsm_map(dev, dev->vol.migr_state);
> >> > > +               map = get_imsm_map(dev, 0);
> >> > >
> >> > >                /* any newly missing disks?
> >> > >                 * (catches single-degraded vs double-degraded)
> >> > >                 */
> >> > >                for (j = 0; j < map->num_members; j++) {
> >> > > -                       __u32 ord = get_imsm_ord_tbl_ent(dev, i, -1);
> >> > > +                       __u32 ord = get_imsm_ord_tbl_ent(dev, i, 0);
> >> >
> >> > This looks wrong.  I noticed this when looking over Przemyslaw's patch [1].
> >> >
> >> > map[0] always contains the destination state of the migration so the
> >> > most reliable source for looking for out of sync disks is map[1].
> >> >
> >>
> >> I am convinced that the patch is good.
> >> We are looking for information what was the state of array during migration (before it was stopped), so we have to use map[0].
> >> map[1] contains information about the state of array before migration, which we do not need.
> >>
> >> Regards,
> >> Lukasz
> >
> > Hi,
> >  do we have agreement on this?  Dan - do you stand by your original concern
> >  or have you seen the light :-)
> >
> > The patch is in, but I'd like to be sure it is right and to be honest I
> > haven't followed the dance of the maps too closely...
> 
> Lukasz is right.  We want to find out if starting the array with the
> current list of disks in the container would regress the state of the
> array recorded in the metadata.  map[0] should always record the best
> possible state of all the slots in the array if any of those have gone
> missing we don't want incremental assembly to proceed.
> 
> Sorry for the noise, my point was 'correct' in isolation but it missed
> that the context is looking for the most optimistic view of the disk.

Great - thanks for clearing that up.

NeilBrown

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