[RFC] ublk: removing the disk of a dead server hangs on a stuck filesystem

[RFC] ublk: removing the disk of a dead server hangs on a stuck filesystem

[Mark Korenberg]

Hi Ming,

When a ublk server dies and the device is not recoverable,
ublk_ch_release_work_fn() stops it with del_gendisk() under ub->mutex.
The disk isn't marked dead, so del_gendisk() reports a normal removal
and fs_bdev_mark_dead() syncs the filesystem on it. All I/O to the disk
fails by then, but if the filesystem is stuck for another reason, the
sync never returns, and STOP_DEV and DEL_DEV of the device sleep on
ub->mutex uninterruptibly until reboot.

I hit it with btrfs on a ublk zoned device whose writeback deadlocked
on its own (a btrfs bug, handled separately); killing the server was
the last resort. It reproduces without btrfs: XFS on a ublk loop
device, its external log on dm-linear over null_blk;
"dmsetup suspend --nolockfs --noflush" the log, write a file and syncfs,
kill -9 the server. The release work then sits in

  xlog_wait_on_iclog <- xfs_fs_sync_fs <- sync_filesystem
  <- fs_bdev_mark_dead <- bdev_mark_dead <- blk_report_disk_dead
  <- __del_gendisk <- del_gendisk <- ublk_stop_dev_unlocked
  <- ublk_ch_release_work_fn

and DEL_DEV in ublk_stop_dev(). Same with ext4 and an external journal.
The series "ublk: don't let one stuck device block control of all ublk
devices" keeps this from blocking ADD_DEV/DEL_DEV of the other devices,
but the device itself stays stuck.

Since the server is gone, the removal should be a surprise one. What I
found trying to fix it:

1. blk_mark_disk_dead() before del_gendisk() under ub->mutex isn't
   enough: fs_bdev_mark_dead() then calls ->shutdown(), which blocks the
   same way (xlog_force_shutdown() waits for the log I/O,
   jbd2_journal_abort() writes the journal superblock), and
   del_gendisk() itself can wait in bdi_unregister() -> wb_shutdown()
   for a flusher stuck in the filesystem. So it has to run without
   ub->mutex.

2. Dropping ub->mutex around the removal opens windows that the
   current code doesn't expect:
   - a STOP_DEV/DEL_DEV waiting on ub->mutex runs a normal, syncing
     del_gendisk() before GD_DEAD is set;
   - a STOP_DEV/DEL_DEV issued right after the server exited, before
     the release work runs, does the syncing removal itself;
   - with UBLK_F_USER_RECOVERY, the release work can set QUIESCED or
     FAIL_IO on a disk that is being removed, and recovery can bring
     it back;
   - once the release work has cleared UB_STATE_OPEN, a new server can
     open /dev/ublkcN and FETCH, which resets the queues' canceling,
     fail_io and force_abort while the old disk still drains;
   - UPDATE_SIZE and REG_BUF/UNREG_BUF can touch the disk meanwhile.

A version that sets UBLK_S_DEV_DEAD under ub->mutex, then does
blk_mark_disk_dead() + del_gendisk() without it and detaches the disk
after, fixes the hang in my tests, but has the recovery and new-server
problems above, and whoever gets there first (the release work, or a
STOP_DEV/DEL_DEV from userspace) can still block unkillably in the
filesystem.

Which direction would you prefer?

(a) An explicit "removing" state, set under ub->mutex, that the release
    work, FETCH, recovery, UPDATE_SIZE and the buffer registration
    check; the removal itself runs from a work item without ub->mutex,
    and STOP_DEV/DEL_DEV wait for it killably.

(b) Leave the removal of a server-less device entirely to the release
    work, run it there without ub->mutex, and make STOP_DEV/DEL_DEV of
    such a device only wait for it, killably.

(c) Something else, e.g. never syncing on the release path and
    accepting that the release work may block, as long as it doesn't
    hold ub->mutex.

I can send the draft of the version above if it helps.

This analysis was done with the help of an AI coding assistant
(Claude); I reviewed and tested it.

Thanks,
Mark