On Wed, 2026-10-07 at 12:42 -0400, Benjamin Marzinski wrote: > 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 <[email protected]> > > 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.
Right, thanks for explaining it. I'd forgotten about this failure scenario. With this in mind, it actually makes sense to retry the resume. You're right, there should be no inactive table, but retrying won't hurt. [*] I'd appreciate a short comment explaining the problem scenario that the retry attempts to avoid (i.e. a short version of Ben's explanation here). Regards Martin [*] I wonder what would happen if there was a _valid_ inactive table. In that case, instead of removing the map, we'd reload it with a different table, which seems wrong. But again, there should be none. -- Dr. Martin Wilck <[email protected]> SUSE Software Solutions Germany GmbH, Frankenstr. 146, 90461 Nürnberg, Germany Geschäftsführer: Stefan Gaiser, Jochen Jaser, Abhinav Puri (HRB 36809,AG Nürnberg)
