Re: Autorebuild, new dynamic udev rules for hot-plugs

[Neil Brown <neilb@suse.de>]

On Fri, 19 Nov 2010 15:12:19 +0000
"Hawrylewicz Czarnowski, Przemyslaw"
<przemyslaw.hawrylewicz.czarnowski@intel.com> wrote:

> Hi,
> 
> I would like to present another patch for our autorebuild tree (it should be applied on the top of Autorebuild series, as devel-3.2 is not stable yet). Dan Williams proposed to reduce overhead associated with passing each hot-plugged block device to mdadm, using just the devices described in policies via mdadm.conf file.
> See patch below for details, comments are as always very welcome.

I am in general in favour of this approach.
It has the benefit that it can very easily be not-used if there turn out to
be problems with it.

Four comments.

1/ I wouldn't write a file in /lib/udev/rules.d/
  I think it should be written to "/dev/.udev/rules.d/"
  which is referred to as the "temporary rules directory"
  in the udev documentation.

2/ I would be good to process the type=disk or type=part part of the
   policy into the rules file as well.

3/ I'm not very comfortable with hard-coding the name of the
   file to be created in the rules.d directory.  Maybe usage could be
      --activate-domains=63-md-whatever

4/ I don't think it is good to have an incomplete file in rules.d that udev
   might accidentally read.  We should create the file with a name with a
   leading '.' (assuming udev ignores those, I haven't checked) and then
   rename it after it has been completely written.

Other than that, it looks pretty good.

Thanks,
NeilBrown

> 
> Date: Thu, 18 Nov 2010 01:19:52 +0100
> Subject: [PATCH] New dynamic hot-plug udev rules for policies
> 
> When introducing policies, new hot-plug rules were added to support
> bare disks. Mdadm was started for each hot plugged block device
> to determine if it could be used as spare or as a replacement member for
> degraded array.
> This patch introduces limitation of range of devices that are handled
> by mdadm. It limits them to the ones specified in domains associated 
> with the actions: spare-same-port, spare and spare-force.
> In order to enable hot-plug for bare disks one must update udev rules
> with command
> 
> 	mdadm --activate-domains
> 
> After mdadm.conf is changed one is obliged to re-run
> "mdadm --activate-domains" command in order to bring the system
> configuration up to date.
> All hot-plugged disks containing metadata are still handled by existing
> rules.
> 
> Note: this patch is just a proposition to minimize overhead of using mdadm for
> each plugged block device. If accepted, it will be incorporated in
> previous implementation of hot-plug for bare disks.
> 
> Signed-off-by: Przemyslaw Czarnowski <przemyslaw.hawrylewicz.czarnowski@intel.com>
> ---
>  Makefile           |    2 +
>  ReadMe.c           |    1 +
>  mdadm.c            |    4 ++
>  mdadm.h            |    9 +++-
>  policy.c           |  137 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  udev-md-raid.rules |    5 +-
>  6 files changed, 154 insertions(+), 4 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 2b88818..eeae8e1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -73,9 +73,11 @@ MAP_FILE = map
>  MDMON_DIR = /dev/.mdadm
>  # place for autoreplace cookies
>  FAILED_SLOTS_DIR = /dev/.mdadm/failed-slots
> +UDEV_RULES_DIR = $(DESTDIR)/lib/udev/rules.d
>  DIRFLAGS = -DMAP_DIR=\"$(MAP_DIR)\" -DMAP_FILE=\"$(MAP_FILE)\"
>  DIRFLAGS += -DMDMON_DIR=\"$(MDMON_DIR)\"
>  DIRFLAGS += -DFAILED_SLOTS_DIR=\"$(FAILED_SLOTS_DIR)\"
> +DIRFLAGS += -DUDEV_RULES_DIR=\"$(UDEV_RULES_DIR)\"
>  CFLAGS = $(CWFLAGS) $(CXFLAGS) -DSendmail=\""$(MAILCMD)"\" $(CONFFILEFLAGS) $(DIRFLAGS)
>  
>  # The glibc TLS ABI requires applications that call clone(2) to set up
> diff --git a/ReadMe.c b/ReadMe.c
> index 54a1998..f17b8a1 100644
> --- a/ReadMe.c
> +++ b/ReadMe.c
> @@ -110,6 +110,7 @@ struct option long_options[] = {
>      {"detail-platform", 0, 0, DetailPlatform},
>      {"kill-subarray", 1, 0, KillSubarray},
>      {"update-subarray", 1, 0, UpdateSubarray},
> +    {"activate-domains", 0, 0, ActivateDomains},
>  
>      /* synonyms */
>      {"monitor",   0, 0, 'F'},
> diff --git a/mdadm.c b/mdadm.c
> index c9a172a..b5403cf 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -228,6 +228,7 @@ int main(int argc, char *argv[])
>  				}
>  				subarray = optarg;
>  			}
> +		case ActivateDomains:
>  		case 'K': if (!mode) newmode = MISC; break;
>  		case NoSharing: newmode = MONITOR; break;
>  		}
> @@ -841,6 +842,7 @@ int main(int argc, char *argv[])
>  		case O(MISC, DetailPlatform):
>  		case O(MISC, KillSubarray):
>  		case O(MISC, UpdateSubarray):
> +		case O(MISC, ActivateDomains):
>  			if (devmode && devmode != opt &&
>  			    (devmode == 'E' || (opt == 'E' && devmode != 'Q'))) {
>  				fprintf(stderr, Name ": --examine/-E cannot be given with ");
> @@ -1421,6 +1423,8 @@ int main(int argc, char *argv[])
>  						free_mdstat(ms);
>  					} while (!last && err);
>  					if (err) rv |= 1;
> +				} else if (devmode == ActivateDomains) {
> +					rv = Activate_Domains();
>  				} else {
>  					fprintf(stderr, Name ": No devices given.\n");
>  					exit(2);
> diff --git a/mdadm.h b/mdadm.h
> index 9b4a1a8..171aa69 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -101,6 +101,11 @@ extern __off64_t lseek64 __P ((int __fd, __off64_t __offset, int __whence));
>  #define FAILED_SLOTS_DIR "/dev/.mdadm/failed-slots"
>  #endif /* FAILED_SLOTS */
>  
> +/* UDEV_RULES_DIR is the place, where udev holds its rules */
> +#ifndef UDEV_RULES_DIR
> +#define UDEV_RULES_DIR "/lib/udev/rules.d"
> +#endif /* UDEV_RULES_DIR */
> +
>  #include	"md_u.h"
>  #include	"md_p.h"
>  #include	"bitmap.h"
> @@ -289,7 +294,8 @@ enum special_options {
>  	KillSubarray,
>  	UpdateSubarray, /* 16 */
>  	IncrementalPath,
> -	NoSharing
> +	NoSharing,
> +	ActivateDomains
>  };
>  
>  /* structures read from config file */
> @@ -971,6 +977,7 @@ extern int CreateBitmap(char *filename, int force, char uuid[16],
>  			unsigned long long array_size,
>  			int major);
>  extern int ExamineBitmap(char *filename, int brief, struct supertype *st);
> +extern int Activate_Domains(void);
>  extern int bitmap_update_uuid(int fd, int *uuid, int swap);
>  extern unsigned long bitmap_sectors(struct bitmap_super_s *bsb);
>  
> diff --git a/policy.c b/policy.c
> index daee52a..e4c53d5 100644
> --- a/policy.c
> +++ b/policy.c
> @@ -753,3 +753,140 @@ void domain_free(struct domainlist *dl)
>  		free(head);
>  	}
>  }
> +
> +/* invocation of udev rule file */
> +char udev_template_start[] =
> +"# do not edit this file, it will be overwritten on update\n"
> +"\n"
> +"SUBSYSTEM!=\"block\", GOTO=\"md_autorebuild_end\"\n"
> +"\n"
> +"ENV{ID_FS_TYPE}==\"linux_raid_member\", GOTO=\"md_autorebuild_end\"\n"
> +"ENV{ID_FS_TYPE}==\"isw_raid_member\", GOTO=\"md_autorebuild_end\"\n"
> +"\n";
> +
> +/* ending of udev rule file */
> +char udev_template_end[] =
> +"\n"
> +"LABEL=\"md_autorebuild_end\"\n"
> +"\n";
> +
> +/* find rule named rule_type and return its value */
> +char *find_rule(struct rule *rule, char *rule_type)
> +{
> +	while (rule) {
> +		if (rule->name == rule_type)
> +			return rule->value;
> +
> +		rule = rule->next;
> +	}
> +	return NULL;
> +}
> +
> +#define UDEV_RULE_FORMAT \
> +"ACTION==\"add\", KERNEL!=\"md*\" ENV{ID_PATH}==\"%s\" " \
> +"RUN+=\"/sbin/mdadm --incremental $env{DEVNAME}\", " \
> +"GOTO=\"md_autorebuild_end\"\n" \
> +
> +/* Write rule in the rule file. Use format from UDEV_RULE_FORMAT */
> +int write_rule(struct rule *rule, int fd)
> +{
> +	char line[1024];
> +	char *r = find_rule(rule, rule_path);
> +	if (!r)
> +		return -1;
> +
> +	snprintf(line, sizeof(line) - 1, UDEV_RULE_FORMAT, r);
> +	return write(fd, line, strlen(line));
> +}
> +
> +/* Generate single entry in udev rule basing on POLICY line found in config
> + * file. Take only those with paths, only first occurrence if paths are equal
> + * and if actions supports handling of spares (>=act_spare_same_slot)
> + */
> +int generate_entries(int fd)
> +{
> +	struct pol_rule *loop, *dup;
> +	char *loop_value, *dup_value;
> +	int duplicate;
> +	int written = 0;
> +
> +	for (loop = config_rules; loop; loop = loop->next) {
> +		if (loop->type != rule_policy)
> +			continue;
> +		duplicate = 0;
> +
> +		/* only policies with paths and with actions supporting
> +		 * bare disks are considered */
> +		loop_value = find_rule(loop->rule, pol_act);
> +		if (!loop_value || map_act(loop_value) < act_spare_same_slot)
> +			continue;
> +		loop_value = find_rule(loop->rule, rule_path);
> +		if (!loop_value)
> +			continue;
> +		for (dup = config_rules; dup != loop; dup = dup->next) {
> +			if (dup->type != rule_policy)
> +				continue;
> +			dup_value = find_rule(dup->rule, pol_act);
> +			if (!dup_value || map_act(dup_value) < act_spare_same_slot)
> +				continue;
> +			dup_value = find_rule(dup->rule, rule_path);
> +			if (!dup_value)
> +				continue;
> +			if (strcmp(loop_value, dup_value) == 0) {
> +				duplicate = 1;
> +				break;
> +			}
> +		}
> +
> +		/* not a dup or first occurrence */
> +		if (!duplicate) {
> +			if (write_rule(loop->rule, fd) == -1)
> +				return 0;
> +			written++;
> +		}
> +	}
> +	return written;
> +}
> +
> +#define AR_UDEV_RULE_FILE UDEV_RULES_DIR "/63-md-raid-autorebuild.rules"
> +
> +/* Activate_Domains routine creates dynamic udev rules used to handle
> + * hot-plug events for bare devices (and making them spares)
> + */
> +int Activate_Domains(void)
> +{
> +	int fd = 0;
> +	int rv;
> +
> +	fd = creat(AR_UDEV_RULE_FILE,
> +		S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
> +	if (fd == -1)
> +		return 1;
> +
> +	/* write static invocation */
> +	if (write(fd, udev_template_start,
> +		sizeof(udev_template_start) - 1) == -1) {
> +		close(fd);
> +		unlink(AR_UDEV_RULE_FILE);
> +		return 1;
> +	}
> +
> +	/* iterate, if none created or error occurred, remove file */
> +	rv = generate_entries(fd);
> +	if (rv <= 0) {
> +		close(fd);
> +		unlink(AR_UDEV_RULE_FILE);
> +		return rv == -1 ? -1 : 0;
> +	}
> +
> +	/* write ending */
> +	if (write(fd, udev_template_end, sizeof(udev_template_end) - 1) == -1) {
> +		close(fd);
> +		unlink(AR_UDEV_RULE_FILE);
> +		return 1;
> +	}
> +
> +	close(fd);
> +
> +	return 0;
> +}
> diff --git a/udev-md-raid.rules b/udev-md-raid.rules
> index 36dd51e..11057bb 100644
> --- a/udev-md-raid.rules
> +++ b/udev-md-raid.rules
> @@ -3,11 +3,10 @@
>  SUBSYSTEM!="block", GOTO="md_end"
>  
>  # handle potential components of arrays
> +ENV{ID_FS_TYPE}=="linux_raid_member", ACTION=="add", RUN+="/sbin/mdadm --incremental $env{DEVNAME}"
>  ENV{ID_FS_TYPE}=="linux_raid_member", ACTION=="remove", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
> +ENV{ID_FS_TYPE}=="isw_raid_member", ACTION=="add", RUN+="/sbin/mdadm --incremental $env{DEVNAME}"
>  ENV{ID_FS_TYPE}=="isw_raid_member", ACTION=="remove", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
> -# try incremental for each block device and not md. Most cases should not create
> -# unnecessary overhead, as no "heavy" disk operations are performed
> -ACTION=="add", KERNEL!="md*", RUN+="/sbin/mdadm --incremental $env{DEVNAME}"
>  
>  # handle md arrays
>  ACTION!="add|change", GOTO="md_end"