On Thu, 2017-08-24 at 12:52 +0800, Ming Lei wrote:
> On Tue, Aug 22, 2017 at 06:45:46PM +0000, Bart Van Assche wrote:
> > On Sat, 2017-08-05 at 14:56 +0800, Ming Lei wrote:
> > > More importantly, for some SCSI devices, driver
> > > tags are host wide, and the number is quite big,
> > > but each lun has very limited queue depth.
> >
> > This may be the case but is not always the case. Another important use-case
> > is one LUN per host and where the queue depth per LUN is identical to the
> > number of host tags.
>
> This patchset won't hurt this case because the BUSY info is returned
> from driver. In this case, BLK_STS_RESOURCE should seldom be returned
> from .queue_rq generally.
>
> Also one important fact is that once q->queue_depth is set, that
> means there is per-request_queue limit on pending I/Os, and the
> single LUN is just the special case which is covered by this whole
> patchset. We don't need to pay special attention in this special case
> at all.
The purpose of my comment was not to ask for further clarification but to
report that the description of this patch is not correct.
> >
> > > +struct request *blk_mq_dispatch_rq_from_ctx(struct blk_mq_hw_ctx *hctx,
> > > + struct blk_mq_ctx *start)
> > > +{
> > > + unsigned off = start ? start->index_hw : 0;
> >
> > Please consider to rename this function into
> > blk_mq_dispatch_rq_from_next_ctx()
> > and to start from start->index_hw + 1 instead of start->index_hw. I think
> > that
> > will not only result in simpler but also in faster code.
>
> I believe this helper with blk_mq_next_ctx(hctx, rq->mq_ctx) together
> will be much simpler and easier to implement, and code can be much
> readable too.
>
> blk_mq_dispatch_rq_from_next_ctx() is ugly and mixing two things
> together.
Sorry but I disagree with both of the above statements.
Bart.
