Re: [PATCH v2 2/2] libmultipath: dm_flush_map__: log if the resume fails after a failed remove

[Benjamin Marzinski <bmarzins@redhat.com>]

On Wed, Oct 07, 2026 at 09:22:36AM +0000, Suresh Thelkar wrote:
> After a failed DM_DEVICE_REMOVE the map was suspended earlier in
> dm_flush_map__(), and the recovery path issued a single fire-and-forget
> DM_DEVICE_RESUME whose result was ignored. If that resume ioctl failed,
> the map could be left suspended with no trace in the logs, which wedges
> any later task in uninterruptible D state and, because sync() walks
> every block device, hangs I/O node-wide until reboot.
> 
> Check the DM_DEVICE_RESUME return value and log at condlog(1) when it
> fails, so a leaked suspend is at least visible in the logs. The map is
> known to have been suspended at the start of the loop, so the ioctl
> return value is authoritative.
> 
> Signed-off-by: Suresh Thelkar <suresh.thelkar@yahoo.com>

I want to discuss this change from v1 a bit. The issue that the second
resume is solving is the same one that lead us to add a second resume to
dm_addmap_reload(). If the device has an invalid inactive table, and you
do a resume, dm will attempt to swap the table, fail, and leave the
device suspended with the invalid table removed. Immediately issuing a
second resume will cause dm to actually resume with the old table. If we
don't manage to resume the device, it seems much less likely that
waiting a second and re-attempting the remove will succeed (I believe
the most common case is us racing with something that temporarily opens
the device, like udev. It may need the device to be unsuspended to
do a quick read and close it again). 

Now, I admit that this is WAY more likely to happen when we are actually
reloading the table, since we just added an inactive table, and dm might
not be able to swap it correctly for any number of reasons. To hit this
issue when removing the device there would need to be an inactive table
(which is unlikely) and dm would have to fail when trying to swap tables
(which is also unlikely). Also, we resume the table in other places in
the code, without caring about this corner case. For consistency's sake,
we should probably either always do it, or only do it when we've loaded
a new table.

Thoughts? Also, I'm fine with this change going in as-is and dealing
with the resume question as a separate issue.

-Ben 

> ---
>  libmultipath/devmapper.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index a5570a85..85c552cf 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -1062,10 +1062,12 @@ int dm_flush_map__ (const char *mapname, int flags, int retries)
>  			else
>  				condlog(2, "failed to remove multipath map %s",
>  					mapname);
> -			if ((flags & DMFL_SUSPEND) && queue_if_no_path != -1) {
> -				dm_simplecmd_noflush(DM_DEVICE_RESUME,
> -						     mapname, udev_flags);
> -			}
> +			/* Resume the map suspended above; log if the resume
> +			 * ioctl fails, so a leaked suspend is visible. */
> +			if ((flags & DMFL_SUSPEND) && queue_if_no_path != -1 &&
> +			    !dm_simplecmd_noflush(DM_DEVICE_RESUME, mapname, udev_flags))
> +				condlog(1, "%s: failed to resume map after failed remove",
> +					mapname);
>  		}
>  		if (retries)
>  			sleep(1);
> -- 
> 2.43.0