[PATCH] fix: detect container early in Create

From 082f13fb974059de27804e0f28d05f11e3c3eb3b Mon Sep 17 00:00:00 2001
From: Anna Czarnowska
Date: Thu, 10 Feb 2011 09:52:42 +0100
Subject: [PATCH] fix: detect container early in Create
Cc: linux-raid@vger.kernel.org, Williams, Dan J , Ciechanowski, Ed

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.

Signed-off-by: Anna Czarnowska
---
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;
+}
--
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix: detect container early in Create

On Thu, 10 Feb 2011 09:14:40 +0000 "Czarnowska, Anna"
wrote:

> >From 082f13fb974059de27804e0f28d05f11e3c3eb3b Mon Sep 17 00:00:00 2001
> From: Anna Czarnowska
> Date: Thu, 10 Feb 2011 09:52:42 +0100
> Subject: [PATCH] fix: detect container early in Create
> Cc: linux-raid@vger.kernel.org, Williams, Dan J , Ciechanowski, Ed
>
> 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
> ---
> 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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

RE: [PATCH] fix: detect container early in Create

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@vger.kernel.org [mailto:linux-raid-
> owner@vger.kernel.org] On Behalf Of NeilBrown
> Sent: Sunday, February 13, 2011 11:53 PM
> To: Czarnowska, Anna
> Cc: linux-raid@vger.kernel.org; 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"
> wrote:
>
> > >From 082f13fb974059de27804e0f28d05f11e3c3eb3b Mon Sep 17 00:00:00
> 2001
> > From: Anna Czarnowska
> > Date: Thu, 10 Feb 2011 09:52:42 +0100
> > Subject: [PATCH] fix: detect container early in Create
> > Cc: linux-raid@vger.kernel.org, Williams, Dan J
> , Ciechanowski, Ed
>
> >
> > 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
> > ---
> > 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@vger.kernel.org
> 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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

RE: [PATCH] fix: detect container early in Create

That's right.
We need to know which disks are making a container and make sure that they belong to the same SAS or SATA controller and load appropriate OROM.
Each controller can have its own OROM.


> -----Original Message-----
> From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> owner@vger.kernel.org] On Behalf Of Czarnowska, Anna
> Sent: Monday, February 14, 2011 12:19 PM
> To: NeilBrown
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: RE: [PATCH] fix: detect container early in Create
>
> 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@vger.kernel.org [mailto:linux-raid-
> > owner@vger.kernel.org] On Behalf Of NeilBrown
> > Sent: Sunday, February 13, 2011 11:53 PM
> > To: Czarnowska, Anna
> > Cc: linux-raid@vger.kernel.org; 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"
> > wrote:
> >
> > > >From 082f13fb974059de27804e0f28d05f11e3c3eb3b Mon Sep 17 00:00:00
> > 2001
> > > From: Anna Czarnowska
> > > Date: Thu, 10 Feb 2011 09:52:42 +0100
> > > Subject: [PATCH] fix: detect container early in Create
> > > Cc: linux-raid@vger.kernel.org, Williams, Dan J
> > , Ciechanowski, Ed
> >
> > >
> > > 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
> > > ---
> > > 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@vger.kernel.org
> > 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@vger.kernel.org
> 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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix: detect container early in Create

On Mon, 14 Feb 2011 11:19:24 +0000 "Czarnowska, Anna"
wrote:

> 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.


OK. How about we treat it the same way as 'layout'?
So if we set a default, we also set a var called 'do_default_chunk' and then
in the same places where we check do_default_layout, we also check
do_default_chunk and apply a similar st-specific default.

Longer term, I think I would like to check validate_geometry to pass
layout and chunksize by reference and if they are UnSet, validate_geometry
can set defaults.
Then we can get rid of ->default_geometry.

Thanks,
NeilBrown


--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html