On Tue, May 16, 2023 at 08:22:21AM +0200, Christoph Hellwig wrote:
> On Mon, May 15, 2023 at 10:46:00PM +0800, Ming Lei wrote:
> > + } else if (this_hctx != rq->mq_hctx || this_ctx != rq->mq_ctx ||
> > + pt != blk_rq_is_passthrough(rq)) {
>
> Can your format this as:
>
> } else if (this_hctx != rq->mq_hctx || this_ctx != rq->mq_ctx ||
> pt != blk_rq_is_passthrough(rq)) {
>
> for readability?
Do you mean indent for 'pt = blk_rq_is_passthrough(rq)' and keep 'pt' aligned
with 'if' in last line? Otherwise, can't see any difference between the two, :-(
>
> > + /*
> > + * Both passthrough and flush request don't belong to
> > + * scheduler, but flush request won't be added to plug
> > + * list, so needn't handle here.
> > + */
> > rq_list_add_tail(&requeue_lastp, rq);
>
> This comment confuses the heck out of me. The check if for passthrough
> vs non-passthrough and doesn't involved flush requests at all.
>
> I'd prefer to drop it, and instead comment on passthrough requests
> not going to the scheduled below where we actually issue other requests
> to the scheduler.
Any request can be in plug list in theory, we just don't add flush request
to plug, that is why the above comment is added. If you don't like the
words for flush request, I can drop it.
>
> > + if (pt) {
> > + spin_lock(&this_hctx->lock);
> > + list_splice_tail_init(&list, &this_hctx->dispatch);
> > + spin_unlock(&this_hctx->lock);
> > + blk_mq_run_hw_queue(this_hctx, from_sched);
>
> .. aka here. But why can't we just use the blk_mq_insert_requests
> for this case anyway?
If the pt request is part of error recovery, it should be issued to
->dispatch list directly, so just for the sake of safety, meantime keep
same behavior with blk_mq_insert_request().
Thanks,
Ming
