Re: [PATCH v2 15/17] ibmvfc: send Cancel MAD down each hw scsi channel

From: Tyrel Datwyler <tyreld@linux.ibm.com>
To: Brian King <brking@linux.vnet.ibm.com>,
	james.bottomley@hansenpartnership.com
Cc: martin.petersen@oracle.com, linux-scsi@vger.kernel.org,
	linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
	brking@linux.ibm.com
Subject: Re: [PATCH v2 15/17] ibmvfc: send Cancel MAD down each hw scsi channel
Date: Wed, 2 Dec 2020 14:09:42 -0800	[thread overview]
Message-ID: <0e1760c8-ced0-cd50-391f-29a5a9ea340a@linux.ibm.com> (raw)
In-Reply-To: <21a7c970-2184-0524-5b42-1920eaa422a2@linux.vnet.ibm.com>

On 12/2/20 10:27 AM, Brian King wrote:
> On 12/1/20 6:53 PM, Tyrel Datwyler wrote:
>> In general the client needs to send Cancel MADs and task management
>> commands down the same channel as the command(s) intended to cancel or
>> abort. The client assigns cancel keys per LUN and thus must send a
>> Cancel down each channel commands were submitted for that LUN. Further,
>> the client then must wait for those cancel completions prior to
>> submitting a LUN RESET or ABORT TASK SET.
>>
>> Allocate event pointers for each possible scsi channel and assign an
>> event for each channel that requires a cancel. Wait for completion each
>> submitted cancel.
>>
>> Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
>> ---
>>  drivers/scsi/ibmvscsi/ibmvfc.c | 106 +++++++++++++++++++++------------
>>  1 file changed, 68 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
>> index 0b6284020f06..97e8eed04b01 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
>> @@ -2339,32 +2339,52 @@ static int ibmvfc_cancel_all(struct scsi_device *sdev, int type)
>>  {
>>  	struct ibmvfc_host *vhost = shost_priv(sdev->host);
>>  	struct ibmvfc_event *evt, *found_evt;
>> -	union ibmvfc_iu rsp;
>> -	int rsp_rc = -EBUSY;
>> +	struct ibmvfc_event **evt_list;
>> +	union ibmvfc_iu *rsp;
>> +	int rsp_rc = 0;
>>  	unsigned long flags;
>>  	u16 status;
>> +	int num_hwq = 1;
>> +	int i;
>> +	int ret = 0;
>>  
>>  	ENTER;
>>  	spin_lock_irqsave(vhost->host->host_lock, flags);
>> -	found_evt = NULL;
>> -	list_for_each_entry(evt, &vhost->sent, queue) {
>> -		if (evt->cmnd && evt->cmnd->device == sdev) {
>> -			found_evt = evt;
>> -			break;
>> +	if (vhost->using_channels && vhost->scsi_scrqs.active_queues)
>> +		num_hwq = vhost->scsi_scrqs.active_queues;
>> +
>> +	evt_list = kcalloc(num_hwq, sizeof(*evt_list), GFP_KERNE> +	rsp = kcalloc(num_hwq, sizeof(*rsp), GFP_KERNEL);
> 
> Can't this just go on the stack? We don't want to be allocating memory
> during error recovery. Or, alternatively, you could put this in the
> vhost structure and protect it with a mutex. We only have enough events
> to single thread these anyway.
Yes, this could just go on the stack.

> 
>> +
>> +	for (i = 0; i < num_hwq; i++) {
>> +		sdev_printk(KERN_INFO, sdev, "Cancelling outstanding commands on queue %d.\n", i);
> 
> Prior to this patch, if there was nothing outstanding to the device and cancel_all was called,
> no messages would get printed. This is changing that behavior. Is that intentional? Additionally,
> it looks like this will get a lot more vebose, logging a message for each hw queue, regardless
> of whether there was anything outstanding. Perhaps you want to move this down to after the check
> for !found_evt?

It would actually print "no commands found to cancel". I think its fair to make
it less verbose or at least make them dbg output for each queue.

-Tyrel

> 
>> +
>> +		found_evt = NULL;
>> +		list_for_each_entry(evt, &vhost->sent, queue) {
>> +			if (evt->cmnd && evt->cmnd->device == sdev && evt->hwq == i) {
>> +				found_evt = evt;
>> +				break;
>> +			}
>>  		}
>> -	}
>>  
> 
> 
> 