On Tue, May 16, 2023 at 04:10:01PM +0800, Ming Lei wrote:
> 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?
Yes.
> > 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.
I just don't think it maks any sense in this context. If we want to
enforce the invariant that there's no flush request I'd rather add a
WARN_ON to not only talk about enforce it. I'm not sure it's really
required, though.
> > > + 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().
But if it is part of error recovery it won't be plugged. Please don't
do weird cargo cult things here and just use the common helpers.
