[PATCH 1/2] DDF: brief_examine_subarrays_ddf: print array name

[PATCH 1/2] DDF: brief_examine_subarrays_ddf: print array name

[mwilck]

Print an array name in brief output. Do this even if no name is set,
faking a simple pseudo name (may happen for some BIOS RAIDs).
This makes DDF and IMSM behave equally.

SUSE's YaST2 needs this in order to detect MD arrays during
installation.

Signed-off-by: Martin Wilck <mwilck@arcor.de>
---
 super-ddf.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/super-ddf.c b/super-ddf.c
index 8bba70a..72a8351 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -30,6 +30,7 @@
 #include "mdmon.h"
 #include "sha1.h"
 #include <values.h>
+#include <ctype.h>
 
 /* a non-official T10 name for creation GUIDs */
 static char T10[] = "Linux-MD";
@@ -1577,14 +1578,22 @@ static void brief_examine_subarrays_ddf(struct supertype *st, int verbose)
 		struct virtual_entry *ve = &ddf->virt->entries[i];
 		struct vcl vcl;
 		char nbuf1[64];
+		char namebuf[sizeof(ve->name)+1], *c;
 		if (all_ff(ve->guid))
 			continue;
 		memcpy(vcl.conf.guid, ve->guid, DDF_GUID_LEN);
 		ddf->currentconf =&vcl;
 		uuid_from_super_ddf(st, info.uuid);
 		fname_from_uuid(st, &info, nbuf1, ':');
-		printf("ARRAY container=%s member=%d UUID=%s\n",
-		       nbuf+5, i, nbuf1+5);
+		memcpy(namebuf, ve->name, sizeof(ve->name));
+		namebuf[sizeof(ve->name)] = '\0';
+		if (namebuf[0] == '\0')
+			sprintf(namebuf, "ddf_%d", i);
+		for (c = namebuf; c < namebuf + sizeof(ve->name); c++)
+			if (isspace(*c))
+				*c = '-';
+		printf("ARRAY /dev/md/%s container=%s member=%d UUID=%s\n",
+		       namebuf, nbuf+5, i, nbuf1+5);
 	}
 }
 
-- 
1.7.3.4

[PATCH 2/2] DDF: new algorithm for subarray UUID

[mwilck]

Some fake RAID BIOSes (in particular, LSI ones) change the
VD GUID at every boot. These GUIDs are not suitable for
identifying an array. Luckily the header GUID appears to
remain constant.

We construct a pseudo-UUID from the header GUID and those
properties of the subarray that we expect to remain constant.

Signed-off-by: Martin Wilck <mwilck@arcor.de>
---
 super-ddf.c |   50 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/super-ddf.c b/super-ddf.c
index 72a8351..f2b8497 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -1682,6 +1682,48 @@ static void detail_super_ddf(struct supertype *st, char *homehost)
 	 */
 }
 
+static void uuid_of_ddf_subarray(const struct ddf_super *ddf,
+				 unsigned int vcnum, int uuid[4])
+{
+	char buf[DDF_GUID_LEN+34], sha[20], *p;
+	struct sha1_ctx ctx;
+	struct vcl *vc;
+	if (memcmp(ddf->controller.guid, T10, 8) == 0) {
+		uuid_from_ddf_guid(ddf->virt->entries[vcnum].guid, uuid);
+		return;
+	}
+	/*
+	 * Some fake RAID BIOSes (in particular, LSI ones) change the
+	 * VD GUID at every boot. These GUIDs are not suitable for
+	 * identifying an array. Luckily the header GUID appears to
+	 * remain constant.
+	 * We construct a pseudo-UUID from the header GUID and those
+	 * properties of the subarray that we expect to remain constant.
+	 */
+	memset(buf, 0, sizeof(buf));
+	p = buf;
+	memcpy(p, ddf->anchor.guid, DDF_GUID_LEN);
+	p += DDF_GUID_LEN;
+	memcpy(p, ddf->virt->entries[vcnum].name, 16);
+	p += 16;
+	for (vc = ddf->conflist; vc; vc = vc->next)
+		if (memcmp(vc->conf.guid, ddf->virt->entries[vcnum].guid,
+			   DDF_GUID_LEN) == 0)
+			break;
+	if (vc) {
+		memcpy(p, &vc->conf.prim_elmnt_count, 8);
+		*(p+6) = '+'; /* ignore sec_elemnt_seq */
+		p += 8;
+		memcpy(p, &vc->conf.array_blocks, 8);
+		p += 8;
+		*((__u16 *) p) = vcnum;
+	}
+	sha1_init_ctx(&ctx);
+	sha1_process_bytes(buf, sizeof(buf), &ctx);
+	sha1_finish_ctx(&ctx, sha);
+	memcpy(uuid, sha, 4*4);
+}
+
 static void brief_detail_super_ddf(struct supertype *st)
 {
 	struct mdinfo info;
@@ -1693,7 +1735,7 @@ static void brief_detail_super_ddf(struct supertype *st)
 	else if (vcnum == DDF_NOTFOUND)
 		return;
 	else
-		uuid_from_ddf_guid(ddf->virt->entries[vcnum].guid, info.uuid);
+		uuid_of_ddf_subarray(ddf, vcnum, info.uuid);
 	fname_from_uuid(st, &info, nbuf,':');
 	printf(" UUID=%s", nbuf + 5);
 }
@@ -1836,13 +1878,11 @@ static void uuid_from_super_ddf(struct supertype *st, int uuid[4])
 	 */
 	struct ddf_super *ddf = st->sb;
 	struct vcl *vcl = ddf->currentconf;
-	char *guid;
 
 	if (vcl)
-		guid = vcl->conf.guid;
+		uuid_of_ddf_subarray(ddf, vcl->vcnum, uuid);
 	else
-		guid = ddf->anchor.guid;
-	uuid_from_ddf_guid(guid, uuid);
+		uuid_from_ddf_guid(ddf->anchor.guid, uuid);
 }
 
 static void getinfo_super_ddf_bvd(struct supertype *st, struct mdinfo *info, char *map);
-- 
1.7.3.4

Re: [PATCH 2/2] DDF: new algorithm for subarray UUID

[Martin Wilck]

On 09/06/2013 11:26 PM, mwilck@arcor.de wrote:
> Some fake RAID BIOSes (in particular, LSI ones) change the
> VD GUID at every boot. These GUIDs are not suitable for
> identifying an array. Luckily the header GUID appears to
> remain constant.
> 
> We construct a pseudo-UUID from the header GUID and those
> properties of the subarray that we expect to remain constant.

Thinking about it once more, it may actually be better to construct the
subarray UUID only from the container GUID and the member number (and
name), because if we implement Grow or Reshape for DDF, the other
properties might change.

I'm open for comments and suggestions.

Martin

Re: [PATCH 1/2] DDF: brief_examine_subarrays_ddf: print array name

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2047 bytes --]

On Fri,  6 Sep 2013 23:26:20 +0200 mwilck@arcor.de wrote:

> Print an array name in brief output. Do this even if no name is set,
> faking a simple pseudo name (may happen for some BIOS RAIDs).
> This makes DDF and IMSM behave equally.
> 
> SUSE's YaST2 needs this in order to detect MD arrays during
> installation.

Thanks, but I don't like this - I would really rather SUSE's YaST2 got fixed.

mdadm.conf is really for mdadm to read.  And the "--brief" output is meant
for inclusion in mdadm.conf.

If other programs want to parse the output of mdadm, they should use
--export, not --brief.

NeilBrown

> 
> Signed-off-by: Martin Wilck <mwilck@arcor.de>
> ---
>  super-ddf.c |   13 +++++++++++--
>  1 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/super-ddf.c b/super-ddf.c
> index 8bba70a..72a8351 100644
> --- a/super-ddf.c
> +++ b/super-ddf.c
> @@ -30,6 +30,7 @@
>  #include "mdmon.h"
>  #include "sha1.h"
>  #include <values.h>
> +#include <ctype.h>
>  
>  /* a non-official T10 name for creation GUIDs */
>  static char T10[] = "Linux-MD";
> @@ -1577,14 +1578,22 @@ static void brief_examine_subarrays_ddf(struct supertype *st, int verbose)
>  		struct virtual_entry *ve = &ddf->virt->entries[i];
>  		struct vcl vcl;
>  		char nbuf1[64];
> +		char namebuf[sizeof(ve->name)+1], *c;
>  		if (all_ff(ve->guid))
>  			continue;
>  		memcpy(vcl.conf.guid, ve->guid, DDF_GUID_LEN);
>  		ddf->currentconf =&vcl;
>  		uuid_from_super_ddf(st, info.uuid);
>  		fname_from_uuid(st, &info, nbuf1, ':');
> -		printf("ARRAY container=%s member=%d UUID=%s\n",
> -		       nbuf+5, i, nbuf1+5);
> +		memcpy(namebuf, ve->name, sizeof(ve->name));
> +		namebuf[sizeof(ve->name)] = '\0';
> +		if (namebuf[0] == '\0')
> +			sprintf(namebuf, "ddf_%d", i);
> +		for (c = namebuf; c < namebuf + sizeof(ve->name); c++)
> +			if (isspace(*c))
> +				*c = '-';
> +		printf("ARRAY /dev/md/%s container=%s member=%d UUID=%s\n",
> +		       namebuf, nbuf+5, i, nbuf1+5);
>  	}
>  }
>  


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/2] DDF: new algorithm for subarray UUID

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

On Sat, 07 Sep 2013 21:23:22 +0200 Martin Wilck <mwilck@arcor.de> wrote:

> On 09/06/2013 11:26 PM, mwilck@arcor.de wrote:
> > Some fake RAID BIOSes (in particular, LSI ones) change the
> > VD GUID at every boot. These GUIDs are not suitable for
> > identifying an array. Luckily the header GUID appears to
> > remain constant.
> > 
> > We construct a pseudo-UUID from the header GUID and those
> > properties of the subarray that we expect to remain constant.
> 
> Thinking about it once more, it may actually be better to construct the
> subarray UUID only from the container GUID and the member number (and
> name), because if we implement Grow or Reshape for DDF, the other
> properties might change.
> 
> I'm open for comments and suggestions.
> 
> Martin

I'd probably just base it on the container GUID and the member number.
Exclude even the name.

It is unfortunate that this change will cause all existing DDF uuids to
change even for controllers where they are currently stable.

Is it possible to detect whether a VD GUID is likely to change, and to only
use the new algorithm for those?

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/2] DDF: brief_examine_subarrays_ddf: print array name

[Martin Wilck]

Hi Neil,

>> Print an array name in brief output. Do this even if no name is set,
>> faking a simple pseudo name (may happen for some BIOS RAIDs).
>> This makes DDF and IMSM behave equally.
>>
>> SUSE's YaST2 needs this in order to detect MD arrays during
>> installation.
> 
> Thanks, but I don't like this - I would really rather SUSE's YaST2 got fixed.
> 
> mdadm.conf is really for mdadm to read.  And the "--brief" output is meant
> for inclusion in mdadm.conf.
> 
> If other programs want to parse the output of mdadm, they should use
> --export, not --brief.

Well, yast is actually not calling --brief. It calls "mdadm --examine
--scan", and --scan implies --brief (comment in mdadm.c). My patch
simply causes DDF and IMSM to print similar output with "mdadm -Es".
Isn't that reasonable?

Martin


> 
> NeilBrown
> 
>>
>> Signed-off-b545d3bca8b105ba10dadbab2fcd31467b5613c11y: Martin Wilck <mwilck@arcor.de>
>> ---
>>  super-ddf.c |   13 +++++++++++--
>>  1 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/super-ddf.c b/super-ddf.c
>> index 8bba70a..72a8351 100644
>> --- a/super-ddf.c
>> +++ b/super-ddf.c
>> @@ -30,6 +30,7 @@
>>  #include "mdmon.h"
>>  #include "sha1.h"
>>  #include <values.h>
>> +#include <ctype.h>
>>  
>>  /* a non-official T10 name for creation GUIDs */
>>  static char T10[] = "Linux-MD";
>> @@ -1577,14 +1578,22 @@ static void brief_examine_subarrays_ddf(struct supertype *st, int verbose)
>>  		struct virtual_entry *ve = &ddf->virt->entries[i];
>>  		struct vcl vcl;
>>  		char nbuf1[64];
>> +		char namebuf[sizeof(ve->name)+1], *c;
>>  		if (all_ff(ve->guid))
>>  			continue;
>>  		memcpy(vcl.conf.guid, ve->guid, DDF_GUID_LEN);
>>  		ddf->currentconf =&vcl;
>>  		uuid_from_super_ddf(st, info.uuid);
>>  		fname_from_uuid(st, &info, nbuf1, ':');
>> -		printf("ARRAY container=%s member=%d UUID=%s\n",
>> -		       nbuf+5, i, nbuf1+5);
>> +		memcpy(namebuf, ve->name, sizeof(ve->name));
>> +		namebuf[sizeof(ve->name)] = '\0';
>> +		if (namebuf[0] == '\0')
>> +			sprintf(namebuf, "ddf_%d", i);
>> +		for (c = namebuf; c < namebuf + sizeof(ve->name); c++)
>> +			if (isspace(*c))
>> +				*c = '-';
>> +		printf("ARRAY /dev/md/%s container=%s member=%d UUID=%s\n",
>> +		       namebuf, nbuf+5, i, nbuf1+5);
>>  	}
>>  }
>>  
> 

Re: [PATCH 2/2] DDF: new algorithm for subarray UUID

[Martin Wilck]

On 09/09/2013 03:20 AM, NeilBrown wrote:
> On Sat, 07 Sep 2013 21:23:22 +0200 Martin Wilck <mwilck@arcor.de> wrote:
> 
>> On 09/06/2013 11:26 PM, mwilck@arcor.de wrote:
>>> Some fake RAID BIOSes (in particular, LSI ones) change the
>>> VD GUID at every boot. These GUIDs are not suitable for
>>> identifying an array. Luckily the header GUID appears to
>>> remain constant.
>>>
>>> We construct a pseudo-UUID from the header GUID and those
>>> properties of the subarray that we expect to remain constant.
>>
>> Thinking about it once more, it may actually be better to construct the
>> subarray UUID only from the container GUID and the member number (and
>> name), because if we implement Grow or Reshape for DDF, the other
>> properties might change.
>>
>> I'm open for comments and suggestions.
>>
>> Martin
> 
> I'd probably just base it on the container GUID and the member number.
> Exclude even the name.

Why that? Does it make sense to expect the UUID to remain the same when
the array name has changed?

> It is unfortunate that this change will cause all existing DDF uuids to
> change even for controllers where they are currently stable.

Only for "foreign" arrays not created by MD - but see below.

> Is it possible to detect whether a VD GUID is likely to change, and to only
> use the new algorithm for those?

I am going to submit a new patch soon where I built in a vendor "black
list". This list currently contains only LSI, so the UUIDs of all
non-LSI arrays will stay the same, and for LSI, to the best of our
current knowledge, the UUIDs aren't constant anyway. I don't know
whether all LSI fake RAIDs behave like this, nor whether the Option ROMs
of other vendors do. Perhaps other people on this list can supply data
points on this.

Martin

> 
> Thanks,
> NeilBrown

[PATCH 2/2] DDF: handle fake RAIDs with changing subarray UUIDs

[mwilck]

Some fake RAID BIOSes (in particular, LSI ones) change the
VD GUID at every boot. These GUIDs are not suitable for
identifying an array. Luckily the header GUID appears to
remain constant.

We construct a pseudo-UUID from the header GUID and those
properties of the subarray that we expect to remain constant.
This is only array name and index; all else might change e.g.
during grow.

Don't do this for all non-MD arrays, only for those known
to use varying volume GUIDs.

This patch obsoletes my previous patch "DDF: new algorithm
for subarray UUID"

Signed-off-by: Martin Wilck <mwilck@arcor.de>
---
 super-ddf.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/super-ddf.c b/super-ddf.c
index 72a8351..19d8494 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -1583,6 +1583,7 @@ static void brief_examine_subarrays_ddf(struct supertype *st, int verbose)
 			continue;
 		memcpy(vcl.conf.guid, ve->guid, DDF_GUID_LEN);
 		ddf->currentconf =&vcl;
+		vcl.vcnum = i;
 		uuid_from_super_ddf(st, info.uuid);
 		fname_from_uuid(st, &info, nbuf1, ':');
 		memcpy(namebuf, ve->name, sizeof(ve->name));
@@ -1682,6 +1683,52 @@ static void detail_super_ddf(struct supertype *st, char *homehost)
 	 */
 }
 
+static const char *vendors_with_variable_volume_UUID[] = {
+	"LSI      ",
+};
+
+static int volume_id_is_reliable(const struct ddf_super *ddf)
+{
+	int n = sizeof(vendors_with_variable_volume_UUID) / 
+		sizeof(vendors_with_variable_volume_UUID[0]);
+	int i;
+	for (i = 0; i < n; i++)
+		if (!memcmp(ddf->controller.guid,
+			vendors_with_variable_volume_UUID[i], 8))
+		return 0;
+	return 1;
+}
+
+static void uuid_of_ddf_subarray(const struct ddf_super *ddf,
+				 unsigned int vcnum, int uuid[4])
+{
+	char buf[DDF_GUID_LEN+18], sha[20], *p;
+	struct sha1_ctx ctx;
+	if (volume_id_is_reliable(ddf)) {
+		uuid_from_ddf_guid(ddf->virt->entries[vcnum].guid, uuid);
+		return;
+	}
+	/*
+	 * Some fake RAID BIOSes (in particular, LSI ones) change the
+	 * VD GUID at every boot. These GUIDs are not suitable for
+	 * identifying an array. Luckily the header GUID appears to
+	 * remain constant.
+	 * We construct a pseudo-UUID from the header GUID and those
+	 * properties of the subarray that we expect to remain constant.
+	 */
+	memset(buf, 0, sizeof(buf));
+	p = buf;
+	memcpy(p, ddf->anchor.guid, DDF_GUID_LEN);
+	p += DDF_GUID_LEN;
+	memcpy(p, ddf->virt->entries[vcnum].name, 16);
+	p += 16;
+	*((__u16 *) p) = vcnum;
+	sha1_init_ctx(&ctx);
+	sha1_process_bytes(buf, sizeof(buf), &ctx);
+	sha1_finish_ctx(&ctx, sha);
+	memcpy(uuid, sha, 4*4);
+}
+
 static void brief_detail_super_ddf(struct supertype *st)
 {
 	struct mdinfo info;
@@ -1693,7 +1740,7 @@ static void brief_detail_super_ddf(struct supertype *st)
 	else if (vcnum == DDF_NOTFOUND)
 		return;
 	else
-		uuid_from_ddf_guid(ddf->virt->entries[vcnum].guid, info.uuid);
+		uuid_of_ddf_subarray(ddf, vcnum, info.uuid);
 	fname_from_uuid(st, &info, nbuf,':');
 	printf(" UUID=%s", nbuf + 5);
 }
@@ -1836,13 +1883,11 @@ static void uuid_from_super_ddf(struct supertype *st, int uuid[4])
 	 */
 	struct ddf_super *ddf = st->sb;
 	struct vcl *vcl = ddf->currentconf;
-	char *guid;
 
 	if (vcl)
-		guid = vcl->conf.guid;
+		uuid_of_ddf_subarray(ddf, vcl->vcnum, uuid);
 	else
-		guid = ddf->anchor.guid;
-	uuid_from_ddf_guid(guid, uuid);
+		uuid_from_ddf_guid(ddf->anchor.guid, uuid);
 }
 
 static void getinfo_super_ddf_bvd(struct supertype *st, struct mdinfo *info, char *map);
-- 
1.7.3.4

Re: [PATCH 2/2] DDF: handle fake RAIDs with changing subarray UUIDs

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3946 bytes --]

On Mon,  9 Sep 2013 20:40:14 +0200 mwilck@arcor.de wrote:

> Some fake RAID BIOSes (in particular, LSI ones) change the
> VD GUID at every boot. These GUIDs are not suitable for
> identifying an array. Luckily the header GUID appears to
> remain constant.
> 
> We construct a pseudo-UUID from the header GUID and those
> properties of the subarray that we expect to remain constant.
> This is only array name and index; all else might change e.g.
> during grow.
> 
> Don't do this for all non-MD arrays, only for those known
> to use varying volume GUIDs.
> 
> This patch obsoletes my previous patch "DDF: new algorithm
> for subarray UUID"
> 
> Signed-off-by: Martin Wilck <mwilck@arcor.de>

Thanks.  Applied.

NeilBrown


> ---
>  super-ddf.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/super-ddf.c b/super-ddf.c
> index 72a8351..19d8494 100644
> --- a/super-ddf.c
> +++ b/super-ddf.c
> @@ -1583,6 +1583,7 @@ static void brief_examine_subarrays_ddf(struct supertype *st, int verbose)
>  			continue;
>  		memcpy(vcl.conf.guid, ve->guid, DDF_GUID_LEN);
>  		ddf->currentconf =&vcl;
> +		vcl.vcnum = i;
>  		uuid_from_super_ddf(st, info.uuid);
>  		fname_from_uuid(st, &info, nbuf1, ':');
>  		memcpy(namebuf, ve->name, sizeof(ve->name));
> @@ -1682,6 +1683,52 @@ static void detail_super_ddf(struct supertype *st, char *homehost)
>  	 */
>  }
>  
> +static const char *vendors_with_variable_volume_UUID[] = {
> +	"LSI      ",
> +};
> +
> +static int volume_id_is_reliable(const struct ddf_super *ddf)
> +{
> +	int n = sizeof(vendors_with_variable_volume_UUID) / 
> +		sizeof(vendors_with_variable_volume_UUID[0]);
> +	int i;
> +	for (i = 0; i < n; i++)
> +		if (!memcmp(ddf->controller.guid,
> +			vendors_with_variable_volume_UUID[i], 8))
> +		return 0;
> +	return 1;
> +}
> +
> +static void uuid_of_ddf_subarray(const struct ddf_super *ddf,
> +				 unsigned int vcnum, int uuid[4])
> +{
> +	char buf[DDF_GUID_LEN+18], sha[20], *p;
> +	struct sha1_ctx ctx;
> +	if (volume_id_is_reliable(ddf)) {
> +		uuid_from_ddf_guid(ddf->virt->entries[vcnum].guid, uuid);
> +		return;
> +	}
> +	/*
> +	 * Some fake RAID BIOSes (in particular, LSI ones) change the
> +	 * VD GUID at every boot. These GUIDs are not suitable for
> +	 * identifying an array. Luckily the header GUID appears to
> +	 * remain constant.
> +	 * We construct a pseudo-UUID from the header GUID and those
> +	 * properties of the subarray that we expect to remain constant.
> +	 */
> +	memset(buf, 0, sizeof(buf));
> +	p = buf;
> +	memcpy(p, ddf->anchor.guid, DDF_GUID_LEN);
> +	p += DDF_GUID_LEN;
> +	memcpy(p, ddf->virt->entries[vcnum].name, 16);
> +	p += 16;
> +	*((__u16 *) p) = vcnum;
> +	sha1_init_ctx(&ctx);
> +	sha1_process_bytes(buf, sizeof(buf), &ctx);
> +	sha1_finish_ctx(&ctx, sha);
> +	memcpy(uuid, sha, 4*4);
> +}
> +
>  static void brief_detail_super_ddf(struct supertype *st)
>  {
>  	struct mdinfo info;
> @@ -1693,7 +1740,7 @@ static void brief_detail_super_ddf(struct supertype *st)
>  	else if (vcnum == DDF_NOTFOUND)
>  		return;
>  	else
> -		uuid_from_ddf_guid(ddf->virt->entries[vcnum].guid, info.uuid);
> +		uuid_of_ddf_subarray(ddf, vcnum, info.uuid);
>  	fname_from_uuid(st, &info, nbuf,':');
>  	printf(" UUID=%s", nbuf + 5);
>  }
> @@ -1836,13 +1883,11 @@ static void uuid_from_super_ddf(struct supertype *st, int uuid[4])
>  	 */
>  	struct ddf_super *ddf = st->sb;
>  	struct vcl *vcl = ddf->currentconf;
> -	char *guid;
>  
>  	if (vcl)
> -		guid = vcl->conf.guid;
> +		uuid_of_ddf_subarray(ddf, vcl->vcnum, uuid);
>  	else
> -		guid = ddf->anchor.guid;
> -	uuid_from_ddf_guid(guid, uuid);
> +		uuid_from_ddf_guid(ddf->anchor.guid, uuid);
>  }
>  
>  static void getinfo_super_ddf_bvd(struct supertype *st, struct mdinfo *info, char *map);


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/2] DDF: new algorithm for subarray UUID

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2274 bytes --]

On Mon, 09 Sep 2013 20:37:10 +0200 Martin Wilck <mwilck@arcor.de> wrote:

> On 09/09/2013 03:20 AM, NeilBrown wrote:
> > On Sat, 07 Sep 2013 21:23:22 +0200 Martin Wilck <mwilck@arcor.de> wrote:
> > 
> >> On 09/06/2013 11:26 PM, mwilck@arcor.de wrote:
> >>> Some fake RAID BIOSes (in particular, LSI ones) change the
> >>> VD GUID at every boot. These GUIDs are not suitable for
> >>> identifying an array. Luckily the header GUID appears to
> >>> remain constant.
> >>>
> >>> We construct a pseudo-UUID from the header GUID and those
> >>> properties of the subarray that we expect to remain constant.
> >>
> >> Thinking about it once more, it may actually be better to construct the
> >> subarray UUID only from the container GUID and the member number (and
> >> name), because if we implement Grow or Reshape for DDF, the other
> >> properties might change.
> >>
> >> I'm open for comments and suggestions.
> >>
> >> Martin
> > 
> > I'd probably just base it on the container GUID and the member number.
> > Exclude even the name.
> 
> Why that? Does it make sense to expect the UUID to remain the same when
> the array name has changed?

I would rather the UUID only changed when it absolutely has too.
A name is just an attributed, not part of the core identity.


> 
> > It is unfortunate that this change will cause all existing DDF uuids to
> > change even for controllers where they are currently stable.
> 
> Only for "foreign" arrays not created by MD - but see below.
> 
> > Is it possible to detect whether a VD GUID is likely to change, and to only
> > use the new algorithm for those?
> 
> I am going to submit a new patch soon where I built in a vendor "black
> list". This list currently contains only LSI, so the UUIDs of all
> non-LSI arrays will stay the same, and for LSI, to the best of our
> current knowledge, the UUIDs aren't constant anyway. I don't know
> whether all LSI fake RAIDs behave like this, nor whether the Option ROMs
> of other vendors do. Perhaps other people on this list can supply data
> points on this.

Thanks.  I'm going with this patch for now.  I'm certainly keen to hear of
any other data points which might suggest some improvement might be possible.

Thanks.
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/2] DDF: brief_examine_subarrays_ddf: print array name

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3080 bytes --]

On Mon, 09 Sep 2013 20:31:11 +0200 Martin Wilck <mwilck@arcor.de> wrote:

> Hi Neil,
> 
> >> Print an array name in brief output. Do this even if no name is set,
> >> faking a simple pseudo name (may happen for some BIOS RAIDs).
> >> This makes DDF and IMSM behave equally.
> >>
> >> SUSE's YaST2 needs this in order to detect MD arrays during
> >> installation.
> > 
> > Thanks, but I don't like this - I would really rather SUSE's YaST2 got fixed.
> > 
> > mdadm.conf is really for mdadm to read.  And the "--brief" output is meant
> > for inclusion in mdadm.conf.
> > 
> > If other programs want to parse the output of mdadm, they should use
> > --export, not --brief.
> 
> Well, yast is actually not calling --brief. It calls "mdadm --examine
> --scan", and --scan implies --brief (comment in mdadm.c). My patch
> simply causes DDF and IMSM to print similar output with "mdadm -Es".
> Isn't that reasonable?

This bit that I really didn't like is where you synthesised a "ddf_%d" name
if the name field was empty.  This is wrong.

There are a couple of other places where the name is extracted from the
ddf metadata: getinfo_super_ddf_bvd and container_content_ddf.
If brief_examine_subarrays_ddf uses the name in the same way following
the same rules (e.g. discarding trailing spaces) then that would be OK.
But if the name is empty, then no name should be given in the 'array' line.

NeilBrown

> 
> Martin
> 
> 
> > 
> > NeilBrown
> > 
> >>
> >> Signed-off-b545d3bca8b105ba10dadbab2fcd31467b5613c11y: Martin Wilck <mwilck@arcor.de>
> >> ---
> >>  super-ddf.c |   13 +++++++++++--
> >>  1 files changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/super-ddf.c b/super-ddf.c
> >> index 8bba70a..72a8351 100644
> >> --- a/super-ddf.c
> >> +++ b/super-ddf.c
> >> @@ -30,6 +30,7 @@
> >>  #include "mdmon.h"
> >>  #include "sha1.h"
> >>  #include <values.h>
> >> +#include <ctype.h>
> >>  
> >>  /* a non-official T10 name for creation GUIDs */
> >>  static char T10[] = "Linux-MD";
> >> @@ -1577,14 +1578,22 @@ static void brief_examine_subarrays_ddf(struct supertype *st, int verbose)
> >>  		struct virtual_entry *ve = &ddf->virt->entries[i];
> >>  		struct vcl vcl;
> >>  		char nbuf1[64];
> >> +		char namebuf[sizeof(ve->name)+1], *c;
> >>  		if (all_ff(ve->guid))
> >>  			continue;
> >>  		memcpy(vcl.conf.guid, ve->guid, DDF_GUID_LEN);
> >>  		ddf->currentconf =&vcl;
> >>  		uuid_from_super_ddf(st, info.uuid);
> >>  		fname_from_uuid(st, &info, nbuf1, ':');
> >> -		printf("ARRAY container=%s member=%d UUID=%s\n",
> >> -		       nbuf+5, i, nbuf1+5);
> >> +		memcpy(namebuf, ve->name, sizeof(ve->name));
> >> +		namebuf[sizeof(ve->name)] = '\0';
> >> +		if (namebuf[0] == '\0')
> >> +			sprintf(namebuf, "ddf_%d", i);
> >> +		for (c = namebuf; c < namebuf + sizeof(ve->name); c++)
> >> +			if (isspace(*c))
> >> +				*c = '-';
> >> +		printf("ARRAY /dev/md/%s container=%s member=%d UUID=%s\n",
> >> +		       namebuf, nbuf+5, i, nbuf1+5);
> >>  	}
> >>  }
> >>  
> > 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/2] DDF: brief_examine_subarrays_ddf: print array name

[Martin Wilck]

On 09/10/2013 02:01 AM, NeilBrown wrote:

>> Well, yast is actually not calling --brief. It calls "mdadm --examine
>> --scan", and --scan implies --brief (comment in mdadm.c). My patch
>> simply causes DDF and IMSM to print similar output with "mdadm -Es".
>> Isn't that reasonable?
> 
> This bit that I really didn't like is where you synthesised a "ddf_%d" name
> if the name field was empty.  This is wrong.

Didn't really like it myself. Thanks for pointing it out.

> There are a couple of other places where the name is extracted from the
> ddf metadata: getinfo_super_ddf_bvd and container_content_ddf.
> If brief_examine_subarrays_ddf uses the name in the same way following
> the same rules (e.g. discarding trailing spaces) then that would be OK.
> But if the name is empty, then no name should be given in the 'array' line.

That means that libstorage's way of parsing mdadm -Es output must be
changed, but that should be possible. Sending new patches.

Martin

Re: [PATCH 1/2] DDF: brief_examine_subarrays_ddf: print array name

[Martin Wilck]

On 09/11/2013 09:50 PM, Martin Wilck wrote:

> That means that libstorage's way of parsing mdadm -Es output must be
> changed, but that should be possible. Sending new patches.

Forgot to mention: It seems that IMSM BIOS forces users to set an array
name. LSI BIOS does not, the name is optional there. It is possible to
have several subarrays, each without name.

Martin

[PATCH 1/2] DDF: factor out array name generation

[mwilck]

The same algorithm was used in getinfo_super_ddf_bvd and
container_content_ddf. Put it in a common function.

Signed-off-by: Martin Wilck <mwilck@arcor.de>
---
 super-ddf.c |   25 +++++++++++++------------
 1 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/super-ddf.c b/super-ddf.c
index 636d7b4..c2ac88b 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -1949,6 +1949,17 @@ static void getinfo_super_ddf(struct supertype *st, struct mdinfo *info, char *m
 	}
 }
 
+/* size of name must be at least 17 bytes! */
+static void _ddf_array_name(char *name, const struct ddf_super *ddf, int i)
+{
+	int j;
+	memcpy(name, ddf->virt->entries[i].name, 16);
+	name[16] = 0;
+	for(j = 0; j < 16; j++)
+		if (name[j] == ' ')
+			name[j] = 0;
+}
+
 static void getinfo_super_ddf_bvd(struct supertype *st, struct mdinfo *info, char *map)
 {
 	struct ddf_super *ddf = st->sb;
@@ -2026,11 +2037,7 @@ static void getinfo_super_ddf_bvd(struct supertype *st, struct mdinfo *info, cha
 		info->container_member);
 	info->safe_mode_delay = DDF_SAFE_MODE_DELAY;
 
-	memcpy(info->name, ddf->virt->entries[info->container_member].name, 16);
-	info->name[16]=0;
-	for(j=0; j<16; j++)
-		if (info->name[j] == ' ')
-			info->name[j] = 0;
+	_ddf_array_name(info->name, ddf, info->container_member);
 
 	if (map)
 		for (j = 0; j < map_disks; j++) {
@@ -3691,7 +3698,6 @@ static struct mdinfo *container_content_ddf(struct supertype *st, char *subarray
 	for (vc = ddf->conflist ; vc ; vc=vc->next)
 	{
 		unsigned int i;
-		unsigned int j;
 		struct mdinfo *this;
 		char *ep;
 		__u32 *cptr;
@@ -3733,12 +3739,7 @@ static struct mdinfo *container_content_ddf(struct supertype *st, char *subarray
 			this->array.state = 1;
 			this->resync_start = MaxSector;
 		}
-		memcpy(this->name, ddf->virt->entries[i].name, 16);
-		this->name[16]=0;
-		for(j=0; j<16; j++)
-			if (this->name[j] == ' ')
-				this->name[j] = 0;
-
+		_ddf_array_name(this->name, ddf, i);
 		memset(this->uuid, 0, sizeof(this->uuid));
 		this->component_size = be64_to_cpu(vc->conf.blocks);
 		this->array.size = this->component_size / 2;
-- 
1.7.3.4

[PATCH 2/2] DDF: brief_examine_subarrays_ddf: print array name

[mwilck]

Print an array name in brief output, like IMSM does.

SUSE's YaST2 (libstorage) needs this in order to detect MD arrays
during installation.

Signed-off-by: Martin Wilck <mwilck@arcor.de>
---
 super-ddf.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/super-ddf.c b/super-ddf.c
index c2ac88b..00a5c8b 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -1518,6 +1518,7 @@ static void getinfo_super_ddf(struct supertype *st, struct mdinfo *info, char *m
 
 static void uuid_from_ddf_guid(const char *guid, int uuid[4]);
 static void uuid_from_super_ddf(struct supertype *st, int uuid[4]);
+static void _ddf_array_name(char *name, const struct ddf_super *ddf, int i);
 
 static unsigned int get_vd_num_of_subarray(struct supertype *st)
 {
@@ -1577,6 +1578,7 @@ static void brief_examine_subarrays_ddf(struct supertype *st, int verbose)
 		struct virtual_entry *ve = &ddf->virt->entries[i];
 		struct vcl vcl;
 		char nbuf1[64];
+		char namebuf[17];
 		if (all_ff(ve->guid))
 			continue;
 		memcpy(vcl.conf.guid, ve->guid, DDF_GUID_LEN);
@@ -1584,7 +1586,9 @@ static void brief_examine_subarrays_ddf(struct supertype *st, int verbose)
 		vcl.vcnum = i;
 		uuid_from_super_ddf(st, info.uuid);
 		fname_from_uuid(st, &info, nbuf1, ':');
-		printf("ARRAY container=%s member=%d UUID=%s\n",
+		_ddf_array_name(namebuf, ddf, i);
+		printf("ARRAY%s%s container=%s member=%d UUID=%s\n",
+		       namebuf[0] == '\0' ? "" : " /dev/md/", namebuf,
 		       nbuf+5, i, nbuf1+5);
 	}
 }
-- 
1.7.3.4

Re: [PATCH 2/2] DDF: brief_examine_subarrays_ddf: print array name

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1734 bytes --]

On Wed, 11 Sep 2013 21:55:35 +0200 mwilck@arcor.de wrote:

> Print an array name in brief output, like IMSM does.
> 
> SUSE's YaST2 (libstorage) needs this in order to detect MD arrays
> during installation.
> 
> Signed-off-by: Martin Wilck <mwilck@arcor.de>
> ---
>  super-ddf.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/super-ddf.c b/super-ddf.c
> index c2ac88b..00a5c8b 100644
> --- a/super-ddf.c
> +++ b/super-ddf.c
> @@ -1518,6 +1518,7 @@ static void getinfo_super_ddf(struct supertype *st, struct mdinfo *info, char *m
>  
>  static void uuid_from_ddf_guid(const char *guid, int uuid[4]);
>  static void uuid_from_super_ddf(struct supertype *st, int uuid[4]);
> +static void _ddf_array_name(char *name, const struct ddf_super *ddf, int i);
>  
>  static unsigned int get_vd_num_of_subarray(struct supertype *st)
>  {
> @@ -1577,6 +1578,7 @@ static void brief_examine_subarrays_ddf(struct supertype *st, int verbose)
>  		struct virtual_entry *ve = &ddf->virt->entries[i];
>  		struct vcl vcl;
>  		char nbuf1[64];
> +		char namebuf[17];
>  		if (all_ff(ve->guid))
>  			continue;
>  		memcpy(vcl.conf.guid, ve->guid, DDF_GUID_LEN);
> @@ -1584,7 +1586,9 @@ static void brief_examine_subarrays_ddf(struct supertype *st, int verbose)
>  		vcl.vcnum = i;
>  		uuid_from_super_ddf(st, info.uuid);
>  		fname_from_uuid(st, &info, nbuf1, ':');
> -		printf("ARRAY container=%s member=%d UUID=%s\n",
> +		_ddf_array_name(namebuf, ddf, i);
> +		printf("ARRAY%s%s container=%s member=%d UUID=%s\n",
> +		       namebuf[0] == '\0' ? "" : " /dev/md/", namebuf,
>  		       nbuf+5, i, nbuf1+5);
>  	}
>  }

This an previous applied - thanks.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/2] DDF: brief_examine_subarrays_ddf: print array name

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 649 bytes --]

On Wed, 11 Sep 2013 21:55:26 +0200 Martin Wilck <mwilck@arcor.de> wrote:

> On 09/11/2013 09:50 PM, Martin Wilck wrote:
> 
> > That means that libstorage's way of parsing mdadm -Es output must be
> > changed, but that should be possible. Sending new patches.
> 
> Forgot to mention: It seems that IMSM BIOS forces users to set an array
> name. LSI BIOS does not, the name is optional there. It is possible to
> have several subarrays, each without name.
> 
>

Yes.  Names are optional.  Each array needs a UUID though.

If no name is found in the metadata or in mdadm.conf you end up with "md127"
or similar I think.

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]