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
