Re: [PATCH 19/23] scsi_dh_alua: Use workqueue for RTPG

[Hannes Reinecke <hare@suse.de>]

On 09/22/2015 09:49 PM, Ewan Milne wrote:
> On Thu, 2015-08-27 at 14:41 +0200, Hannes Reinecke wrote:
>> The current ALUA device_handler has two drawbacks:
>> - We're sending a 'SET TARGET PORT GROUP' command to every LUN,
>>   disregarding the fact that several LUNs might be in a port group
>>   and will be automatically switched whenever _any_ LUN within
>>   that port group receives the command.
>> - Whenever a LUN is in 'transitioning' mode we cannot block I/O
>>   to that LUN, instead the controller has to abort the command.
>>   This leads to increased traffic across the wire and heavy load
>>   on the controller during switchover.
> 
> I'm not sure I understand what this means - why couldn't we block I/O?
> and what does 'heavy load' mean?  Aborting commands is 'heavy load'?
> 
If we're getting a sense code indicating that the LUN is in
transitioning _and_ we're blocking I/O we never ever send down I/Os to
that driver anymore, so we cannot receive any sense codes indicating the
transitioning is done.
At the same time, every I/O we're sending down will be returned by the
storage I/O with a sense code, requiring us to retry the command.
Hence we're constantly retrying I/O.

[ .. ]
>> @@ -811,10 +1088,17 @@ failed:
>>  static void alua_bus_detach(struct scsi_device *sdev)
>>  {
>>  	struct alua_dh_data *h = sdev->handler_data;
>> +	struct alua_port_group *pg;
>>  
>> -	if (h->pg) {
>> -		kref_put(&h->pg->kref, release_port_group);
>> -		h->pg = NULL;
>> +	spin_lock(&h->pg_lock);
>> +	pg = h->pg;
>> +	rcu_assign_pointer(h->pg, NULL);
>> +	spin_unlock(&h->pg_lock);
>> +	synchronize_rcu();
>> +	if (pg) {
>> +		if (pg->rtpg_sdev)
>> +			flush_workqueue(pg->work_q);
>> +		kref_put(&pg->kref, release_port_group);
>>  	}
>>  	sdev->handler_data = NULL;
>>  	kfree(h);
> 
> So, you've already had a bit of discussion with Christoph about this,
> the main portion of your ALUA rewrite, and I won't go over all of that,
> except to say that I'd have to agree that having separate work queues
> for the different RTPG/STPG functions and having them manipulate each
> other's flags seems like we'd be better off having just one work
> function that did everything.  Less messy and easier to maintain.
> 
> Also, it seems like wrong ordering of kref_get() vs. scsi_device_get(),
> in alua_rtpg_queue() since they are released as kref_put() then
> scsi_device_put()?
> 
Yeah, I've reworked the reference counting.
And reverted the workqueue handling to use the original model.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html