Hi Neil, I can't fully agree with you. Loading orom in validate_geometry_imsm is a good idea but it will only work when -e imsm option is given. When we have an imsm container with sda and sdb and just call mdadm -CR v0 -l 0 -n 2 /dev/sd[ab] then default geometry will not be called and chunk will still be set to 512. We need to realize that /dev/sda is already in an imsm container to get the correct setting. Regards Anna > -----Original Message----- > From: linux-raid-owner@xxxxxxxxxxxxxxx [mailto:linux-raid- > owner@xxxxxxxxxxxxxxx] On Behalf Of NeilBrown > Sent: Sunday, February 13, 2011 11:53 PM > To: Czarnowska, Anna > Cc: linux-raid@xxxxxxxxxxxxxxx; Williams, Dan J; Ciechanowski, Ed; > Neubauer, Wojciech > Subject: Re: [PATCH] fix: detect container early in Create > > On Thu, 10 Feb 2011 09:14:40 +0000 "Czarnowska, Anna" > <anna.czarnowska@xxxxxxxxx> wrote: > > > >From 082f13fb974059de27804e0f28d05f11e3c3eb3b Mon Sep 17 00:00:00 > 2001 > > From: Anna Czarnowska <anna.czarnowska@xxxxxxxxx> > > Date: Thu, 10 Feb 2011 09:52:42 +0100 > > Subject: [PATCH] fix: detect container early in Create > > Cc: linux-raid@xxxxxxxxxxxxxxx, Williams, Dan J > <dan.j.williams@xxxxxxxxx>, Ciechanowski, Ed > <ed.ciechanowski@xxxxxxxxx> > > > > When creating raid0 subarray in a container giving list of > > devices on the command line Create always sets chunk=512 > > default_geometry will only return chunk!=0 for imsm > > when we have superblock loaded. > > When a list of disks is given instead of a container > > we load superblock later, after wrong chunk size has been set. > > This causes Create to fail for imsm. > > > > New function check_container is added to check if first > > non "missing" device on given list is a container member. > > If it is, we load container so we can get correct chunk. > > At this stage we can abort creation if > > - all given devices are "missing" or > > - device can't be open for reading or > > - metadata given on command line does not match container we found. > > > > As the container check was a part of validate geometry for ddf and > imsm > > metadata this part of code has been replaced by check_container. > > I don't like this patch. > > Choosing the correct default chunk size does not require loading > the metadata. It only requires finding the option rom which > you can easily do in default_geometry_imsm just like it is done in > validate_geometry_imsm_container. > > There may be a case for factoring code out of both ddf and intel and > putting it in util.c - I'm not sure. But it should be a separate > patch with a separate justification. > > so: not applied. > > Thanks, > NeilBrown > > > > > > Signed-off-by: Anna Czarnowska <anna.czarnowska@xxxxxxxxx> > > --- > > Create.c | 21 +++++++++++++++ > > mdadm.h | 1 + > > super-ddf.c | 78 ++++++----------------------------------------- > --------- > > super-intel.c | 58 +---------------------------------------- > > util.c | 79 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 5 files changed, 111 insertions(+), 126 deletions(-) > > > > diff --git a/Create.c b/Create.c > > index a0669fe..8208b19 100644 > > --- a/Create.c > > +++ b/Create.c > > @@ -64,6 +64,21 @@ static int default_layout(struct supertype *st, > int level, int verbose) > > return layout; > > } > > > > +static int devices_in_container(struct mddev_dev *devlist, struct > supertype **stp, int verbose) > > +{ > > + struct mddev_dev *dv; > > + > > + for(dv = devlist; dv; dv = dv->next) > > + if (strcasecmp(dv->devname, "missing") == 0) > > + continue; > > + else > > + break; > > + if (!dv) { > > + fprintf(stderr, Name ": Cannot create this array on missing > devices\n"); > > + return -1; > > + } > > + return check_container(dv->devname, stp, verbose); > > +} > > > > int Create(struct supertype *st, char *mddev, > > int chunk, int level, int layout, unsigned long long size, > > @@ -230,6 +245,12 @@ int Create(struct supertype *st, char *mddev, > > case 6: > > case 0: > > if (chunk == 0) { > > + /* check if listed devices are in a container > > + * if so, load the container */ > > + if ((!st || !st->sb) && > > + devices_in_container(devlist, &st, verbose) < 0) > > + /* error already printed */ > > + return 1; > > if (st && st->ss->default_geometry) > > st->ss->default_geometry(st, NULL, NULL, > &chunk); > > chunk = chunk ? : 512; > > diff --git a/mdadm.h b/mdadm.h > > index 608095f..9be236f 100644 > > --- a/mdadm.h > > +++ b/mdadm.h > > @@ -1133,6 +1133,7 @@ extern struct mdinfo > *container_choose_spares(struct supertype *st, > > struct domainlist *domlist, > > char *spare_group, > > const char *metadata, int get_one); > > +extern int check_container(char *devname, struct supertype **stp, > int verbose); > > extern int move_spare(char *from_devname, char *to_devname, dev_t > devid); > > extern int add_disk(int mdfd, struct supertype *st, > > struct mdinfo *sra, struct mdinfo *info); > > diff --git a/super-ddf.c b/super-ddf.c > > index 287fa7f..611626f 100644 > > --- a/super-ddf.c > > +++ b/super-ddf.c > > @@ -2555,10 +2555,6 @@ static int validate_geometry_ddf(struct > supertype *st, > > char *dev, unsigned long long *freesize, > > int verbose) > > { > > - int fd; > > - struct mdinfo *sra; > > - int cfd; > > - > > /* ddf potentially supports lots of things, but it depends on > > * what devices are offered (and maybe kernel version?) > > * If given unused devices, we will make a container. > > @@ -2601,79 +2597,21 @@ static int validate_geometry_ddf(struct > supertype *st, > > return 1; > > } > > > > - if (st->sb) { > > - /* A container has already been opened, so we are > > - * creating in there. Maybe a BVD, maybe an SVD. > > + if (st->sb || check_container(dev, &st, verbose) == 1) { > > + /* A container has already been opened > > + * or dev is a ddf container member, so we are > > + * creating in there. > > + * Maybe a BVD, maybe an SVD. > > * Should make a distinction one day. > > */ > > return validate_geometry_ddf_bvd(st, level, layout, > raiddisks, > > chunk, size, dev, freesize, > > verbose); > > } > > - /* This is the first device for the array. > > - * If it is a container, we read it in and do automagic > allocations, > > - * no other devices should be given. > > - * Otherwise it must be a member device of a container, and we > > - * do manual allocation. > > - * Later we should check for a BVD and make an SVD. > > - */ > > - fd = open(dev, O_RDONLY|O_EXCL, 0); > > - if (fd >= 0) { > > - sra = sysfs_read(fd, 0, GET_VERSION); > > - close(fd); > > - if (sra && sra->array.major_version == -1 && > > - strcmp(sra->text_version, "ddf") == 0) { > > - > > - /* load super */ > > - /* find space for 'n' devices. */ > > - /* remember the devices */ > > - /* Somehow return the fact that we have enough */ > > - } > > - > > - if (verbose) > > - fprintf(stderr, > > - Name ": ddf: Cannot create this array " > > - "on device %s - a container is required.\n", > > - dev); > > - return 0; > > - } > > - if (errno != EBUSY || (fd = open(dev, O_RDONLY, 0)) < 0) { > > - if (verbose) > > - fprintf(stderr, Name ": ddf: Cannot open %s: %s\n", > > - dev, strerror(errno)); > > - return 0; > > - } > > - /* Well, it is in use by someone, maybe a 'ddf' container. */ > > - cfd = open_container(fd); > > - if (cfd < 0) { > > - close(fd); > > - if (verbose) > > - fprintf(stderr, Name ": ddf: Cannot use %s: %s\n", > > - dev, strerror(EBUSY)); > > - return 0; > > - } > > - sra = sysfs_read(cfd, 0, GET_VERSION); > > - close(fd); > > - if (sra && sra->array.major_version == -1 && > > - strcmp(sra->text_version, "ddf") == 0) { > > - /* This is a member of a ddf container. Load the container > > - * and try to create a bvd > > - */ > > - struct ddf_super *ddf; > > - if (load_super_ddf_all(st, cfd, (void **)&ddf, NULL) == 0) > { > > - st->sb = ddf; > > - st->container_dev = fd2devnum(cfd); > > - close(cfd); > > - return validate_geometry_ddf_bvd(st, level, layout, > > - raiddisks, chunk, size, > > - dev, freesize, > > - verbose); > > - } > > - close(cfd); > > - } else /* device may belong to a different container */ > > - return 0; > > + if (verbose) > > + fprintf(stderr, Name ": ddf: failed container membership > check\n"); > > + return 0; > > > > - return 1; > > } > > > > static int > > diff --git a/super-intel.c b/super-intel.c > > index 29f8fc4..4d4ff89 100644 > > --- a/super-intel.c > > +++ b/super-intel.c > > @@ -4413,10 +4413,6 @@ static int validate_geometry_imsm(struct > supertype *st, int level, int layout, > > char *dev, unsigned long long *freesize, > > int verbose) > > { > > - int fd, cfd; > > - struct mdinfo *sra; > > - int is_member = 0; > > - > > /* if given unused devices create a container > > * if given given devices in a container create a member volume > > */ > > @@ -4446,64 +4442,14 @@ static int validate_geometry_imsm(struct > supertype *st, int level, int layout, > > } > > return 1; > > } > > - if (st->sb) { > > + if (st->sb || check_container(dev, &st, verbose) == 1) { > > /* creating in a given container */ > > return validate_geometry_imsm_volume(st, level, layout, > > raiddisks, chunk, size, > > dev, freesize, verbose); > > } > > - > > - /* This device needs to be a device in an 'imsm' container */ > > - fd = open(dev, O_RDONLY|O_EXCL, 0); > > - if (fd >= 0) { > > - if (verbose) > > - fprintf(stderr, > > - Name ": Cannot create this array on device > %s\n", > > - dev); > > - close(fd); > > - return 0; > > - } > > - if (errno != EBUSY || (fd = open(dev, O_RDONLY, 0)) < 0) { > > - if (verbose) > > - fprintf(stderr, Name ": Cannot open %s: %s\n", > > - dev, strerror(errno)); > > - return 0; > > - } > > - /* Well, it is in use by someone, maybe an 'imsm' container. */ > > - cfd = open_container(fd); > > - close(fd); > > - if (cfd < 0) { > > - if (verbose) > > - fprintf(stderr, Name ": Cannot use %s: It is busy\n", > > - dev); > > - return 0; > > - } > > - sra = sysfs_read(cfd, 0, GET_VERSION); > > - if (sra && sra->array.major_version == -1 && > > - strcmp(sra->text_version, "imsm") == 0) > > - is_member = 1; > > - sysfs_free(sra); > > - if (is_member) { > > - /* This is a member of a imsm container. Load the > container > > - * and try to create a volume > > - */ > > - struct intel_super *super; > > - > > - if (load_super_imsm_all(st, cfd, (void **) &super, NULL) == > 0) { > > - st->sb = super; > > - st->container_dev = fd2devnum(cfd); > > - close(cfd); > > - return validate_geometry_imsm_volume(st, level, > layout, > > - raiddisks, chunk, > > - size, dev, > > - freesize, verbose); > > - } > > - } > > - > > if (verbose) > > - fprintf(stderr, Name ": failed container membership > check\n"); > > - > > - close(cfd); > > + fprintf(stderr, Name ": imsm: failed container membership > check\n"); > > return 0; > > } > > > > diff --git a/util.c b/util.c > > index 87c23dc..6aeb789 100644 > > --- a/util.c > > +++ b/util.c > > @@ -1976,3 +1976,82 @@ struct mdinfo *container_choose_spares(struct > supertype *st, > > } > > return disks; > > } > > + > > +/* Returns > > + * 1: if devname is a member of container with metadata type > matching stp > > + * -1: if device not suitable for creating array > > + * 0: if not in container but device may be suitable for native > array > > + */ > > +int check_container(char *devname, struct supertype **stp, int > verbose) > > +{ > > + int fd, cfd, is_member = 0; > > + struct mdinfo *sra; > > + struct supertype *st = NULL; > > + int container_expected = (*stp && (*stp)->ss->external); > > + > > + fd = open(devname, O_RDONLY|O_EXCL, 0); > > + if (fd >= 0) { > > + /* not in container but don't stop create > > + * when creating native array */ > > + close(fd); > > + if (container_expected) { > > + fprintf(stderr, Name > > + ": Cannot create this array on device %s " > > + "- a container is required.\n", > > + devname); > > + return -1; > > + } > > + return 0; > > + } > > + if (errno != EBUSY || (fd = open(devname, O_RDONLY, 0)) < 0) { > > + if (verbose) > > + fprintf(stderr, Name ": Cannot open %s: %s\n", > > + devname, strerror(errno)); > > + /* we can't create anything on this disk */ > > + return -1; > > + } > > + /* Well, it is in use by someone, maybe a container. */ > > + cfd = open_container(fd); > > + close(fd); > > + if (cfd < 0) { > > + if (container_expected) { > > + fprintf(stderr, Name ": Cannot use %s: It is busy\n", > > + devname); > > + return -1; > > + } > > + return 0; > > + } > > + sra = sysfs_read(cfd, 0, GET_VERSION); > > + if (sra && sra->array.major_version == -1) { > > + int i; > > + for (i = 0; st == NULL && superlist[i] ; i++) > > + st = superlist[i]->match_metadata_desc(sra- > >text_version); > > + if (st) > > + is_member = 1; > > + } > > + sysfs_free(sra); > > + if (is_member) { > > + /* This is a member of a container. > > + * if *stp we expect some type of metadata > > + * check if it matches with what we have found > > + * then load the container */ > > + if (*stp && st->ss != (*stp)->ss) { > > + fprintf(stderr, Name ": Cannot use %s: " > > + "member of %s container\n", > > + devname, st->ss->name); > > + close(cfd); > > + free(st); > > + return -1; > > + } > > + if (st->ss->load_container(st, cfd, NULL) == 0) { > > + free(*stp); > > + *stp = st; > > + } else /* still return ok status > > + * validate_geometry will fail anyway because !st->sb > */ > > + if (verbose) > > + fprintf(stderr, Name ": Failed to load > container" > > + "information for %s\n", devname); > > + } > > + close(cfd); > > + return is_member; > > +} > > -- > 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 -- 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
