Re: [PATCH 2/4] ublk: refactor recovery configuration flag helpers

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

On Wed, Jun 26, 2024 at 11:22:43AM -0600, Uday Shankar wrote:
> On Tue, Jun 18, 2024 at 10:11:51AM +0800, Ming Lei wrote:
> > On Mon, Jun 17, 2024 at 01:44:49PM -0600, Uday Shankar wrote:
> > > ublk currently supports the following behaviors on ublk server exit:
> > > 
> > > A: outstanding I/Os get errors, subsequently issued I/Os get errors
> > > B: outstanding I/Os get errors, subsequently issued I/Os queue
> > > C: outstanding I/Os get reissued, subsequently issued I/Os queue
> > > 
> > > and the following behaviors for recovery of preexisting block devices by
> > > a future incarnation of the ublk server:
> > > 
> > > 1: ublk devices stopped on ublk server exit (no recovery possible)
> > > 2: ublk devices are recoverable using start/end_recovery commands
> > > 
> > > The userspace interface allows selection of combinations of these
> > > behaviors using flags specified at device creation time, namely:
> > > 
> > > default behavior: A + 1
> > > UBLK_F_USER_RECOVERY: B + 2
> > > UBLK_F_USER_RECOVERY|UBLK_F_USER_RECOVERY_REISSUE: C + 2
> > 
> > ublk is supposed to support A, B & C for both 1 and both 2, but it may
> > depend on how ublk server is implemented.
> > 
> > In cover letter, it is mentioned that "A + 2 is a currently unsupported
> > behavior", can you explain it a bit? Such as, how does ublk server
> > handle the I/O error? And when/how to recover? why doesn't this way
> > work?
> 
> Sorry if this was unclear - the behaviors I describe in A, B, C, 1, 2
> are all referring to what is seen by the application using the ublk
> block device when the ublk server crashes. There is no sense in which

Yes, usually the app using ublk is supposed to be completely generic,
and won't be taken into account.

> the ublk server can "handle" the I/O error because during this time,
> there is no ublk server and all decisions on how to handle I/O are made
> by ublk_drv directly (based on configuration flags specified when the
> device was created).
> 
> If the ublk server created the device with UBLK_F_USER_RECOVERY, then
> when the ublk server has crashed (and not restarted yet), I/Os issued by
> the application will queue/hang until the ublk server comes back and
> recovers the device, because the underlying request_queue is left in a
> quiesced state. So in this case, behavior A is not possible.

When ublk server is crashed, ublk_abort_requests() will be called to fail
queued inflight requests. Meantime ubq->canceling is set to requeue
new request instead of forwarding it to ublk server.

So behavior A should be supported easily by failing request in
ublk_queue_rq() if ubq->canceling is set.

> 
> If the ublk server created the device without UBLK_F_USER_RECOVERY, then
> when the ublk server has crashed (and not restarted yet), I/Os issued by
> the application will immediately error (since in this case, ublk will
> call del_gendisk).  However, when the ublk server restarts, it cannot
> recover the existing ublk device - the disk has been deleted and the
> ublk device is in state UBLK_S_DEV_DEAD from which recovery is not
> permitted. So in this case, behavior 2 is not possible.

UBLK_F_USER_RECOVERY is supposed for supporting to recover device, and
if this flag isn't enabled, we don't support the feature simply, so
looks behavior 2 isn't one valid case, is it?

> 
> Hence A + 2 is impossible with the current ublk_drv implementation.
> Please correct me if I missed something.

Please see if the above reply can address this case.


Thanks,
Ming