Re: [PATCH 13/23] scsi_dh_alua: Use separate alua_port_group structure

2015-09-22 Thread Ewan Milne 
On Thu, 2015-08-27 at 14:41 +0200, Hannes Reinecke wrote:
> The port group needs to be a separate structure as several
> LUNs might belong to the same group.
> 
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/scsi/device_handler/scsi_dh_alua.c | 211 
> +++--
>  1 file changed, 139 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
> b/drivers/scsi/device_handler/scsi_dh_alua.c
> index ef4363a..d1010dd 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -64,9 +64,13 @@
>  #define ALUA_OPTIMIZE_STPG   1
>  #define ALUA_RTPG_EXT_HDR_UNSUPP 2
>  
> -struct alua_dh_data {
> +static LIST_HEAD(port_group_list);
> +static DEFINE_SPINLOCK(port_group_lock);
> +
> +struct alua_port_group {
> + struct kref kref;
> + struct list_headnode;
>   int group_id;
> - int rel_port;
>   int tpgs;
>   int state;
>   int pref;
> @@ -75,6 +79,12 @@ struct alua_dh_data {
>   unsigned char   *buff;
>   int bufflen;
>   unsigned char   transition_tmo;
> +};
> +
> +struct alua_dh_data {
> + struct alua_port_group  *pg;
> + int rel_port;
> + int tpgs;
>   struct scsi_device  *sdev;
>   activate_complete   callback_fn;
>   void*callback_data;
> @@ -86,21 +96,35 @@ struct alua_dh_data {
>  static char print_alua_state(int);
>  static int alua_check_sense(struct scsi_device *, struct scsi_sense_hdr *);
>  
> -static int realloc_buffer(struct alua_dh_data *h, unsigned len)
> +static int realloc_buffer(struct alua_port_group *pg, unsigned len)
>  {
> - if (h->buff && h->buff != h->inq)
> - kfree(h->buff);
> + if (pg->buff && pg->buff != pg->inq)
> + kfree(pg->buff);
>  
> - h->buff = kmalloc(len, GFP_NOIO);
> - if (!h->buff) {
> - h->buff = h->inq;
> - h->bufflen = ALUA_INQUIRY_SIZE;
> + pg->buff = kmalloc(len, GFP_NOIO);
> + if (!pg->buff) {
> + pg->buff = pg->inq;
> + pg->bufflen = ALUA_INQUIRY_SIZE;
>   return 1;
>   }
> - h->bufflen = len;
> + pg->bufflen = len;
>   return 0;
>  }
>  
> +static void release_port_group(struct kref *kref)
> +{
> + struct alua_port_group *pg;
> +
> + pg = container_of(kref, struct alua_port_group, kref);
> + printk(KERN_WARNING "alua: release port group %d\n", pg->group_id);
> + spin_lock(_group_lock);
> + list_del(>node);
> + spin_unlock(_group_lock);
> + if (pg->buff && pg->inq != pg->buff)
> + kfree(pg->buff);
> + kfree(pg);
> +}
> +
>  /*
>   * submit_rtpg - Issue a REPORT TARGET GROUP STATES command
>   * @sdev: sdev the command should be sent to
> @@ -225,6 +249,8 @@ static int alua_check_tpgs(struct scsi_device *sdev, 
> struct alua_dh_data *h)
>  static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
>  {
>   unsigned char *d;
> + int group_id = -1;
> + struct alua_port_group *pg = NULL;
>  
>   if (!sdev->vpd_pg83)
>   return SCSI_DH_DEV_UNSUPP;
> @@ -241,7 +267,7 @@ static int alua_check_vpd(struct scsi_device *sdev, 
> struct alua_dh_data *h)
>   break;
>   case 0x5:
>   /* Target port group */
> - h->group_id = get_unaligned_be16([6]);
> + group_id = get_unaligned_be16([6]);
>   break;
>   default:
>   break;
> @@ -249,7 +275,7 @@ static int alua_check_vpd(struct scsi_device *sdev, 
> struct alua_dh_data *h)
>   d += d[3] + 4;
>   }
>  
> - if (h->group_id == -1) {
> + if (group_id == -1) {
>   /*
>* Internal error; TPGS supported but required
>* VPD identification descriptors not present.
> @@ -258,15 +284,33 @@ static int alua_check_vpd(struct scsi_device *sdev, 
> struct alua_dh_data *h)
>   sdev_printk(KERN_INFO, sdev,
>   "%s: No target port descriptors found\n",
>   ALUA_DH_NAME);
> - h->state = TPGS_STATE_OPTIMIZED;
>   h->tpgs = TPGS_MODE_NONE;
>   return SCSI_DH_DEV_UNSUPP;
>   }
>   sdev_printk(KERN_INFO, sdev,
>   "%s: port group %02x rel port %02x\n",
> - ALUA_DH_NAME, h->group_id, h->rel_port);
> + ALUA_DH_NAME, group_id, h->rel_port);
>  
> - return 0;
> + pg = kzalloc(sizeof(struct alua_port_group), GFP_KERNEL);
> + if (!pg) {
> + sdev_printk(KERN_WARNING, sdev,
> + "%s: kzalloc port group failed\n",
> +

Re: [PATCH 13/23] scsi_dh_alua: Use separate alua_port_group structure

2015-09-01 Thread Christoph Hellwig 
> +struct alua_dh_data {
> + struct alua_port_group  *pg;
> + int rel_port;
> + int tpgs;

The tpgs file doesn't make much sense in the new alua_dh_data
structure.  Please return it explicitly from alua_check_tpgs and
assign it directly to the member in struct alua_port_group.

> +static void release_port_group(struct kref *kref)
> +{
> + struct alua_port_group *pg;
> +
> + pg = container_of(kref, struct alua_port_group, kref);
> + printk(KERN_WARNING "alua: release port group %d\n", pg->group_id);

Why is this a warning?  I'd consider it nothing but debug noise.

> - err = alua_rtpg(sdev, h, 0);
> - if (err != SCSI_DH_OK)
> + if (err != SCSI_DH_OK || !h->pg)
>   goto out;
>  
> + kref_get(>pg->kref);

What prevents this kref_get from racing with the last kref_put?

I think all the kref_get calls in this patch need to be
kref_get_unless_zero with proper error handling.

> + rcu_read_lock();
> + pg = rcu_dereference(h->pg);
> + if (!pg) {
> + rcu_read_unlock();
> + return -ENXIO;
> + }

What is this rcu_read_lock supposed to protect against given
that struct alua_port_group isn't RCU freed?  I think this needs
to be another kref_get_unless_zero.

> +
>   if (optimize)
> - h->flags |= ALUA_OPTIMIZE_STPG;
> + pg->flags |= ALUA_OPTIMIZE_STPG;
>   else
> - h->flags &= ~ALUA_OPTIMIZE_STPG;
> + pg->flags |= ~ALUA_OPTIMIZE_STPG;
> + rcu_read_unlock();

The read-modify-write of pg->flags will need some sort of locking.
Seems like most modifications of it are under pg->rtpg_lock, so
I'd suggest to enforce that as a strict rule.

> + err = alua_rtpg(sdev, h->pg, 1);
> + if (err != SCSI_DH_OK) {
> + kref_put(>pg->kref, release_port_group);
> + goto out;

goto out_put_pg;


out_put_pg:
> + kref_put(>pg->kref, release_port_group);
>  out:
>   if (fn)
>   fn(data, err);
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/23] scsi_dh_alua: Use separate alua_port_group structure

2015-09-01 Thread Christoph Hellwig 
Ok, coming back to this path as it's the start of somethign building
up over various patches:

> + pg = kzalloc(sizeof(struct alua_port_group), GFP_KERNEL);
> + if (!pg) {
> + sdev_printk(KERN_WARNING, sdev,
> + "%s: kzalloc port group failed\n",
> + ALUA_DH_NAME);
> + /* Temporary failure, bypass */
> + return SCSI_DH_DEV_TEMP_BUSY;
> + }
> + pg->group_id = group_id;
> + pg->buff = pg->inq;
> + pg->bufflen = ALUA_INQUIRY_SIZE;
> + pg->tpgs = h->tpgs;
> + pg->state = TPGS_STATE_OPTIMIZED;
> + kref_init(>kref);
> + spin_lock(_group_lock);
> + list_add(>node, _group_list);
> + h->pg = pg;
> + spin_unlock(_group_lock);
> +
> + return SCSI_DH_OK;

All this code isn't really checking the VPD anymore.  Please keep
the existing alua_check_vpd as a low-level helper, and move this
new functionality into a separate alua_find_get_pg function that
calls the original alua_check_vpd.  Also please return the
pg from it so that we c

> @@ -596,13 +643,12 @@ static int alua_initialize(struct scsi_device *sdev, 
> struct alua_dh_data *h)
>   goto out;
>  
>   err = alua_check_vpd(sdev, h);
> - if (err != SCSI_DH_OK)
> - goto out;
> -
> - err = alua_rtpg(sdev, h, 0);
> - if (err != SCSI_DH_OK)
> + if (err != SCSI_DH_OK || !h->pg)
>   goto out;

How could we end up here without h->pg? Either way that should
move into a conditional together with the kref_get which should
become the not_zero variant if it is kept.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/23] scsi_dh_alua: Use separate alua_port_group structure

2015-09-01 Thread Hannes Reinecke 
On 09/01/2015 03:44 PM, Christoph Hellwig wrote:
> On Tue, Sep 01, 2015 at 03:02:29PM +0200, Hannes Reinecke wrote:
 +  rcu_read_lock();
 +  pg = rcu_dereference(h->pg);
 +  if (!pg) {
 +  rcu_read_unlock();
 +  return -ENXIO;
 +  }
>>>
>>> What is this rcu_read_lock supposed to protect against given
>>> that struct alua_port_group isn't RCU freed?  I think this needs
>>> to be another kref_get_unless_zero.
>>>
>> It's not freed now, but it'll be with one of the next patches (ie with
>> the 'rescan' patch).
>> I just kept it here for consistency, following the rule to always
>> enclose 'rcu_dereference' with 'rcu_read_lock()/rcu_read_unlock()'
>> pairs. Irrespective on whether it's being freed or not.
> 
> After this patch this is the only RCU call in the driver.  I don't
> think adding it here make any sense.
> 
Right, I'll be moving it to the rescan patch then.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 13/23] scsi_dh_alua: Use separate alua_port_group structure

2015-08-27 Thread Hannes Reinecke 
The port group needs to be a separate structure as several
LUNs might belong to the same group.

Signed-off-by: Hannes Reinecke h...@suse.de
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 211 +++--
 1 file changed, 139 insertions(+), 72 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
b/drivers/scsi/device_handler/scsi_dh_alua.c
index ef4363a..d1010dd 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -64,9 +64,13 @@
 #define ALUA_OPTIMIZE_STPG 1
 #define ALUA_RTPG_EXT_HDR_UNSUPP   2
 
-struct alua_dh_data {
+static LIST_HEAD(port_group_list);
+static DEFINE_SPINLOCK(port_group_lock);
+
+struct alua_port_group {
+   struct kref kref;
+   struct list_headnode;
int group_id;
-   int rel_port;
int tpgs;
int state;
int pref;
@@ -75,6 +79,12 @@ struct alua_dh_data {
unsigned char   *buff;
int bufflen;
unsigned char   transition_tmo;
+};
+
+struct alua_dh_data {
+   struct alua_port_group  *pg;
+   int rel_port;
+   int tpgs;
struct scsi_device  *sdev;
activate_complete   callback_fn;
void*callback_data;
@@ -86,21 +96,35 @@ struct alua_dh_data {
 static char print_alua_state(int);
 static int alua_check_sense(struct scsi_device *, struct scsi_sense_hdr *);
 
-static int realloc_buffer(struct alua_dh_data *h, unsigned len)
+static int realloc_buffer(struct alua_port_group *pg, unsigned len)
 {
-   if (h-buff  h-buff != h-inq)
-   kfree(h-buff);
+   if (pg-buff  pg-buff != pg-inq)
+   kfree(pg-buff);
 
-   h-buff = kmalloc(len, GFP_NOIO);
-   if (!h-buff) {
-   h-buff = h-inq;
-   h-bufflen = ALUA_INQUIRY_SIZE;
+   pg-buff = kmalloc(len, GFP_NOIO);
+   if (!pg-buff) {
+   pg-buff = pg-inq;
+   pg-bufflen = ALUA_INQUIRY_SIZE;
return 1;
}
-   h-bufflen = len;
+   pg-bufflen = len;
return 0;
 }
 
+static void release_port_group(struct kref *kref)
+{
+   struct alua_port_group *pg;
+
+   pg = container_of(kref, struct alua_port_group, kref);
+   printk(KERN_WARNING alua: release port group %d\n, pg-group_id);
+   spin_lock(port_group_lock);
+   list_del(pg-node);
+   spin_unlock(port_group_lock);
+   if (pg-buff  pg-inq != pg-buff)
+   kfree(pg-buff);
+   kfree(pg);
+}
+
 /*
  * submit_rtpg - Issue a REPORT TARGET GROUP STATES command
  * @sdev: sdev the command should be sent to
@@ -225,6 +249,8 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct 
alua_dh_data *h)
 static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 {
unsigned char *d;
+   int group_id = -1;
+   struct alua_port_group *pg = NULL;
 
if (!sdev-vpd_pg83)
return SCSI_DH_DEV_UNSUPP;
@@ -241,7 +267,7 @@ static int alua_check_vpd(struct scsi_device *sdev, struct 
alua_dh_data *h)
break;
case 0x5:
/* Target port group */
-   h-group_id = get_unaligned_be16(d[6]);
+   group_id = get_unaligned_be16(d[6]);
break;
default:
break;
@@ -249,7 +275,7 @@ static int alua_check_vpd(struct scsi_device *sdev, struct 
alua_dh_data *h)
d += d[3] + 4;
}
 
-   if (h-group_id == -1) {
+   if (group_id == -1) {
/*
 * Internal error; TPGS supported but required
 * VPD identification descriptors not present.
@@ -258,15 +284,33 @@ static int alua_check_vpd(struct scsi_device *sdev, 
struct alua_dh_data *h)
sdev_printk(KERN_INFO, sdev,
%s: No target port descriptors found\n,
ALUA_DH_NAME);
-   h-state = TPGS_STATE_OPTIMIZED;
h-tpgs = TPGS_MODE_NONE;
return SCSI_DH_DEV_UNSUPP;
}
sdev_printk(KERN_INFO, sdev,
%s: port group %02x rel port %02x\n,
-   ALUA_DH_NAME, h-group_id, h-rel_port);
+   ALUA_DH_NAME, group_id, h-rel_port);
 
-   return 0;
+   pg = kzalloc(sizeof(struct alua_port_group), GFP_KERNEL);
+   if (!pg) {
+   sdev_printk(KERN_WARNING, sdev,
+   %s: kzalloc port group failed\n,
+   ALUA_DH_NAME);
+   /* Temporary failure, bypass */
+   return SCSI_DH_DEV_TEMP_BUSY;
+   }
+   pg-group_id = group_id;
+   pg-buff = pg-inq;
+   pg-bufflen =