[dm-devel] Re: [Bug 217014] Setting "user_friendly_names" to "yes" causes dm-mp to occasionally miss a mpath device during configuration

[Christophe Varoqui]

Nice fix, thank you.
Merged.

Le lundi 27 novembre 2006 à 11:30 -0500, bugzilla at redhat.com a écrit :
> Please do not reply directly to this email. All additional
> comments should be made in the comments box of this bug report.
> 
> Summary: Setting "user_friendly_names" to "yes" causes dm-mp to occasionally miss a mpath device during configuration
> 
> 
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=217014
> 
> 
> 
> 
> 
> ------- Additional Comments From egoggin at emc.com  2006-11-27 11:29 EST -------
> The posix file byte range locks used to provide atomicity for accessing the 
> entries in the multipath bindings file get released from whenever __any__ 
> descriptor or FILE structure for that file is closed.  This patch delays the 
> fclose() for the FILE structures used within lookup_binding() and 
> rlookup_binding() until there is no more need for the atomicity.
> 
> Without this patch I could fairly easily get two multipath mpath0 entries 
> in /var/lib/multipath/bindings by running 8 concurrent instances of multipath
> (8) while with the patch I cannot get this problem to occur.
> 
> diff --git a/libmultipath/alias.c b/libmultipath/alias.c
> index 6d103d7..86cae9b 100644
> --- a/libmultipath/alias.c
> +++ b/libmultipath/alias.c
> @@ -166,28 +166,14 @@ fail:
>  
> 
>  static int
> -lookup_binding(int fd, char *map_wwid, char **map_alias)
> +lookup_binding(FILE *f, char *map_wwid, char **map_alias)
>  {
>  	char buf[LINE_MAX];
> -	FILE *f;
>  	unsigned int line_nr = 0;
> -	int scan_fd;
>  	int id = 0;
>  
>  	*map_alias = NULL;
> -	scan_fd = dup(fd);
> -	if (scan_fd < 0) {
> -		condlog(0, "Cannot dup bindings file descriptor : %s",
> -			strerror(errno));
> -		return -1;
> -	}
> -	f = fdopen(scan_fd, "r");
> -	if (!f) {
> -		condlog(0, "cannot fdopen on bindings file descriptor : %s",
> -			strerror(errno));
> -		close(scan_fd);
> -		return -1;
> -	}
> +
>  	while (fgets(buf, LINE_MAX, f)) {
>  		char *c, *alias, *wwid;
>  		int curr_id;
> @@ -215,38 +201,22 @@ lookup_binding(int fd, char *map_wwid, c
>  			if (*map_alias == NULL)
>  				condlog(0, "Cannot copy alias from bindings "
>  					"file : %s", strerror(errno));
> -			fclose(f);
>  			return id;
>  		}
>  	}
>  	condlog(3, "No matching wwid [%s] in bindings file.", map_wwid);
> -	fclose(f);
>  	return id;
>  }	
>  
>  static int
> -rlookup_binding(int fd, char **map_wwid, char *map_alias)
> +rlookup_binding(FILE *f, char **map_wwid, char *map_alias)
>  {
>  	char buf[LINE_MAX];
> -	FILE *f;
>  	unsigned int line_nr = 0;
> -	int scan_fd;
>  	int id = 0;
>  
>  	*map_wwid = NULL;
> -	scan_fd = dup(fd);
> -	if (scan_fd < 0) {
> -		condlog(0, "Cannot dup bindings file descriptor : %s",
> -			strerror(errno));
> -		return -1;
> -	}
> -	f = fdopen(scan_fd, "r");
> -	if (!f) {
> -		condlog(0, "cannot fdopen on bindings file descriptor : %s",
> -			strerror(errno));
> -		close(scan_fd);
> -		return -1;
> -	}
> +
>  	while (fgets(buf, LINE_MAX, f)) {
>  		char *c, *alias, *wwid;
>  		int curr_id;
> @@ -274,12 +244,10 @@ rlookup_binding(int fd, char **map_wwid,
>  			if (*map_wwid == NULL)
>  				condlog(0, "Cannot copy alias from bindings "
>  					"file : %s", strerror(errno));
> -			fclose(f);
>  			return id;
>  		}
>  	}
>  	condlog(3, "No matching alias [%s] in bindings file.", map_alias);
> -	fclose(f);
>  	return id;
>  }	
>  
> @@ -327,7 +295,8 @@ char *
>  get_user_friendly_alias(char *wwid, char *file)
>  {
>  	char *alias;
> -	int fd, id;
> +	int fd, scan_fd, id;
> +	FILE *f;
>  
>  	if (!wwid || *wwid == '\0') {
>  		condlog(3, "Cannot find binding for empty WWID");
> @@ -337,14 +306,37 @@ get_user_friendly_alias(char *wwid, char
>  	fd = open_bindings_file(file);
>  	if (fd < 0)
>  		return NULL;
> -	id = lookup_binding(fd, wwid, &alias);
> +
> +	scan_fd = dup(fd);
> +	if (scan_fd < 0) {
> +		condlog(0, "Cannot dup bindings file descriptor : %s",
> +			strerror(errno));
> +		close(fd);
> +		return NULL;
> +	}
> +
> +	f = fdopen(scan_fd, "r");
> +	if (!f) {
> +		condlog(0, "cannot fdopen on bindings file descriptor : %s",
> +			strerror(errno));
> +		close(scan_fd);
> +		close(fd);
> +		return NULL;
> +	}
> +
> +	id = lookup_binding(f, wwid, &alias);
>  	if (id < 0) {
> +		fclose(f);
> +		close(scan_fd);
>  		close(fd);
>  		return NULL;
>  	}
> +
>  	if (!alias)
>  		alias = allocate_binding(fd, wwid, id);
>  
> +	fclose(f);
> +	close(scan_fd);
>  	close(fd);
>  	return alias;
>  }
> @@ -353,7 +345,8 @@ char *
>  get_user_friendly_wwid(char *alias, char *file)
>  {
>  	char *wwid;
> -	int fd, id;
> +	int fd, scan_fd, id;
> +	FILE *f;
>  
>  	if (!alias || *alias == '\0') {
>  		condlog(3, "Cannot find binding for empty alias");
> @@ -363,12 +356,34 @@ get_user_friendly_wwid(char *alias, char
>  	fd = open_bindings_file(file);
>  	if (fd < 0)
>  		return NULL;
> -	id = rlookup_binding(fd, &wwid, alias);
> +
> +	scan_fd = dup(fd);
> +	if (scan_fd < 0) {
> +		condlog(0, "Cannot dup bindings file descriptor : %s",
> +			strerror(errno));
> +		close(fd);
> +		return NULL;
> +	}
> +
> +	f = fdopen(scan_fd, "r");
> +	if (!f) {
> +		condlog(0, "cannot fdopen on bindings file descriptor : %s",
> +			strerror(errno));
> +		close(scan_fd);
> +		close(fd);
> +		return NULL;
> +	}
> +
> +	id = rlookup_binding(f, &wwid, alias);
>  	if (id < 0) {
> +		fclose(f);
> +		close(scan_fd);
>  		close(fd);
>  		return NULL;
>  	}
>  
> +	fclose(f);
> +	close(scan_fd);
>  	close(fd);
>  	return wwid;
>  }
>