On Tue, Aug 14, 2018 at 10:47:21AM +0800, jianchao.wang wrote:

Hi Ming

On 08/14/2018 10:10 AM, Ming Lei wrote:

quoted

On Tue, Aug 14, 2018 at 09:29:25AM +0800, jianchao.wang wrote:

quoted

Hi Ming

On 08/13/2018 06:48 PM, Ming Lei wrote:

quoted

It is nothing to do with where the admin request is sent, so no any
difference wrt. this issue between with and without this patchset,
right?

I'm afraid not.

For example:
  A scsi host has 8 LUNs associated with it.
  Before this patch set,
  When we send out the admin command, the budget is _per_ LUN, 1/8 of the total tags.
  After this patch set,
  When we send out the admin command, the budget is equal to _one_ LUN, 1/8 of the total tags.

However, the 1/8 above is different.
  Before the patch set, every LUN's admin command has 1/8 budget to use which is per LUN.

Strictly speaking, it is that all admin command and all other IOs share the 1/8 budget
if they aimed at same LUN.

Yes.

quoted

quoted

  After this patch set, all the 8 LUNs admin command has to share the 1/8 budget.

That only means number of active admin commands won't be bigger than 1/8 budget, which
is one extra implicit limit on admin queue. However, other LUN's budget is still 1/8.

So performance for IO queue won't be affected at all, will it?

scsi_execute_* can't be called often, it is really in slow path, so I
don't think there is any possible performance effect with this patch, or do
you have other performance concern wrt. this patch?

We still have q->queue_depth for enhancing any limit for admin queue, but up to now,
not see it is necessary.

I agree with you that the performance will not be affected.

OK, thanks for your confirm.

But the adminq's budget here looks weird.
We don't reserve budget for admin queue (not count tag->active_queues for it).
But the admin queue has to comply to the limit in hctx_may_queue.

Since the there isn't many in-flight admin commands usually, we could take
admin queue here out of the limit of hctx_may_queue, then things could be clearer. :)

I just didn't want to add one line code in the fast path of hctx_may_queue()
because it isn't necessary.

Now looks this way may have one implicit benefit: avoid too many in-flight admin
requests.

I will leave the code in this way, but add comment like below to hctx_may_queue():

	Needn't to deal with admin queue specially here even though we don't
	take it account to tags->active_queues, so blk_queue_admin() can be
	avoided to check in the fast path, also with implicit benefit of
	limiting too many in-flight admin requests.


Thanks, 
Ming