Re: [PATCH V2] ublk: fix deadlock when reading partition table

[Ming Lei <ming.lei@redhat.com>]

On Sat, Dec 13, 2025 at 11:41:52PM -0700, Jens Axboe wrote:
> On 12/12/25 7:28 PM, Ming Lei wrote:
> > On Fri, Dec 12, 2025 at 12:49:49PM -0700, Jens Axboe wrote:
> >> On 12/12/25 7:34 AM, Ming Lei wrote:
> >>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> >>> index df9831783a13..38f138f248e6 100644
> >>> --- a/drivers/block/ublk_drv.c
> >>> +++ b/drivers/block/ublk_drv.c
> >>> @@ -1080,12 +1080,20 @@ static inline struct ublk_uring_cmd_pdu *ublk_get_uring_cmd_pdu(
> >>>  	return io_uring_cmd_to_pdu(ioucmd, struct ublk_uring_cmd_pdu);
> >>>  }
> >>>  
> >>> +static void ublk_end_request(struct request *req, blk_status_t error)
> >>> +{
> >>> +	local_bh_disable();
> >>> +	blk_mq_end_request(req, error);
> >>> +	local_bh_enable();
> >>> +}
> >>
> >> This is really almost too ugly to live, as a work-around for what just
> >> happens to be in __fput_deferred()... Surely we can come up with
> >> something better here? Heck even a PF_ flag would be better than this,
> >> imho.
> > 
> > task flag will switch to release all files opened by current from wq context,
> > and there may be chance to cause regression, especially this fix needs to
> > backport to stable.
> 
> I don't mean in general for the task, just across the completion. It'd
> cause the exact same punts to async puts as the current patch.

Technically it is very similar with the posted path, just task flag way touches
core code, especially there are only 4bits left.

> 
> > So I'd suggest to take it for safe stable purpose.
> 
> I'm really having a hard time with it - and I have to defend it once I
> send it further upstream. And I can tell you who's going to hate it, the
> guy that pulls from me. We might get lucky that he doesn't look at it.
> But the underlying issue here is that the patch is one nasty bandaid,
> not that Linus would yell at it, with good reason imho.

Understood.

IMHO, even this patch is a workaround and looks ugly, but it has enough
benefits too:

- driver only change and won't touch core code
- it is for handling abnormal userspace behavior(close(disk_fd) before
  completing block IO)
- correct & safe because it is always fine to complete IO request from irq
  context
- easy to backport

> 
> At the same time, I don't really have other good suggestions. Let me
> ponder it a bit, about to get on a flight anyway and -rc1 has been cut
> at this point regardless.
> 
> Obviously this isn't a ublk induced issue, it's all down to the lock
> grabbing that happens there. Maybe bdev_release() could do a deferred
> put if a trylock of ->open_mutex fails?

There is risk to break application since some cases need to drain
bdev_release before returning to userspace.

The issue for ublk is actually triggered by something abnormal: submit AIO
& close(ublk disk) in client application, then fput() is called when the
submitted AIO is done, it will cause deferred fput handler to wq for any block
IO completed from irq handler.


Thanks,
Ming