[PATCH 2/3] Assemble imsm spares in matching domain only

From 9aa8c183c10c121d663bcf0f715cf9c17daa048d Mon Sep 17 00:00:00 2001
From: Anna Czarnowska
Date: Thu, 23 Dec 2010 12:12:19 +0100
Subject: [PATCH 2/3] Assemble imsm spares in matching domain only
Cc: linux-raid@vger.kernel.org, Williams, Dan J , Ciechanowski, Ed

Imsm spare will only be taken if it matches domain of
identified members of currently assembled array.

This implies that:
- spare with null domain will match first array assembled.
- if array has null domain then no spare will match

If we allow spares to set st they may block assembly of subarrays.
This is because in autossembly tmpdev->used=0 for a spare not matching
any array. If we find such spare before container and set st, the content
will not get assembled.

We allow uuid_zero match any uuid in assembly as unsuitable spares will
be rejected on domain check.

Signed-off-by: Anna Czarnowska
---
Assemble.c | 44 +++++++++++++++++++++++++++++++++++++++++---
1 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index ac489e8..4b8ab5c 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -78,7 +78,8 @@ static int ident_matches(struct mddev_ident *ident,
{

if (ident->uuid_set && (!update || strcmp(update, "uuid")!= 0) &&
- same_uuid(content->uuid, ident->uuid, tst->ss->swapuuid)==0) {
+ same_uuid(content->uuid, ident->uuid, tst->ss->swapuuid)==0 &&
+ memcmp(content->uuid, uuid_zero, sizeof(int[4]))) {
if (devname)
fprintf(stderr, Name ": %s has wrong uuid.\n",
devname);
@@ -231,7 +232,8 @@ int Assemble(struct supertype *st, char *mddev,
char *name = NULL;
int trustworthy;
char chosen_name[1024];
-
+ struct domainlist *domains = NULL;
+
if (get_linux_version() < 2004000)
old_linux = 1;

@@ -350,6 +352,7 @@ int Assemble(struct supertype *st, char *mddev,
} else
found_container = 1;
} else {
+ pol = devnum_policy(stb.st_rdev);
if (!tst && (tst = guess_super(dfd)) == NULL) {
if (report_missmatch)
fprintf(stderr, Name ": no recogniseable superblock on %s\n",
@@ -366,7 +369,7 @@ int Assemble(struct supertype *st, char *mddev,
tst->ss->name, devname);
tmpdev->used = 2;
} else if (auto_assem && st == NULL &&
- !conf_test_metadata(tst->ss->name, (pol = devnum_policy(stb.st_rdev)),
+ !conf_test_metadata(tst->ss->name, pol,
tst->ss->match_home(tst, homehost) == 1)) {
if (report_missmatch)
fprintf(stderr, Name ": %s has metadata type %s for which "
@@ -392,6 +395,7 @@ int Assemble(struct supertype *st, char *mddev,
if (st)
st->ss->free_super(st);
dev_policy_free(pol);
+ domain_free(domains);
return 1;
}

@@ -459,6 +463,7 @@ int Assemble(struct supertype *st, char *mddev,
devname);
st->ss->free_super(st);
dev_policy_free(pol);
+ domain_free(domains);
return 1;
}
if (verbose > 0)
@@ -479,6 +484,12 @@ int Assemble(struct supertype *st, char *mddev,
report_missmatch ? devname : NULL))
goto loop;

+ if (!memcmp(content->uuid, uuid_zero, sizeof(int[4]))) {
+ /* this is imsm_spare - do not set st */
+ tmpdev->used = 3;
+ goto loop;
+ }
+
if (st == NULL)
st = dup_super(tst);
if (st->minor_version == -1)
@@ -522,17 +533,44 @@ int Assemble(struct supertype *st, char *mddev,
tst->ss->free_super(tst);
st->ss->free_super(st);
dev_policy_free(pol);
+ domain_free(domains);
return 1;
}
tmpdev->used = 1;
}
loop:
+ /* Collect domain information from members only */
+ if (tmpdev && tmpdev->used == 1)
+ domain_merge(&domains, pol, tst?tst->ss->name:NULL);
dev_policy_free(pol);
pol = NULL;
if (tst)
tst->ss->free_super(tst);
}

+ /* Now reject spares that don't match domains of identified members */
+ for (tmpdev = devlist; tmpdev; tmpdev = tmpdev->next) {
+ struct stat stb;
+ if (tmpdev->used != 3)
+ continue;
+ if (stat(tmpdev->devname, &stb)< 0) {
+ fprintf(stderr, Name ": fstat failed for %s: %s\n",
+ tmpdev->devname, strerror(errno));
+ tmpdev->used = 2;
+ } else {
+ struct dev_policy *pol = NULL;
+ pol = devnum_policy(stb.st_rdev);
+ if (domain_test(domains, pol, NULL))
+ /* take this spare if domains match */
+ tmpdev->used = 1;
+ else
+ /* if domains don't match mark as unused */
+ tmpdev->used = 0;
+ dev_policy_free(pol);
+ }
+ }
+ domain_free(domains);
+
if (!st || !st->sb || !content)
return 2;

--
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 2/3] Assemble imsm spares in matching domain only

Thanks for these three.

I have applied the first,
applied the seconds with a couple of changes as mentioned below,
but I haven't applied the first. I don't like the fact that knowledge about
imsm is explicitly hard-coded into mdadm.c All explicit knowledge about imsm
should be in super-intel.c...

I think you can make this work by adding some code in to Assemble() just near:
/* Now reject spares that don't match domains of identified members */

at that point, if (!st || !st->sb), then we could include all those spares
which have ->used == 3.
i.e. when there are now more arrays to be found, all the spares get included
into one last array.
It might make sense to have an new super_switch field which gives the name of
the array-of-spares as there is no name stored in the spares to use ... just
a thought though, you might not need that.

Could you try that and see if you can make it work?

The issues I fixed with this second patch are:


> @@ -78,7 +78,8 @@ static int ident_matches(struct mddev_ident *ident,
> {
>
> if (ident->uuid_set && (!update || strcmp(update, "uuid")!= 0) &&
> - same_uuid(content->uuid, ident->uuid, tst->ss->swapuuid)==0) {
> + same_uuid(content->uuid, ident->uuid, tst->ss->swapuuid)==0 &&
> + memcmp(content->uuid, uuid_zero, sizeof(int[4]))) {
> if (devname)
> fprintf(stderr, Name ": %s has wrong uuid.\n",
> devname);

I really really don't like that way of testing the return value of memcmp (or
strcmp). It make it very hard to read - for me at least.
I *always* want to see one a comparison with zero with memcmp or strcmp.

So:
memcmp() == 0
means the values are equal,
memcmp() > 0
means the first is greater than the second
etc. So the comparison operator just has to be moved in my thoughts
in between the two values and I can see clearly what is intended.

> @@ -350,6 +352,7 @@ int Assemble(struct supertype *st, char *mddev,
> } else
> found_container = 1;
> } else {
> + pol = devnum_policy(stb.st_rdev);
> if (!tst && (tst = guess_super(dfd)) == NULL) {
> if (report_missmatch)
> fprintf(stderr, Name ": no recogniseable superblock on %s\n",
> @@ -366,7 +369,7 @@ int Assemble(struct supertype *st, char *mddev,
> tst->ss->name, devname);
> tmpdev->used = 2;
> } else if (auto_assem && st == NULL &&
> - !conf_test_metadata(tst->ss->name, (pol = devnum_policy(stb.st_rdev)),
> + !conf_test_metadata(tst->ss->name, pol,
> tst->ss->match_home(tst, homehost) == 1)) {
> if (report_missmatch)
> fprintf(stderr, Name ": %s has metadata type %s for which "

I have reverted this change. I only want the devnum_policy to be computed if
it is needed.

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

RE: [PATCH 2/3] Assemble imsm spares in matching domain only

> -----Original Message-----
> From: Neil Brown [mailto:neilb@suse.de]
> Sent: Sunday, December 26, 2010 12:23 PM
> To: Czarnowska, Anna
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Hawrylewicz Czarnowski, Przemyslaw; Labun, Marcin; Neubauer, Wojciech
> Subject: Re: [PATCH 2/3] Assemble imsm spares in matching domain only
>
>
> Thanks for these three.
>
> I have applied the first,
> applied the seconds with a couple of changes as mentioned below,
> but I haven't applied the first. I don't like the fact that knowledge
> about
> imsm is explicitly hard-coded into mdadm.c All explicit knowledge
> about imsm
> should be in super-intel.c...
>
> I think you can make this work by adding some code in to Assemble()
> just near:
> /* Now reject spares that don't match domains of identified
> members */
>
> at that point, if (!st || !st->sb), then we could include all those
> spares
> which have ->used == 3.
> i.e. when there are now more arrays to be found, all the spares get
> included
> into one last array.

I have implemented it this way but:
(!st || !st->sb) does not necessarily mean we are assembling "last" array.
This also happens when we find no devices for an array from config file.
So if the first array is missing we take all spares into spare-container
and they don't have a chance to be matched with other arrays.
Should we ignore this case as a bad config?

With auto assembly if we have this condition it means we can take all spares
that did not match anything before.
This is what we want but there is another problem: we do not attempt auto assembly
if we manage to assemble anything from config file.
This means that when there is a good config file then
only the spares matching some array will be assembled.
We will not come back for the remaining ones.

If mdadm.c is a bad place to force spares assembly then maybe it makes sense
to add an ARRAY line with uuid=0 as last array in config?
This would take care of spares that have different domains than all arrays.

> It might make sense to have an new super_switch field which gives the
> name of
> the array-of-spares as there is no name stored in the spares to use ...
> just
> a thought though, you might not need that.

Monitor doesn't need to know the name of the array-of-spares. It always checks all.

>
> Could you try that and see if you can make it work?
>
> The issues I fixed with this second patch are:
>
>
> > @@ -78,7 +78,8 @@ static int ident_matches(struct mddev_ident *ident,
> > {
> >
> > if (ident->uuid_set && (!update || strcmp(update, "uuid")!= 0) &&
> > - same_uuid(content->uuid, ident->uuid, tst->ss->swapuuid)==0)
> {
> > + same_uuid(content->uuid, ident->uuid, tst->ss->swapuuid)==0
> &&
> > + memcmp(content->uuid, uuid_zero, sizeof(int[4]))) {
> > if (devname)
> > fprintf(stderr, Name ": %s has wrong uuid.\n",
> > devname);
>
> I really really don't like that way of testing the return value of
> memcmp (or
> strcmp). It make it very hard to read - for me at least.
> I *always* want to see one a comparison with zero with memcmp or
> strcmp.
>
> So:
> memcmp() == 0
> means the values are equal,
> memcmp() > 0
> means the first is greater than the second
> etc. So the comparison operator just has to be moved in my thoughts
> in between the two values and I can see clearly what is intended.
>
> > @@ -350,6 +352,7 @@ int Assemble(struct supertype *st, char *mddev,
> > } else
> > found_container = 1;
> > } else {
> > + pol = devnum_policy(stb.st_rdev);
> > if (!tst && (tst = guess_super(dfd)) == NULL) {
> > if (report_missmatch)
> > fprintf(stderr, Name ": no recogniseable
> superblock on %s\n",
> > @@ -366,7 +369,7 @@ int Assemble(struct supertype *st, char *mddev,
> > tst->ss->name, devname);
> > tmpdev->used = 2;
> > } else if (auto_assem && st == NULL &&
> > - !conf_test_metadata(tst->ss->name, (pol =
> devnum_policy(stb.st_rdev)),
> > + !conf_test_metadata(tst->ss->name, pol,
> > tst->ss->match_home(tst,
> homehost) == 1)) {
> > if (report_missmatch)
> > fprintf(stderr, Name ": %s has metadata
> type %s for which "
>
> I have reverted this change. I only want the devnum_policy to be
> computed if
> it is needed.

It was needed to build array domain. I add it later on devices with used=1 only.

> Thanks,
> NeilBrown

Regards
Anna
--
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 2/3] Assemble imsm spares in matching domain only

On Tue, 4 Jan 2011 14:04:13 +0000 "Czarnowska, Anna"
wrote:

>
>
> > -----Original Message-----
> > From: Neil Brown [mailto:neilb@suse.de]
> > Sent: Sunday, December 26, 2010 12:23 PM
> > To: Czarnowska, Anna
> > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > Hawrylewicz Czarnowski, Przemyslaw; Labun, Marcin; Neubauer, Wojciech
> > Subject: Re: [PATCH 2/3] Assemble imsm spares in matching domain only
> >
> >
> > Thanks for these three.
> >
> > I have applied the first,
> > applied the seconds with a couple of changes as mentioned below,
> > but I haven't applied the first. I don't like the fact that knowledge
> > about
> > imsm is explicitly hard-coded into mdadm.c All explicit knowledge
> > about imsm
> > should be in super-intel.c...
> >
> > I think you can make this work by adding some code in to Assemble()
> > just near:
> > /* Now reject spares that don't match domains of identified
> > members */
> >
> > at that point, if (!st || !st->sb), then we could include all those
> > spares
> > which have ->used == 3.
> > i.e. when there are now more arrays to be found, all the spares get
> > included
> > into one last array.
>
> I have implemented it this way but:
> (!st || !st->sb) does not necessarily mean we are assembling "last" array.
> This also happens when we find no devices for an array from config file.
> So if the first array is missing we take all spares into spare-container
> and they don't have a chance to be matched with other arrays.
> Should we ignore this case as a bad config?

I think we should only automatically gather the spares when auto_assem is
true.
So the condition would be
if (auto_assem && (!st || !st->sb))

>
> With auto assembly if we have this condition it means we can take all spares
> that did not match anything before.
> This is what we want but there is another problem: we do not attempt auto assembly
> if we manage to assemble anything from config file.
> This means that when there is a good config file then
> only the spares matching some array will be assembled.
> We will not come back for the remaining ones.
>
> If mdadm.c is a bad place to force spares assembly then maybe it makes sense
> to add an ARRAY line with uuid=0 as last array in config?
> This would take care of spares that have different domains than all arrays.

Yes, I think having an array with uuid=0 to collect all the remaining spares
seems like a good idea.
It may turn out not to work quite the way we want, but I think we should try
it an get some experience with how well it works. I think it is at least
close to the correct solution.

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