Re: [PATCH 15/33] libmultipath: move suspend logic to _dm_flush_map

["Benjamin Marzinski" <bmarzins@redhat.com>]

On Tue, Feb 28, 2017 at 05:23:11PM +0100, Martin Wilck wrote:
> From: Martin Wilck <mwilck@suse.de>
> 
> The function dm_suspend_and_flush() introduced in 9a4ff93
> tries to remove child maps (partitions) after suspending
> the mpath device. This may lock up if removing the partitions
> requires I/O. It's better to use the following sequence
> of actions: 1) clear queue_if_no_path; 2) remove partitions;
> 3) suspend; 4) remove (or resume and restore queue_if_no_path
> in case of failure).
> 
> This patch modifies the implementation by moving the
> queue_if_no_path/suspend logic into _dm_flush_map().
> A call to _dm_flush_map() with need_suspend=1 replaces
> the previous call to dm_suspend_and_flush().
> 
> With this change, the mpath device is only suspended after
> removing partmaps, avoiding the deadlock.

This patch drops support for disabling partitions, by removing
-       /* if the device currently has no partitions, do not
-          run kpartx on it if you fail to delete it */
-       if (do_foreach_partmaps(mapname, has_partmap, NULL) == 0)
-               udev_flags |= MPATH_UDEV_NO_KPARTX_FLAG;

 
> Fixes: 9a4ff93 "Switch off 'queue_if_no_path' before removing maps"
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/devmapper.c | 102 ++++++++++++++++++++---------------------------
>  libmultipath/devmapper.h |   9 +++--
>  2 files changed, 49 insertions(+), 62 deletions(-)
> 
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 1576dd01..044be2be 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -760,12 +760,6 @@ out:
>  }
>  
>  static int
> -has_partmap(const char *name, void *data)
> -{
> -	return 1;
> -}
> -
> -static int
>  partmap_in_use(const char *name, void *data)
>  {
>  	int part_count, *ret_count = (int *)data;
> @@ -785,9 +779,13 @@ partmap_in_use(const char *name, void *data)
>  	return 0;
>  }
>  
> -int _dm_flush_map(const char *mapname, int need_sync, int deferred_remove)
> +int _dm_flush_map (const char * mapname, int need_sync, int deferred_remove,
> +		   int need_suspend, int retries)
>  {
>  	int r;
> +	int queue_if_no_path = 0;
> +	unsigned long long mapsize;
> +	char params[PARAMS_SIZE] = {0};
>  
>  	if (!dm_is_mpath(mapname))
>  		return 0; /* nothing to do */
> @@ -797,6 +795,16 @@ int _dm_flush_map(const char *mapname, int need_sync, int deferred_remove)
>  	if (!do_deferred(deferred_remove) && partmap_in_use(mapname, NULL))
>  			return 1;
>  
> +	if (need_suspend &&
> +	    !dm_get_map(mapname, &mapsize, params) &&
> +	    strstr(params, "queue_if_no_path")) {
> +		if (!dm_queue_if_no_path((char *)mapname, 0))
> +			queue_if_no_path = 1;
> +		else
> +			/* Leave queue_if_no_path alone if unset failed */
> +			queue_if_no_path = -1;
> +	}
> +
>  	if (dm_remove_partmaps(mapname, need_sync, deferred_remove))
>  		return 1;
>  
> @@ -805,17 +813,36 @@ int _dm_flush_map(const char *mapname, int need_sync, int deferred_remove)
>  		return 1;
>  	}
>  
> -	r = dm_device_remove(mapname, need_sync, deferred_remove);
> +	do {
> +		if (need_suspend && queue_if_no_path != -1)
> +			dm_simplecmd_flush(DM_DEVICE_SUSPEND, mapname, 0);
>  
> -	if (r) {
> -		if (do_deferred(deferred_remove) && dm_map_present(mapname)) {
> -			condlog(4, "multipath map %s remove deferred",
> +		r = dm_device_remove(mapname, need_sync, deferred_remove);
> +
> +		if (r) {
> +			if (do_deferred(deferred_remove)
> +			    && dm_map_present(mapname)) {
> +				condlog(4, "multipath map %s remove deferred",
> +					mapname);
> +				return 2;
> +			}
> +			condlog(4, "multipath map %s removed", mapname);
> +			return 0;
> +		} else {
> +			condlog(2, "failed to remove multipath map %s",
>  				mapname);
> -			return 2;

This resume command needs to disable adding back partitions if there
aren't any, otherwise it could create partitions on a device that wasn't
supposed to have any.

> +			if (need_suspend && queue_if_no_path != -1) {
> +				dm_simplecmd_noflush(DM_DEVICE_RESUME,
> +						     mapname, 0);
> +			}
>  		}
> -		condlog(4, "multipath map %s removed", mapname);
> -		return 0;
> -	}
> +		if (retries)
> +			sleep(1);
> +	} while (retries-- > 0);
> +
> +	if (queue_if_no_path == 1)
> +		dm_queue_if_no_path((char *)mapname, 1);
> +
>  	return 1;
>  }
>  
> @@ -824,7 +851,7 @@ int _dm_flush_map(const char *mapname, int need_sync, int deferred_remove)
>  int
>  dm_flush_map_nopaths(const char * mapname, int deferred_remove)
>  {
> -	return _dm_flush_map(mapname, 1, deferred_remove);
> +	return _dm_flush_map(mapname, 1, deferred_remove, 0, 0);
>  }
>  
>  #else
> @@ -832,52 +859,11 @@ dm_flush_map_nopaths(const char * mapname, int deferred_remove)
>  int
>  dm_flush_map_nopaths(const char * mapname, int deferred_remove)
>  {
> -	return _dm_flush_map(mapname, 1, 0);
> +	return _dm_flush_map(mapname, 1, 0, 0, 0);
>  }
>  
>  #endif
>  
> -int dm_suspend_and_flush_map (const char * mapname, int retries)
> -{
> -	int need_reset = 0, queue_if_no_path = 0;
> -	unsigned long long mapsize;
> -	char params[PARAMS_SIZE] = {0};
> -	int udev_flags = 0;
> -
> -	if (!dm_is_mpath(mapname))
> -		return 0; /* nothing to do */
> -
> -	/* if the device currently has no partitions, do not
> -	   run kpartx on it if you fail to delete it */
> -	if (do_foreach_partmaps(mapname, has_partmap, NULL) == 0)
> -		udev_flags |= MPATH_UDEV_NO_KPARTX_FLAG;
> -
> -	if (!dm_get_map(mapname, &mapsize, params)) {
> -		if (strstr(params, "queue_if_no_path"))
> -			queue_if_no_path = 1;
> -	}
> -
> -	if (queue_if_no_path && dm_queue_if_no_path((char *)mapname, 0) == 0)
> -		need_reset = 1;
> -
> -	do {
> -		if (!queue_if_no_path || need_reset)
> -			dm_simplecmd_flush(DM_DEVICE_SUSPEND, mapname, 0);
> -
> -		if (!dm_flush_map(mapname)) {
> -			condlog(4, "multipath map %s removed", mapname);
> -			return 0;
> -		}
> -		dm_simplecmd_noflush(DM_DEVICE_RESUME, mapname, udev_flags);
> -		if (retries)
> -			sleep(1);
> -	} while (retries-- > 0);
> -	condlog(2, "failed to remove multipath map %s", mapname);
> -	if (need_reset)
> -		dm_queue_if_no_path((char *)mapname, 1);
> -	return 1;
> -}
> -
>  int dm_flush_maps (int retries)
>  {
>  	int r = 0;
> diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> index 3ea43297..aca4454b 100644
> --- a/libmultipath/devmapper.h
> +++ b/libmultipath/devmapper.h
> @@ -36,12 +36,13 @@ int dm_get_map(const char *, unsigned long long *, char *);
>  int dm_get_status(char *, char *);
>  int dm_type(const char *, char *);
>  int dm_is_mpath(const char *);
> -int _dm_flush_map (const char *, int, int);
> +int _dm_flush_map (const char *, int, int, int, int);
>  int dm_flush_map_nopaths(const char * mapname, int deferred_remove);
> -#define dm_flush_map(mapname) _dm_flush_map(mapname, 1, 0)
> -#define dm_flush_map_nosync(mapname) _dm_flush_map(mapname, 0, 0)
> +#define dm_flush_map(mapname) _dm_flush_map(mapname, 1, 0, 0, 0)
> +#define dm_flush_map_nosync(mapname) _dm_flush_map(mapname, 0, 0, 0, 0)
> +#define dm_suspend_and_flush_map(mapname, retries) \
> +	_dm_flush_map(mapname, 1, 0, 1, retries)
>  int dm_cancel_deferred_remove(struct multipath *mpp);
> -int dm_suspend_and_flush_map(const char * mapname, int retries);
>  int dm_flush_maps (int retries);
>  int dm_fail_path(char * mapname, char * path);
>  int dm_reinstate_path(char * mapname, char * path);
> -- 
> 2.11.0
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel