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.
