Re: [PATCH 4/4] ublk: support device recovery without I/O queueing

[Uday Shankar <ushankar@purestorage.com>]

On Tue, Jul 02, 2024 at 09:46:00PM +0800, Ming Lei wrote:
> I'd suggest to add one per-ublk-queue flag for this purpose instead of
> device state, then fetching device can be avoided in fast io path, please see
> similar example of `->force_abort`.

Done in v2.

> >  	/* fill iod to slot in io cmd buffer */
> >  	res = ublk_setup_iod(ubq, rq);
> >  	if (unlikely(res != BLK_STS_OK))
> > @@ -1602,7 +1616,15 @@ static void ublk_nosrv_work(struct work_struct *work)
> >  	mutex_lock(&ub->mutex);
> >  	if (ub->dev_info.state != UBLK_S_DEV_LIVE)
> >  		goto unlock;
> > -	__ublk_quiesce_dev(ub);
> > +
> > +	if (ublk_nosrv_should_queue_io(ub)) {
> 
> Here ublk_nosrv_should_queue_io() doesn't cover UBLK_F_USER_RECOVERY_REISSUE.

Not sure what you mean here. I don't see an issue, can you explain?

> 
> > +		__ublk_quiesce_dev(ub);
> > +	} else {
> > +		blk_mq_quiesce_queue(ub->ub_disk->queue);
> > +		ub->dev_info.state = UBLK_S_DEV_FAIL_IO;
> > +		blk_mq_unquiesce_queue(ub->ub_disk->queue);
> > +	}
> 
> If the above extra device state is saved, the whole change can be simplified,
> and __ublk_quiesce_dev() still can be called for
> UBLK_F_USER_RECOVERY_NOQUEUE, and UBLK_S_DEV_QUIESCED can cover this new flag,
> meantime setting one per-ublk-queue flag, such as, ->fail_io_in_recovery.

I don't think it's a good idea to have the state UBLK_S_DEV_QUIESCED
cover the new flag. Then we will have a case where the state is
UBLK_S_DEV_QUIESCED but the queue is not actually quiesced... that just
seems confusing. I added a per-queue flag and used it in the fast path,
but kept the new state as well.

> > +/*
> > + * - Block devices are recoverable if ublk server exits and restarts
> > + * - Outstanding I/O when ublk server exits is met with errors
> > + * - I/O issued while there is no ublk server is met with errors
> > + */
> > +#define UBLK_F_USER_RECOVERY_NOQUEUE (1ULL << 9)
> 
> Maybe UBLK_F_USER_RECOVERY_FAIL_IO is more readable?

Sure, changed the name.