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.