[PATCH] imsm: assign UUID of spare device based on ARRAY *device* keyword in mdadm configuration file.

[PATCH] imsm: assign UUID of spare device based on ARRAY *device* keyword in mdadm configuration file.

[Hawrylewicz Czarnowski, Przemyslaw]

If config has list of devices associated with container and it comprises given spare drive, UUID is set apporopriately to this container.
It allows to assign spare with proper container according
to content in devices parameter in configuration file.

Signed-off-by: Przemyslaw Czarnowski <przemyslaw.hawrylewicz.czarnowski@intel.com>
---
 super-intel.c |   48 +++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index a196ca3..41a4cc6 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -1497,10 +1497,19 @@ static void getinfo_super_imsm_volume(struct supertype *st, struct mdinfo *info)
 	uuid_from_super_imsm(st, info->uuid);
 }
 
+static int matchfd2devnum(int fd, int maj, int min)
+{
+	struct stat st;
+	if (fstat(fd, &st) != 0)
+		return 1;
+	return ((int)major(st.st_rdev) == maj && (int)minor(st.st_rdev) == min);
+}
+
+
 /* check the config file to see if we can return a real uuid for this spare */
-static void fixup_container_spare_uuid(struct mdinfo *inf)
+static void fixup_container_spare_uuid(struct mdinfo *inf, struct dl *disk)
 {
-	struct mddev_ident_s *array_list;
+	struct mddev_ident_s *array_list, *first_match = NULL;
 
 	if (inf->array.level != LEVEL_CONTAINER ||
 	    memcmp(inf->uuid, uuid_match_any, sizeof(int[4])) != 0)
@@ -1520,12 +1529,41 @@ static void fixup_container_spare_uuid(struct mdinfo *inf)
 				_sst = NULL;
 
 			if (_sst) {
-				memcpy(inf->uuid, array_list->uuid, sizeof(int[4]));
+				char *devices = array_list->devices;
+
+				if (!first_match)
+					first_match = array_list;
+
+				while (disk && devices && *devices) {
+					char path[PATH_MAX];
+					char *p = devices;
+					int fd;
+
+					devices = strchr(devices, ',');
+					if (!devices)
+						devices = p + strlen(p);
+					if (devices-p < PATH_MAX) {
+						strncpy(path, p, devices - p);
+						if ((fd = open(path, O_RDONLY)) >= 0) {
+							if (matchfd2devnum(fd, disk->major, disk->minor)) {
+								free(_sst);
+								close(fd);
+								goto match;
+							}
+							close(fd);
+						}
+						if (*devices == ',')
+							devices++;
+					}
+				}
 				free(_sst);
-				break;
 			}
 		}
 	}
+match:;
+	if (!array_list)
+		array_list = first_match;
+	memcpy(inf->uuid, array_list->uuid, sizeof(int[4]));
 }
 
 static void getinfo_super_imsm(struct supertype *st, struct mdinfo *info)
@@ -1584,7 +1622,7 @@ static void getinfo_super_imsm(struct supertype *st, struct mdinfo *info)
 		uuid_from_super_imsm(st, info->uuid);
 	else {
 		memcpy(info->uuid, uuid_match_any, sizeof(int[4]));
-		fixup_container_spare_uuid(info);
+		fixup_container_spare_uuid(info, super->disks);
 	}
 }
 
-- 
1.6.4.2

Re: [PATCH] imsm: assign UUID of spare device based on ARRAY *device* keyword in mdadm configuration file.

[Dan Williams]

On Thu, Apr 8, 2010 at 7:48 AM, Hawrylewicz Czarnowski, Przemyslaw
<przemyslaw.hawrylewicz.czarnowski@intel.com> wrote:
> If config has list of devices associated with container and it comprises given spare drive, UUID is set apporopriately to this container.
> It allows to assign spare with proper container according
> to content in devices parameter in configuration file.
>

I guess it would be nice if a user could direct spares via the
configuration file, but what gets honored in the future when this
value conflicts with a container group id in the metadata?  Especially
when the actual device associated with a given name /dev/sda changes
from one boot to the next, potentially even across controllers, or the
admin moves the drive and forgets to update the configuration file.
As much as possible I would like everything to be determined from the
platform / metadata and discourage using the 'devices' line in the
configuration file.

So, I'm on the fence with this one.

A couple technical comments below:

> Signed-off-by: Przemyslaw Czarnowski <przemyslaw.hawrylewicz.czarnowski@intel.com>
> ---
>  super-intel.c |   48 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 43 insertions(+), 5 deletions(-)
>
> diff --git a/super-intel.c b/super-intel.c
> index a196ca3..41a4cc6 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -1497,10 +1497,19 @@ static void getinfo_super_imsm_volume(struct supertype *st, struct mdinfo *info)
>        uuid_from_super_imsm(st, info->uuid);
>  }
>
> +static int matchfd2devnum(int fd, int maj, int min)
> +{
> +       struct stat st;
> +       if (fstat(fd, &st) != 0)
> +               return 1;
> +       return ((int)major(st.st_rdev) == maj && (int)minor(st.st_rdev) == min);
> +}
> +
> +
>  /* check the config file to see if we can return a real uuid for this spare */
> -static void fixup_container_spare_uuid(struct mdinfo *inf)
> +static void fixup_container_spare_uuid(struct mdinfo *inf, struct dl *disk)
>  {
> -       struct mddev_ident_s *array_list;
> +       struct mddev_ident_s *array_list, *first_match = NULL;
>
>        if (inf->array.level != LEVEL_CONTAINER ||
>            memcmp(inf->uuid, uuid_match_any, sizeof(int[4])) != 0)
> @@ -1520,12 +1529,41 @@ static void fixup_container_spare_uuid(struct mdinfo *inf)
>                                _sst = NULL;
>
>                        if (_sst) {
> -                               memcpy(inf->uuid, array_list->uuid, sizeof(int[4]));
> +                               char *devices = array_list->devices;
> +
> +                               if (!first_match)
> +                                       first_match = array_list;
> +
> +                               while (disk && devices && *devices) {
> +                                       char path[PATH_MAX];
> +                                       char *p = devices;
> +                                       int fd;
> +
> +                                       devices = strchr(devices, ',');
> +                                       if (!devices)
> +                                               devices = p + strlen(p);
> +                                       if (devices-p < PATH_MAX) {
> +                                               strncpy(path, p, devices - p);
> +                                               if ((fd = open(path, O_RDONLY)) >= 0) {
> +                                                       if (matchfd2devnum(fd, disk->major, disk->minor)) {
> +                                                               free(_sst);
> +                                                               close(fd);
> +                                                               goto match;
> +                                                       }
> +                                                       close(fd);
> +                                               }
> +                                               if (*devices == ',')
> +                                                       devices++;
> +                                       }
> +                               }

This routine is already available: match_oneof() in config.c

>                                free(_sst);
> -                               break;
>                        }
>                }
>        }
> +match:;

Don't need a semicolon after a label.

--
Dan
--
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] imsm: assign UUID of spare device based on ARRAY *device* keyword in mdadm configuration file.

[Hawrylewicz Czarnowski, Przemyslaw]

>-----Original Message-----
>From: dan.j.williams@gmail.com [mailto:dan.j.williams@gmail.com] On Behalf
>Of Dan Williams
>Sent: Thursday, April 15, 2010 12:15 AM
>To: Hawrylewicz Czarnowski, Przemyslaw
>Cc: Neil Brown; linux-raid@vger.kernel.org; Ciechanowski, Ed
>Subject: Re: [PATCH] imsm: assign UUID of spare device based on ARRAY
>*device* keyword in mdadm configuration file.
>
>On Thu, Apr 8, 2010 at 7:48 AM, Hawrylewicz Czarnowski, Przemyslaw
><przemyslaw.hawrylewicz.czarnowski@intel.com> wrote:
>> If config has list of devices associated with container and it comprises
>given spare drive, UUID is set apporopriately to this container.
>> It allows to assign spare with proper container according
>> to content in devices parameter in configuration file.
>>
>
>I guess it would be nice if a user could direct spares via the
>configuration file, but what gets honored in the future when this
>value conflicts with a container group id in the metadata?  Especially
You are right, but at this moment it is *always* first container. This proposal gives a way how user can point out the right one.

>when the actual device associated with a given name /dev/sda changes
>from one boot to the next, potentially even across controllers, or the
>admin moves the drive and forgets to update the configuration file.
but it will need to rewrite this part of config to respect eg. /dev/disk/by-id patterns which are constant, which is far beyond this patch.

>As much as possible I would like everything to be determined from the
>platform / metadata and discourage using the 'devices' line in the
>configuration file.
As I have written above, the solution available now generates more opportunities to make a mistake.

>
>So, I'm on the fence with this one.
>
>A couple technical comments below:
>
>> Signed-off-by: Przemyslaw Czarnowski
><przemyslaw.hawrylewicz.czarnowski@intel.com>
>> ---
>>  super-intel.c |   48 +++++++++++++++++++++++++++++++++++++++++++-----

[cut]

>> +
>>  /* check the config file to see if we can return a real uuid for this
>spare */
>> -static void fixup_container_spare_uuid(struct mdinfo *inf)
>> +static void fixup_container_spare_uuid(struct mdinfo *inf, struct dl
>*disk)
>>  {
>> -       struct mddev_ident_s *array_list;
>> +       struct mddev_ident_s *array_list, *first_match = NULL;
>>
>>        if (inf->array.level != LEVEL_CONTAINER ||
>>            memcmp(inf->uuid, uuid_match_any, sizeof(int[4])) != 0)
>> @@ -1520,12 +1529,41 @@ static void fixup_container_spare_uuid(struct
>mdinfo *inf)
>>                                _sst = NULL;
>>
>>                        if (_sst) {
>> -                               memcpy(inf->uuid, array_list->uuid,
>sizeof(int[4]));
>> +                               char *devices = array_list->devices;
>> +
>> +                               if (!first_match)
>> +                                       first_match = array_list;
>> +
>> +                               while (disk && devices && *devices) {
>> +                                       char path[PATH_MAX];
>> +                                       char *p = devices;
>> +                                       int fd;
>> +
>> +                                       devices = strchr(devices, ',');
>> +                                       if (!devices)
>> +                                               devices = p + strlen(p);
>> +                                       if (devices-p < PATH_MAX) {
>> +                                               strncpy(path, p, devices
>- p);
>> +                                               if ((fd = open(path,
>O_RDONLY)) >= 0) {
>> +                                                       if
>(matchfd2devnum(fd, disk->major, disk->minor)) {
>> +
>free(_sst);
>> +
>close(fd);
>> +                                                               goto
>match;
>> +                                                       }
>> +                                                       close(fd);
>> +                                               }
>> +                                               if (*devices == ',')
>> +                                                       devices++;
>> +                                       }
>> +                               }
>
>This routine is already available: match_oneof() in config.c
This one is more specific, but you right, it may be generalized and reused.

>
>>                                free(_sst);
>> -                               break;
>>                        }
>>                }
>>        }
>> +match:;
>
>Don't need a semicolon after a label.
It had left from the previous version of code when compiler complained that there is no semicolon.

>
>--
>Dan
--
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] imsm: assign UUID of spare device based on ARRAY *device* keyword in mdadm configuration file.

[Dan Williams]

>>On Thu, Apr 8, 2010 at 7:48 AM, Hawrylewicz Czarnowski, Przemyslaw
>><przemyslaw.hawrylewicz.czarnowski@intel.com> wrote:
>>> If config has list of devices associated with container and it comprises
>>given spare drive, UUID is set apporopriately to this container.
>>> It allows to assign spare with proper container according
>>> to content in devices parameter in configuration file.
>>>
>>
>>I guess it would be nice if a user could direct spares via the
>>configuration file, but what gets honored in the future when this
>>value conflicts with a container group id in the metadata?  Especially
> You are right, but at this moment it is *always* first container. This proposal gives a way how user can point out the right one.

We are already fixing this problem in a more comprehensive way with
the spare migration and DOMAIN code.  This 'devices' option will be
deprecated in short order, and its presence would just complicate the
policy decisions.  For example, what takes precedence the DOMAIN
keyword, 'devices' list, or at a future date a spare pool id in the
metadata?  What would you rather write a DOMAIN line to specify the
hardware port ranges or a 'devices' line to identify individual disk
devices and update the configuration file on an ongoing basis?

>>when the actual device associated with a given name /dev/sda changes
>>from one boot to the next, potentially even across controllers, or the
>>admin moves the drive and forgets to update the configuration file.
> but it will need to rewrite this part of config to respect eg. /dev/disk/by-id patterns which are constant, which is far beyond this patch.

...but now you have hard coded something that would be better to float
between containers on an automatic as needed basis.  The only degree
of freedom I see missing in the current proposals is to have the
ability to disable spare migration to a given container.  That may be
some nice icing to allow some containers/arrays to have higher rebuild
priority, but let's finish the cake first :-).

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