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: > > On Tue, Aug 14, 2018 at 09:29:25AM +0800, jianchao.wang wrote: > >> Hi Ming > >> > >> On 08/13/2018 06:48 PM, Ming Lei wrote: > >>> 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. > > > > >> 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
