Re: [PATCH 3/3] imsm: FIX: UT '08imsm-overlap' fails

[NeilBrown <neilb@suse.de>]

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

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

> On Wed, Dec 14, 2011 at 7:07 AM, Adam Kwolek <adam.kwolek@intel.com> 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@intel.com>
> > ---
> >
> >  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?
It seems a lit simpler - does it do enough?

Or is it just a precursor, and then we apply Adam's patch changing the same
code but in a different location?

Slightly confused...

NeilBrown


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