-----Original Message-----
From: Williams, Dan J [mailto:dan.j.williams@intel.com]
Sent: Friday, December 16, 2011 5:19 AM
To: Kwolek, Adam
Cc: NeilBrown; Labun, Marcin; Ciechanowski, Ed; linux-raid@vger.kernel.org
Subject: Re: [PATCH 3/3] imsm: FIX: UT '08imsm-overlap' fails

2011/12/15 Kwolek, Adam [off-list ref]:

quoted

From: Williams, Dan J [mailto:dan.j.williams@intel.com]
Sent: Thursday, December 15, 2011 6:16 AM
To: NeilBrown
Cc: Labun, Marcin; Ciechanowski, Ed; linux-raid@vger.kernel.org;
Kwolek, Adam
Subject: Re: [PATCH 3/3] imsm: FIX: UT '08imsm-overlap' fails


On Dec 14, 2011 8:03 PM, "NeilBrown" [off-list ref] wrote:

quoted

On Wed, 14 Dec 2011 10:21:07 -0800 "Williams, Dan J"
[off-list ref] wrote:

quoted

On Wed, Dec 14, 2011 at 7:07 AM, Adam Kwolek

[off-list ref] wrote:

quoted

quoted

quoted

quoted

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.

quoted

quoted

quoted

quoted

Signed-off-by: Adam Kwolek <redacted>
---

 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.

quoted

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

quoted

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.

quoted

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.

quoted

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html