> -----Original Message----- > From: Williams, Dan J [mailto:dan.j.williams@xxxxxxxxx] > Sent: Friday, December 16, 2011 5:19 AM > To: Kwolek, Adam > Cc: NeilBrown; Labun, Marcin; Ciechanowski, Ed; linux-raid@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 3/3] imsm: FIX: UT '08imsm-overlap' fails > > 2011/12/15 Kwolek, Adam <adam.kwolek@xxxxxxxxx>: > > > > > > From: Williams, Dan J [mailto:dan.j.williams@xxxxxxxxx] > > Sent: Thursday, December 15, 2011 6:16 AM > > To: NeilBrown > > Cc: Labun, Marcin; Ciechanowski, Ed; linux-raid@xxxxxxxxxxxxxxx; > > Kwolek, Adam > > Subject: Re: [PATCH 3/3] imsm: FIX: UT '08imsm-overlap' fails > > > > > > On Dec 14, 2011 8:03 PM, "NeilBrown" <neilb@xxxxxxx> wrote: > >> > >> On Wed, 14 Dec 2011 10:21:07 -0800 "Williams, Dan J" > >> <dan.j.williams@xxxxxxxxx> wrote: > >> > >> > On Wed, Dec 14, 2011 at 7:07 AM, Adam Kwolek > <adam.kwolek@xxxxxxxxx> wrote: > >> > > Problem was introduced by patch (2011-09-19): > >> > > Create: Allow to create two volumes of different sizes within > >> > > one container > >> > > > >> > > To resolve problem check requested disks number not against all > >> > > disks recorded in metadata, but against disks number set in array map > structure. > >> > > > >> > > Signed-off-by: Adam Kwolek <adam.kwolek@xxxxxxxxx> > >> > > --- > >> > > > >> > > super-intel.c | 24 ++++++++++++++++++++---- > >> > > 1 files changed, 20 insertions(+), 4 deletions(-) > >> > > > >> > > diff --git a/super-intel.c b/super-intel.c index 5e1d278..3c10d29 > >> > > 100644 > >> > > --- a/super-intel.c > >> > > +++ b/super-intel.c > >> > > @@ -5318,10 +5318,26 @@ static int > >> > > validate_geometry_imsm_volume(struct supertype *st, int level, > >> > > > >> > > mpb = super->anchor; > >> > > > >> > > - if (mpb->num_raid_devs > 0 && mpb->num_disks != > >> > > raiddisks) { > >> > > - fprintf(stderr, Name ": the option-rom requires all " > >> > > - "member disks to be a member of all > >> > > volumes.\n"); > >> > > - return 0; > >> > > + /* check if currently verified volume spans the same > >> > > + disks number > >> > > + * as all other arrays that belongs to metadata. > >> > > + * Do not allow to create volume with different disks > >> > > + number > >> > > + * than curently is used. > >> > > + */ > >> > > + for (i = 0; i < mpb->num_raid_devs; i++) { > >> > > + /* There is any already existing array in > >> > > + metadata > >> > > + */ > >> > > + struct imsm_dev *dev = get_imsm_dev(super, i); > >> > > + struct imsm_map *map = NULL; > >> > > + if (dev) > >> > > + map = get_imsm_map(dev, MAP_0); > >> > > + if (map) { > >> > > + if (raiddisks != map->num_members) { > >> > > + fprintf(stderr, Name ": the option-rom requires" > >> > > + " all member disks to be a member of " > >> > > + "all volumes.\n"); > >> > > + return 0; > >> > > + } > >> > > + } > >> > > >> > I'd prefer to move this check back to the other 'orom' checks in > >> > validate_geometry_imsm_volume(), and make it dependent on the > >> > presence of the orom. > >> > > >> > diff --git a/super-intel.c b/super-intel.c index 07d47b5..9885b98 > >> > 100644 > >> > --- a/super-intel.c > >> > +++ b/super-intel.c > >> > @@ -5122,12 +5122,6 @@ static int > >> > validate_geometry_imsm_volume(struct > >> > supertype *st, int level, > >> > if (!super) > >> > return 0; > >> > > >> > - if (mpb->num_raid_devs > 0 && mpb->num_disks != raiddisks) { > >> > - fprintf(stderr, Name ": the option-rom requires all " > >> > - "member disks to be a member of all > >> > volumes.\n"); > >> > - return 0; > >> > - } > >> > - > >> > if (!validate_geometry_imsm_orom(super, level, layout, > >> > raiddisks, chunk, verbose)) { > >> > fprintf(stderr, Name ": RAID gemetry validation failed. " > >> > "Cannot proceed with the action(s).\n"); @@ > >> > -5206,6 +5200,11 @@ static int validate_geometry_imsm_volume(struct > >> > supertype *st, int level, > >> > fprintf(stderr, Name ": The option-rom requires all member" > >> > " disks to be a member of all volumes\n"); > >> > return 0; > >> > + } else if (super->orom && mpb->num_raid_devs > 0 && > >> > + mpb->num_disks != raiddisks) { > >> > + fprintf(stderr, Name ": The option-rom requires all member" > >> > + " disks to be a member of all volumes\n"); > >> > + return 0; > >> > } > >> > > >> > /* retrieve the largest free space block */ > >> > >> > >> Hi Dan, > >> I think you are presenting this as an alternate - is that correct? > > Yes. > >> It seems a lit simpler - does it do enough? > > I believe so, I'd like Adam to confirm. > > > > I'm not sure if checking mpb->num_disks != raiddisks is enough /or it is > rather too much/. > > In disk list could be different number of members than in map (e.g. rebuild > when disks list is longer). > > Yes but num_disks should always be authoritative for the container. > > > It was UT08* failure cause and I thing using maps length is better. > > > > I'm not sure, if making it dependent on the presence of the orom makes us > happy from compatibility point of view. > > That's exactly the point this restriction only matters when the orom is > present. Which is why 08imsm-overlap specifies IMSM_NO_PLATFORM=1. > Bypassing this restriction allows more stressful testing of the auto-layout > code. > > > We shouldn't make metadata OROM incompatible. Every array can be > mounted on OROM platform later, what scenario for such case? > > Specifying IMSM_NO_PLATFORM means all orom / platform compatibility > checks are turned off. I've checked, you are right. OROM check will resolve problem. Do you post your patch or I should do it? We are short of time regarding Neil's plans about v3.2.3 BR Adam -- To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
