Date:   Fri, 15 Jan 2021 09:47:33 +0800
From:   Ming Lei <ming.lei@...hat.com>
To:     Brian King <brking@...ux.vnet.ibm.com>
Cc:     Tyrel Datwyler <tyreld@...ux.ibm.com>,
        james.bottomley@...senpartnership.com, martin.petersen@...cle.com,
        linux-scsi@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org, brking@...ux.ibm.com,
        james.smart@...adcom.com
Subject: Re: [PATCH v4 01/21] ibmvfc: add vhost fields and defaults for MQ
 enablement

On Thu, Jan 14, 2021 at 11:24:35AM -0600, Brian King wrote:
> On 1/13/21 7:27 PM, Ming Lei wrote:
> > On Wed, Jan 13, 2021 at 11:13:07AM -0600, Brian King wrote:
> >> On 1/12/21 6:33 PM, Tyrel Datwyler wrote:
> >>> On 1/12/21 2:54 PM, Brian King wrote:
> >>>> On 1/11/21 5:12 PM, Tyrel Datwyler wrote:
> >>>>> Introduce several new vhost fields for managing MQ state of the adapter
> >>>>> as well as initial defaults for MQ enablement.
> >>>>>
> >>>>> Signed-off-by: Tyrel Datwyler <tyreld@...ux.ibm.com>
> >>>>> ---
> >>>>>  drivers/scsi/ibmvscsi/ibmvfc.c | 8 ++++++++
> >>>>>  drivers/scsi/ibmvscsi/ibmvfc.h | 9 +++++++++
> >>>>>  2 files changed, 17 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> >>>>> index ba95438a8912..9200fe49c57e 100644
> >>>>> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> >>>>> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
> >>>>> @@ -3302,6 +3302,7 @@ static struct scsi_host_template driver_template = {
> >>>>>  	.max_sectors = IBMVFC_MAX_SECTORS,
> >>>>>  	.shost_attrs = ibmvfc_attrs,
> >>>>>  	.track_queue_depth = 1,
> >>>>> +	.host_tagset = 1,
> >>>>
> >>>> This doesn't seem right. You are setting host_tagset, which means you want a
> >>>> shared, host wide, tag set for commands. It also means that the total
> >>>> queue depth for the host is can_queue. However, it looks like you are allocating
> >>>> max_requests events for each sub crq, which means you are over allocating memory.
> >>>
> >>> With the shared tagset yes the queue depth for the host is can_queue, but this
> >>> also implies that the max queue depth for each hw queue is also can_queue. So,
> >>> in the worst case that all commands are queued down the same hw queue we need an
> >>> event pool with can_queue commands.
> >>>
> >>>>
> >>>> Looking at this closer, we might have bigger problems. There is a host wide
> >>>> max number of commands that the VFC host supports, which gets returned on
> >>>> NPIV Login. This value can change across a live migration event.
> >>>
> >>> From what I understand the max commands can only become less.
> >>>
> >>>>
> >>>> The ibmvfc driver, which does the same thing the lpfc driver does, modifies
> >>>> can_queue on the scsi_host *after* the tag set has been allocated. This looks
> >>>> to be a concern with ibmvfc, not sure about lpfc, as it doesn't look like
> >>>> we look at can_queue once the tag set is setup, and I'm not seeing a good way
> >>>> to dynamically change the host queue depth once the tag set is setup. 
> >>>>
> >>>> Unless I'm missing something, our best options appear to either be to implement
> >>>> our own host wide busy reference counting, which doesn't sound very good, or
> >>>> we need to add some API to block / scsi that allows us to dynamically change
> >>>> can_queue.
> >>>
> >>> Changing can_queue won't do use any good with the shared tagset becasue each
> >>> queue still needs to be able to queue can_queue number of commands in the worst
> >>> case.
> >>
> >> The issue I'm trying to highlight here is the following scenario:
> >>
> >> 1. We set shost->can_queue, then call scsi_add_host, which allocates the tag set.
> >>
> >> 2. On our NPIV login response from the VIOS, we might get a lower value than we
> >> initially set in shost->can_queue, so we update it, but nobody ever looks at it
> >> again, and we don't have any protection against sending too many commands to the host.
> >>
> >>
> >> Basically, we no longer have any code that ensures we don't send more
> >> commands to the VIOS than we are told it supports. According to the architecture,
> >> if we actually do this, the VIOS will do an h_free_crq, which would be a bit
> >> of a bug on our part.
> >>
> >> I don't think it was ever clearly defined in the API that a driver can
> >> change shost->can_queue after calling scsi_add_host, but up until
> >> commit 6eb045e092efefafc6687409a6fa6d1dabf0fb69, this worked and now
> >> it doesn't. 
> > 
> > Actually it isn't related with commit 6eb045e092ef, because blk_mq_alloc_tag_set()
> > uses .can_queue to create driver tag sbitmap and request pool.
> > 
> > So even thought without 6eb045e092ef, the updated .can_queue can't work
> > as expected because the max driver tag depth has been fixed by blk-mq already.
> 
> There are two scenarios here. In the scenario of someone increasing can_queue
> after the tag set is allocated, I agree, blk-mq will never take advantage
> of this. However, in the scenario of someone *decreasing* can_queue after the
> tag set is allocated, prior to 6eb045e092ef, the shost->host_busy code provided
> this protection.

When .can_queue is decreased, blk-mq still may allocate driver tag which is >
.can_queue, this way might break driver/device too, but it depends on how driver
uses req->tag.

> 
> > 
> > What 6eb045e092ef does is just to remove the double check on max
> > host-wide allowed commands because that has been respected by blk-mq
> > driver tag allocation already.
> > 
> >>
> >> I started looking through drivers that do this, and so far, it looks like the
> >> following drivers do: ibmvfc, lpfc, aix94xx, libfc, BusLogic, and likely others...
> >>
> >> We probably need an API that lets us change shost->can_queue dynamically.
> > 
> > I'd suggest to confirm changing .can_queue is one real usecase.
> 
> For ibmvfc, the total number of commands that the scsi host supports is very
> much a dynamic value. It can increase and it can decrease. Live migrating
> a logical partition from one system to another is the usual cause of
> such a capability change. For ibmvfc, at least, this only ever happens
> when we've self blocked the host and have sent back all outstanding I/O.

This one looks a good use case, and the new API may have to freeze request
queues of all LUNs, and the operation is very expensive and slow. 

> 
> However, looking at other drivers that modify can_queue dynamically, this
> doesn't always hold true. Looking at libfc, it looks to dynamically ramp
> up and ramp down can_queue based on its ability to handle requests.

This one looks hard to use the new API which isn't supposed to be called
in fast path. And changing host wide resource is really not good in fast
path, IMO.

> 
> There are certainly a number of other drivers that change can_queue
> after the tag set has been allocated. Some of these drivers could
> likely be changed to avoid doing this, but changing them all will likely
> be difficult.

It is still better to understand why these drivers have to update
.can_queue dynamically.


Thanks,
Ming

Powered by blists - more mailing lists