Re: [PATCH 22/23] scsi_dh_alua: update 'access_state' field

[Hannes Reinecke <hare@suse.de>]

On 02/11/2016 09:37 PM, Ewan Milne wrote:
> On Mon, 2016-02-08 at 15:34 +0100, Hannes Reinecke wrote:
>> Track attached SCSI devices and update the 'access_state' field
>> whenever an ALUA state change has been detected.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/scsi/device_handler/scsi_dh_alua.c | 45 ++++++++++++++++++++++++++++--
>>  1 file changed, 43 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
>> index 51126b78..7bcc075 100644
>> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
>> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
>> @@ -24,6 +24,7 @@
>>  #include <linux/module.h>
>>  #include <asm/unaligned.h>
>>  #include <scsi/scsi.h>
>> +#include <scsi/scsi_proto.h>
>>  #include <scsi/scsi_dbg.h>
>>  #include <scsi/scsi_eh.h>
>>  #include <scsi/scsi_dh.h>
>> @@ -75,6 +76,7 @@ struct alua_port_group {
>>  	struct kref		kref;
>>  	struct rcu_head		rcu;
>>  	struct list_head	node;
>> +	struct list_head	dh_list;
>>  	unsigned char		device_id_str[256];
>>  	int			device_id_len;
>>  	int			group_id;
>> @@ -92,6 +94,7 @@ struct alua_port_group {
>>  };
>>  
>>  struct alua_dh_data {
>> +	struct list_head	node;
>>  	struct alua_port_group	*pg;
>>  	int			group_id;
>>  	spinlock_t		pg_lock;
>> @@ -247,6 +250,7 @@ struct alua_port_group *alua_alloc_pg(struct scsi_device *sdev,
>>  	INIT_DELAYED_WORK(&pg->rtpg_work, alua_rtpg_work);
>>  	INIT_LIST_HEAD(&pg->rtpg_list);
>>  	INIT_LIST_HEAD(&pg->node);
>> +	INIT_LIST_HEAD(&pg->dh_list);
>>  	spin_lock_init(&pg->lock);
>>  
>>  	spin_lock(&port_group_lock);
>> @@ -328,6 +332,8 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
>>  {
>>  	int rel_port = -1, group_id;
>>  	struct alua_port_group *pg, *old_pg = NULL;
>> +	bool pg_updated;
>> +	unsigned long flags;
>>  
>>  	group_id = scsi_vpd_tpg_id(sdev, &rel_port);
>>  	if (group_id < 0) {
>> @@ -357,13 +363,25 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
>>  	old_pg = h->pg;
>>  	if (old_pg != pg) {
>>  		/* port group has changed. Update to new port group */
>> +		if (h->pg) {
>> +			spin_lock_irqsave(&old_pg->lock, flags);
>> +			list_del_rcu(&h->node);
>> +			spin_unlock_irqrestore(&old_pg->lock, flags);
>> +		}
>>  		rcu_assign_pointer(h->pg, pg);
>> +		pg_updated = true;
>>  	}
>>  	if (sdev->synchronous_alua)
>>  		pg->flags |= ALUA_SYNC_STPG;
>>  	alua_rtpg_queue(h->pg, sdev, NULL, true);
>>  	spin_unlock(&h->pg_lock);
>>  
>> +	if (pg_updated) {
>> +		spin_lock_irqsave(&pg->lock, flags);
>> +		list_add_rcu(&h->node, &pg->dh_list);
>> +		spin_unlock_irqrestore(&pg->lock, flags);
>> +	}
>> +
>>  	if (old_pg)
>>  		kref_put(&old_pg->kref, release_port_group);
>>  
>> @@ -613,8 +631,18 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>>  			if (spin_trylock_irqsave(&tmp_pg->lock, flags)) {
>>  				if ((tmp_pg == pg) ||
>>  				    !(tmp_pg->flags & ALUA_PG_RUNNING)) {
>> +					struct alua_dh_data *h;
>> +
>>  					tmp_pg->state = desc[0] & 0x0f;
>>  					tmp_pg->pref = desc[0] >> 7;
>> +					rcu_read_lock();
>> +					list_for_each_entry_rcu(h,
>> +						&tmp_pg->dh_list, node) {
>> +						/* h->sdev should always be valid */
>> +						BUG_ON(!h->sdev);
>> +						h->sdev->access_state = desc[0];
> 
> Here you did not mask desc[0] with 0x0f like you did in other places in the code.
> 
Because the access_state field is a 1:1 copy of the 'desc[0]' field,
containing both the value and the pref bit.

>> +					}
>> +					rcu_read_unlock();
>>  				}
>>  				if (tmp_pg == pg)
>>  					valid_states = desc[1];
>> @@ -646,10 +674,19 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>>  			pg->interval = 2;
>>  			err = SCSI_DH_RETRY;
>>  		} else {
>> +			struct alua_dh_data *h;
>> +
>>  			/* Transitioning time exceeded, set port to standby */
>>  			err = SCSI_DH_IO;
>>  			pg->state = SCSI_ACCESS_STATE_STANDBY;
>>  			pg->expiry = 0;
>> +			rcu_read_lock();
>> +			list_for_each_entry_rcu(h, &pg->dh_list, node) {
>> +				BUG_ON(!h->sdev);
>> +				h->sdev->access_state = (pg->state & 0x0f) |
>> +					(pg->pref << 7);
> 
> Since SCSI_ACCESS_STATE_PREFERRED is #defined as 0x80 in scsi_proto.h you
> should use | (pg->pref ? SCSI_ACCESS_STATE_PREFERRED : 0) here rather than
> just shifting, otherwise you will have a hard to find bug if somebody
> changes the constant.  (But I would prefer keeping them separate anyway.)
> 
_Actually_ I was pondering to make it the other way around, namely
shifting pg->pref on top of pg->state, then this whole thing would
become a simple assignment.
But I didn't as I wanted to keep the original code as-is.

So yeah, there is an inconsistency, but some I would like to resolve
in exactly the other direction :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (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