asynchronous ALUA device handler

PATCH In review Hannes Reinecke 3 participants 64 messages Sep 29, 2015 – Oct 13, 2015 SCSI

lore

Share & tools

lore

Hannes Reinecke

author

2015-09-29 10:47 UTC #0 lore raw

v5 cover asynchronous ALUA device handler

Hi all,

here the next round of my update to the ALUA device handler. The main features are:

- Topology discovery: the device handler creates a separate port_group structure, which is used to update all paths to the same port group. With that we achieve a significant reduction of the number of RTPGs.
- Asynchronous state update: The ALUA state is now updated from a workqueue item, so all concurrent RTPG calls are coaleasced. The ALUA state update is also triggered by sense codes indicating an ALUA state change.
- Use the existing vpd page 0x83 to detect device IDs
- Add generic 'access_state' attribute to discover the current ALUA state from userspace.

The entire tree can be found at

kernel/hare/scsi-devel branch alua.v5

on git.kernel.org

As usual, reviews and comments are welcome.

Changes to v1:
- Split off rtpg workqueue into separate items
- User per-port workqueues
- Incorporate review from Bart
- Incorporate review from hch
Changes to v2:
- Fixup patch descriptions
- Split off 'Improve error handling' patch
Changes to v3:
- Implement generic 'scsi_vpd_lun_id()' function to parse vpd page 0x83
Changes to v4:
- Revert back to a single workqueue item
- Implement generic 'scsi_vpd_tpg_id()' function
- Add 'access_state' attribute to struct scsi_device
- Incorporate review from Ewan Milne
- Incorporate further reviews from hch
- Reshuffle patches

Hannes Reinecke (36):
scsi_dh: move 'dh_state' sysfs attribute to generic code
scsi: ignore errors from scsi_dh_add_device()
scsi_dh_alua: Disable ALUA handling for non-disk devices
scsi_dh_alua: Use vpd_pg83 information
scsi_dh_alua: improved logging
scsi_dh_alua: sanitze sense code handling
scsi_dh_alua: use standard logging functions
scsi_dh_alua: return standard SCSI return codes in submit_rtpg
scsi_dh_alua: fixup description of stpg_endio()
scsi: remove scsi_show_sense_hdr()
scsi_dh_alua: use flag for RTPG extended header
scsi_dh_alua: use unaligned access macros
scsi_dh_alua: Pass buffer as function argument
scsi_dh_alua: separate out alua_stpg()
scsi_dh_alua: Make stpg synchronous
scsi_dh_alua: call alua_rtpg() if stpg fails
scsi_dh_alua: switch to scsi_execute_req_flags()
scsi_dh_alua: rework alua_check_tpgs() to return the tpgs mode
scsi_dh_alua: Use separate alua_port_group structure
scsi_dh_alua: allocate RTPG buffer separately
scsi_dh_alua: simplify sense code handling
scsi: Add scsi_vpd_lun_id()
scsi_dh_alua: use unique device id
scsi: Add scsi_vpd_tpg_id()
scsi_dh_alua: simplify alua_initialize()
revert "scsi_dh_alua: ALUA hander attach should succeed while TPG is
transitioning"
scsi_dh_alua: Use workqueue for RTPG
scsi_dh_alua: Recheck state on unit attention
scsi_dh_alua: update all port states
scsi_dh_alua: Send TEST UNIT READY to poll for transitioning
scsi: rescan VPD attributes
scsi_dh: add 'rescan' callback
scsi: Add 'access_state' attribute
scsi_dh_alua: use common definitions for ALUA state
scsi_dh_alua: update 'access_state' field
scsi_dh_alua: Update version to 2.0

 drivers/scsi/device_handler/scsi_dh_alua.c | 1173 +++++++++++++++++-----------
 drivers/scsi/scsi.c                        |   20 +-
 drivers/scsi/scsi_dh.c                     |   68 +-
 drivers/scsi/scsi_lib.c                    |  184 +++++
 drivers/scsi/scsi_scan.c                   |   11 +
 drivers/scsi/scsi_sysfs.c                  |  122 ++-
 drivers/scsi/ses.c                         |   13 +-
 include/scsi/scsi_dbg.h                    |    2 -
 include/scsi/scsi_device.h                 |    4 +
 include/scsi/scsi_dh.h                     |    2 +
 include/scsi/scsi_proto.h                  |   13 +
 11 files changed, 1081 insertions(+), 531 deletions(-)

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:47 UTC #1 lore raw

PATCH v5 1/36 scsi_dh: move 'dh_state' sysfs attribute to generic code

As scsi_dh.c is now always compiled in we should be moving the 'dh_state' attribute to the generic code.

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/scsi_dh.c    | 68 +----------------------------------------------
 drivers/scsi/scsi_sysfs.c | 58 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 67 deletions(-)

drivers/scsi/scsi_dh.c +1 ~~-67~~

index edb044a..1584080 100644
@@ -153,76 +153,11 @@ static void scsi_dh_handler_detach(struct scsi_device *sdev)
 	module_put(sdev->handler->module);
 }
 
-/*
- * Functions for sysfs attribute 'dh_state'
- */
-static ssize_t
-store_dh_state(struct device *dev, struct device_attribute *attr,
-	       const char *buf, size_t count)
-{
-	struct scsi_device *sdev = to_scsi_device(dev);
-	struct scsi_device_handler *scsi_dh;
-	int err = -EINVAL;
-
-	if (sdev->sdev_state == SDEV_CANCEL ||
-	    sdev->sdev_state == SDEV_DEL)
-		return -ENODEV;
-
-	if (!sdev->handler) {
-		/*
-		 * Attach to a device handler
-		 */
-		scsi_dh = scsi_dh_lookup(buf);
-		if (!scsi_dh)
-			return err;
-		err = scsi_dh_handler_attach(sdev, scsi_dh);
-	} else {
-		if (!strncmp(buf, "detach", 6)) {
-			/*
-			 * Detach from a device handler
-			 */
-			sdev_printk(KERN_WARNING, sdev,
-				    "can't detach handler %s.\n",
-				    sdev->handler->name);
-			err = -EINVAL;
-		} else if (!strncmp(buf, "activate", 8)) {
-			/*
-			 * Activate a device handler
-			 */
-			if (sdev->handler->activate)
-				err = sdev->handler->activate(sdev, NULL, NULL);
-			else
-				err = 0;
-		}
-	}
-
-	return err<0?err:count;
-}
-
-static ssize_t
-show_dh_state(struct device *dev, struct device_attribute *attr, char *buf)
-{
-	struct scsi_device *sdev = to_scsi_device(dev);
-
-	if (!sdev->handler)
-		return snprintf(buf, 20, "detached\n");
-
-	return snprintf(buf, 20, "%s\n", sdev->handler->name);
-}
-
-static struct device_attribute scsi_dh_state_attr =
-	__ATTR(dh_state, S_IRUGO | S_IWUSR, show_dh_state,
-	       store_dh_state);
-
 int scsi_dh_add_device(struct scsi_device *sdev)
 {
 	struct scsi_device_handler *devinfo = NULL;
 	const char *drv;
-	int err;
-
-	err = device_create_file(&sdev->sdev_gendev, &scsi_dh_state_attr);
-	if (err)
-		return err;
+	int err = 0;
 
 	drv = scsi_dh_find_driver(sdev);
 	if (drv)

@@ -236,7 +171,6 @@ void scsi_dh_remove_device(struct scsi_device *sdev)
 {
 	if (sdev->handler)
 		scsi_dh_handler_detach(sdev);
-	device_remove_file(&sdev->sdev_gendev, &scsi_dh_state_attr);
 }
 
 /*

drivers/scsi/scsi_sysfs.c +58 -0

76 lines not on this page · raw message

Signature · 1 line

1.8.5.6

Bart Van Assche

Reviewed-by on v5 1/36

2015-10-01 23:18 UTC #2 lore raw

Re: [PATCH 01/36] scsi_dh: move 'dh_state' sysfs attribute to generic code

Quoted 5 lines · Hannes Reinecke

As scsi_dh.c is now always compiled in we should be moving the 'dh_state' attribute to the generic code.
Reviewed-by Christoph Hellwig <[email protected]>
Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Bart Van Assche <[email protected]>

Hannes Reinecke

author

2015-09-29 10:47 UTC #3 lore raw

PATCH v5 2/36 scsi: ignore errors from scsi_dh_add_device()

In reply to Hannes Reinecke

device handler initialisation might fail due to a number of reasons. But as device_handlers are optional this shouldn't cause us to disable the device entirely.
So just ignore errors from scsi_dh_add_device().

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/scsi_sysfs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/scsi/scsi_sysfs.c +4 -3

index 5d64c3f..81d05ec 100644
@@ -1090,11 +1090,12 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 	}
 
 	error = scsi_dh_add_device(sdev);
-	if (error) {
+	if (error)
+		/*
+		 * device_handler is optional, so any error can be ignored
+		 */
 		sdev_printk(KERN_INFO, sdev,
 				"failed to add device handler: %d\n", error);
-		return error;
-	}
 
 	device_enable_async_suspend(&sdev->sdev_dev);
 	error = device_add(&sdev->sdev_dev);

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:47 UTC #4 lore raw

PATCH v5 3/36 scsi_dh_alua: Disable ALUA handling for non-disk devices

In reply to Hannes Reinecke

Non-disk devices might support ALUA, but the firmware implementation is untested and frequently broken. As we're don't actually need it disable ALUA support for non-disk device for now.

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Bart Van Assche <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Reviewed-by Martin K. Petersen <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/scsi/device_handler/scsi_dh_alua.c +12 -0

index cc2773b..7d01ef0 100644
@@ -320,6 +320,18 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
 {
 	int err = SCSI_DH_OK;
 
+	/*
+	 * ALUA support for non-disk devices is fraught with
+	 * difficulties, so disable it for now.
+	 */
+	if (sdev->type != TYPE_DISK) {
+		h->tpgs = TPGS_MODE_NONE;
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: disable for non-disk devices\n",
+			    ALUA_DH_NAME);
+		return SCSI_DH_DEV_UNSUPP;
+	}
+
 	h->tpgs = scsi_device_tpgs(sdev);
 	switch (h->tpgs) {
 	case TPGS_MODE_EXPLICIT|TPGS_MODE_IMPLICIT:

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:47 UTC #5 lore raw

PATCH v5 4/36 scsi_dh_alua: Use vpd_pg83 information

In reply to Hannes Reinecke

The SCSI device now has the VPD page 0x83 information attached, so there is no need to query it again.

Reviewed-by Martin K. Petersen <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 83 +++++-------------------------
 1 file changed, 13 insertions(+), 70 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +13 ~~-70~~

index 7d01ef0..f15b977 100644
@@ -131,43 +131,6 @@ static struct request *get_alua_req(struct scsi_device *sdev,
 }
 
 /*
- * submit_vpd_inquiry - Issue an INQUIRY VPD page 0x83 command
- * @sdev: sdev the command should be sent to
- */
-static int submit_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
-{
-	struct request *rq;
-	int err = SCSI_DH_RES_TEMP_UNAVAIL;
-
-	rq = get_alua_req(sdev, h->buff, h->bufflen, READ);
-	if (!rq)
-		goto done;
-
-	/* Prepare the command. */
-	rq->cmd[0] = INQUIRY;
-	rq->cmd[1] = 1;
-	rq->cmd[2] = 0x83;
-	rq->cmd[4] = h->bufflen;
-	rq->cmd_len = COMMAND_SIZE(INQUIRY);
-
-	rq->sense = h->sense;
-	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
-	rq->sense_len = h->senselen = 0;
-
-	err = blk_execute_rq(rq->q, NULL, rq, 1);
-	if (err == -EIO) {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: evpd inquiry failed with %x\n",
-			    ALUA_DH_NAME, rq->errors);
-		h->senselen = rq->sense_len;
-		err = SCSI_DH_IO;
-	}
-	blk_put_request(rq);
-done:
-	return err;
-}
-
-/*
  * submit_rtpg - Issue a REPORT TARGET GROUP STATES command
  * @sdev: sdev the command should be sent to
  */

@@ -359,43 +322,24 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
 }
 
 /*
- * alua_vpd_inquiry - Evaluate INQUIRY vpd page 0x83
+ * alua_check_vpd - Evaluate INQUIRY vpd page 0x83
  * @sdev: device to be checked
  *
  * Extract the relative target port and the target port group
  * descriptor from the list of identificators.
  */
-static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
+static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 {
-	int len;
-	unsigned err;
 	unsigned char *d;
 
- retry:
-	err = submit_vpd_inquiry(sdev, h);
-
-	if (err != SCSI_DH_OK)
-		return err;
-
-	/* Check if vpd page exceeds initial buffer */
-	len = (h->buff[2] << 8) + h->buff[3] + 4;
-	if (len > h->bufflen) {
-		/* Resubmit with the correct length */
-		if (realloc_buffer(h, len)) {
-			sdev_printk(KERN_WARNING, sdev,
-				    "%s: kmalloc buffer failed\n",
-				    ALUA_DH_NAME);
-			/* Temporary failure, bypass */
-			return SCSI_DH_DEV_TEMP_BUSY;
-		}
-		goto retry;
-	}
+	if (!sdev->vpd_pg83)
+		return SCSI_DH_DEV_UNSUPP;
 
 	/*
-	 * Now look for the correct descriptor.
+	 * Look for the correct descriptor.
 	 */
-	d = h->buff + 4;
-	while (d < h->buff + len) {
+	d = sdev->vpd_pg83 + 4;
+	while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
 		switch (d[1] & 0xf) {
 		case 0x4:
 			/* Relative target port */

@@ -422,14 +366,13 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 			    ALUA_DH_NAME);
 		h->state = TPGS_STATE_OPTIMIZED;
 		h->tpgs = TPGS_MODE_NONE;
-		err = SCSI_DH_DEV_UNSUPP;
-	} else {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: port group %02x rel port %02x\n",
-			    ALUA_DH_NAME, h->group_id, h->rel_port);
+		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);
 
-	return err;
+	return 0;
 }
 
 static char print_alua_state(int state)

@@ -692,7 +635,7 @@ static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 	if (err != SCSI_DH_OK)
 		goto out;
 
-	err = alua_vpd_inquiry(sdev, h);
+	err = alua_check_vpd(sdev, h);
 	if (err != SCSI_DH_OK)
 		goto out;

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:47 UTC #6 lore raw

PATCH v5 5/36 scsi_dh_alua: improved logging

In reply to Hannes Reinecke

Issue different logging messages if ALUA is not supported or the TPGS setting is invalid.

Reviewed-by Martin K. Petersen <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +8 -2

index f15b977..a20c8bf 100644
@@ -310,12 +310,18 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
 		sdev_printk(KERN_INFO, sdev, "%s: supports implicit TPGS\n",
 			    ALUA_DH_NAME);
 		break;
-	default:
-		h->tpgs = TPGS_MODE_NONE;
+	case TPGS_MODE_NONE:
 		sdev_printk(KERN_INFO, sdev, "%s: not supported\n",
 			    ALUA_DH_NAME);
 		err = SCSI_DH_DEV_UNSUPP;
 		break;
+	default:
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: unsupported TPGS setting %d\n",
+			    ALUA_DH_NAME, h->tpgs);
+		h->tpgs = TPGS_MODE_NONE;
+		err = SCSI_DH_DEV_UNSUPP;
+		break;
 	}
 
 	return err;

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:47 UTC #7 lore raw

PATCH v5 6/36 scsi_dh_alua: sanitze sense code handling

In reply to Hannes Reinecke

The only check for a valid sense code is calling scsi_normalize_sense() and check the return value. So drop the pointless checks and rely on scsi_normalize_sense() to figure out if the sense code is valid. With that we can also remove the 'senselen' field.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +7 ~~-11~~

index a20c8bf..5334bba 100644
@@ -73,7 +73,6 @@ struct alua_dh_data {
 	int			bufflen;
 	unsigned char		transition_tmo;
 	unsigned char		sense[SCSI_SENSE_BUFFERSIZE];
-	int			senselen;
 	struct scsi_device	*sdev;
 	activate_complete	callback_fn;
 	void			*callback_data;

@@ -158,14 +157,13 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
 
 	rq->sense = h->sense;
 	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
-	rq->sense_len = h->senselen = 0;
+	rq->sense_len = 0;
 
 	err = blk_execute_rq(rq->q, NULL, rq, 1);
 	if (err == -EIO) {
 		sdev_printk(KERN_INFO, sdev,
 			    "%s: rtpg failed with %x\n",
 			    ALUA_DH_NAME, rq->errors);
-		h->senselen = rq->sense_len;
 		err = SCSI_DH_IO;
 	}
 	blk_put_request(rq);

@@ -194,9 +192,8 @@ static void stpg_endio(struct request *req, int error)
 		goto done;
 	}
 
-	if (req->sense_len > 0) {
-		err = scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
-					   &sense_hdr);
+	if (scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
+				 &sense_hdr)) {
 		if (!err) {
 			err = SCSI_DH_IO;
 			goto done;

@@ -265,7 +262,7 @@ static unsigned submit_stpg(struct alua_dh_data *h)
 
 	rq->sense = h->sense;
 	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
-	rq->sense_len = h->senselen = 0;
+	rq->sense_len = 0;
 	rq->end_io_data = h;
 
 	blk_execute_rq_nowait(rq->q, NULL, rq, 1, stpg_endio);

@@ -508,10 +505,9 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
  retry:
 	err = submit_rtpg(sdev, h, rtpg_ext_hdr_req);
 
-	if (err == SCSI_DH_IO && h->senselen > 0) {
-		err = scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
-					   &sense_hdr);
-		if (!err)
+	if (err == SCSI_DH_IO) {
+		if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
+					  &sense_hdr))
 			return SCSI_DH_IO;
 
 		/*

Signature · 1 line

1.8.5.6

Bart Van Assche

Reviewed-by on v5 6/36

2015-10-01 23:39 UTC #8 lore raw

Re: [PATCH 06/36] scsi_dh_alua: sanitze sense code handling

Quoted 6 lines · Hannes Reinecke

The only check for a valid sense code is calling scsi_normalize_sense() and check the return value. So drop the pointless checks and rely on scsi_normalize_sense() to figure out if the sense code is valid. With that we can also remove the 'senselen' field.
Signed-off-by Hannes Reinecke<[email protected]>

Reviewed-by Bart Van Assche <[email protected]>

Hannes Reinecke

author

2015-09-29 10:47 UTC #9 lore raw

PATCH v5 7/36 scsi_dh_alua: use standard logging functions

In reply to Hannes Reinecke

Use standard logging functions instead of hand-crafted ones.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +13 ~~-14~~

index 5334bba..018faf0 100644
@@ -23,6 +23,7 @@
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <scsi/scsi.h>
+#include <scsi/scsi_dbg.h>
 #include <scsi/scsi_eh.h>
 #include <scsi/scsi_dh.h>

@@ -194,19 +195,14 @@ static void stpg_endio(struct request *req, int error)
 
 	if (scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
 				 &sense_hdr)) {
-		if (!err) {
-			err = SCSI_DH_IO;
-			goto done;
-		}
 		err = alua_check_sense(h->sdev, &sense_hdr);
 		if (err == ADD_TO_MLQUEUE) {
 			err = SCSI_DH_RETRY;
 			goto done;
 		}
-		sdev_printk(KERN_INFO, h->sdev,
-			    "%s: stpg sense code: %02x/%02x/%02x\n",
-			    ALUA_DH_NAME, sense_hdr.sense_key,
-			    sense_hdr.asc, sense_hdr.ascq);
+		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n",
+			    ALUA_DH_NAME);
+		scsi_print_sense_hdr(h->sdev, ALUA_DH_NAME, &sense_hdr);
 		err = SCSI_DH_IO;
 	} else if (error)
 		err = SCSI_DH_IO;

@@ -526,13 +522,16 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		}
 
 		err = alua_check_sense(sdev, &sense_hdr);
-		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry))
+		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry)) {
+			sdev_printk(KERN_ERR, sdev, "%s: rtpg retry\n",
+				    ALUA_DH_NAME);
+			scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
 			goto retry;
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: rtpg sense code %02x/%02x/%02x\n",
-			    ALUA_DH_NAME, sense_hdr.sense_key,
-			    sense_hdr.asc, sense_hdr.ascq);
-		err = SCSI_DH_IO;
+		}
+		sdev_printk(KERN_ERR, sdev, "%s: rtpg failed\n",
+			    ALUA_DH_NAME);
+		scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
+		return SCSI_DH_IO;
 	}
 	if (err != SCSI_DH_OK)
 		return err;

Signature · 1 line

1.8.5.6

Bart Van Assche

2015-10-02 00:04 UTC #10 lore raw

Re: [PATCH 07/36] scsi_dh_alua: use standard logging functions

drivers/scsi/device_handler/scsi_dh_alua.c static void stpg_endio(struct request *req, int error) · line 200

Quoted 4 lines · Hannes Reinecke

drivers/scsi/device_handler/scsi_dh_alua.c +0 -4
-		if (!err) {
-			err = SCSI_DH_IO;
-			goto done;
-		}

The assignment to 'err' was removed in patch 06/36 so this part should be moved to patch 06/36. Otherwise this patch looks fine to me.

Bart.

Hannes Reinecke

author

2015-09-29 10:47 UTC #11 lore raw

PATCH v5 8/36 scsi_dh_alua: return standard SCSI return codes in submit_rtpg

In reply to Hannes Reinecke

Fixup submit_rtpg() to always return a standard SCSI return code.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 33 +++++++++++++++---------------
 1 file changed, 17 insertions(+), 16 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +17 ~~-16~~

index 018faf0..d4e5691 100644
@@ -138,11 +138,13 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
 			    bool rtpg_ext_hdr_req)
 {
 	struct request *rq;
-	int err = SCSI_DH_RES_TEMP_UNAVAIL;
+	int err = 0;
 
 	rq = get_alua_req(sdev, h->buff, h->bufflen, READ);
-	if (!rq)
+	if (!rq) {
+		err = DRIVER_BUSY << 24;
 		goto done;
+	}
 
 	/* Prepare the command. */
 	rq->cmd[0] = MAINTENANCE_IN;

@@ -160,13 +162,9 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
 	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
 	rq->sense_len = 0;
 
-	err = blk_execute_rq(rq->q, NULL, rq, 1);
-	if (err == -EIO) {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: rtpg failed with %x\n",
-			    ALUA_DH_NAME, rq->errors);
-		err = SCSI_DH_IO;
-	}
+	blk_execute_rq(rq->q, NULL, rq, 1);
+	if (rq->errors)
+		err = rq->errors;
 	blk_put_request(rq);
 done:
 	return err;

@@ -487,7 +485,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 	struct scsi_sense_hdr sense_hdr;
 	int len, k, off, valid_states = 0;
 	unsigned char *ucp;
-	unsigned err;
+	unsigned err, retval;
 	bool rtpg_ext_hdr_req = 1;
 	unsigned long expiry, interval = 0;
 	unsigned int tpg_desc_tbl_off;

@@ -499,12 +497,17 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		expiry = round_jiffies_up(jiffies + h->transition_tmo * HZ);
 
  retry:
-	err = submit_rtpg(sdev, h, rtpg_ext_hdr_req);
-
-	if (err == SCSI_DH_IO) {
+	retval = submit_rtpg(sdev, h, rtpg_ext_hdr_req);
+	if (retval) {
 		if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
-					  &sense_hdr))
+					  &sense_hdr)) {
+			sdev_printk(KERN_INFO, sdev,
+				    "%s: rtpg failed, result %d\n",
+				    ALUA_DH_NAME, retval);
+			if (driver_byte(retval) == DRIVER_BUSY)
+				return SCSI_DH_DEV_TEMP_BUSY;
 			return SCSI_DH_IO;
+		}
 
 		/*
 		 * submit_rtpg() has failed on existing arrays

@@ -533,8 +536,6 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
 		return SCSI_DH_IO;
 	}
-	if (err != SCSI_DH_OK)
-		return err;
 
 	len = (h->buff[0] << 24) + (h->buff[1] << 16) +
 		(h->buff[2] << 8) + h->buff[3] + 4;

Signature · 1 line

1.8.5.6

Bart Van Assche

2015-10-01 23:58 UTC #12 lore raw

Re: [PATCH 08/36] scsi_dh_alua: return standard SCSI return codes in submit_rtpg

drivers/scsi/device_handler/scsi_dh_alua.c static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, · line 167

Quoted 3 lines · Hannes Reinecke

drivers/scsi/device_handler/scsi_dh_alua.c +3 -0
+	blk_execute_rq(rq->q, NULL, rq, 1);
+	if (rq->errors)
+		err = rq->errors;

Not all code in the block layer uses the SCSI error codes. Do we need code to convert negative error codes into a SCSI error code here ? An example:

void blk_mq_abort_requeue_list(struct request_queue *q)

{
        [ ... ]
		rq->errors = -EIO;
        [ ... ]
}

EXPORT_SYMBOL(blk_mq_abort_requeue_list);

Bart.

Hannes Reinecke

author

2015-10-02 06:03 UTC #13 lore raw

Re: [PATCH 08/36] scsi_dh_alua: return standard SCSI return codes in submit_rtpg

Quoted 9 lines · Bart Van Assche

drivers/scsi/device_handler/scsi_dh_alua.c static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, · line 167
Quoted 3 lines · Hannes Reinecke
drivers/scsi/device_handler/scsi_dh_alua.c +3 -0
+	blk_execute_rq(rq->q, NULL, rq, 1);
+	if (rq->errors)
+		err = rq->errors;
Not all code in the block layer uses the SCSI error codes. Do we need code to convert negative error codes into a SCSI error code here ? An example:

Quoted 8 lines

void blk_mq_abort_requeue_list(struct request_queue *q)
{
        [ ... ]
		rq->errors = -EIO;
        [ ... ]
}
EXPORT_SYMBOL(blk_mq_abort_requeue_list);

Sigh. In principle, yes. In practise, however, there are plenty of other instances where the SCSI core assumes rq->errors to be holding a valid SCSI error code.
And as the device handler is plain SCSI I guess we're fine here.

Separating SCSI error codes from block-layer error codes would be a nice topic for LSF ...

Cheers,

Hannes

Signature · 9 lines

Dr. Hannes Reinecke		               zSeries & Storage
[email protected]			               +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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hannes Reinecke

author

2015-09-29 10:47 UTC #14 lore raw

PATCH v5 9/36 scsi_dh_alua: fixup description of stpg_endio()

In reply to Hannes Reinecke

Fixup copy-and-paste error in the description of stpg_endio().

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Reviewed-by Martin K. Petersen <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +2 -4

index d4e5691..4ff9b90 100644
@@ -171,13 +171,11 @@ done:
 }
 
 /*
- * alua_stpg - Evaluate SET TARGET GROUP STATES
+ * stpg_endio - Evaluate SET TARGET GROUP STATES
  * @sdev: the device to be evaluated
  * @state: the new target group state
  *
- * Send a SET TARGET GROUP STATES command to the device.
- * We only have to test here if we should resubmit the command;
- * any other error is assumed as a failure.
+ * Evaluate a SET TARGET GROUP STATES command response.
  */
 static void stpg_endio(struct request *req, int error)
 {

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:47 UTC #15 lore raw

PATCH v5 10/36 scsi: remove scsi_show_sense_hdr()

In reply to Hannes Reinecke

Last caller is gone, so remove it.

Reviewed-by Bart Van Assche <[email protected]>

Reviewed-by Reviewed-by: Christoph Hellwig <[email protected]>

Reviewed-by Martin K. Petersen <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 include/scsi/scsi_dbg.h | 2 --
 1 file changed, 2 deletions(-)

include/scsi/scsi_dbg.h +0 -2

index f8170e9..56710e0 100644
@@ -12,8 +12,6 @@ extern size_t __scsi_format_command(char *, size_t,
 				   const unsigned char *, size_t);
 extern void scsi_show_extd_sense(const struct scsi_device *, const char *,
 				 unsigned char, unsigned char);
-extern void scsi_show_sense_hdr(const struct scsi_device *, const char *,
-				const struct scsi_sense_hdr *);
 extern void scsi_print_sense_hdr(const struct scsi_device *, const char *,
 				 const struct scsi_sense_hdr *);
 extern void scsi_print_sense(const struct scsi_cmnd *);

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:47 UTC #16 lore raw

PATCH v5 11/36 scsi_dh_alua: use flag for RTPG extended header

In reply to Hannes Reinecke

We should be using a flag when RTPG extended header is not supported, that saves us sending RTPG twice for older arrays.

Reviewed-by Bart Van Assche <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Reviewed-by Martin K. Petersen <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +7 -8

index 4ff9b90..a963f51 100644
@@ -59,8 +59,9 @@
 #define ALUA_FAILOVER_TIMEOUT		60
 #define ALUA_FAILOVER_RETRIES		5
 
-/* flags passed from user level */
+/* device handler flags */
 #define ALUA_OPTIMIZE_STPG		1
+#define ALUA_RTPG_EXT_HDR_UNSUPP	2
 
 struct alua_dh_data {
 	int			group_id;

@@ -134,8 +135,7 @@ static struct request *get_alua_req(struct scsi_device *sdev,
  * submit_rtpg - Issue a REPORT TARGET GROUP STATES command
  * @sdev: sdev the command should be sent to
  */
-static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
-			    bool rtpg_ext_hdr_req)
+static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 {
 	struct request *rq;
 	int err = 0;

@@ -148,7 +148,7 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
 
 	/* Prepare the command. */
 	rq->cmd[0] = MAINTENANCE_IN;
-	if (rtpg_ext_hdr_req)
+	if (!(h->flags & ALUA_RTPG_EXT_HDR_UNSUPP))
 		rq->cmd[1] = MI_REPORT_TARGET_PGS | MI_EXT_HDR_PARAM_FMT;
 	else
 		rq->cmd[1] = MI_REPORT_TARGET_PGS;

@@ -484,7 +484,6 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 	int len, k, off, valid_states = 0;
 	unsigned char *ucp;
 	unsigned err, retval;
-	bool rtpg_ext_hdr_req = 1;
 	unsigned long expiry, interval = 0;
 	unsigned int tpg_desc_tbl_off;
 	unsigned char orig_transition_tmo;

@@ -495,7 +494,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		expiry = round_jiffies_up(jiffies + h->transition_tmo * HZ);
 
  retry:
-	retval = submit_rtpg(sdev, h, rtpg_ext_hdr_req);
+	retval = submit_rtpg(sdev, h);
 	if (retval) {
 		if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
 					  &sense_hdr)) {

@@ -515,10 +514,10 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		 * The retry without rtpg_ext_hdr_req set
 		 * handles this.
 		 */
-		if (rtpg_ext_hdr_req == 1 &&
+		if (!(h->flags & ALUA_RTPG_EXT_HDR_UNSUPP) &&
 		    sense_hdr.sense_key == ILLEGAL_REQUEST &&
 		    sense_hdr.asc == 0x24 && sense_hdr.ascq == 0) {
-			rtpg_ext_hdr_req = 0;
+			h->flags |= ALUA_RTPG_EXT_HDR_UNSUPP;
 			goto retry;
 		}

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:47 UTC #17 lore raw

PATCH v5 12/36 scsi_dh_alua: use unaligned access macros

In reply to Hannes Reinecke

Use 'get_unaligned_XX' and 'put_unaligned_XX' instead of open-coding it.

Reviewed-by Martin K. Petersen <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +8 ~~-15~~

index a963f51..8571e57 100644
@@ -22,6 +22,7 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <asm/unaligned.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_dbg.h>
 #include <scsi/scsi_eh.h>

@@ -152,10 +153,7 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 		rq->cmd[1] = MI_REPORT_TARGET_PGS | MI_EXT_HDR_PARAM_FMT;
 	else
 		rq->cmd[1] = MI_REPORT_TARGET_PGS;
-	rq->cmd[6] = (h->bufflen >> 24) & 0xff;
-	rq->cmd[7] = (h->bufflen >> 16) & 0xff;
-	rq->cmd[8] = (h->bufflen >>  8) & 0xff;
-	rq->cmd[9] = h->bufflen & 0xff;
+	put_unaligned_be32(h->bufflen, &rq->cmd[6]);
 	rq->cmd_len = COMMAND_SIZE(MAINTENANCE_IN);
 
 	rq->sense = h->sense;

@@ -236,8 +234,7 @@ static unsigned submit_stpg(struct alua_dh_data *h)
 	/* Prepare the data buffer */
 	memset(h->buff, 0, stpg_len);
 	h->buff[4] = TPGS_STATE_OPTIMIZED & 0x0f;
-	h->buff[6] = (h->group_id >> 8) & 0xff;
-	h->buff[7] = h->group_id & 0xff;
+	put_unaligned_be16(h->group_id, &h->buff[6]);
 
 	rq = get_alua_req(sdev, h->buff, stpg_len, WRITE);
 	if (!rq)

@@ -246,10 +243,7 @@ static unsigned submit_stpg(struct alua_dh_data *h)
 	/* Prepare the command. */
 	rq->cmd[0] = MAINTENANCE_OUT;
 	rq->cmd[1] = MO_SET_TARGET_PGS;
-	rq->cmd[6] = (stpg_len >> 24) & 0xff;
-	rq->cmd[7] = (stpg_len >> 16) & 0xff;
-	rq->cmd[8] = (stpg_len >>  8) & 0xff;
-	rq->cmd[9] = stpg_len & 0xff;
+	put_unaligned_be32(stpg_len, &rq->cmd[6]);
 	rq->cmd_len = COMMAND_SIZE(MAINTENANCE_OUT);
 
 	rq->sense = h->sense;

@@ -338,11 +332,11 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 		switch (d[1] & 0xf) {
 		case 0x4:
 			/* Relative target port */
-			h->rel_port = (d[6] << 8) + d[7];
+			h->rel_port = get_unaligned_be16(&d[6]);
 			break;
 		case 0x5:
 			/* Target port group */
-			h->group_id = (d[6] << 8) + d[7];
+			h->group_id = get_unaligned_be16(&d[6]);
 			break;
 		default:
 			break;

@@ -534,8 +528,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		return SCSI_DH_IO;
 	}
 
-	len = (h->buff[0] << 24) + (h->buff[1] << 16) +
-		(h->buff[2] << 8) + h->buff[3] + 4;
+	len = get_unaligned_be32(&h->buff[0]) + 4;
 
 	if (len > h->bufflen) {
 		/* Resubmit with the correct length */

@@ -570,7 +563,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 	     k < len;
 	     k += off, ucp += off) {
 
-		if (h->group_id == (ucp[2] << 8) + ucp[3]) {
+		if (h->group_id == get_unaligned_be16(&ucp[2])) {
 			h->state = ucp[0] & 0x0f;
 			h->pref = ucp[0] >> 7;
 			valid_states = ucp[1];

Signature · 1 line

1.8.5.6

Bart Van Assche

Reviewed-by on v5 12/36

2015-10-02 00:01 UTC #18 lore raw

Re: [PATCH 12/36] scsi_dh_alua: use unaligned access macros

Quoted 6 lines · Hannes Reinecke

Use 'get_unaligned_XX' and 'put_unaligned_XX' instead of open-coding it.
Reviewed-by Martin K. Petersen <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Bart Van Assche <[email protected]>

Hannes Reinecke

author

2015-09-29 10:47 UTC #19 lore raw

PATCH v5 13/36 scsi_dh_alua: Pass buffer as function argument

In reply to Hannes Reinecke

Pass in the buffer as a function argument for submit_rtpg().

Reviewed-by Martin K. Petersen <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Reviewed-by Bart Van Assche <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +7 -6

index 8571e57..63423b2 100644
@@ -136,12 +136,13 @@ static struct request *get_alua_req(struct scsi_device *sdev,
  * submit_rtpg - Issue a REPORT TARGET GROUP STATES command
  * @sdev: sdev the command should be sent to
  */
-static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
+static unsigned submit_rtpg(struct scsi_device *sdev, unsigned char *buff,
+			    int bufflen, unsigned char *sense, int flags)
 {
 	struct request *rq;
 	int err = 0;
 
-	rq = get_alua_req(sdev, h->buff, h->bufflen, READ);
+	rq = get_alua_req(sdev, buff, bufflen, READ);
 	if (!rq) {
 		err = DRIVER_BUSY << 24;
 		goto done;

@@ -149,14 +150,14 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 
 	/* Prepare the command. */
 	rq->cmd[0] = MAINTENANCE_IN;
-	if (!(h->flags & ALUA_RTPG_EXT_HDR_UNSUPP))
+	if (!(flags & ALUA_RTPG_EXT_HDR_UNSUPP))
 		rq->cmd[1] = MI_REPORT_TARGET_PGS | MI_EXT_HDR_PARAM_FMT;
 	else
 		rq->cmd[1] = MI_REPORT_TARGET_PGS;
-	put_unaligned_be32(h->bufflen, &rq->cmd[6]);
+	put_unaligned_be32(bufflen, &rq->cmd[6]);
 	rq->cmd_len = COMMAND_SIZE(MAINTENANCE_IN);
 
-	rq->sense = h->sense;
+	rq->sense = sense;
 	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
 	rq->sense_len = 0;

@@ -488,7 +489,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		expiry = round_jiffies_up(jiffies + h->transition_tmo * HZ);
 
  retry:
-	retval = submit_rtpg(sdev, h);
+	retval = submit_rtpg(sdev, h->buff, h->bufflen, h->sense, h->flags);
 	if (retval) {
 		if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
 					  &sense_hdr)) {

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:47 UTC #20 lore raw

PATCH v5 14/36 scsi_dh_alua: separate out alua_stpg()

In reply to Hannes Reinecke

Seperate out SET TARGET PORT GROUP functionality into a separate function alua_stpg().

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 86 ++++++++++++++++++------------
 1 file changed, 52 insertions(+), 34 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +52 ~~-34~~

index 63423b2..59fe3e9 100644
@@ -614,6 +614,56 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 }
 
 /*
+ * alua_stpg - Issue a SET TARGET GROUP STATES command
+ *
+ * Issue a SET TARGET GROUP STATES command and evaluate the
+ * response. Returns SCSI_DH_RETRY per default to trigger
+ * a re-evaluation of the target group state.
+ */
+static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h,
+			  activate_complete fn, void *data)
+{
+	int err = SCSI_DH_OK;
+	int stpg = 0;
+
+	if (!(h->tpgs & TPGS_MODE_EXPLICIT)) {
+		/* Only implicit ALUA supported */
+		return err;
+	}
+
+	switch (h->state) {
+	case TPGS_STATE_NONOPTIMIZED:
+		stpg = 1;
+		if ((h->flags & ALUA_OPTIMIZE_STPG) &&
+		    !h->pref &&
+		    (h->tpgs & TPGS_MODE_IMPLICIT))
+			stpg = 0;
+		break;
+	case TPGS_STATE_STANDBY:
+	case TPGS_STATE_UNAVAILABLE:
+		stpg = 1;
+		break;
+	case TPGS_STATE_OFFLINE:
+		err = SCSI_DH_IO;
+		break;
+	case TPGS_STATE_TRANSITIONING:
+		err = SCSI_DH_RETRY;
+		break;
+	default:
+		break;
+	}
+
+	if (stpg) {
+		h->callback_fn = fn;
+		h->callback_data = data;
+		err = submit_stpg(h);
+		if (err != SCSI_DH_OK)
+			h->callback_fn = h->callback_data = NULL;
+	}
+	return err;
+}
+
+/*
  * alua_initialize - Initialize ALUA state
  * @sdev: the device to be initialized
  *

@@ -690,7 +740,6 @@ static int alua_activate(struct scsi_device *sdev,
 {
 	struct alua_dh_data *h = sdev->handler_data;
 	int err = SCSI_DH_OK;
-	int stpg = 0;
 
 	err = alua_rtpg(sdev, h, 1);
 	if (err != SCSI_DH_OK)

@@ -699,41 +748,10 @@ static int alua_activate(struct scsi_device *sdev,
 	if (optimize_stpg)
 		h->flags |= ALUA_OPTIMIZE_STPG;
 
-	if (h->tpgs & TPGS_MODE_EXPLICIT) {
-		switch (h->state) {
-		case TPGS_STATE_NONOPTIMIZED:
-			stpg = 1;
-			if ((h->flags & ALUA_OPTIMIZE_STPG) &&
-			    (!h->pref) &&
-			    (h->tpgs & TPGS_MODE_IMPLICIT))
-				stpg = 0;
-			break;
-		case TPGS_STATE_STANDBY:
-		case TPGS_STATE_UNAVAILABLE:
-			stpg = 1;
-			break;
-		case TPGS_STATE_OFFLINE:
-			err = SCSI_DH_IO;
-			break;
-		case TPGS_STATE_TRANSITIONING:
-			err = SCSI_DH_RETRY;
-			break;
-		default:
-			break;
-		}
-	}
-
-	if (stpg) {
-		h->callback_fn = fn;
-		h->callback_data = data;
-		err = submit_stpg(h);
-		if (err == SCSI_DH_OK)
-			return 0;
-		h->callback_fn = h->callback_data = NULL;
-	}
+	err = alua_stpg(sdev, h, fn, data);
 
 out:
-	if (fn)
+	if (err != SCSI_DH_OK && fn)
 		fn(data, err);
 	return 0;
 }

Signature · 1 line

1.8.5.6

Bart Van Assche

2015-10-02 00:07 UTC #21 lore raw

Re: [PATCH 14/36] scsi_dh_alua: separate out alua_stpg()

Quoted 1 line · Hannes Reinecke

Seperate out SET TARGET PORT GROUP functionality into a separate

^ Separate ?

Quoted 12 lines

function alua_stpg().
Signed-off-by Hannes Reinecke <[email protected]>
---
  drivers/scsi/device_handler/scsi_dh_alua.c | 86 ++++++++++++++++++------------
  1 file changed, 52 insertions(+), 34 deletions(-)
drivers/scsi/device_handler/scsi_dh_alua.c +0 -0
index 63423b2..59fe3e9 100644

drivers/scsi/device_handler/scsi_dh_alua.c static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_ · line 622

Quoted 9 lines

drivers/scsi/device_handler/scsi_dh_alua.c +6 -0

@@ -614,6 +614,56 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
  }
 
  /*
+ * alua_stpg - Issue a SET TARGET GROUP STATES command
+ *
+ * Issue a SET TARGET GROUP STATES command and evaluate the
+ * response. Returns SCSI_DH_RETRY per default to trigger
+ * a re-evaluation of the target group state.
+ */

In SPC-4 I found the following description next to the STPG command: "SET TARGET PORT GROUPS". How about using the official SPC-4 description ?

Quoted 90 lines

drivers/scsi/device_handler/scsi_dh_alua.c +44 ~~-31~~

+static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h,
+			  activate_complete fn, void *data)
+{
+	int err = SCSI_DH_OK;
+	int stpg = 0;
+
+	if (!(h->tpgs & TPGS_MODE_EXPLICIT)) {
+		/* Only implicit ALUA supported */
+		return err;
+	}
+
+	switch (h->state) {
+	case TPGS_STATE_NONOPTIMIZED:
+		stpg = 1;
+		if ((h->flags & ALUA_OPTIMIZE_STPG) &&
+		    !h->pref &&
+		    (h->tpgs & TPGS_MODE_IMPLICIT))
+			stpg = 0;
+		break;
+	case TPGS_STATE_STANDBY:
+	case TPGS_STATE_UNAVAILABLE:
+		stpg = 1;
+		break;
+	case TPGS_STATE_OFFLINE:
+		err = SCSI_DH_IO;
+		break;
+	case TPGS_STATE_TRANSITIONING:
+		err = SCSI_DH_RETRY;
+		break;
+	default:
+		break;
+	}
+
+	if (stpg) {
+		h->callback_fn = fn;
+		h->callback_data = data;
+		err = submit_stpg(h);
+		if (err != SCSI_DH_OK)
+			h->callback_fn = h->callback_data = NULL;
+	}
+	return err;
+}
+
+/*
   * alua_initialize - Initialize ALUA state
   * @sdev: the device to be initialized
   *

@@ -690,7 +740,6 @@ static int alua_activate(struct scsi_device *sdev,
  {
  	struct alua_dh_data *h = sdev->handler_data;
  	int err = SCSI_DH_OK;
-	int stpg = 0;
 
  	err = alua_rtpg(sdev, h, 1);
  	if (err != SCSI_DH_OK)

@@ -699,41 +748,10 @@ static int alua_activate(struct scsi_device *sdev,
  	if (optimize_stpg)
  		h->flags |= ALUA_OPTIMIZE_STPG;
 
-	if (h->tpgs & TPGS_MODE_EXPLICIT) {
-		switch (h->state) {
-		case TPGS_STATE_NONOPTIMIZED:
-			stpg = 1;
-			if ((h->flags & ALUA_OPTIMIZE_STPG) &&
-			    (!h->pref) &&
-			    (h->tpgs & TPGS_MODE_IMPLICIT))
-				stpg = 0;
-			break;
-		case TPGS_STATE_STANDBY:
-		case TPGS_STATE_UNAVAILABLE:
-			stpg = 1;
-			break;
-		case TPGS_STATE_OFFLINE:
-			err = SCSI_DH_IO;
-			break;
-		case TPGS_STATE_TRANSITIONING:
-			err = SCSI_DH_RETRY;
-			break;
-		default:
-			break;
-		}
-	}
-
-	if (stpg) {
-		h->callback_fn = fn;
-		h->callback_data = data;
-		err = submit_stpg(h);
-		if (err == SCSI_DH_OK)
-			return 0;

drivers/scsi/device_handler/scsi_dh_alua.c static int alua_activate(struct scsi_device *sdev, · line 757

Quoted 10 lines

drivers/scsi/device_handler/scsi_dh_alua.c +2 -3

-		h->callback_fn = h->callback_data = NULL;
-	}
+	err = alua_stpg(sdev, h, fn, data);
 
  out:
-	if (fn)
+	if (err != SCSI_DH_OK && fn)
  		fn(data, err);
  	return 0;
  }

The old implementation calls the callback function 'fn' if the flag TPGS_MODE_EXPLICIT has not been set but the new implementation not. Please mention this in the patch description if this behavior change is intentional.

Bart.

Hannes Reinecke

author

2015-10-02 06:06 UTC #22 lore raw

Re: [PATCH 14/36] scsi_dh_alua: separate out alua_stpg()

Quoted 128 lines · Bart Van Assche

Quoted 1 line · Hannes Reinecke

Seperate out SET TARGET PORT GROUP functionality into a separate

^ Separate ?

Quoted 14 lines

function alua_stpg().
Signed-off-by Hannes Reinecke <[email protected]>
---
  drivers/scsi/device_handler/scsi_dh_alua.c | 86
++++++++++++++++++------------
  1 file changed, 52 insertions(+), 34 deletions(-)
drivers/scsi/device_handler/scsi_dh_alua.c +0 -0
index 63423b2..59fe3e9 100644

drivers/scsi/device_handler/scsi_dh_alua.c static int alua_rtpg(struct scsi_device · line 623

Quoted 10 lines

drivers/scsi/device_handler/scsi_dh_alua.c +6 -0

@@ -614,6 +614,56 @@ static int alua_rtpg(struct scsi_device
 *sdev, struct alua_dh_data *h, int wait_
  }
 
  /*
+ * alua_stpg - Issue a SET TARGET GROUP STATES command
+ *
+ * Issue a SET TARGET GROUP STATES command and evaluate the
+ * response. Returns SCSI_DH_RETRY per default to trigger
+ * a re-evaluation of the target group state.
+ */

In SPC-4 I found the following description next to the STPG command:
"SET TARGET PORT GROUPS". How about using the official SPC-4 description ?

Quoted 96 lines

drivers/scsi/device_handler/scsi_dh_alua.c +44 -0

+static unsigned alua_stpg(struct scsi_device *sdev, struct
 alua_dh_data *h,
+              activate_complete fn, void *data)
+{
+    int err = SCSI_DH_OK;
+    int stpg = 0;
+
+    if (!(h->tpgs & TPGS_MODE_EXPLICIT)) {
+        /* Only implicit ALUA supported */
+        return err;
+    }
+
+    switch (h->state) {
+    case TPGS_STATE_NONOPTIMIZED:
+        stpg = 1;
+        if ((h->flags & ALUA_OPTIMIZE_STPG) &&
+            !h->pref &&
+            (h->tpgs & TPGS_MODE_IMPLICIT))
+            stpg = 0;
+        break;
+    case TPGS_STATE_STANDBY:
+    case TPGS_STATE_UNAVAILABLE:
+        stpg = 1;
+        break;
+    case TPGS_STATE_OFFLINE:
+        err = SCSI_DH_IO;
+        break;
+    case TPGS_STATE_TRANSITIONING:
+        err = SCSI_DH_RETRY;
+        break;
+    default:
+        break;
+    }
+
+    if (stpg) {
+        h->callback_fn = fn;
+        h->callback_data = data;
+        err = submit_stpg(h);
+        if (err != SCSI_DH_OK)
+            h->callback_fn = h->callback_data = NULL;
+    }
+    return err;
+}
+
+/*
   * alua_initialize - Initialize ALUA state

* @sdev: the device to be initialized
*

drivers/scsi/device_handler/scsi_dh_alua.c +0 -1

@@ -690,7 +740,6 @@ static int alua_activate(struct scsi_device
 *sdev,
  {
      struct alua_dh_data *h = sdev->handler_data;
      int err = SCSI_DH_OK;
-    int stpg = 0;
 
      err = alua_rtpg(sdev, h, 1);

      if (err != SCSI_DH_OK)

drivers/scsi/device_handler/scsi_dh_alua.c +1 ~~-32~~

@@ -699,41 +748,10 @@ static int alua_activate(struct scsi_device
 *sdev,
      if (optimize_stpg)
          h->flags |= ALUA_OPTIMIZE_STPG;
 
-    if (h->tpgs & TPGS_MODE_EXPLICIT) {
-        switch (h->state) {
-        case TPGS_STATE_NONOPTIMIZED:
-            stpg = 1;
-            if ((h->flags & ALUA_OPTIMIZE_STPG) &&
-                (!h->pref) &&
-                (h->tpgs & TPGS_MODE_IMPLICIT))
-                stpg = 0;
-            break;
-        case TPGS_STATE_STANDBY:
-        case TPGS_STATE_UNAVAILABLE:
-            stpg = 1;
-            break;
-        case TPGS_STATE_OFFLINE:
-            err = SCSI_DH_IO;
-            break;
-        case TPGS_STATE_TRANSITIONING:
-            err = SCSI_DH_RETRY;
-            break;
-        default:
-            break;
-        }
-    }
-
-    if (stpg) {
-        h->callback_fn = fn;
-        h->callback_data = data;
-        err = submit_stpg(h);
-        if (err == SCSI_DH_OK)
-            return 0;
-        h->callback_fn = h->callback_data = NULL;
-    }
+    err = alua_stpg(sdev, h, fn, data);

Quoted 13 lines

Quoted 7 lines
drivers/scsi/device_handler/scsi_dh_alua.c +1 -1
  out:
-    if (fn)
+    if (err != SCSI_DH_OK && fn)
          fn(data, err);
      return 0;
  }
The old implementation calls the callback function 'fn' if the flag TPGS_MODE_EXPLICIT has not been set but the new implementation not. Please mention this in the patch description if this behavior change is intentional.

Actually, this looks more like an error with the old implementation. 'fn' is the callback signalling that the failover command has been completed, and the caller (in most case dm-multipath) relies on 'fn' to be executed for further processing.
So 'fn' should be executed, irrespective of the TPGS_MODE_EXPLICIT flag.

Cheers,

Hannes

Signature · 9 lines

Dr. Hannes Reinecke		               zSeries & Storage
[email protected]			               +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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bart Van Assche

2015-10-02 15:04 UTC #23 lore raw

Re: [PATCH 14/36] scsi_dh_alua: separate out alua_stpg()

Quoted 11 lines · Hannes Reinecke

Quoted 5 lines · Bart Van Assche
The old implementation calls the callback function 'fn' if the flag TPGS_MODE_EXPLICIT has not been set but the new implementation not. Please mention this in the patch description if this behavior change is intentional.
Actually, this looks more like an error with the old implementation. 'fn' is the callback signalling that the failover command has been completed, and the caller (in most case dm-multipath) relies on 'fn' to be executed for further processing.
So 'fn' should be executed, irrespective of the TPGS_MODE_EXPLICIT flag.

Thank you for the feedback. I think reviewers would be grateful if this change would be documented in the patch description.

Bart.

Hannes Reinecke

author

2015-09-29 10:47 UTC #24 lore raw

PATCH v5 15/36 scsi_dh_alua: Make stpg synchronous

In reply to Hannes Reinecke

The 'activate_complete' function needs to be executed after stpg has finished, so we can as well execute stpg synchronously and call the function directly.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 133 +++++++++++------------------
 1 file changed, 50 insertions(+), 83 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +50 ~~-83~~

index 59fe3e9..7c3789b 100644
@@ -170,76 +170,28 @@ done:
 }
 
 /*
- * stpg_endio - Evaluate SET TARGET GROUP STATES
- * @sdev: the device to be evaluated
- * @state: the new target group state
- *
- * Evaluate a SET TARGET GROUP STATES command response.
- */
-static void stpg_endio(struct request *req, int error)
-{
-	struct alua_dh_data *h = req->end_io_data;
-	struct scsi_sense_hdr sense_hdr;
-	unsigned err = SCSI_DH_OK;
-
-	if (host_byte(req->errors) != DID_OK ||
-	    msg_byte(req->errors) != COMMAND_COMPLETE) {
-		err = SCSI_DH_IO;
-		goto done;
-	}
-
-	if (scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
-				 &sense_hdr)) {
-		err = alua_check_sense(h->sdev, &sense_hdr);
-		if (err == ADD_TO_MLQUEUE) {
-			err = SCSI_DH_RETRY;
-			goto done;
-		}
-		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n",
-			    ALUA_DH_NAME);
-		scsi_print_sense_hdr(h->sdev, ALUA_DH_NAME, &sense_hdr);
-		err = SCSI_DH_IO;
-	} else if (error)
-		err = SCSI_DH_IO;
-
-	if (err == SCSI_DH_OK) {
-		h->state = TPGS_STATE_OPTIMIZED;
-		sdev_printk(KERN_INFO, h->sdev,
-			    "%s: port group %02x switched to state %c\n",
-			    ALUA_DH_NAME, h->group_id,
-			    print_alua_state(h->state));
-	}
-done:
-	req->end_io_data = NULL;
-	__blk_put_request(req->q, req);
-	if (h->callback_fn) {
-		h->callback_fn(h->callback_data, err);
-		h->callback_fn = h->callback_data = NULL;
-	}
-	return;
-}
-
-/*
  * submit_stpg - Issue a SET TARGET GROUP STATES command
  *
  * Currently we're only setting the current target port group state
  * to 'active/optimized' and let the array firmware figure out
  * the states of the remaining groups.
  */
-static unsigned submit_stpg(struct alua_dh_data *h)
+static unsigned submit_stpg(struct scsi_device *sdev, int group_id,
+			    unsigned char *sense)
 {
 	struct request *rq;
+	unsigned char stpg_data[8];
 	int stpg_len = 8;
-	struct scsi_device *sdev = h->sdev;
+	int err = 0;
 
 	/* Prepare the data buffer */
-	memset(h->buff, 0, stpg_len);
-	h->buff[4] = TPGS_STATE_OPTIMIZED & 0x0f;
-	put_unaligned_be16(h->group_id, &h->buff[6]);
+	memset(stpg_data, 0, stpg_len);
+	stpg_data[4] = TPGS_STATE_OPTIMIZED & 0x0f;
+	put_unaligned_be16(group_id, &stpg_data[6]);
 
-	rq = get_alua_req(sdev, h->buff, stpg_len, WRITE);
+	rq = get_alua_req(sdev, stpg_data, stpg_len, WRITE);
 	if (!rq)
-		return SCSI_DH_RES_TEMP_UNAVAIL;
+		return DRIVER_BUSY << 24;
 
 	/* Prepare the command. */
 	rq->cmd[0] = MAINTENANCE_OUT;

@@ -247,13 +199,17 @@ static unsigned submit_stpg(struct alua_dh_data *h)
 	put_unaligned_be32(stpg_len, &rq->cmd[6]);
 	rq->cmd_len = COMMAND_SIZE(MAINTENANCE_OUT);
 
-	rq->sense = h->sense;
+	rq->sense = sense;
 	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
 	rq->sense_len = 0;
-	rq->end_io_data = h;
 
-	blk_execute_rq_nowait(rq->q, NULL, rq, 1, stpg_endio);
-	return SCSI_DH_OK;
+	blk_execute_rq(rq->q, NULL, rq, 1);
+	if (rq->errors)
+		err = rq->errors;
+
+	blk_put_request(rq);
+
+	return err;
 }
 
 /*

@@ -620,47 +576,59 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
  * response. Returns SCSI_DH_RETRY per default to trigger
  * a re-evaluation of the target group state.
  */
-static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h,
-			  activate_complete fn, void *data)
+static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
 {
-	int err = SCSI_DH_OK;
-	int stpg = 0;
+	int retval;
+	struct scsi_sense_hdr sense_hdr;
 
 	if (!(h->tpgs & TPGS_MODE_EXPLICIT)) {
-		/* Only implicit ALUA supported */
-		return err;
+		/* Only implicit ALUA supported, retry */
+		return SCSI_DH_RETRY;
 	}
-
 	switch (h->state) {
+	case TPGS_STATE_OPTIMIZED:
+		return SCSI_DH_OK;
 	case TPGS_STATE_NONOPTIMIZED:
-		stpg = 1;
 		if ((h->flags & ALUA_OPTIMIZE_STPG) &&
 		    !h->pref &&
 		    (h->tpgs & TPGS_MODE_IMPLICIT))
-			stpg = 0;
+			return SCSI_DH_OK;
 		break;
 	case TPGS_STATE_STANDBY:
 	case TPGS_STATE_UNAVAILABLE:
-		stpg = 1;
 		break;
 	case TPGS_STATE_OFFLINE:
-		err = SCSI_DH_IO;
+		return SCSI_DH_IO;
 		break;
 	case TPGS_STATE_TRANSITIONING:
-		err = SCSI_DH_RETRY;
 		break;
 	default:
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: stpg failed, unhandled TPGS state %d",
+			    ALUA_DH_NAME, h->state);
+		return SCSI_DH_NOSYS;
 		break;
 	}
+	/* Set state to transitioning */
+	h->state = TPGS_STATE_TRANSITIONING;
+	retval = submit_stpg(sdev, h->group_id, h->sense);
 
-	if (stpg) {
-		h->callback_fn = fn;
-		h->callback_data = data;
-		err = submit_stpg(h);
-		if (err != SCSI_DH_OK)
-			h->callback_fn = h->callback_data = NULL;
+	if (retval) {
+		if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
+					  &sense_hdr)) {
+			sdev_printk(KERN_INFO, sdev,
+				    "%s: stpg failed, result %d",
+				    ALUA_DH_NAME, retval);
+			if (driver_byte(retval) == DRIVER_BUSY)
+				return SCSI_DH_DEV_TEMP_BUSY;
+		} else {
+			sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n",
+				    ALUA_DH_NAME);
+			scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
+		}
 	}
-	return err;
+	/* Retry RTPG */
+	return SCSI_DH_RETRY;
 }
 
 /*

@@ -748,10 +716,9 @@ static int alua_activate(struct scsi_device *sdev,
 	if (optimize_stpg)
 		h->flags |= ALUA_OPTIMIZE_STPG;
 
-	err = alua_stpg(sdev, h, fn, data);
-
+	err = alua_stpg(sdev, h);
 out:
-	if (err != SCSI_DH_OK && fn)
+	if (fn)
 		fn(data, err);
 	return 0;
 }

Signature · 1 line

1.8.5.6

Bart Van Assche

2015-10-02 17:33 UTC #25 lore raw

Re: [PATCH 15/36] scsi_dh_alua: Make stpg synchronous

Quoted 3 lines · Hannes Reinecke

The 'activate_complete' function needs to be executed after stpg has finished, so we can as well execute stpg synchronously and call the function directly.

Hello Hannes,

Another patch in this series moves invocation of the STPG commands to the context of a workqueue. The setup on which I have been testing this patch series consists of an initiator system with four ports and a target system with eight ports and 100 LUNs. As a result, 4*8*100=3200 /dev/sd* devices are created on the initiator system and monitored by the scsi_dh_alua handler. How many kernel threads will be needed to monitor all these paths concurrently and how much memory will be required for all these kernel threads ? What if the number of LUNs would be even higher, e.g. 1000 ? Sorry but because of scalability concerns my preference is that the RTPG and STPG commands are invoked asynchronously.

Thanks,

Bart.

Hannes Reinecke

author

2015-10-07 20:48 UTC #26 lore raw

Re: [PATCH 15/36] scsi_dh_alua: Make stpg synchronous

Quoted 7 lines · Bart Van Assche

Quoted 3 lines · Hannes Reinecke
The 'activate_complete' function needs to be executed after stpg has finished, so we can as well execute stpg synchronously and call the function directly.
Hello Hannes,

Quoted 11 lines

Another patch in this series moves invocation of the STPG commands to the context of a workqueue. The setup on which I have been testing this patch series consists of an initiator system with four ports and a target system with eight ports and 100 LUNs. As a result, 4*8*100=3200 /dev/sd* devices are created on the initiator system and monitored by the scsi_dh_alua handler. How many kernel threads will be needed to monitor all these paths concurrently and how much memory will be required for all these kernel threads ? What if the number of LUNs would be even higher, e.g. 1000 ? Sorry but because of scalability concerns my preference is that the RTPG and STPG commands are invoked asynchronously.

The paths are not 'monitored' in the normal sense, ie the workqueue is not always active. The workqueue items is active only if a state change needs to be handled. I doubt that the overall resource usage will increase here, as originally we would need to execute '->activate' for every path.
With the new design we will be starting a workqueue item _per port group and LUN_, which will run until the new state could be successfully retrieved.

I have considered using an asynchronous implementation, but sadly scsi_execute() and friends doesn't have an asynchronous version. Also we would need to handle quite a substantial bulk of computation
from within an interrupt handler, which would make the operation
really tricky.

We should be doing some analysis here to figure out if there really is an increased memory usage and if this would turn out to be an issue.

Cheers,

Hannes

Signature · 8 lines

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hannes Reinecke

author

2015-09-29 10:47 UTC #27 lore raw

PATCH v5 16/36 scsi_dh_alua: call alua_rtpg() if stpg fails

In reply to Hannes Reinecke

If the call to SET TARGET PORT GROUPS fails we have no idea what state the array is left in, so we need to issue a call to REPORT TARGET PORT GROUPS in these cases.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/scsi/device_handler/scsi_dh_alua.c +2 -0

index 7c3789b..b645593 100644
@@ -717,6 +717,8 @@ static int alua_activate(struct scsi_device *sdev,
 		h->flags |= ALUA_OPTIMIZE_STPG;
 
 	err = alua_stpg(sdev, h);
+	if (err == SCSI_DH_RETRY)
+		err = alua_rtpg(sdev, h, 1);
 out:
 	if (fn)
 		fn(data, err);

Signature · 1 line

1.8.5.6

Bart Van Assche

Reviewed-by on v5 16/36

2015-10-02 17:14 UTC #28 lore raw

Re: [PATCH 16/36] scsi_dh_alua: call alua_rtpg() if stpg fails

Quoted 5 lines · Hannes Reinecke

If the call to SET TARGET PORT GROUPS fails we have no idea what state the array is left in, so we need to issue a call to REPORT TARGET PORT GROUPS in these cases.
Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Bart Van Assche <[email protected]>

Hannes Reinecke

author

2015-09-29 10:47 UTC #29 lore raw

PATCH v5 17/36 scsi_dh_alua: switch to scsi_execute_req_flags()

In reply to Hannes Reinecke

All commands are issued synchronously, so no need to open-code scsi_execute_req_flags() anymore. And we can get rid of the static sense code structure element. scsi_execute_req_flags() will be setting REQ_QUIET and REQ_PREEMPT, but that is perfectly fine as we're evaluating and logging any errors ourselves and we really need to send the command even if the device is quiesced.

Reviewed-by Christoph Hellwig <[email protected]>

Reviewed-by Ewan Milne <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 125 +++++++++--------------------
 1 file changed, 36 insertions(+), 89 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +36 ~~-89~~

index b645593..b72cea6 100644
@@ -75,7 +75,6 @@ struct alua_dh_data {
 	unsigned char		*buff;
 	int			bufflen;
 	unsigned char		transition_tmo;
-	unsigned char		sense[SCSI_SENSE_BUFFERSIZE];
 	struct scsi_device	*sdev;
 	activate_complete	callback_fn;
 	void			*callback_data;

@@ -102,71 +101,30 @@ static int realloc_buffer(struct alua_dh_data *h, unsigned len)
 	return 0;
 }
 
-static struct request *get_alua_req(struct scsi_device *sdev,
-				    void *buffer, unsigned buflen, int rw)
-{
-	struct request *rq;
-	struct request_queue *q = sdev->request_queue;
-
-	rq = blk_get_request(q, rw, GFP_NOIO);
-
-	if (IS_ERR(rq)) {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: blk_get_request failed\n", __func__);
-		return NULL;
-	}
-	blk_rq_set_block_pc(rq);
-
-	if (buflen && blk_rq_map_kern(q, rq, buffer, buflen, GFP_NOIO)) {
-		blk_put_request(rq);
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: blk_rq_map_kern failed\n", __func__);
-		return NULL;
-	}
-
-	rq->cmd_flags |= REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
-			 REQ_FAILFAST_DRIVER;
-	rq->retries = ALUA_FAILOVER_RETRIES;
-	rq->timeout = ALUA_FAILOVER_TIMEOUT * HZ;
-
-	return rq;
-}
-
 /*
  * submit_rtpg - Issue a REPORT TARGET GROUP STATES command
  * @sdev: sdev the command should be sent to
  */
-static unsigned submit_rtpg(struct scsi_device *sdev, unsigned char *buff,
-			    int bufflen, unsigned char *sense, int flags)
+static int submit_rtpg(struct scsi_device *sdev, unsigned char *buff,
+		       int bufflen, struct scsi_sense_hdr *sshdr, int flags)
 {
-	struct request *rq;
-	int err = 0;
-
-	rq = get_alua_req(sdev, buff, bufflen, READ);
-	if (!rq) {
-		err = DRIVER_BUSY << 24;
-		goto done;
-	}
+	u8 cdb[COMMAND_SIZE(MAINTENANCE_IN)];
+	int req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
+		REQ_FAILFAST_DRIVER;
 
 	/* Prepare the command. */
-	rq->cmd[0] = MAINTENANCE_IN;
+	memset(cdb, 0x0, COMMAND_SIZE(MAINTENANCE_IN));
+	cdb[0] = MAINTENANCE_IN;
 	if (!(flags & ALUA_RTPG_EXT_HDR_UNSUPP))
-		rq->cmd[1] = MI_REPORT_TARGET_PGS | MI_EXT_HDR_PARAM_FMT;
+		cdb[1] = MI_REPORT_TARGET_PGS | MI_EXT_HDR_PARAM_FMT;
 	else
-		rq->cmd[1] = MI_REPORT_TARGET_PGS;
-	put_unaligned_be32(bufflen, &rq->cmd[6]);
-	rq->cmd_len = COMMAND_SIZE(MAINTENANCE_IN);
-
-	rq->sense = sense;
-	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
-	rq->sense_len = 0;
-
-	blk_execute_rq(rq->q, NULL, rq, 1);
-	if (rq->errors)
-		err = rq->errors;
-	blk_put_request(rq);
-done:
-	return err;
+		cdb[1] = MI_REPORT_TARGET_PGS;
+	put_unaligned_be32(bufflen, &cdb[6]);
+
+	return scsi_execute_req_flags(sdev, cdb, DMA_FROM_DEVICE,
+				      buff, bufflen, sshdr,
+				      ALUA_FAILOVER_TIMEOUT * HZ,
+				      ALUA_FAILOVER_RETRIES, NULL, req_flags);
 }
 
 /*

@@ -176,40 +134,30 @@ done:
  * to 'active/optimized' and let the array firmware figure out
  * the states of the remaining groups.
  */
-static unsigned submit_stpg(struct scsi_device *sdev, int group_id,
-			    unsigned char *sense)
+static int submit_stpg(struct scsi_device *sdev, int group_id,
+		       struct scsi_sense_hdr *sshdr)
 {
-	struct request *rq;
+	u8 cdb[COMMAND_SIZE(MAINTENANCE_OUT)];
 	unsigned char stpg_data[8];
 	int stpg_len = 8;
-	int err = 0;
+	int req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
+		REQ_FAILFAST_DRIVER;
 
 	/* Prepare the data buffer */
 	memset(stpg_data, 0, stpg_len);
 	stpg_data[4] = TPGS_STATE_OPTIMIZED & 0x0f;
 	put_unaligned_be16(group_id, &stpg_data[6]);
 
-	rq = get_alua_req(sdev, stpg_data, stpg_len, WRITE);
-	if (!rq)
-		return DRIVER_BUSY << 24;
-
 	/* Prepare the command. */
-	rq->cmd[0] = MAINTENANCE_OUT;
-	rq->cmd[1] = MO_SET_TARGET_PGS;
-	put_unaligned_be32(stpg_len, &rq->cmd[6]);
-	rq->cmd_len = COMMAND_SIZE(MAINTENANCE_OUT);
-
-	rq->sense = sense;
-	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
-	rq->sense_len = 0;
-
-	blk_execute_rq(rq->q, NULL, rq, 1);
-	if (rq->errors)
-		err = rq->errors;
-
-	blk_put_request(rq);
-
-	return err;
+	memset(cdb, 0x0, COMMAND_SIZE(MAINTENANCE_OUT));
+	cdb[0] = MAINTENANCE_OUT;
+	cdb[1] = MO_SET_TARGET_PGS;
+	put_unaligned_be32(stpg_len, &cdb[6]);
+
+	return scsi_execute_req_flags(sdev, cdb, DMA_TO_DEVICE,
+				      stpg_data, stpg_len,
+				      sshdr, ALUA_FAILOVER_TIMEOUT * HZ,
+				      ALUA_FAILOVER_RETRIES, NULL, req_flags);
 }
 
 /*

@@ -445,14 +393,14 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		expiry = round_jiffies_up(jiffies + h->transition_tmo * HZ);
 
  retry:
-	retval = submit_rtpg(sdev, h->buff, h->bufflen, h->sense, h->flags);
+	retval = submit_rtpg(sdev, h->buff, h->bufflen, &sense_hdr, h->flags);
+
 	if (retval) {
-		if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
-					  &sense_hdr)) {
+		if (!scsi_sense_valid(&sense_hdr)) {
 			sdev_printk(KERN_INFO, sdev,
 				    "%s: rtpg failed, result %d\n",
 				    ALUA_DH_NAME, retval);
-			if (driver_byte(retval) == DRIVER_BUSY)
+			if (driver_byte(retval) == DRIVER_ERROR)
 				return SCSI_DH_DEV_TEMP_BUSY;
 			return SCSI_DH_IO;
 		}

@@ -611,15 +559,14 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
 	}
 	/* Set state to transitioning */
 	h->state = TPGS_STATE_TRANSITIONING;
-	retval = submit_stpg(sdev, h->group_id, h->sense);
+	retval = submit_stpg(sdev, h->group_id, &sense_hdr);
 
 	if (retval) {
-		if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
-					  &sense_hdr)) {
+		if (!scsi_sense_valid(&sense_hdr)) {
 			sdev_printk(KERN_INFO, sdev,
 				    "%s: stpg failed, result %d",
 				    ALUA_DH_NAME, retval);
-			if (driver_byte(retval) == DRIVER_BUSY)
+			if (driver_byte(retval) == DRIVER_ERROR)
 				return SCSI_DH_DEV_TEMP_BUSY;
 		} else {
 			sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n",

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:47 UTC #30 lore raw

PATCH v5 18/36 scsi_dh_alua: rework alua_check_tpgs() to return the tpgs mode

In reply to Hannes Reinecke

Instead of returning an error code in alua_check_tpgs() we should rather return the tpgs mode directly and have a cleanup syntax.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +11 ~~-14~~

index b72cea6..c6fcfa5 100644
@@ -167,24 +167,23 @@ static int submit_stpg(struct scsi_device *sdev, int group_id,
  * Examine the TPGS setting of the sdev to find out if ALUA
  * is supported.
  */
-static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
+static int alua_check_tpgs(struct scsi_device *sdev)
 {
-	int err = SCSI_DH_OK;
+	int tpgs = TPGS_MODE_NONE;
 
 	/*
 	 * ALUA support for non-disk devices is fraught with
 	 * difficulties, so disable it for now.
 	 */
 	if (sdev->type != TYPE_DISK) {
-		h->tpgs = TPGS_MODE_NONE;
 		sdev_printk(KERN_INFO, sdev,
 			    "%s: disable for non-disk devices\n",
 			    ALUA_DH_NAME);
-		return SCSI_DH_DEV_UNSUPP;
+		return tpgs;
 	}
 
-	h->tpgs = scsi_device_tpgs(sdev);
-	switch (h->tpgs) {
+	tpgs = scsi_device_tpgs(sdev);
+	switch (tpgs) {
 	case TPGS_MODE_EXPLICIT|TPGS_MODE_IMPLICIT:
 		sdev_printk(KERN_INFO, sdev,
 			    "%s: supports implicit and explicit TPGS\n",

@@ -201,18 +200,16 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
 	case TPGS_MODE_NONE:
 		sdev_printk(KERN_INFO, sdev, "%s: not supported\n",
 			    ALUA_DH_NAME);
-		err = SCSI_DH_DEV_UNSUPP;
 		break;
 	default:
 		sdev_printk(KERN_INFO, sdev,
 			    "%s: unsupported TPGS setting %d\n",
-			    ALUA_DH_NAME, h->tpgs);
-		h->tpgs = TPGS_MODE_NONE;
-		err = SCSI_DH_DEV_UNSUPP;
+			    ALUA_DH_NAME, tpgs);
+		tpgs = TPGS_MODE_NONE;
 		break;
 	}
 
-	return err;
+	return tpgs;
 }
 
 /*

@@ -587,10 +584,10 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
  */
 static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 {
-	int err;
+	int err = SCSI_DH_DEV_UNSUPP;
 
-	err = alua_check_tpgs(sdev, h);
-	if (err != SCSI_DH_OK)
+	h->tpgs = alua_check_tpgs(sdev);
+	if (h->tpgs == TPGS_MODE_NONE)
 		goto out;
 
 	err = alua_check_vpd(sdev, h);

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:47 UTC #31 lore raw

PATCH v5 19/36 scsi_dh_alua: Use separate alua_port_group structure

In reply to Hannes Reinecke

The port group needs to be a separate structure as several LUNs might belong to the same group.

Reviewed-by Ewan Milne <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 227 +++++++++++++++++++----------
 include/scsi/scsi_dh.h                     |   1 +
 2 files changed, 155 insertions(+), 73 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +154 ~~-73~~

index c6fcfa5..f3b455f 100644
@@ -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_head	node;
 	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			group_id;
+	int			rel_port;
 	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

@@ -161,6 +185,40 @@ static int submit_stpg(struct scsi_device *sdev, int group_id,
 }
 
 /*
+ * alua_get_pg - Allocate a new port_group structure
+ * @sdev: scsi device
+ * @h: alua device_handler data
+ * @group_id: port group id
+ *
+ * Allocate a new port_group structure for a given
+ * device.
+ */
+struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
+				    int group_id, int tpgs)
+{
+	struct alua_port_group *pg = NULL;
+
+	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);
+		return NULL;
+	}
+	pg->group_id = group_id;
+	pg->buff = pg->inq;
+	pg->bufflen = ALUA_INQUIRY_SIZE;
+	pg->tpgs = tpgs;
+	pg->state = TPGS_STATE_OPTIMIZED;
+	kref_init(&pg->kref);
+	spin_lock(&port_group_lock);
+	list_add(&pg->node, &port_group_list);
+	spin_unlock(&port_group_lock);
+
+	return pg;
+}
+
+/*
  * alua_check_tpgs - Evaluate TPGS setting
  * @sdev: device to be checked
  *

@@ -255,8 +313,6 @@ 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,

@@ -374,7 +430,7 @@ static int alua_check_sense(struct scsi_device *sdev,
  * Returns SCSI_DH_DEV_OFFLINED if the path is
  * found to be unusable.
  */
-static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_for_transition)
+static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int wait_for_transition)
 {
 	struct scsi_sense_hdr sense_hdr;
 	int len, k, off, valid_states = 0;

@@ -384,13 +440,14 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 	unsigned int tpg_desc_tbl_off;
 	unsigned char orig_transition_tmo;
 
-	if (!h->transition_tmo)
+	if (!pg->transition_tmo)
 		expiry = round_jiffies_up(jiffies + ALUA_FAILOVER_TIMEOUT * HZ);
 	else
-		expiry = round_jiffies_up(jiffies + h->transition_tmo * HZ);
+		expiry = round_jiffies_up(jiffies + pg->transition_tmo * HZ);
 
  retry:
-	retval = submit_rtpg(sdev, h->buff, h->bufflen, &sense_hdr, h->flags);
+	retval = submit_rtpg(sdev, pg->buff, pg->bufflen,
+			     &sense_hdr, pg->flags);
 
 	if (retval) {
 		if (!scsi_sense_valid(&sense_hdr)) {

@@ -410,10 +467,10 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		 * The retry without rtpg_ext_hdr_req set
 		 * handles this.
 		 */
-		if (!(h->flags & ALUA_RTPG_EXT_HDR_UNSUPP) &&
+		if (!(pg->flags & ALUA_RTPG_EXT_HDR_UNSUPP) &&
 		    sense_hdr.sense_key == ILLEGAL_REQUEST &&
 		    sense_hdr.asc == 0x24 && sense_hdr.ascq == 0) {
-			h->flags |= ALUA_RTPG_EXT_HDR_UNSUPP;
+			pg->flags |= ALUA_RTPG_EXT_HDR_UNSUPP;
 			goto retry;
 		}

@@ -430,11 +487,11 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		return SCSI_DH_IO;
 	}
 
-	len = get_unaligned_be32(&h->buff[0]) + 4;
+	len = get_unaligned_be32(&pg->buff[0]) + 4;
 
-	if (len > h->bufflen) {
+	if (len > pg->bufflen) {
 		/* Resubmit with the correct length */
-		if (realloc_buffer(h, len)) {
+		if (realloc_buffer(pg, len)) {
 			sdev_printk(KERN_WARNING, sdev,
 				    "%s: kmalloc buffer failed\n",__func__);
 			/* Temporary failure, bypass */

@@ -443,31 +500,33 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		goto retry;
 	}
 
-	orig_transition_tmo = h->transition_tmo;
-	if ((h->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR && h->buff[5] != 0)
-		h->transition_tmo = h->buff[5];
+	orig_transition_tmo = pg->transition_tmo;
+	if ((pg->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR &&
+	    pg->buff[5] != 0)
+		pg->transition_tmo = pg->buff[5];
 	else
-		h->transition_tmo = ALUA_FAILOVER_TIMEOUT;
+		pg->transition_tmo = ALUA_FAILOVER_TIMEOUT;
 
-	if (wait_for_transition && (orig_transition_tmo != h->transition_tmo)) {
+	if (wait_for_transition &&
+	    (orig_transition_tmo != pg->transition_tmo)) {
 		sdev_printk(KERN_INFO, sdev,
 			    "%s: transition timeout set to %d seconds\n",
-			    ALUA_DH_NAME, h->transition_tmo);
-		expiry = jiffies + h->transition_tmo * HZ;
+			    ALUA_DH_NAME, pg->transition_tmo);
+		expiry = jiffies + pg->transition_tmo * HZ;
 	}
 
-	if ((h->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR)
+	if ((pg->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR)
 		tpg_desc_tbl_off = 8;
 	else
 		tpg_desc_tbl_off = 4;
 
-	for (k = tpg_desc_tbl_off, ucp = h->buff + tpg_desc_tbl_off;
+	for (k = tpg_desc_tbl_off, ucp = pg->buff + tpg_desc_tbl_off;
 	     k < len;
 	     k += off, ucp += off) {
 
-		if (h->group_id == get_unaligned_be16(&ucp[2])) {
-			h->state = ucp[0] & 0x0f;
-			h->pref = ucp[0] >> 7;
+		if (pg->group_id == get_unaligned_be16(&ucp[2])) {
+			pg->state = ucp[0] & 0x0f;
+			pg->pref = ucp[0] >> 7;
 			valid_states = ucp[1];
 		}
 		off = 8 + (ucp[7] * 4);

@@ -475,8 +534,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 
 	sdev_printk(KERN_INFO, sdev,
 		    "%s: port group %02x state %c %s supports %c%c%c%c%c%c%c\n",
-		    ALUA_DH_NAME, h->group_id, print_alua_state(h->state),
-		    h->pref ? "preferred" : "non-preferred",
+		    ALUA_DH_NAME, pg->group_id, print_alua_state(pg->state),
+		    pg->pref ? "preferred" : "non-preferred",
 		    valid_states&TPGS_SUPPORT_TRANSITION?'T':'t',
 		    valid_states&TPGS_SUPPORT_OFFLINE?'O':'o',
 		    valid_states&TPGS_SUPPORT_LBA_DEPENDENT?'L':'l',

@@ -485,7 +544,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		    valid_states&TPGS_SUPPORT_NONOPTIMIZED?'N':'n',
 		    valid_states&TPGS_SUPPORT_OPTIMIZED?'A':'a');
 
-	switch (h->state) {
+	switch (pg->state) {
 	case TPGS_STATE_TRANSITIONING:
 		if (wait_for_transition) {
 			if (time_before(jiffies, expiry)) {

@@ -500,7 +559,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		}
 
 		/* Transitioning time exceeded, set port to standby */
-		h->state = TPGS_STATE_STANDBY;
+		pg->state = TPGS_STATE_STANDBY;
 		break;
 	case TPGS_STATE_OFFLINE:
 		/* Path unusable */

@@ -521,22 +580,22 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
  * response. Returns SCSI_DH_RETRY per default to trigger
  * a re-evaluation of the target group state.
  */
-static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
+static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 {
 	int retval;
 	struct scsi_sense_hdr sense_hdr;
 
-	if (!(h->tpgs & TPGS_MODE_EXPLICIT)) {
+	if (!(pg->tpgs & TPGS_MODE_EXPLICIT)) {
 		/* Only implicit ALUA supported, retry */
 		return SCSI_DH_RETRY;
 	}
-	switch (h->state) {
+	switch (pg->state) {
 	case TPGS_STATE_OPTIMIZED:
 		return SCSI_DH_OK;
 	case TPGS_STATE_NONOPTIMIZED:
-		if ((h->flags & ALUA_OPTIMIZE_STPG) &&
-		    !h->pref &&
-		    (h->tpgs & TPGS_MODE_IMPLICIT))
+		if ((pg->flags & ALUA_OPTIMIZE_STPG) &&
+		    !pg->pref &&
+		    (pg->tpgs & TPGS_MODE_IMPLICIT))
 			return SCSI_DH_OK;
 		break;
 	case TPGS_STATE_STANDBY:

@@ -550,13 +609,13 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
 	default:
 		sdev_printk(KERN_INFO, sdev,
 			    "%s: stpg failed, unhandled TPGS state %d",
-			    ALUA_DH_NAME, h->state);
+			    ALUA_DH_NAME, pg->state);
 		return SCSI_DH_NOSYS;
 		break;
 	}
 	/* Set state to transitioning */
-	h->state = TPGS_STATE_TRANSITIONING;
-	retval = submit_stpg(sdev, h->group_id, &sense_hdr);
+	pg->state = TPGS_STATE_TRANSITIONING;
+	retval = submit_stpg(sdev, pg->group_id, &sense_hdr);
 
 	if (retval) {
 		if (!scsi_sense_valid(&sense_hdr)) {

@@ -566,7 +625,7 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
 			if (driver_byte(retval) == DRIVER_ERROR)
 				return SCSI_DH_DEV_TEMP_BUSY;
 		} else {
-			sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n",
+			sdev_printk(KERN_INFO, sdev, "%s: stpg failed\n",
 				    ALUA_DH_NAME);
 			scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
 		}

@@ -584,20 +643,24 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
  */
 static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 {
-	int err = SCSI_DH_DEV_UNSUPP;
+	int err = SCSI_DH_DEV_UNSUPP, tpgs;
 
-	h->tpgs = alua_check_tpgs(sdev);
-	if (h->tpgs == TPGS_MODE_NONE)
+	tpgs = alua_check_tpgs(sdev);
+	if (tpgs == TPGS_MODE_NONE)
 		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)
+	h->pg = alua_get_pg(sdev, h->group_id, tpgs);
+	if (!h->pg) {
+		err = SCSI_DH_NOMEM;
 		goto out;
-
+	}
+	kref_get(&h->pg->kref);
+	err = alua_rtpg(sdev, h->pg, 0);
+	kref_put(&h->pg->kref, release_port_group);
 out:
 	return err;
 }

@@ -613,6 +676,7 @@ out:
 static int alua_set_params(struct scsi_device *sdev, const char *params)
 {
 	struct alua_dh_data *h = sdev->handler_data;
+	struct alua_port_group *pg = NULL;
 	unsigned int optimize = 0, argc;
 	const char *p = params;
 	int result = SCSI_DH_OK;

@@ -625,10 +689,14 @@ static int alua_set_params(struct scsi_device *sdev, const char *params)
 	if ((sscanf(p, "%u", &optimize) != 1) || (optimize > 1))
 		return -EINVAL;
 
+	pg = h->pg;
+	if (!pg)
+		return -ENXIO;
+
 	if (optimize)
-		h->flags |= ALUA_OPTIMIZE_STPG;
+		pg->flags |= ALUA_OPTIMIZE_STPG;
 	else
-		h->flags &= ~ALUA_OPTIMIZE_STPG;
+		pg->flags |= ~ALUA_OPTIMIZE_STPG;
 
 	return result;
 }

@@ -653,16 +721,23 @@ static int alua_activate(struct scsi_device *sdev,
 	struct alua_dh_data *h = sdev->handler_data;
 	int err = SCSI_DH_OK;
 
-	err = alua_rtpg(sdev, h, 1);
-	if (err != SCSI_DH_OK)
+	if (!h->pg)
 		goto out;
 
+	kref_get(&h->pg->kref);
+
 	if (optimize_stpg)
-		h->flags |= ALUA_OPTIMIZE_STPG;
+		h->pg->flags |= ALUA_OPTIMIZE_STPG;
 
-	err = alua_stpg(sdev, h);
+	err = alua_rtpg(sdev, h->pg, 1);
+	if (err != SCSI_DH_OK) {
+		kref_put(&h->pg->kref, release_port_group);
+		goto out;
+	}
+	err = alua_stpg(sdev, h->pg);
 	if (err == SCSI_DH_RETRY)
-		err = alua_rtpg(sdev, h, 1);
+		err = alua_rtpg(sdev, h->pg, 1);
+	kref_put(&h->pg->kref, release_port_group);
 out:
 	if (fn)
 		fn(data, err);

@@ -678,13 +753,19 @@ out:
 static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 {
 	struct alua_dh_data *h = sdev->handler_data;
+	int state;
 	int ret = BLKPREP_OK;
 
-	if (h->state == TPGS_STATE_TRANSITIONING)
+	if (!h->pg)
+		return ret;
+	kref_get(&h->pg->kref);
+	state = h->pg->state;
+	kref_put(&h->pg->kref, release_port_group);
+	if (state == TPGS_STATE_TRANSITIONING)
 		ret = BLKPREP_DEFER;
-	else if (h->state != TPGS_STATE_OPTIMIZED &&
-		 h->state != TPGS_STATE_NONOPTIMIZED &&
-		 h->state != TPGS_STATE_LBA_DEPENDENT) {
+	else if (state != TPGS_STATE_OPTIMIZED &&
+		 state != TPGS_STATE_NONOPTIMIZED &&
+		 state != TPGS_STATE_LBA_DEPENDENT) {
 		ret = BLKPREP_KILL;
 		req->cmd_flags |= REQ_QUIET;
 	}

@@ -699,20 +780,18 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 static int alua_bus_attach(struct scsi_device *sdev)
 {
 	struct alua_dh_data *h;
-	int err;
+	int err, ret = -EINVAL;
 
 	h = kzalloc(sizeof(*h) , GFP_KERNEL);
 	if (!h)
 		return -ENOMEM;
-	h->tpgs = TPGS_MODE_UNINITIALIZED;
-	h->state = TPGS_STATE_OPTIMIZED;
-	h->group_id = -1;
+	h->pg = NULL;
 	h->rel_port = -1;
-	h->buff = h->inq;
-	h->bufflen = ALUA_INQUIRY_SIZE;
 	h->sdev = sdev;
 
 	err = alua_initialize(sdev, h);
+	if (err == SCSI_DH_NOMEM)
+		ret = -ENOMEM;
 	if (err != SCSI_DH_OK && err != SCSI_DH_DEV_OFFLINED)
 		goto failed;

@@ -720,7 +799,7 @@ static int alua_bus_attach(struct scsi_device *sdev)
 	return 0;
 failed:
 	kfree(h);
-	return -EINVAL;
+	return ret;
 }
 
 /*

@@ -731,8 +810,10 @@ static void alua_bus_detach(struct scsi_device *sdev)
 {
 	struct alua_dh_data *h = sdev->handler_data;
 
-	if (h->buff && h->inq != h->buff)
-		kfree(h->buff);
+	if (h->pg) {
+		kref_put(&h->pg->kref, release_port_group);
+		h->pg = NULL;
+	}
 	sdev->handler_data = NULL;
 	kfree(h);
 }

include/scsi/scsi_dh.h +1 -0

index 85d7317..7e184c6 100644
@@ -52,6 +52,7 @@ enum {
 	SCSI_DH_TIMED_OUT,
 	SCSI_DH_RES_TEMP_UNAVAIL,
 	SCSI_DH_DEV_OFFLINED,
+	SCSI_DH_NOMEM,
 	SCSI_DH_NOSYS,
 	SCSI_DH_DRIVER_MAX,
 };

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:47 UTC #32 lore raw

PATCH v5 20/36 scsi_dh_alua: allocate RTPG buffer separately

In reply to Hannes Reinecke

The RTPG buffer will only evaluated within alua_rtpg(), so we can allocate it locally there and avoid having to put it into the global structure.

Reviewed-by Ewan Milne <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 56 +++++++++++-------------------
 1 file changed, 21 insertions(+), 35 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +21 ~~-35~~

index f3b455f..5908ef2 100644
@@ -56,7 +56,7 @@
 #define TPGS_MODE_IMPLICIT		0x1
 #define TPGS_MODE_EXPLICIT		0x2
 
-#define ALUA_INQUIRY_SIZE		36
+#define ALUA_RTPG_SIZE			128
 #define ALUA_FAILOVER_TIMEOUT		60
 #define ALUA_FAILOVER_RETRIES		5

@@ -75,9 +75,6 @@ struct alua_port_group {
 	int			state;
 	int			pref;
 	unsigned		flags; /* used for optimizing STPG */
-	unsigned char		inq[ALUA_INQUIRY_SIZE];
-	unsigned char		*buff;
-	int			bufflen;
 	unsigned char		transition_tmo;
 };

@@ -96,21 +93,6 @@ 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_port_group *pg, unsigned len)
-{
-	if (pg->buff && pg->buff != pg->inq)
-		kfree(pg->buff);
-
-	pg->buff = kmalloc(len, GFP_NOIO);
-	if (!pg->buff) {
-		pg->buff = pg->inq;
-		pg->bufflen = ALUA_INQUIRY_SIZE;
-		return 1;
-	}
-	pg->bufflen = len;
-	return 0;
-}
-
 static void release_port_group(struct kref *kref)
 {
 	struct alua_port_group *pg;

@@ -120,8 +102,6 @@ static void release_port_group(struct kref *kref)
 	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);
 }

@@ -206,8 +186,6 @@ struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
 		return NULL;
 	}
 	pg->group_id = group_id;
-	pg->buff = pg->inq;
-	pg->bufflen = ALUA_INQUIRY_SIZE;
 	pg->tpgs = tpgs;
 	pg->state = TPGS_STATE_OPTIMIZED;
 	kref_init(&pg->kref);

@@ -433,8 +411,8 @@ static int alua_check_sense(struct scsi_device *sdev,
 static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int wait_for_transition)
 {
 	struct scsi_sense_hdr sense_hdr;
-	int len, k, off, valid_states = 0;
-	unsigned char *ucp;
+	int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;
+	unsigned char *ucp, *buff;
 	unsigned err, retval;
 	unsigned long expiry, interval = 0;
 	unsigned int tpg_desc_tbl_off;

@@ -445,15 +423,19 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 	else
 		expiry = round_jiffies_up(jiffies + pg->transition_tmo * HZ);
 
+	buff = kzalloc(bufflen, GFP_KERNEL);
+	if (!buff)
+		return SCSI_DH_DEV_TEMP_BUSY;
+
  retry:
-	retval = submit_rtpg(sdev, pg->buff, pg->bufflen,
-			     &sense_hdr, pg->flags);
+	retval = submit_rtpg(sdev, buff, bufflen, &sense_hdr, pg->flags);
 
 	if (retval) {
 		if (!scsi_sense_valid(&sense_hdr)) {
 			sdev_printk(KERN_INFO, sdev,
 				    "%s: rtpg failed, result %d\n",
 				    ALUA_DH_NAME, retval);
+			kfree(buff);
 			if (driver_byte(retval) == DRIVER_ERROR)
 				return SCSI_DH_DEV_TEMP_BUSY;
 			return SCSI_DH_IO;

@@ -484,14 +466,18 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 		sdev_printk(KERN_ERR, sdev, "%s: rtpg failed\n",
 			    ALUA_DH_NAME);
 		scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
+		kfree(buff);
 		return SCSI_DH_IO;
 	}
 
-	len = get_unaligned_be32(&pg->buff[0]) + 4;
+	len = get_unaligned_be32(&buff[0]) + 4;
 
-	if (len > pg->bufflen) {
+	if (len > bufflen) {
 		/* Resubmit with the correct length */
-		if (realloc_buffer(pg, len)) {
+		kfree(buff);
+		bufflen = len;
+		buff = kmalloc(bufflen, GFP_KERNEL);
+		if (!buff) {
 			sdev_printk(KERN_WARNING, sdev,
 				    "%s: kmalloc buffer failed\n",__func__);
 			/* Temporary failure, bypass */

@@ -501,9 +487,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 	}
 
 	orig_transition_tmo = pg->transition_tmo;
-	if ((pg->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR &&
-	    pg->buff[5] != 0)
-		pg->transition_tmo = pg->buff[5];
+	if ((buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR && buff[5] != 0)
+		pg->transition_tmo = buff[5];
 	else
 		pg->transition_tmo = ALUA_FAILOVER_TIMEOUT;

@@ -515,12 +500,12 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 		expiry = jiffies + pg->transition_tmo * HZ;
 	}
 
-	if ((pg->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR)
+	if ((buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR)
 		tpg_desc_tbl_off = 8;
 	else
 		tpg_desc_tbl_off = 4;
 
-	for (k = tpg_desc_tbl_off, ucp = pg->buff + tpg_desc_tbl_off;
+	for (k = tpg_desc_tbl_off, ucp = buff + tpg_desc_tbl_off;
 	     k < len;
 	     k += off, ucp += off) {

@@ -570,6 +555,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 		err = SCSI_DH_OK;
 		break;
 	}
+	kfree(buff);
 	return err;
 }

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:48 UTC #33 lore raw

PATCH v5 21/36 scsi_dh_alua: simplify sense code handling

In reply to Hannes Reinecke

Most sense code is already handled in the generic code, so we shouldn't be adding special cases here. However, when doing so we need to check for unit attention whenever we're sending an internal command.

Reviewed-by Ewan Milne <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 36 +++++++++---------------------
 1 file changed, 10 insertions(+), 26 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +10 ~~-26~~

index 5908ef2..398ea81 100644
@@ -91,7 +91,6 @@ struct alua_dh_data {
 #define ALUA_POLICY_SWITCH_ALL		1
 
 static char print_alua_state(int);
-static int alua_check_sense(struct scsi_device *, struct scsi_sense_hdr *);
 
 static void release_port_group(struct kref *kref)
 {

@@ -332,28 +331,6 @@ static int alua_check_sense(struct scsi_device *sdev,
 			 * LUN Not Accessible - ALUA state transition
 			 */
 			return ADD_TO_MLQUEUE;
-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x0b)
-			/*
-			 * LUN Not Accessible -- Target port in standby state
-			 */
-			return SUCCESS;
-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x0c)
-			/*
-			 * LUN Not Accessible -- Target port in unavailable state
-			 */
-			return SUCCESS;
-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x12)
-			/*
-			 * LUN Not Ready -- Offline
-			 */
-			return SUCCESS;
-		if (sdev->allow_restart &&
-		    sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x02)
-			/*
-			 * if the device is not started, we need to wake
-			 * the error handler to start the motor
-			 */
-			return FAILED;
 		break;
 	case UNIT_ATTENTION:
 		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00)

@@ -455,9 +432,16 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 			pg->flags |= ALUA_RTPG_EXT_HDR_UNSUPP;
 			goto retry;
 		}
-
-		err = alua_check_sense(sdev, &sense_hdr);
-		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry)) {
+		/*
+		 * Retry on ALUA state transition or if any
+		 * UNIT ATTENTION occurred.
+		 */
+		if (sense_hdr.sense_key == NOT_READY &&
+		    sense_hdr.asc == 0x04 && sense_hdr.ascq == 0x0a)
+			err = SCSI_DH_RETRY;
+		else if (sense_hdr.sense_key == UNIT_ATTENTION)
+			err = SCSI_DH_RETRY;
+		if (err == SCSI_DH_RETRY && time_before(jiffies, expiry)) {
 			sdev_printk(KERN_ERR, sdev, "%s: rtpg retry\n",
 				    ALUA_DH_NAME);
 			scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:47 UTC #34 lore raw

PATCH v5 22/36 scsi: Add scsi_vpd_lun_id()

In reply to Hannes Reinecke

Add a function scsi_vpd_lun_id() to return a unique device identifcation based on the designation descriptors of VPD page 0x83.

As devices might implement several descriptors the order of preference is:
- NAA IEE Registered Extended
- EUI-64 based 16-byte
- EUI-64 based 12-byte
- NAA IEEE Registered
- NAA IEEE Extended
A SCSI name string descriptor is preferred to all of them if the identification is longer than 16 bytes.

The returned unique device identification will be formatted as a SCSI Name string to avoid clashes between different designator types.

Reviewed-by Ewan Milne <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/scsi_lib.c    | 132 +++++++++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h |   1 +
 2 files changed, 133 insertions(+)

drivers/scsi/scsi_lib.c +132 -0

index cbfc599..de3dcef 100644
@@ -3154,3 +3154,135 @@ void sdev_enable_disk_events(struct scsi_device *sdev)
 	atomic_dec(&sdev->disk_events_disable_depth);
 }
 EXPORT_SYMBOL(sdev_enable_disk_events);
+
+/*
+ * scsi_vpd_lun_id - return a unique device identification
+ * @sdev: SCSI device
+ * @id:   buffer for the identification
+ * @id_len:  length of the buffer
+ *
+ * Copies a unique device identification into @id based
+ * on the information in the VPD page 0x83 of the device.
+ * The string will be formatted as a SCSI name string.
+ *
+ * Returns the length of the identification or error on failure.
+ * If the identifier is longer than the supplied buffer the actual
+ * identifier length is returned and the buffer is not zero-padded.
+ */
+int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
+{
+	u8 cur_id_type = 0xff;
+	u8 cur_id_size = 0;
+	unsigned char *d, *cur_id_str;
+	int id_size = -EAGAIN;
+
+	if (!sdev->vpd_pg83)
+		return -ENXIO;
+
+	/*
+	 * Look for the correct descriptor.
+	 * Order of preference for lun descriptor:
+	 * - SCSI name string
+	 * - NAA IEEE Registered Extended
+	 * - EUI-64 based 16-byte
+	 * - EUI-64 based 12-byte
+	 * - NAA IEEE Registered
+	 * - NAA IEEE Extended
+	 * as longer descriptors reduce the likelyhood
+	 * of identification clashes.
+	 */
+
+	/* The id string must be at least 20 bytes + terminating NULL byte */
+	if (id_len < 21)
+		return -EINVAL;
+
+	memset(id, 0, id_len);
+	d = sdev->vpd_pg83 + 4;
+	while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
+		/* Skip designators not referring to the LUN */
+		if ((d[1] & 0x30) != 0x00)
+			goto next_desig;
+
+		switch (d[1] & 0xf) {
+		case 0x2:
+			/* EUI-64 */
+			if (cur_id_size > d[3])
+				break;
+			/* Prefer NAA IEEE Registered Extended */
+			if (cur_id_type == 0x3 &&
+			    cur_id_size == d[3])
+				break;
+			cur_id_size = d[3];
+			cur_id_str = d + 4;
+			cur_id_type = d[1] & 0xf;
+			switch (cur_id_size) {
+			case 8:
+				id_size = snprintf(id, id_len,
+						   "eui.%8phN",
+						   cur_id_str);
+				break;
+			case 12:
+				id_size = snprintf(id, id_len,
+						   "eui.%12phN",
+						   cur_id_str);
+				break;
+			case 16:
+				id_size = snprintf(id, id_len,
+						   "eui.%16phN",
+						   cur_id_str);
+				break;
+			default:
+				cur_id_size = 0;
+				break;
+			}
+			break;
+		case 0x3:
+			/* NAA */
+			if (cur_id_size > d[3])
+				break;
+			cur_id_size = d[3];
+			cur_id_str = d + 4;
+			cur_id_type = d[1] & 0xf;
+			switch (cur_id_size) {
+			case 8:
+				id_size = snprintf(id, id_len,
+						   "naa.%8phN",
+						   cur_id_str);
+				break;
+			case 16:
+				id_size = snprintf(id, id_len,
+						   "naa.%16phN",
+						   cur_id_str);
+				break;
+			default:
+				cur_id_size = 0;
+				break;
+			}
+			break;
+		case 0x8:
+			/* SCSI name string */
+			if (cur_id_size + 4 > d[3])
+				break;
+			/* Prefer others for truncated descriptor */
+			if (cur_id_size && d[3] > id_len)
+				break;
+			cur_id_size = id_size = d[3];
+			cur_id_str = d + 4;
+			cur_id_type = d[1] & 0xf;
+			if (cur_id_size >= id_len)
+				cur_id_size = id_len - 1;
+			memcpy(id, cur_id_str, cur_id_size);
+			/* Decrease priority for truncated descriptor */
+			if (cur_id_size != id_size)
+				cur_id_size = 6;
+			break;
+		default:
+			break;
+		}
+next_desig:
+		d += d[3] + 4;
+	}
+
+	return id_size;
+}
+EXPORT_SYMBOL(scsi_vpd_lun_id);

include/scsi/scsi_device.h +1 -0

index fe89d7c..6a9e6b2 100644
@@ -414,6 +414,7 @@ static inline int scsi_execute_req(struct scsi_device *sdev,
 }
 extern void sdev_disable_disk_events(struct scsi_device *sdev);
 extern void sdev_enable_disk_events(struct scsi_device *sdev);
+extern int scsi_vpd_lun_id(struct scsi_device *, char *, size_t);
 
 #ifdef CONFIG_PM
 extern int scsi_autopm_get_device(struct scsi_device *);

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:48 UTC #35 lore raw

PATCH v5 23/36 scsi_dh_alua: use unique device id

In reply to Hannes Reinecke

Use scsi_vpd_lun_id() to assign a unique device identification to the alua port group structure.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 65 ++++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 3 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +62 -3

index 398ea81..e2e6177 100644
@@ -70,6 +70,8 @@ static DEFINE_SPINLOCK(port_group_lock);
 struct alua_port_group {
 	struct kref		kref;
 	struct list_head	node;
+	unsigned char		device_id_str[256];
+	int			device_id_len;
 	int			group_id;
 	int			tpgs;
 	int			state;

@@ -163,6 +165,25 @@ static int submit_stpg(struct scsi_device *sdev, int group_id,
 				      ALUA_FAILOVER_RETRIES, NULL, req_flags);
 }
 
+struct alua_port_group *alua_lookup_pg(char *id_str, size_t id_size,
+				       int group_id)
+{
+	struct alua_port_group *pg = NULL;
+
+	list_for_each_entry(pg, &port_group_list, node) {
+		if (pg->group_id != group_id)
+			continue;
+		if (pg->device_id_len != id_size)
+			continue;
+		if (strncmp(pg->device_id_str, id_str, id_size))
+			continue;
+		kref_get(&pg->kref);
+		return pg;
+	}
+
+	return NULL;
+}
+
 /*
  * alua_get_pg - Allocate a new port_group structure
  * @sdev: scsi device

@@ -173,9 +194,16 @@ static int submit_stpg(struct scsi_device *sdev, int group_id,
  * device.
  */
 struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
-				    int group_id, int tpgs)
+				    int group_id, int tpgs,
+				    char *id_str, size_t id_size)
 {
-	struct alua_port_group *pg = NULL;
+	struct alua_port_group *pg = NULL, *tmp_pg;
+
+	spin_lock(&port_group_lock);
+	pg = alua_lookup_pg(id_str, id_size, group_id);
+	spin_unlock(&port_group_lock);
+	if (pg)
+		return pg;
 
 	pg = kzalloc(sizeof(struct alua_port_group), GFP_KERNEL);
 	if (!pg) {

@@ -184,11 +212,22 @@ struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
 			    ALUA_DH_NAME);
 		return NULL;
 	}
+	strncpy(pg->device_id_str, id_str, sizeof(pg->device_id_str));
+
+	pg->device_id_len = id_size;
 	pg->group_id = group_id;
 	pg->tpgs = tpgs;
 	pg->state = TPGS_STATE_OPTIMIZED;
 	kref_init(&pg->kref);
+
+	/* Re-check list again to catch concurrent updates */
 	spin_lock(&port_group_lock);
+	tmp_pg = alua_lookup_pg(id_str, id_size, group_id);
+	if (tmp_pg) {
+		spin_unlock(&port_group_lock);
+		kfree(pg);
+		return tmp_pg;
+	}
 	list_add(&pg->node, &port_group_list);
 	spin_unlock(&port_group_lock);

@@ -614,6 +653,8 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 {
 	int err = SCSI_DH_DEV_UNSUPP, tpgs;
+	char device_id_str[256];
+	int device_id_len;
 
 	tpgs = alua_check_tpgs(sdev);
 	if (tpgs == TPGS_MODE_NONE)

@@ -623,7 +664,25 @@ static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 	if (err != SCSI_DH_OK)
 		goto out;
 
-	h->pg = alua_get_pg(sdev, h->group_id, tpgs);
+	device_id_len = scsi_vpd_lun_id(sdev, device_id_str,
+					sizeof(device_id_str));
+	if (device_id_len <= 0) {
+		/*
+		 * Internal error: TPGS supported but no device
+		 * identifcation found. Disable ALUA support.
+		 */
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: No device descriptors found\n",
+			    ALUA_DH_NAME);
+		goto out;
+	}
+	sdev_printk(KERN_INFO, sdev,
+		    "%s: device %s port group %02x "
+		    "rel port %02x\n", ALUA_DH_NAME,
+		    device_id_str, h->group_id, h->rel_port);
+
+	h->pg = alua_get_pg(sdev, h->group_id, tpgs,
+			    device_id_str, device_id_len);
 	if (!h->pg) {
 		err = SCSI_DH_NOMEM;
 		goto out;

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:47 UTC #36 lore raw

PATCH v5 24/36 scsi: Add scsi_vpd_tpg_id()

In reply to Hannes Reinecke

Implement scsi_vpd_tpg_id() to extract the target
port group id and the relative port id from
SCSI VPD page 0x83.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/scsi_lib.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h |  1 +
 2 files changed, 43 insertions(+)

drivers/scsi/scsi_lib.c +42 -0

index de3dcef..fd4203c 100644
@@ -23,6 +23,7 @@
 #include <linux/scatterlist.h>
 #include <linux/blk-mq.h>
 #include <linux/ratelimit.h>
+#include <asm/unaligned.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>

@@ -3286,3 +3287,44 @@ next_desig:
 	return id_size;
 }
 EXPORT_SYMBOL(scsi_vpd_lun_id);
+
+/*
+ * scsi_vpd_tpg_id - return a target port group identifier
+ * @sdev: SCSI device
+ *
+ * Returns the Target Port Group identifier from the information
+ * froom VPD page 0x83 of the device.
+ *
+ * Returns the identifier or error on failure.
+ */
+int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id)
+{
+	unsigned char *d;
+	int group_id = -EAGAIN, rel_port = -1;
+
+	if (!sdev->vpd_pg83)
+		return -ENXIO;
+
+	d = sdev->vpd_pg83 + 4;
+	while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
+		switch (d[1] & 0xf) {
+		case 0x4:
+			/* Relative target port */
+			rel_port = get_unaligned_be16(&d[6]);
+			break;
+		case 0x5:
+			/* Target port group */
+			group_id = get_unaligned_be16(&d[6]);
+			break;
+		default:
+			break;
+		}
+		d += d[3] + 4;
+	}
+
+	if (group_id >= 0 && rel_id && rel_port != -1)
+		*rel_id = rel_port;
+
+	return group_id;
+}
+EXPORT_SYMBOL(scsi_vpd_tpg_id);

include/scsi/scsi_device.h +1 -0

index 6a9e6b2..cc6e763 100644
@@ -415,6 +415,7 @@ static inline int scsi_execute_req(struct scsi_device *sdev,
 extern void sdev_disable_disk_events(struct scsi_device *sdev);
 extern void sdev_enable_disk_events(struct scsi_device *sdev);
 extern int scsi_vpd_lun_id(struct scsi_device *, char *, size_t);
+extern int scsi_vpd_tpg_id(struct scsi_device *, int *);
 
 #ifdef CONFIG_PM
 extern int scsi_autopm_get_device(struct scsi_device *);

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:48 UTC #37 lore raw

PATCH v5 25/36 scsi_dh_alua: simplify alua_initialize()

In reply to Hannes Reinecke

Rework alua_check_vpd() to use scsi_vpd_get_tpg() and move the port group selection into the function, too. With that we can simplify alua_initialize() to just call alua_check_tpgs() and alua_check_vpd();

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 99 ++++++++++--------------------
 1 file changed, 34 insertions(+), 65 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +34 ~~-65~~

index e2e6177..1755591 100644
@@ -82,7 +82,6 @@ struct alua_port_group {
 
 struct alua_dh_data {
 	struct alua_port_group	*pg;
-	int			group_id;
 	int			rel_port;
 	struct scsi_device	*sdev;
 	activate_complete	callback_fn;

@@ -92,6 +91,7 @@ struct alua_dh_data {
 #define ALUA_POLICY_SWITCH_CURRENT	0
 #define ALUA_POLICY_SWITCH_ALL		1
 
+static int alua_rtpg(struct scsi_device *, struct alua_port_group *, int);
 static char print_alua_state(int);
 
 static void release_port_group(struct kref *kref)

@@ -292,35 +292,18 @@ static int alua_check_tpgs(struct scsi_device *sdev)
  *
  * Extract the relative target port and the target port group
  * descriptor from the list of identificators.
+ *
+ * Returns 0 or SCSI_DH_ error code on failure.
  */
-static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
+static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
+			  int tpgs)
 {
-	unsigned char *d;
-
-	if (!sdev->vpd_pg83)
-		return SCSI_DH_DEV_UNSUPP;
-
-	/*
-	 * Look for the correct descriptor.
-	 */
-	d = sdev->vpd_pg83 + 4;
-	while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
-		switch (d[1] & 0xf) {
-		case 0x4:
-			/* Relative target port */
-			h->rel_port = get_unaligned_be16(&d[6]);
-			break;
-		case 0x5:
-			/* Target port group */
-			h->group_id = get_unaligned_be16(&d[6]);
-			break;
-		default:
-			break;
-		}
-		d += d[3] + 4;
-	}
+	int rel_port = -1, group_id;
+	char id_str[256];
+	int id_size;
 
-	if (h->group_id == -1) {
+	group_id = scsi_vpd_tpg_id(sdev, &rel_port);
+	if (group_id < 0) {
 		/*
 		 * Internal error; TPGS supported but required
 		 * VPD identification descriptors not present.

@@ -331,11 +314,29 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 			    ALUA_DH_NAME);
 		return SCSI_DH_DEV_UNSUPP;
 	}
+	h->rel_port = rel_port;
+
+	id_size = scsi_vpd_lun_id(sdev, id_str, 256);
+	if (id_size <= 0) {
+		/*
+		 * Internal error: TPGS supported but no device
+		 * identifcation found. Disable ALUA support.
+		 */
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: No device descriptors found\n",
+			    ALUA_DH_NAME);
+		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);
+		    "%s: device %s port group %02x "
+		    "rel port %02x\n", ALUA_DH_NAME,
+		    id_str, group_id, h->rel_port);
 
-	return 0;
+	h->pg = alua_get_pg(sdev, group_id, tpgs, id_str, id_size);
+	if (!h->pg)
+		return SCSI_DH_NOMEM;
+
+	return alua_rtpg(sdev, h->pg, 0);
 }
 
 static char print_alua_state(int state)

@@ -653,46 +654,14 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 {
 	int err = SCSI_DH_DEV_UNSUPP, tpgs;
-	char device_id_str[256];
-	int device_id_len;
 
 	tpgs = alua_check_tpgs(sdev);
-	if (tpgs == TPGS_MODE_NONE)
-		goto out;
-
-	err = alua_check_vpd(sdev, h);
-	if (err != SCSI_DH_OK)
-		goto out;
+	if (tpgs != TPGS_MODE_NONE)
+		err = alua_check_vpd(sdev, h, tpgs);
 
-	device_id_len = scsi_vpd_lun_id(sdev, device_id_str,
-					sizeof(device_id_str));
-	if (device_id_len <= 0) {
-		/*
-		 * Internal error: TPGS supported but no device
-		 * identifcation found. Disable ALUA support.
-		 */
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: No device descriptors found\n",
-			    ALUA_DH_NAME);
-		goto out;
-	}
-	sdev_printk(KERN_INFO, sdev,
-		    "%s: device %s port group %02x "
-		    "rel port %02x\n", ALUA_DH_NAME,
-		    device_id_str, h->group_id, h->rel_port);
-
-	h->pg = alua_get_pg(sdev, h->group_id, tpgs,
-			    device_id_str, device_id_len);
-	if (!h->pg) {
-		err = SCSI_DH_NOMEM;
-		goto out;
-	}
-	kref_get(&h->pg->kref);
-	err = alua_rtpg(sdev, h->pg, 0);
-	kref_put(&h->pg->kref, release_port_group);
-out:
 	return err;
 }
+
 /*
  * alua_set_params - set/unset the optimize flag
  * @sdev: device on the path to be activated

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:47 UTC #38 lore raw

PATCH v5 26/36 revert "scsi_dh_alua: ALUA hander attach should succeed while TPG is transitioning"

In reply to Hannes Reinecke

This reverts commit a8e5a2d593cbfccf530c3382c2c328d2edaa7b66

Obsoleted by the next patch.

Reviewed-by Ewan Milne <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 31 ++++++++++++------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +12 ~~-19~~

index 1755591..4113b76 100644
@@ -91,7 +91,7 @@ struct alua_dh_data {
 #define ALUA_POLICY_SWITCH_CURRENT	0
 #define ALUA_POLICY_SWITCH_ALL		1
 
-static int alua_rtpg(struct scsi_device *, struct alua_port_group *, int);
+static int alua_rtpg(struct scsi_device *, struct alua_port_group *);
 static char print_alua_state(int);
 
 static void release_port_group(struct kref *kref)

@@ -336,7 +336,7 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 	if (!h->pg)
 		return SCSI_DH_NOMEM;
 
-	return alua_rtpg(sdev, h->pg, 0);
+	return alua_rtpg(sdev, h->pg);
 }
 
 static char print_alua_state(int state)

@@ -419,13 +419,12 @@ static int alua_check_sense(struct scsi_device *sdev,
 /*
  * alua_rtpg - Evaluate REPORT TARGET GROUP STATES
  * @sdev: the device to be evaluated.
- * @wait_for_transition: if nonzero, wait ALUA_FAILOVER_TIMEOUT seconds for device to exit transitioning state
  *
  * Evaluate the Target Port Group State.
  * Returns SCSI_DH_DEV_OFFLINED if the path is
  * found to be unusable.
  */
-static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int wait_for_transition)
+static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 {
 	struct scsi_sense_hdr sense_hdr;
 	int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;

@@ -516,8 +515,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 	else
 		pg->transition_tmo = ALUA_FAILOVER_TIMEOUT;
 
-	if (wait_for_transition &&
-	    (orig_transition_tmo != pg->transition_tmo)) {
+	if (orig_transition_tmo != pg->transition_tmo) {
 		sdev_printk(KERN_INFO, sdev,
 			    "%s: transition timeout set to %d seconds\n",
 			    ALUA_DH_NAME, pg->transition_tmo);

@@ -555,19 +553,14 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 
 	switch (pg->state) {
 	case TPGS_STATE_TRANSITIONING:
-		if (wait_for_transition) {
-			if (time_before(jiffies, expiry)) {
-				/* State transition, retry */
-				interval += 2000;
-				msleep(interval);
-				goto retry;
-			}
-			err = SCSI_DH_RETRY;
-		} else {
-			err = SCSI_DH_OK;
+		if (time_before(jiffies, expiry)) {
+			/* State transition, retry */
+			interval += 2000;
+			msleep(interval);
+			goto retry;
 		}
-
 		/* Transitioning time exceeded, set port to standby */
+		err = SCSI_DH_RETRY;
 		pg->state = TPGS_STATE_STANDBY;
 		break;
 	case TPGS_STATE_OFFLINE:

@@ -727,14 +720,14 @@ static int alua_activate(struct scsi_device *sdev,
 	if (optimize_stpg)
 		h->pg->flags |= ALUA_OPTIMIZE_STPG;
 
-	err = alua_rtpg(sdev, h->pg, 1);
+	err = alua_rtpg(sdev, h->pg);
 	if (err != SCSI_DH_OK) {
 		kref_put(&h->pg->kref, release_port_group);
 		goto out;
 	}
 	err = alua_stpg(sdev, h->pg);
 	if (err == SCSI_DH_RETRY)
-		err = alua_rtpg(sdev, h->pg, 1);
+		err = alua_rtpg(sdev, h->pg);
 	kref_put(&h->pg->kref, release_port_group);
 out:
 	if (fn)

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:48 UTC #39 lore raw

PATCH v5 27/36 scsi_dh_alua: Use workqueue for RTPG

In reply to Hannes Reinecke

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.

With this patch the RTPG handling is moved to a per-portgroup workqueue. This reduces the number of 'REPORT TARGET PORT GROUP' and 'SET TARGET PORT GROUPS' sent to the controller as we're sending them now per port group, and not per device as previously. It also allows us to block I/O to any LUN / port group found to be in 'transitioning' ALUA mode, as the workqueue item will be requeued until the controller moves out of transitioning.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 327 +++++++++++++++++++++++------
 1 file changed, 267 insertions(+), 60 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +267 ~~-60~~

index 4113b76..1b7153f 100644
@@ -59,13 +59,23 @@
 #define ALUA_RTPG_SIZE			128
 #define ALUA_FAILOVER_TIMEOUT		60
 #define ALUA_FAILOVER_RETRIES		5
+#define ALUA_RTPG_DELAY_MSECS		5
 
 /* device handler flags */
-#define ALUA_OPTIMIZE_STPG		1
-#define ALUA_RTPG_EXT_HDR_UNSUPP	2
+#define ALUA_OPTIMIZE_STPG		0x01
+#define ALUA_RTPG_EXT_HDR_UNSUPP	0x02
+/* State machine flags */
+#define ALUA_PG_RUN_RTPG		0x10
+#define ALUA_PG_RUN_STPG		0x20
+#define ALUA_PG_RUNNING			0x40
+
+static uint optimize_stpg;
+module_param(optimize_stpg, uint, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(optimize_stpg, "Allow use of a non-optimized path, rather than sending a STPG, when implicit TPGS is supported (0=No,1=Yes). Default is 0.");
 
 static LIST_HEAD(port_group_list);
 static DEFINE_SPINLOCK(port_group_lock);
+static struct workqueue_struct *kaluad_wq;
 
 struct alua_port_group {
 	struct kref		kref;

@@ -78,12 +88,25 @@ struct alua_port_group {
 	int			pref;
 	unsigned		flags; /* used for optimizing STPG */
 	unsigned char		transition_tmo;
+	unsigned long		expiry;
+	unsigned long		interval;
+	struct delayed_work	rtpg_work;
+	spinlock_t		lock;
+	struct list_head	rtpg_list;
+	struct scsi_device	*rtpg_sdev;
 };
 
 struct alua_dh_data {
 	struct alua_port_group	*pg;
 	int			rel_port;
+	spinlock_t		pg_lock;
 	struct scsi_device	*sdev;
+	int			init_error;
+	struct mutex		init_mutex;
+};
+
+struct alua_queue_data {
+	struct list_head	entry;
 	activate_complete	callback_fn;
 	void			*callback_data;
 };

@@ -91,8 +114,10 @@ struct alua_dh_data {
 #define ALUA_POLICY_SWITCH_CURRENT	0
 #define ALUA_POLICY_SWITCH_ALL		1
 
-static int alua_rtpg(struct scsi_device *, struct alua_port_group *);
-static char print_alua_state(int);
+static void alua_rtpg_work(struct work_struct *work);
+static void alua_rtpg_queue(struct alua_port_group *pg,
+			    struct scsi_device *sdev,
+			    struct alua_queue_data *qdata);
 
 static void release_port_group(struct kref *kref)
 {

@@ -103,6 +128,7 @@ static void release_port_group(struct kref *kref)
 	spin_lock(&port_group_lock);
 	list_del(&pg->node);
 	spin_unlock(&port_group_lock);
+	WARN_ON(pg->rtpg_sdev);
 	kfree(pg);
 }

@@ -168,7 +194,7 @@ static int submit_stpg(struct scsi_device *sdev, int group_id,
 struct alua_port_group *alua_lookup_pg(char *id_str, size_t id_size,
 				       int group_id)
 {
-	struct alua_port_group *pg = NULL;
+	struct alua_port_group *pg;
 
 	list_for_each_entry(pg, &port_group_list, node) {
 		if (pg->group_id != group_id)

@@ -218,18 +244,26 @@ struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
 	pg->group_id = group_id;
 	pg->tpgs = tpgs;
 	pg->state = TPGS_STATE_OPTIMIZED;
+	if (optimize_stpg)
+		pg->flags |= ALUA_OPTIMIZE_STPG;
 	kref_init(&pg->kref);
+	INIT_DELAYED_WORK(&pg->rtpg_work, alua_rtpg_work);
+	INIT_LIST_HEAD(&pg->rtpg_list);
+	INIT_LIST_HEAD(&pg->node);
+	spin_lock_init(&pg->lock);
 
 	/* Re-check list again to catch concurrent updates */
 	spin_lock(&port_group_lock);
 	tmp_pg = alua_lookup_pg(id_str, id_size, group_id);
 	if (tmp_pg) {
 		spin_unlock(&port_group_lock);
-		kfree(pg);
-		return tmp_pg;
+		kref_put(&pg->kref, release_port_group);
+		pg = tmp_pg;
+		tmp_pg = NULL;
+	} else {
+		list_add(&pg->node, &port_group_list);
+		spin_unlock(&port_group_lock);
 	}
-	list_add(&pg->node, &port_group_list);
-	spin_unlock(&port_group_lock);
 
 	return pg;
 }

@@ -293,7 +327,7 @@ static int alua_check_tpgs(struct scsi_device *sdev)
  * Extract the relative target port and the target port group
  * descriptor from the list of identificators.
  *
- * Returns 0 or SCSI_DH_ error code on failure.
+ * Returns the target port group id or -1 on failure
  */
 static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 			  int tpgs)

@@ -301,6 +335,8 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 	int rel_port = -1, group_id;
 	char id_str[256];
 	int id_size;
+	struct alua_port_group *pg = NULL, *old_pg = NULL;
+	bool pg_found = false;
 
 	group_id = scsi_vpd_tpg_id(sdev, &rel_port);
 	if (group_id < 0) {

@@ -332,11 +368,37 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 		    "rel port %02x\n", ALUA_DH_NAME,
 		    id_str, group_id, h->rel_port);
 
-	h->pg = alua_get_pg(sdev, group_id, tpgs, id_str, id_size);
-	if (!h->pg)
+	pg = alua_get_pg(sdev, group_id, tpgs, id_str, id_size);
+	if (!pg)
 		return SCSI_DH_NOMEM;
 
-	return alua_rtpg(sdev, h->pg);
+	/* Check for existing port_group references */
+	spin_lock(&h->pg_lock);
+	if (h->pg) {
+		old_pg = pg;
+		/* port_group has changed. Update to new port group */
+		if (h->pg != pg) {
+			old_pg = h->pg;
+			rcu_assign_pointer(h->pg, pg);
+			h->pg->expiry = 0;
+			pg_found = true;
+		}
+	} else {
+		rcu_assign_pointer(h->pg, pg);
+		pg_found = true;
+	}
+	alua_rtpg_queue(h->pg, sdev, NULL);
+	spin_unlock(&h->pg_lock);
+
+	if (pg_found)
+		synchronize_rcu();
+	if (old_pg) {
+		if (old_pg->rtpg_sdev)
+			flush_delayed_work(&old_pg->rtpg_work);
+		kref_put(&old_pg->kref, release_port_group);
+	}
+
+	return SCSI_DH_OK;
 }
 
 static char print_alua_state(int state)

@@ -430,14 +492,15 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;
 	unsigned char *ucp, *buff;
 	unsigned err, retval;
-	unsigned long expiry, interval = 0;
 	unsigned int tpg_desc_tbl_off;
 	unsigned char orig_transition_tmo;
 
-	if (!pg->transition_tmo)
-		expiry = round_jiffies_up(jiffies + ALUA_FAILOVER_TIMEOUT * HZ);
-	else
-		expiry = round_jiffies_up(jiffies + pg->transition_tmo * HZ);
+	if (!pg->expiry) {
+		if (!pg->transition_tmo)
+			pg->expiry = round_jiffies_up(jiffies + ALUA_FAILOVER_TIMEOUT * HZ);
+		else
+			pg->expiry = round_jiffies_up(jiffies + pg->transition_tmo * HZ);
+	}
 
 	buff = kzalloc(bufflen, GFP_KERNEL);
 	if (!buff)

@@ -480,16 +543,18 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 			err = SCSI_DH_RETRY;
 		else if (sense_hdr.sense_key == UNIT_ATTENTION)
 			err = SCSI_DH_RETRY;
-		if (err == SCSI_DH_RETRY && time_before(jiffies, expiry)) {
+		if (err == SCSI_DH_RETRY &&
+		    pg->expiry != 0 && time_before(jiffies, pg->expiry)) {
 			sdev_printk(KERN_ERR, sdev, "%s: rtpg retry\n",
 				    ALUA_DH_NAME);
 			scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
-			goto retry;
+			return err;
 		}
 		sdev_printk(KERN_ERR, sdev, "%s: rtpg failed\n",
 			    ALUA_DH_NAME);
 		scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
 		kfree(buff);
+		pg->expiry = 0;
 		return SCSI_DH_IO;
 	}

@@ -504,6 +569,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 			sdev_printk(KERN_WARNING, sdev,
 				    "%s: kmalloc buffer failed\n",__func__);
 			/* Temporary failure, bypass */
+			pg->expiry = 0;
 			return SCSI_DH_DEV_TEMP_BUSY;
 		}
 		goto retry;

@@ -519,7 +585,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		sdev_printk(KERN_INFO, sdev,
 			    "%s: transition timeout set to %d seconds\n",
 			    ALUA_DH_NAME, pg->transition_tmo);
-		expiry = jiffies + pg->transition_tmo * HZ;
+		pg->expiry = jiffies + pg->transition_tmo * HZ;
 	}
 
 	if ((buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR)

@@ -553,23 +619,26 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 
 	switch (pg->state) {
 	case TPGS_STATE_TRANSITIONING:
-		if (time_before(jiffies, expiry)) {
+		if (time_before(jiffies, pg->expiry)) {
 			/* State transition, retry */
-			interval += 2000;
-			msleep(interval);
-			goto retry;
+			pg->interval = 2;
+			err = SCSI_DH_RETRY;
+		} else {
+			/* Transitioning time exceeded, set port to standby */
+			err = SCSI_DH_IO;
+			pg->state = TPGS_STATE_STANDBY;
+			pg->expiry = 0;
 		}
-		/* Transitioning time exceeded, set port to standby */
-		err = SCSI_DH_RETRY;
-		pg->state = TPGS_STATE_STANDBY;
 		break;
 	case TPGS_STATE_OFFLINE:
 		/* Path unusable */
 		err = SCSI_DH_DEV_OFFLINED;
+		pg->expiry = 0;
 		break;
 	default:
 		/* Useable path if active */
 		err = SCSI_DH_OK;
+		pg->expiry = 0;
 		break;
 	}
 	kfree(buff);

@@ -637,6 +706,106 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	return SCSI_DH_RETRY;
 }
 
+static void alua_rtpg_work(struct work_struct *work)
+{
+	struct alua_port_group *pg =
+		container_of(work, struct alua_port_group, rtpg_work.work);
+	struct scsi_device *sdev;
+	LIST_HEAD(qdata_list);
+	int err = SCSI_DH_OK;
+	struct alua_queue_data *qdata, *tmp;
+	unsigned long flags;
+
+	spin_lock_irqsave(&pg->lock, flags);
+	sdev = pg->rtpg_sdev;
+	if (!sdev) {
+		WARN_ON(pg->flags & ALUA_PG_RUN_RTPG ||
+			pg->flags & ALUA_PG_RUN_STPG);
+		spin_unlock_irqrestore(&pg->lock, flags);
+		return;
+	}
+	pg->flags |= ALUA_PG_RUNNING;
+	if (pg->flags & ALUA_PG_RUN_RTPG) {
+		spin_unlock_irqrestore(&pg->lock, flags);
+		err = alua_rtpg(sdev, pg);
+		spin_lock_irqsave(&pg->lock, flags);
+		if (err == SCSI_DH_RETRY) {
+			pg->flags &= ~ALUA_PG_RUNNING;
+			spin_unlock_irqrestore(&pg->lock, flags);
+			queue_delayed_work(kaluad_wq, &pg->rtpg_work,
+					   pg->interval * HZ);
+			return;
+		}
+		pg->flags &= ~ALUA_PG_RUN_RTPG;
+		if (err != SCSI_DH_OK)
+			pg->flags &= ~ALUA_PG_RUN_STPG;
+	}
+	if (pg->flags & ALUA_PG_RUN_STPG) {
+		spin_unlock_irqrestore(&pg->lock, flags);
+		err = alua_stpg(sdev, pg);
+		spin_lock_irqsave(&pg->lock, flags);
+		pg->flags &= ~ALUA_PG_RUN_STPG;
+		if (err == SCSI_DH_RETRY) {
+			pg->flags |= ALUA_PG_RUN_RTPG;
+			pg->interval = 0;
+			pg->flags &= ~ALUA_PG_RUNNING;
+			spin_unlock_irqrestore(&pg->lock, flags);
+			queue_delayed_work(kaluad_wq, &pg->rtpg_work,
+					   pg->interval * HZ);
+			return;
+		}
+	}
+
+	list_splice_init(&pg->rtpg_list, &qdata_list);
+	pg->rtpg_sdev = NULL;
+	spin_unlock_irqrestore(&pg->lock, flags);
+
+	list_for_each_entry_safe(qdata, tmp, &qdata_list, entry) {
+		list_del(&qdata->entry);
+		if (qdata->callback_fn)
+			qdata->callback_fn(qdata->callback_data, err);
+		kfree(qdata);
+	}
+	spin_lock_irqsave(&pg->lock, flags);
+	pg->flags &= ~ALUA_PG_RUNNING;
+	spin_unlock_irqrestore(&pg->lock, flags);
+	scsi_device_put(sdev);
+	kref_put(&pg->kref, release_port_group);
+}
+
+static void alua_rtpg_queue(struct alua_port_group *pg,
+			    struct scsi_device *sdev,
+			    struct alua_queue_data *qdata)
+{
+	int start_queue = 0;
+	unsigned long flags;
+
+	if (!pg)
+		return;
+
+	spin_lock_irqsave(&pg->lock, flags);
+	if (qdata) {
+		list_add_tail(&qdata->entry, &pg->rtpg_list);
+		pg->flags |= ALUA_PG_RUN_STPG;
+	}
+	if (pg->rtpg_sdev == NULL) {
+		pg->interval = 0;
+		pg->flags |= ALUA_PG_RUN_RTPG;
+		kref_get(&pg->kref);
+		pg->rtpg_sdev = sdev;
+		scsi_device_get(sdev);
+		start_queue = 1;
+	}
+	spin_unlock_irqrestore(&pg->lock, flags);
+
+	if (start_queue &&
+	    !queue_delayed_work(kaluad_wq, &pg->rtpg_work,
+				msecs_to_jiffies(ALUA_RTPG_DELAY_MSECS))) {
+		scsi_device_put(sdev);
+		kref_put(&pg->kref, release_port_group);
+	}
+}
+
 /*
  * alua_initialize - Initialize ALUA state
  * @sdev: the device to be initialized

@@ -648,10 +817,12 @@ static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 {
 	int err = SCSI_DH_DEV_UNSUPP, tpgs;
 
+	mutex_lock(&h->init_mutex);
 	tpgs = alua_check_tpgs(sdev);
 	if (tpgs != TPGS_MODE_NONE)
 		err = alua_check_vpd(sdev, h, tpgs);
-
+	h->init_error = err;
+	mutex_unlock(&h->init_mutex);
 	return err;
 }

@@ -671,6 +842,7 @@ static int alua_set_params(struct scsi_device *sdev, const char *params)
 	unsigned int optimize = 0, argc;
 	const char *p = params;
 	int result = SCSI_DH_OK;
+	unsigned long flags;
 
 	if ((sscanf(params, "%u", &argc) != 1) || (argc != 1))
 		return -EINVAL;

@@ -680,22 +852,23 @@ static int alua_set_params(struct scsi_device *sdev, const char *params)
 	if ((sscanf(p, "%u", &optimize) != 1) || (optimize > 1))
 		return -EINVAL;
 
-	pg = h->pg;
-	if (!pg)
+	rcu_read_lock();
+	pg = rcu_dereference(h->pg);
+	if (!pg) {
+		rcu_read_unlock();
 		return -ENXIO;
-
+	}
+	spin_lock_irqsave(&pg->lock, flags);
 	if (optimize)
 		pg->flags |= ALUA_OPTIMIZE_STPG;
 	else
 		pg->flags |= ~ALUA_OPTIMIZE_STPG;
+	spin_unlock_irqrestore(&pg->lock, flags);
+	rcu_read_unlock();
 
 	return result;
 }
 
-static uint optimize_stpg;
-module_param(optimize_stpg, uint, S_IRUGO|S_IWUSR);
-MODULE_PARM_DESC(optimize_stpg, "Allow use of a non-optimized path, rather than sending a STPG, when implicit TPGS is supported (0=No,1=Yes). Default is 0.");
-
 /*
  * alua_activate - activate a path
  * @sdev: device on the path to be activated

@@ -711,24 +884,34 @@ static int alua_activate(struct scsi_device *sdev,
 {
 	struct alua_dh_data *h = sdev->handler_data;
 	int err = SCSI_DH_OK;
+	struct alua_queue_data *qdata;
+	struct alua_port_group *pg;
 
-	if (!h->pg)
+	qdata = kzalloc(sizeof(*qdata), GFP_KERNEL);
+	if (!qdata) {
+		err = SCSI_DH_RES_TEMP_UNAVAIL;
 		goto out;
+	}
+	qdata->callback_fn = fn;
+	qdata->callback_data = data;
 
-	kref_get(&h->pg->kref);
-
-	if (optimize_stpg)
-		h->pg->flags |= ALUA_OPTIMIZE_STPG;
-
-	err = alua_rtpg(sdev, h->pg);
-	if (err != SCSI_DH_OK) {
-		kref_put(&h->pg->kref, release_port_group);
+	mutex_lock(&h->init_mutex);
+	rcu_read_lock();
+	pg = rcu_dereference(h->pg);
+	if (!pg) {
+		rcu_read_unlock();
+		kfree(qdata);
+		err = h->init_error;
+		mutex_unlock(&h->init_mutex);
 		goto out;
 	}
-	err = alua_stpg(sdev, h->pg);
-	if (err == SCSI_DH_RETRY)
-		err = alua_rtpg(sdev, h->pg);
-	kref_put(&h->pg->kref, release_port_group);
+	mutex_unlock(&h->init_mutex);
+	fn = NULL;
+	kref_get(&pg->kref);
+	rcu_read_unlock();
+
+	alua_rtpg_queue(pg, sdev, qdata);
+	kref_put(&pg->kref, release_port_group);
 out:
 	if (fn)
 		fn(data, err);

@@ -744,14 +927,19 @@ out:
 static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 {
 	struct alua_dh_data *h = sdev->handler_data;
-	int state;
+	struct alua_port_group *pg;
+	int state = TPGS_STATE_OPTIMIZED;
 	int ret = BLKPREP_OK;
 
-	if (!h->pg)
-		return ret;
-	kref_get(&h->pg->kref);
-	state = h->pg->state;
-	kref_put(&h->pg->kref, release_port_group);
+	rcu_read_lock();
+	pg = rcu_dereference(h->pg);
+	if (pg) {
+		state = pg->state;
+		/* Defer I/O while rtpg_work is active */
+		if (pg->rtpg_sdev)
+			state = TPGS_STATE_TRANSITIONING;
+	}
+	rcu_read_unlock();
 	if (state == TPGS_STATE_TRANSITIONING)
 		ret = BLKPREP_DEFER;
 	else if (state != TPGS_STATE_OPTIMIZED &&

@@ -776,10 +964,13 @@ static int alua_bus_attach(struct scsi_device *sdev)
 	h = kzalloc(sizeof(*h) , GFP_KERNEL);
 	if (!h)
 		return -ENOMEM;
-	h->pg = NULL;
+	spin_lock_init(&h->pg_lock);
+	rcu_assign_pointer(h->pg, NULL);
 	h->rel_port = -1;
+	h->init_error = SCSI_DH_OK;
 	h->sdev = sdev;
 
+	mutex_init(&h->init_mutex);
 	err = alua_initialize(sdev, h);
 	if (err == SCSI_DH_NOMEM)
 		ret = -ENOMEM;

@@ -800,10 +991,18 @@ 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);
+	h->sdev = NULL;
+	spin_unlock(&h->pg_lock);
+	if (pg) {
+		synchronize_rcu();
+		if (pg->rtpg_sdev)
+			flush_delayed_work(&pg->rtpg_work);
+		kref_put(&pg->kref, release_port_group);
 	}
 	sdev->handler_data = NULL;
 	kfree(h);

@@ -824,16 +1023,24 @@ static int __init alua_init(void)
 {
 	int r;
 
+	kaluad_wq = create_workqueue("kaluad_wq");
+	if (!kaluad_wq) {
+		/* Temporary failure, bypass */
+		return SCSI_DH_DEV_TEMP_BUSY;
+	}
 	r = scsi_register_device_handler(&alua_dh);
-	if (r != 0)
+	if (r != 0) {
 		printk(KERN_ERR "%s: Failed to register scsi device handler",
 			ALUA_DH_NAME);
+		destroy_workqueue(kaluad_wq);
+	}
 	return r;
 }
 
 static void __exit alua_exit(void)
 {
 	scsi_unregister_device_handler(&alua_dh);
+	destroy_workqueue(kaluad_wq);
 }
 
 module_init(alua_init);

Signature · 1 line

1.8.5.6

kbuild test robot

build bot

2015-09-29 13:28 UTC #40 lore raw

Re: [PATCH 27/36] scsi_dh_alua: Use workqueue for RTPG

reproduce:

Hi Hannes,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

reproduce:

  # apt-get install sparse
  make ARCH=x86_64 allmodconfig
  make C=1 CF=-D__CHECK_ENDIAN__

sparse warnings: (new ones prefixed by >>)

drivers/scsi/device_handler/scsi_dh_alua.c:142:16: sparse: Variable length array is used. drivers/scsi/device_handler/scsi_dh_alua.c:171:16: sparse: Variable length array is used. drivers/scsi/device_handler/scsi_dh_alua.c:194:24: sparse: symbol 'alua_lookup_pg' was not declared. Should it be static? drivers/scsi/device_handler/scsi_dh_alua.c:222:24: sparse: symbol 'alua_get_pg' was not declared. Should it be static?

>> drivers/scsi/device_handler/scsi_dh_alua.c:856:14: sparse: incompatible types in comparison expression (different address spaces)

drivers/scsi/device_handler/scsi_dh_alua.c:900:14: sparse: incompatible types in comparison expression (different address spaces) drivers/scsi/device_handler/scsi_dh_alua.c:935:14: sparse: incompatible types in comparison expression (different address spaces)

vim +856 drivers/scsi/device_handler/scsi_dh_alua.c

840 struct alua_dh_data *h = sdev->handler_data;
841 struct alua_port_group *pg = NULL;
842 unsigned int optimize = 0, argc;
843 const char *p = params;
844 int result = SCSI_DH_OK;
845 unsigned long flags;
846
847 if ((sscanf(params, "%u", &argc) != 1) || (argc != 1))
848 return -EINVAL;
849
850 while (*p++)
851 ;
852 if ((sscanf(p, "%u", &optimize) != 1) || (optimize > 1))
853 return -EINVAL;
854
855 rcu_read_lock();

 > 856		pg = rcu_dereference(h->pg);

857 if (!pg) {
858 rcu_read_unlock();
859 return -ENXIO;
860 }
861 spin_lock_irqsave(&pg->lock, flags);
862 if (optimize)
863 pg->flags |= ALUA_OPTIMIZE_STPG;
864 else

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Bart Van Assche

2015-10-01 23:34 UTC #41 lore raw

Re: [PATCH 27/36] scsi_dh_alua: Use workqueue for RTPG

In reply to Hannes Reinecke

Quoted 56 lines · Hannes Reinecke

drivers/scsi/device_handler/scsi_dh_alua.c +56 -0

+static void alua_rtpg_work(struct work_struct *work)
+{
+	struct alua_port_group *pg =
+		container_of(work, struct alua_port_group, rtpg_work.work);
+	struct scsi_device *sdev;
+	LIST_HEAD(qdata_list);
+	int err = SCSI_DH_OK;
+	struct alua_queue_data *qdata, *tmp;
+	unsigned long flags;
+
+	spin_lock_irqsave(&pg->lock, flags);
+	sdev = pg->rtpg_sdev;
+	if (!sdev) {
+		WARN_ON(pg->flags & ALUA_PG_RUN_RTPG ||
+			pg->flags & ALUA_PG_RUN_STPG);
+		spin_unlock_irqrestore(&pg->lock, flags);
+		return;
+	}
+	pg->flags |= ALUA_PG_RUNNING;
+	if (pg->flags & ALUA_PG_RUN_RTPG) {
+		spin_unlock_irqrestore(&pg->lock, flags);
+		err = alua_rtpg(sdev, pg);
+		spin_lock_irqsave(&pg->lock, flags);
+		if (err == SCSI_DH_RETRY) {
+			pg->flags &= ~ALUA_PG_RUNNING;
+			spin_unlock_irqrestore(&pg->lock, flags);
+			queue_delayed_work(kaluad_wq, &pg->rtpg_work,
+					   pg->interval * HZ);
+			return;
+		}
+		pg->flags &= ~ALUA_PG_RUN_RTPG;
+		if (err != SCSI_DH_OK)
+			pg->flags &= ~ALUA_PG_RUN_STPG;
+	}
+	if (pg->flags & ALUA_PG_RUN_STPG) {
+		spin_unlock_irqrestore(&pg->lock, flags);
+		err = alua_stpg(sdev, pg);
+		spin_lock_irqsave(&pg->lock, flags);
+		pg->flags &= ~ALUA_PG_RUN_STPG;
+		if (err == SCSI_DH_RETRY) {
+			pg->flags |= ALUA_PG_RUN_RTPG;
+			pg->interval = 0;
+			pg->flags &= ~ALUA_PG_RUNNING;
+			spin_unlock_irqrestore(&pg->lock, flags);
+			queue_delayed_work(kaluad_wq, &pg->rtpg_work,
+					   pg->interval * HZ);
+			return;
+		}
+	}
+
+	list_splice_init(&pg->rtpg_list, &qdata_list);
+	pg->rtpg_sdev = NULL;
+	spin_unlock_irqrestore(&pg->lock, flags);
+
+	list_for_each_entry_safe(qdata, tmp, &qdata_list, entry) {
+		list_del(&qdata->entry);

drivers/scsi/device_handler/scsi_dh_alua.c static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg) · line 774

Quoted 10 lines

drivers/scsi/device_handler/scsi_dh_alua.c +10 -0

+		if (qdata->callback_fn)
+			qdata->callback_fn(qdata->callback_data, err);
+		kfree(qdata);
+	}
+	spin_lock_irqsave(&pg->lock, flags);
+	pg->flags &= ~ALUA_PG_RUNNING;
+	spin_unlock_irqrestore(&pg->lock, flags);
+	scsi_device_put(sdev);
+	kref_put(&pg->kref, release_port_group);
+}

With this patch series applied kmemleak reports several leaks that were not reported without this patch. Is scsi_device_put() + kref_put() always called before this function returns without requeueing the work item ? Shouldn't the return value of queue_delayed_work() be checked ? The leaks reported by kmemleak are:

unreferenced object 0xffff880423d31728 (size 128):
comm "kworker/2:3", pid 3589, jiffies 4294946634 (age 501.720s) hex dump (first 32 bytes):

    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff814ed75e>] kmemleak_alloc+0x4e/0xb0
    [<ffffffff811a9128>] kmem_cache_alloc_trace+0xc8/0x210
    [<ffffffffa0269d78>] alua_rtpg_work+0x1b8/0xc10 [scsi_dh_alua]
    [<ffffffff8108d9a8>] process_one_work+0x1d8/0x610
    [<ffffffff8108def4>] worker_thread+0x114/0x460
    [<ffffffff810941e8>] kthread+0xf8/0x110
    [<ffffffff814f5c2f>] ret_from_fork+0x3f/0x70
    [<ffffffffffffffff>] 0xffffffffffffffff
unreferenced object 0xffff88042ba00150 (size 8):
  comm "srp_daemon", pid 608, jiffies 4294946454 (age 503.510s)
  hex dump (first 8 bytes):
    68 6f 73 74 31 35 00 a5                          host15..
  backtrace:
    [<ffffffff814ed75e>] kmemleak_alloc+0x4e/0xb0
    [<ffffffff811abd53>] __kmalloc_track_caller+0xe3/0x240
    [<ffffffff81295ce2>] kvasprintf+0x52/0x80
    [<ffffffff8128adbf>] kobject_set_name_vargs+0x1f/0x60
    [<ffffffff81382217>] dev_set_name+0x47/0x50
    [<ffffffffa000dcaa>] scsi_host_alloc+0x32a/0x4b0 [scsi_mod]
    [<ffffffffa0279524>] srp_create_target+0x54/0x1410 [ib_srp]
    [<ffffffff81381bf8>] dev_attr_store+0x18/0x30
    [<ffffffff812300f4>] sysfs_kf_write+0x44/0x60
    [<ffffffff8122f724>] kernfs_fop_write+0x144/0x190
    [<ffffffff811b8788>] __vfs_write+0x28/0xe0
    [<ffffffff811b8e19>] vfs_write+0xa9/0x190
    [<ffffffff811b9b19>] SyS_write+0x49/0xa0
    [<ffffffff814f5876>] entry_SYSCALL_64_fastpath+0x16/0x7a
    [<ffffffffffffffff>] 0xffffffffffffffff

Bart.

Hannes Reinecke

author

2015-10-02 05:59 UTC #42 lore raw

Re: [PATCH 27/36] scsi_dh_alua: Use workqueue for RTPG

Quoted 65 lines · Bart Van Assche

Quoted 64 lines · Hannes Reinecke

drivers/scsi/device_handler/scsi_dh_alua.c +64 -0

+static void alua_rtpg_work(struct work_struct *work)
+{
+    struct alua_port_group *pg =
+        container_of(work, struct alua_port_group, rtpg_work.work);
+    struct scsi_device *sdev;
+    LIST_HEAD(qdata_list);
+    int err = SCSI_DH_OK;
+    struct alua_queue_data *qdata, *tmp;
+    unsigned long flags;
+
+    spin_lock_irqsave(&pg->lock, flags);
+    sdev = pg->rtpg_sdev;
+    if (!sdev) {
+        WARN_ON(pg->flags & ALUA_PG_RUN_RTPG ||
+            pg->flags & ALUA_PG_RUN_STPG);
+        spin_unlock_irqrestore(&pg->lock, flags);
+        return;
+    }
+    pg->flags |= ALUA_PG_RUNNING;
+    if (pg->flags & ALUA_PG_RUN_RTPG) {
+        spin_unlock_irqrestore(&pg->lock, flags);
+        err = alua_rtpg(sdev, pg);
+        spin_lock_irqsave(&pg->lock, flags);
+        if (err == SCSI_DH_RETRY) {
+            pg->flags &= ~ALUA_PG_RUNNING;
+            spin_unlock_irqrestore(&pg->lock, flags);
+            queue_delayed_work(kaluad_wq, &pg->rtpg_work,
+                       pg->interval * HZ);
+            return;
+        }
+        pg->flags &= ~ALUA_PG_RUN_RTPG;
+        if (err != SCSI_DH_OK)
+            pg->flags &= ~ALUA_PG_RUN_STPG;
+    }
+    if (pg->flags & ALUA_PG_RUN_STPG) {
+        spin_unlock_irqrestore(&pg->lock, flags);
+        err = alua_stpg(sdev, pg);
+        spin_lock_irqsave(&pg->lock, flags);
+        pg->flags &= ~ALUA_PG_RUN_STPG;
+        if (err == SCSI_DH_RETRY) {
+            pg->flags |= ALUA_PG_RUN_RTPG;
+            pg->interval = 0;
+            pg->flags &= ~ALUA_PG_RUNNING;
+            spin_unlock_irqrestore(&pg->lock, flags);
+            queue_delayed_work(kaluad_wq, &pg->rtpg_work,
+                       pg->interval * HZ);
+            return;
+        }
+    }
+
+    list_splice_init(&pg->rtpg_list, &qdata_list);
+    pg->rtpg_sdev = NULL;
+    spin_unlock_irqrestore(&pg->lock, flags);
+
+    list_for_each_entry_safe(qdata, tmp, &qdata_list, entry) {
+        list_del(&qdata->entry);
+        if (qdata->callback_fn)
+            qdata->callback_fn(qdata->callback_data, err);
+        kfree(qdata);
+    }
+    spin_lock_irqsave(&pg->lock, flags);
+    pg->flags &= ~ALUA_PG_RUNNING;
+    spin_unlock_irqrestore(&pg->lock, flags);
+    scsi_device_put(sdev);

Quoted 9 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+    kref_put(&pg->kref, release_port_group);
+}
With this patch series applied kmemleak reports several leaks that were not reported without this patch. Is scsi_device_put() + kref_put() always called before this function returns without requeueing the work item ? Shouldn't the return value of queue_delayed_work() be checked ? The leaks reported by kmemleak are:

Yes, you are right. I need to check queue_delayed_work() and issue a scsi_device_put()/kref_put() if the item is already queued.

Cheers,

Hannes

Signature · 9 lines

Dr. Hannes Reinecke		               zSeries & Storage
[email protected]			               +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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hannes Reinecke

author

2015-09-29 10:48 UTC #43 lore raw

PATCH v5 28/36 scsi_dh_alua: Recheck state on unit attention

In reply to Hannes Reinecke

When we receive a unit attention code of 'ALUA state changed' we should recheck the state, as it might be due to an implicit ALUA state transition. This allows us to return NEEDS_RETRY instead of ADD_TO_MLQUEUE, allowing to terminate the retries after a certain time.
At the same time a workqueue item might already be queued, which should be started immediately to avoid any delays.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 58 ++++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 11 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +47 ~~-11~~

index 1b7153f..21f0d20 100644
@@ -117,7 +117,8 @@ struct alua_queue_data {
 static void alua_rtpg_work(struct work_struct *work);
 static void alua_rtpg_queue(struct alua_port_group *pg,
 			    struct scsi_device *sdev,
-			    struct alua_queue_data *qdata);
+			    struct alua_queue_data *qdata, bool force);
+static void alua_check(struct scsi_device *sdev, bool force);
 
 static void release_port_group(struct kref *kref)
 {

@@ -387,7 +388,7 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 		rcu_assign_pointer(h->pg, pg);
 		pg_found = true;
 	}
-	alua_rtpg_queue(h->pg, sdev, NULL);
+	alua_rtpg_queue(h->pg, sdev, NULL, true);
 	spin_unlock(&h->pg_lock);
 
 	if (pg_found)

@@ -428,18 +429,24 @@ static int alua_check_sense(struct scsi_device *sdev,
 {
 	switch (sense_hdr->sense_key) {
 	case NOT_READY:
-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x0a)
+		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x0a) {
 			/*
 			 * LUN Not Accessible - ALUA state transition
 			 */
-			return ADD_TO_MLQUEUE;
+			alua_check(sdev, false);
+			return NEEDS_RETRY;
+		}
 		break;
 	case UNIT_ATTENTION:
-		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00)
+		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00) {
 			/*
-			 * Power On, Reset, or Bus Device Reset, just retry.
+			 * Power On, Reset, or Bus Device Reset.
+			 * Might have obscured a state transition,
+			 * so schedule a recheck.
 			 */
+			alua_check(sdev, true);
 			return ADD_TO_MLQUEUE;
+		}
 		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x04)
 			/*
 			 * Device internal reset

@@ -450,16 +457,20 @@ static int alua_check_sense(struct scsi_device *sdev,
 			 * Mode Parameters Changed
 			 */
 			return ADD_TO_MLQUEUE;
-		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x06)
+		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x06) {
 			/*
 			 * ALUA state changed
 			 */
+			alua_check(sdev, true);
 			return ADD_TO_MLQUEUE;
-		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x07)
+		}
+		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x07) {
 			/*
 			 * Implicit ALUA state transition failed
 			 */
+			alua_check(sdev, true);
 			return ADD_TO_MLQUEUE;
+		}
 		if (sense_hdr->asc == 0x3f && sense_hdr->ascq == 0x03)
 			/*
 			 * Inquiry data has changed

@@ -775,7 +786,7 @@ static void alua_rtpg_work(struct work_struct *work)
 
 static void alua_rtpg_queue(struct alua_port_group *pg,
 			    struct scsi_device *sdev,
-			    struct alua_queue_data *qdata)
+			    struct alua_queue_data *qdata, bool force)
 {
 	int start_queue = 0;
 	unsigned long flags;

@@ -795,7 +806,9 @@ static void alua_rtpg_queue(struct alua_port_group *pg,
 		pg->rtpg_sdev = sdev;
 		scsi_device_get(sdev);
 		start_queue = 1;
-	}
+	} else if (!(pg->flags & ALUA_PG_RUN_RTPG) && force)
+		start_queue = 1;
+
 	spin_unlock_irqrestore(&pg->lock, flags);
 
 	if (start_queue &&

@@ -910,7 +923,7 @@ static int alua_activate(struct scsi_device *sdev,
 	kref_get(&pg->kref);
 	rcu_read_unlock();
 
-	alua_rtpg_queue(pg, sdev, qdata);
+	alua_rtpg_queue(pg, sdev, qdata, true);
 	kref_put(&pg->kref, release_port_group);
 out:
 	if (fn)

@@ -919,6 +932,29 @@ out:
 }
 
 /*
+ * alua_check - check path status
+ * @sdev: device on the path to be checked
+ *
+ * Check the device status
+ */
+static void alua_check(struct scsi_device *sdev, bool force)
+{
+	struct alua_dh_data *h = sdev->handler_data;
+	struct alua_port_group *pg;
+
+	rcu_read_lock();
+	pg = rcu_dereference(h->pg);
+	if (!pg) {
+		rcu_read_unlock();
+		return;
+	}
+	kref_get(&pg->kref);
+	rcu_read_unlock();
+	alua_rtpg_queue(pg, sdev, NULL, force);
+	kref_put(&pg->kref, release_port_group);
+}
+
+/*
  * alua_prep_fn - request callback
  *
  * Fail I/O to all paths not in state

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:48 UTC #44 lore raw

PATCH v5 29/36 scsi_dh_alua: update all port states

In reply to Hannes Reinecke

When we read in the target port group state we should be updating all affected port groups, otherwise we risk running out of sync.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 31 +++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +22 -9

index 21f0d20..dd8682e 100644
@@ -500,11 +500,13 @@ static int alua_check_sense(struct scsi_device *sdev,
 static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 {
 	struct scsi_sense_hdr sense_hdr;
+	struct alua_port_group *tmp_pg;
 	int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;
-	unsigned char *ucp, *buff;
+	unsigned char *desc, *buff;
 	unsigned err, retval;
 	unsigned int tpg_desc_tbl_off;
 	unsigned char orig_transition_tmo;
+	unsigned long flags;
 
 	if (!pg->expiry) {
 		if (!pg->transition_tmo)

@@ -604,16 +606,27 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	else
 		tpg_desc_tbl_off = 4;
 
-	for (k = tpg_desc_tbl_off, ucp = buff + tpg_desc_tbl_off;
+	for (k = tpg_desc_tbl_off, desc = buff + tpg_desc_tbl_off;
 	     k < len;
-	     k += off, ucp += off) {
-
-		if (pg->group_id == get_unaligned_be16(&ucp[2])) {
-			pg->state = ucp[0] & 0x0f;
-			pg->pref = ucp[0] >> 7;
-			valid_states = ucp[1];
+	     k += off, desc += off) {
+		u16 group_id = get_unaligned_be16(&desc[2]);
+
+		spin_lock_irqsave(&port_group_lock, flags);
+		list_for_each_entry(tmp_pg, &port_group_list, node) {
+			if (tmp_pg->group_id != group_id)
+				continue;
+			if (tmp_pg->device_id_len != pg->device_id_len)
+				continue;
+			if (strncmp(tmp_pg->device_id_str, pg->device_id_str,
+				    tmp_pg->device_id_len))
+				continue;
+			tmp_pg->state = desc[0] & 0x0f;
+			tmp_pg->pref = desc[0] >> 7;
+			if (tmp_pg == pg)
+				valid_states = desc[1];
 		}
-		off = 8 + (ucp[7] * 4);
+		spin_unlock_irqrestore(&port_group_lock, flags);
+		off = 8 + (desc[7] * 4);
 	}
 
 	sdev_printk(KERN_INFO, sdev,

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:48 UTC #45 lore raw

PATCH v5 30/36 scsi_dh_alua: Send TEST UNIT READY to poll for transitioning

In reply to Hannes Reinecke

Sending a 'REPORT TARGET PORT GROUP' command is a costly operation, as the array has to gather information about all ports. So instead of using RTPG to poll for a status update when a port is in transitioning we should be sending a TEST UNIT READY, and wait for the sense code to report success.

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Ewan Milne <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 37 ++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

drivers/scsi/device_handler/scsi_dh_alua.c +37 -0

index dd8682e..7476c8a 100644
@@ -490,6 +490,30 @@ static int alua_check_sense(struct scsi_device *sdev,
 }
 
 /*
+ * alua_tur - Send a TEST UNIT READY
+ * @sdev: device to which the TEST UNIT READY command should be send
+ *
+ * Send a TEST UNIT READY to @sdev to figure out the device state
+ * Returns SCSI_DH_RETRY if the sense code is NOT READY/ALUA TRANSITIONING,
+ * SCSI_DH_OK if no error occured, and SCSI_DH_IO otherwise.
+ */
+static int alua_tur(struct scsi_device *sdev)
+{
+	struct scsi_sense_hdr sense_hdr;
+	int retval;
+
+	retval = scsi_test_unit_ready(sdev, ALUA_FAILOVER_TIMEOUT * HZ,
+				      ALUA_FAILOVER_RETRIES, &sense_hdr);
+	if (sense_hdr.sense_key == NOT_READY &&
+	    sense_hdr.asc == 0x04 && sense_hdr.ascq == 0x0a)
+		return SCSI_DH_RETRY;
+	else if (retval)
+		return SCSI_DH_IO;
+	else
+		return SCSI_DH_OK;
+}
+
+/*
  * alua_rtpg - Evaluate REPORT TARGET GROUP STATES
  * @sdev: the device to be evaluated.
  *

@@ -750,7 +774,20 @@ static void alua_rtpg_work(struct work_struct *work)
 	}
 	pg->flags |= ALUA_PG_RUNNING;
 	if (pg->flags & ALUA_PG_RUN_RTPG) {
+		int state = pg->state;
+
 		spin_unlock_irqrestore(&pg->lock, flags);
+		if (state == TPGS_STATE_TRANSITIONING) {
+			if (alua_tur(sdev) == SCSI_DH_RETRY) {
+				spin_lock_irqsave(&pg->lock, flags);
+				pg->flags &= ~ALUA_PG_RUNNING;
+				spin_unlock_irqrestore(&pg->lock, flags);
+				queue_delayed_work(kaluad_wq, &pg->rtpg_work,
+						   pg->interval * HZ);
+				return;
+			}
+			/* Send RTPG on failure or if TUR indicates SUCCESS */
+		}
 		err = alua_rtpg(sdev, pg);
 		spin_lock_irqsave(&pg->lock, flags);
 		if (err == SCSI_DH_RETRY) {

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:47 UTC #46 lore raw

PATCH v5 31/36 scsi: rescan VPD attributes

In reply to Hannes Reinecke

This patch implements a VPD page rescan if the 'rescan' sysfs attribute is triggered.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/scsi.c        | 20 +++++++++++++++++---
 drivers/scsi/scsi_lib.c    | 27 ++++++++++++++++++---------
 drivers/scsi/scsi_scan.c   |  4 ++++
 drivers/scsi/scsi_sysfs.c  |  8 ++++++--
 drivers/scsi/ses.c         | 13 +++++++++----
 include/scsi/scsi_device.h |  1 +
 6 files changed, 55 insertions(+), 18 deletions(-)

drivers/scsi/scsi.c +17 -3

index 207d6a7..f1c0fb5 100644
@@ -803,7 +803,7 @@ void scsi_attach_vpd(struct scsi_device *sdev)
 	int vpd_len = SCSI_VPD_PG_LEN;
 	int pg80_supported = 0;
 	int pg83_supported = 0;
-	unsigned char *vpd_buf;
+	unsigned char *vpd_buf, *orig_vpd_buf = NULL;
 
 	if (sdev->skip_vpd_pages)
 		return;

@@ -849,8 +849,16 @@ retry_pg80:
 			kfree(vpd_buf);
 			goto retry_pg80;
 		}
+		mutex_lock(&sdev->inquiry_mutex);
+		orig_vpd_buf = sdev->vpd_pg80;
 		sdev->vpd_pg80_len = result;
-		sdev->vpd_pg80 = vpd_buf;
+		rcu_assign_pointer(sdev->vpd_pg80, vpd_buf);
+		mutex_unlock(&sdev->inquiry_mutex);
+		synchronize_rcu();
+		if (orig_vpd_buf) {
+			kfree(orig_vpd_buf);
+			orig_vpd_buf = NULL;
+		}
 		vpd_len = SCSI_VPD_PG_LEN;
 	}

@@ -870,8 +878,14 @@ retry_pg83:
 			kfree(vpd_buf);
 			goto retry_pg83;
 		}
+		mutex_lock(&sdev->inquiry_mutex);
+		orig_vpd_buf = sdev->vpd_pg83;
 		sdev->vpd_pg83_len = result;
-		sdev->vpd_pg83 = vpd_buf;
+		rcu_assign_pointer(sdev->vpd_pg83, vpd_buf);
+		mutex_unlock(&sdev->inquiry_mutex);
+		synchronize_rcu();
+		if (orig_vpd_buf)
+			kfree(orig_vpd_buf);
 	}
 }

drivers/scsi/scsi_lib.c +18 -9

index fd4203c..b069b1e 100644
@@ -3174,11 +3174,15 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
 {
 	u8 cur_id_type = 0xff;
 	u8 cur_id_size = 0;
-	unsigned char *d, *cur_id_str;
+	unsigned char *d, *cur_id_str, *vpd_pg83;
 	int id_size = -EAGAIN;
 
-	if (!sdev->vpd_pg83)
+	rcu_read_lock();
+	vpd_pg83 = rcu_dereference(sdev->vpd_pg83);
+	if (!vpd_pg83) {
+		rcu_read_unlock();
 		return -ENXIO;
+	}
 
 	/*
 	 * Look for the correct descriptor.

@@ -3198,8 +3202,8 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
 		return -EINVAL;
 
 	memset(id, 0, id_len);
-	d = sdev->vpd_pg83 + 4;
-	while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
+	d = vpd_pg83 + 4;
+	while (d < vpd_pg83 + sdev->vpd_pg83_len) {
 		/* Skip designators not referring to the LUN */
 		if ((d[1] & 0x30) != 0x00)
 			goto next_desig;

@@ -3283,6 +3287,7 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
 next_desig:
 		d += d[3] + 4;
 	}
+	rcu_read_unlock();
 
 	return id_size;
 }

@@ -3299,14 +3304,17 @@ EXPORT_SYMBOL(scsi_vpd_lun_id);
  */
 int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id)
 {
-	unsigned char *d;
+	unsigned char *d, *vpd_pg83;
 	int group_id = -EAGAIN, rel_port = -1;
 
-	if (!sdev->vpd_pg83)
+	rcu_read_lock();
+	vpd_pg83 = rcu_dereference(sdev->vpd_pg83);
+	if (!vpd_pg83) {
+		rcu_read_unlock();
 		return -ENXIO;
-
-	d = sdev->vpd_pg83 + 4;
-	while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
+	}
+	d = vpd_pg83 + 4;
+	while (d < vpd_pg83 + sdev->vpd_pg83_len) {
 		switch (d[1] & 0xf) {
 		case 0x4:
 			/* Relative target port */

@@ -3321,6 +3329,7 @@ int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id)
 		}
 		d += d[3] + 4;
 	}
+	rcu_read_unlock();
 
 	if (group_id >= 0 && rel_id && rel_port != -1)
 		*rel_id = rel_port;

drivers/scsi/scsi_scan.c +4 -0

index f9f3f82..190d743 100644
@@ -235,6 +235,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	INIT_LIST_HEAD(&sdev->starved_entry);
 	INIT_LIST_HEAD(&sdev->event_list);
 	spin_lock_init(&sdev->list_lock);
+	mutex_init(&sdev->inquiry_mutex);
 	INIT_WORK(&sdev->event_work, scsi_evt_thread);
 	INIT_WORK(&sdev->requeue_work, scsi_requeue_run_queue);

@@ -1516,6 +1517,9 @@ EXPORT_SYMBOL(scsi_add_device);
 void scsi_rescan_device(struct device *dev)
 {
 	device_lock(dev);
+
+	scsi_attach_vpd(to_scsi_device(dev));
+
 	if (dev->driver && try_module_get(dev->driver->owner)) {
 		struct scsi_driver *drv = to_scsi_driver(dev->driver);

drivers/scsi/scsi_sysfs.c +6 -2

index 81d05ec..37799dc 100644
@@ -759,11 +759,15 @@ show_vpd_##_page(struct file *filp, struct kobject *kobj,	\
 {									\
 	struct device *dev = container_of(kobj, struct device, kobj);	\
 	struct scsi_device *sdev = to_scsi_device(dev);			\
+	int ret;							\
 	if (!sdev->vpd_##_page)						\
 		return -EINVAL;						\
-	return memory_read_from_buffer(buf, count, &off,		\
-				       sdev->vpd_##_page,		\
+	rcu_read_lock();						\
+	ret = memory_read_from_buffer(buf, count, &off,			\
+				      rcu_dereference(sdev->vpd_##_page), \
 				       sdev->vpd_##_page##_len);	\
+	rcu_read_unlock();						\
+	return ret;						\
 }									\
 static struct bin_attribute dev_attr_vpd_##_page = {		\
 	.attr =	{.name = __stringify(vpd_##_page), .mode = S_IRUGO },	\

drivers/scsi/ses.c +9 -4

index dcb0d76..415f25b 100644
@@ -553,18 +553,22 @@ static void ses_enclosure_data_process(struct enclosure_device *edev,
 static void ses_match_to_enclosure(struct enclosure_device *edev,
 				   struct scsi_device *sdev)
 {
-	unsigned char *desc;
+	unsigned char *desc, *vpd_pg83;
 	struct efd efd = {
 		.addr = 0,
 	};
 
 	ses_enclosure_data_process(edev, to_scsi_device(edev->edev.parent), 0);
 
-	if (!sdev->vpd_pg83_len)
+	rcu_read_lock();
+	vpd_pg83 = rcu_dereference(sdev->vpd_pg83);
+	if (!vpd_pg83) {
+		rcu_read_unlock();
 		return;
+	}
 
-	desc = sdev->vpd_pg83 + 4;
-	while (desc < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
+	desc = vpd_pg83 + 4;
+	while (desc < vpd_pg83 + sdev->vpd_pg83_len) {
 		enum scsi_protocol proto = desc[0] >> 4;
 		u8 code_set = desc[0] & 0x0f;
 		u8 piv = desc[1] & 0x80;

@@ -578,6 +582,7 @@ static void ses_match_to_enclosure(struct enclosure_device *edev,
 
 		desc += len + 4;
 	}
+	rcu_read_unlock();
 	if (efd.addr) {
 		efd.dev = &sdev->sdev_gendev;

include/scsi/scsi_device.h +1 -0

index cc6e763..cfc23a4 100644
@@ -109,6 +109,7 @@ struct scsi_device {
 	char type;
 	char scsi_level;
 	char inq_periph_qual;	/* PQ from INQUIRY data */	
+	struct mutex inquiry_mutex;
 	unsigned char inquiry_len;	/* valid bytes in 'inquiry' */
 	unsigned char * inquiry;	/* INQUIRY response data */
 	const char * vendor;		/* [back_compat] point into 'inquiry' ... */

Signature · 1 line

1.8.5.6

kbuild test robot

build bot

2015-09-29 13:40 UTC #47 lore raw

Re: [PATCH 31/36] scsi: rescan VPD attributes

reproduce:

Hi Hannes,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

reproduce:

  # apt-get install sparse
  make ARCH=x86_64 allmodconfig
  make C=1 CF=-D__CHECK_ENDIAN__

sparse warnings: (new ones prefixed by >>)

>> drivers/scsi/scsi_lib.c:3181:20: sparse: incompatible types in comparison expression (different address spaces)

drivers/scsi/scsi_lib.c:3311:20: sparse: incompatible types in comparison expression (different address spaces)
--
drivers/scsi/scsi_sysfs.c:202:25: sparse: symbol 'dev_attr_hstate' was not declared. Should it be static? drivers/scsi/scsi_sysfs.c:377:24: sparse: symbol 'scsi_shost_attr_group' was not declared. Should it be static?

>> drivers/scsi/scsi_sysfs.c:778:1: sparse: incompatible types in comparison expression (different address spaces)
   drivers/scsi/scsi_sysfs.c:779:1: sparse: incompatible types in comparison expression (different address spaces)
--
>> drivers/scsi/ses.c:564:20: sparse: incompatible types in comparison expression (different address spaces)

vim +3181 drivers/scsi/scsi_lib.c

3165 * Copies a unique device identification into @id based
3166 * on the information in the VPD page 0x83 of the device.
3167 * The string will be formatted as a SCSI name string.
3168 *
3169 * Returns the length of the identification or error on failure.
3170 * If the identifier is longer than the supplied buffer the actual
3171 * identifier length is returned and the buffer is not zero-padded.
3172 */
3173 int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
3174 {
3175 u8 cur_id_type = 0xff;
3176 u8 cur_id_size = 0;
3177 unsigned char *d, *cur_id_str, *vpd_pg83;
3178 int id_size = -EAGAIN;
3179
3180 rcu_read_lock();

> 3181		vpd_pg83 = rcu_dereference(sdev->vpd_pg83);

3182 if (!vpd_pg83) {
3183 rcu_read_unlock();
3184 return -ENXIO;
3185 }
3186
3187 /*
3188 * Look for the correct descriptor.
3189 * Order of preference for lun descriptor:

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

kbuild test robot

build bot

2015-09-29 13:41 UTC #48 lore raw

[PATCH] scsi: fix ifnullfree.cocci warnings

In reply to Hannes Reinecke

drivers/scsi/scsi.c:888:3-8: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant…

drivers/scsi/scsi.c:888:3-8: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values.

NULL check before some freeing functions is not needed.

Based on checkpatch warning
"kfree(NULL) is safe this check is probably not required" and kfreeaddr.cocci by Julia Lawall.

Generated by: scripts/coccinelle/free/ifnullfree.cocci

CC Hannes Reinecke <[email protected]>

Signed-off-by Fengguang Wu <[email protected]>

---

 scsi.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/scsi/scsi.c +1 -2

@@ -884,8 +884,7 @@ retry_pg83:
 		rcu_assign_pointer(sdev->vpd_pg83, vpd_buf);
 		mutex_unlock(&sdev->inquiry_mutex);
 		synchronize_rcu();
-		if (orig_vpd_buf)
-			kfree(orig_vpd_buf);
+		kfree(orig_vpd_buf);
 	}
 }

kbuild test robot

build bot

2015-09-29 13:41 UTC #49 lore raw

Re: [PATCH 31/36] scsi: rescan VPD attributes

In reply to Hannes Reinecke

coccinelle warnings: (new ones prefixed by >>)

Hi Hannes,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

coccinelle warnings: (new ones prefixed by >>)

>> drivers/scsi/scsi.c:888:3-8: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values.

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Hannes Reinecke

author

2015-09-29 10:48 UTC #50 lore raw

PATCH v5 32/36 scsi_dh: add 'rescan' callback

In reply to Hannes Reinecke

If a device needs to be rescanned the device_handler might need to be rechecked, too.
So add a 'rescan' callback to the device handler and call it upon scsi_rescan_device().

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 8 ++++++++
 drivers/scsi/scsi_lib.c                    | 1 +
 drivers/scsi/scsi_scan.c                   | 8 +++++++-
 include/scsi/scsi_dh.h                     | 1 +
 4 files changed, 17 insertions(+), 1 deletion(-)

drivers/scsi/device_handler/scsi_dh_alua.c +8 -0

index 7476c8a..4ed889b 100644
@@ -1038,6 +1038,13 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 
 }
 
+static void alua_rescan(struct scsi_device *sdev)
+{
+	struct alua_dh_data *h = sdev->handler_data;
+
+	alua_initialize(sdev, h);
+}
+
 /*
  * alua_bus_attach - Attach device handler
  * @sdev: device to be attached to

@@ -1102,6 +1109,7 @@ static struct scsi_device_handler alua_dh = {
 	.prep_fn = alua_prep_fn,
 	.check_sense = alua_check_sense,
 	.activate = alua_activate,
+	.rescan = alua_rescan,
 	.set_params = alua_set_params,
 };

drivers/scsi/scsi_lib.c +1 -0

index b069b1e..fef76d9 100644
@@ -2699,6 +2699,7 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
 		envp[idx++] = "SDEV_MEDIA_CHANGE=1";
 		break;
 	case SDEV_EVT_INQUIRY_CHANGE_REPORTED:
+		scsi_rescan_device(&sdev->sdev_gendev);
 		envp[idx++] = "SDEV_UA=INQUIRY_DATA_HAS_CHANGED";
 		break;
 	case SDEV_EVT_CAPACITY_CHANGE_REPORTED:

drivers/scsi/scsi_scan.c +7 -1

index 190d743..5d3e2ae 100644
@@ -43,6 +43,7 @@
 #include <scsi/scsi_devinfo.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_transport.h>
+#include <scsi/scsi_dh.h>
 #include <scsi/scsi_eh.h>
 
 #include "scsi_priv.h"

@@ -1516,9 +1517,14 @@ EXPORT_SYMBOL(scsi_add_device);
 
 void scsi_rescan_device(struct device *dev)
 {
+	struct scsi_device *sdev = to_scsi_device(dev);
+
 	device_lock(dev);
 
-	scsi_attach_vpd(to_scsi_device(dev));
+	scsi_attach_vpd(sdev);
+
+	if (sdev->handler && sdev->handler->rescan)
+		sdev->handler->rescan(sdev);
 
 	if (dev->driver && try_module_get(dev->driver->owner)) {
 		struct scsi_driver *drv = to_scsi_driver(dev->driver);

include/scsi/scsi_dh.h +1 -0

index 7e184c6..c7bba2b 100644
@@ -71,6 +71,7 @@ struct scsi_device_handler {
 	int (*activate)(struct scsi_device *, activate_complete, void *);
 	int (*prep_fn)(struct scsi_device *, struct request *);
 	int (*set_params)(struct scsi_device *, const char *);
+	void (*rescan)(struct scsi_device *);
 };
 
 #ifdef CONFIG_SCSI_DH

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:48 UTC #51 lore raw

PATCH v5 33/36 scsi: Add 'access_state' attribute

In reply to Hannes Reinecke

Add an 'access_state' attribute to struct scsi_device to display the asymmetric LUN access state.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/scsi_scan.c   |  1 +
 drivers/scsi/scsi_sysfs.c  | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h |  1 +
 include/scsi/scsi_proto.h  | 13 ++++++++++++
 4 files changed, 64 insertions(+)

drivers/scsi/scsi_scan.c +1 -0

index 5d3e2ae..76e771e 100644
@@ -230,6 +230,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	sdev->lun = lun;
 	sdev->channel = starget->channel;
 	sdev->sdev_state = SDEV_CREATED;
+	sdev->access_state = SCSI_ACCESS_STATE_UNKNOWN;
 	INIT_LIST_HEAD(&sdev->siblings);
 	INIT_LIST_HEAD(&sdev->same_target_siblings);
 	INIT_LIST_HEAD(&sdev->cmd_list);

drivers/scsi/scsi_sysfs.c +49 -0

67 lines not on this page · raw message

include/scsi/scsi_device.h +1 -0

index cfc23a4..666359f 100644
@@ -200,6 +200,7 @@ struct scsi_device {
 	struct scsi_device_handler *handler;
 	void			*handler_data;
 
+	enum scsi_access_state	access_state;
 	enum scsi_device_state sdev_state;
 	unsigned long		sdev_data[0];
 } __attribute__((aligned(sizeof(unsigned long))));

include/scsi/scsi_proto.h +13 -0

index a9fbf1b..80e85e7 100644
@@ -277,5 +277,18 @@ struct scsi_lun {
 	__u8 scsi_lun[8];
 };
 
+/* SPC asymmetric access states */
+enum scsi_access_state {
+	SCSI_ACCESS_STATE_OPTIMAL = 0,
+	SCSI_ACCESS_STATE_ACTIVE,
+	SCSI_ACCESS_STATE_STANDBY,
+	SCSI_ACCESS_STATE_UNAVAILABLE,
+	SCSI_ACCESS_STATE_LBA,
+	SCSI_ACCESS_STATE_OFFLINE = 0xe,
+	SCSI_ACCESS_STATE_TRANSITIONING = 0xf,
+	SCSI_ACCESS_STATE_UNKNOWN = 0x70,
+};
+#define SCSI_ACCESS_STATE_MASK 0x0f
+#define SCSI_ACCESS_STATE_PREFERRED 0x80
 
 #endif /* _SCSI_PROTO_H_ */

Signature · 1 line

1.8.5.6

kbuild test robot

build bot

2015-09-29 13:51 UTC #52 lore raw

Re: [PATCH 33/36] scsi: Add 'access_state' attribute

reproduce:

Hi Hannes,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

reproduce:

  # apt-get install sparse
  make ARCH=x86_64 allmodconfig
  make C=1 CF=-D__CHECK_ENDIAN__

sparse warnings: (new ones prefixed by >>)

>> drivers/scsi/scsi_sysfs.c:98:12: sparse: symbol 'scsi_access_state_name' was not declared. Should it be static?

drivers/scsi/scsi_sysfs.c:230:25: sparse: symbol 'dev_attr_hstate' was not declared. Should it be static? drivers/scsi/scsi_sysfs.c:405:24: sparse: symbol 'scsi_shost_attr_group' was not declared. Should it be static? drivers/scsi/scsi_sysfs.c:806:1: sparse: incompatible types in comparison expression (different address spaces) drivers/scsi/scsi_sysfs.c:807:1: sparse: incompatible types in comparison expression (different address spaces)

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Bart Van Assche

2015-10-01 23:04 UTC #53 lore raw

Re: [PATCH 33/36] scsi: Add 'access_state' attribute

In reply to Hannes Reinecke

drivers/scsi/scsi_sysfs.c const char *scsi_host_state_name(enum scsi_host_state state) · line 86

Quoted 3 lines · Hannes Reinecke

drivers/scsi/scsi_sysfs.c +3 -0

+static const struct {
+	enum scsi_access_state	value;
+	char			*name;

Had you considered to use "const char *" here instead of "char *" ?

drivers/scsi/scsi_sysfs.c const char *scsi_host_state_name(enum scsi_host_state state) · line 110

Quoted 13 lines

drivers/scsi/scsi_sysfs.c +13 -0

+const char *scsi_access_state_name(enum scsi_access_state state)
+{
+	int i;
+	char *name = NULL;
+
+	for (i = 0; i < ARRAY_SIZE(sdev_access_states); i++) {
+		if (sdev_access_states[i].value == state) {
+			name = sdev_access_states[i].name;
+			break;
+		}
+	}
+	return name;
+}

The return value of this function is passed without further processing to the format specifier "%s". How about initializing "name" to "(?)" to avoid that "(null)" is printed if the access state is not recognized ?

Quoted 8 lines

drivers/scsi/scsi_sysfs.c +8 -0

+static ssize_t
+sdev_show_access_state(struct device *dev,
+		       struct device_attribute *attr,
+		       char *buf)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	enum scsi_access_state access_state;
+	bool pref = false;

drivers/scsi/scsi_sysfs.c sdev_store_dh_state(struct device *dev, struct device_attribute *attr, · line 981

Quoted 10 lines

drivers/scsi/scsi_sysfs.c +10 -0

+
+	if (sdev->access_state & SCSI_ACCESS_STATE_PREFERRED)
+		pref = true;
+
+	access_state = (sdev->access_state & SCSI_ACCESS_STATE_MASK);
+
+	return snprintf(buf, 32, "%s%s\n",
+			scsi_access_state_name(access_state),
+			pref ? " preferred" :"");
+}

Shouldn't this function check whether a device handler has been associated with sdev and also whether the active device handler is the ALUA device handler ? Otherwise incorrect data will be reported before the ALUA device handler has been loaded, after it has been unloaded or if another device handler is active.

Additionally, please insert a separator between the preferred state and the access state name. Had you considered to use PAGE_SIZE instead of "32" as output buffer length ? Magic constants in code always make the reader wonder where these come from ...

How about reporting the target port group ID next to the ALUA state ? I think that data would be very useful because it allows users to verify which LUNs have been associated with which port group by this patch series.

Thanks,

Bart.

Hannes Reinecke

author

2015-09-29 10:48 UTC #54 lore raw

PATCH v5 34/36 scsi_dh_alua: use common definitions for ALUA state

In reply to Hannes Reinecke

scsi_proto.h now contains definitions for the ALUA state, so we don't have to carry them in the device handler.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 62 +++++++++++++-----------------
 1 file changed, 27 insertions(+), 35 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +27 ~~-35~~

index 4ed889b..d9c7b42 100644
@@ -31,14 +31,6 @@
 #define ALUA_DH_NAME "alua"
 #define ALUA_DH_VER "1.3"
 
-#define TPGS_STATE_OPTIMIZED		0x0
-#define TPGS_STATE_NONOPTIMIZED		0x1
-#define TPGS_STATE_STANDBY		0x2
-#define TPGS_STATE_UNAVAILABLE		0x3
-#define TPGS_STATE_LBA_DEPENDENT	0x4
-#define TPGS_STATE_OFFLINE		0xe
-#define TPGS_STATE_TRANSITIONING	0xf
-
 #define TPGS_SUPPORT_NONE		0x00
 #define TPGS_SUPPORT_OPTIMIZED		0x01
 #define TPGS_SUPPORT_NONOPTIMIZED	0x02

@@ -177,7 +169,7 @@ static int submit_stpg(struct scsi_device *sdev, int group_id,
 
 	/* Prepare the data buffer */
 	memset(stpg_data, 0, stpg_len);
-	stpg_data[4] = TPGS_STATE_OPTIMIZED & 0x0f;
+	stpg_data[4] = SCSI_ACCESS_STATE_OPTIMAL & 0x0f;
 	put_unaligned_be16(group_id, &stpg_data[6]);
 
 	/* Prepare the command. */

@@ -244,7 +236,7 @@ struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
 	pg->device_id_len = id_size;
 	pg->group_id = group_id;
 	pg->tpgs = tpgs;
-	pg->state = TPGS_STATE_OPTIMIZED;
+	pg->state = SCSI_ACCESS_STATE_OPTIMAL;
 	if (optimize_stpg)
 		pg->flags |= ALUA_OPTIMIZE_STPG;
 	kref_init(&pg->kref);

@@ -402,22 +394,22 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 	return SCSI_DH_OK;
 }
 
-static char print_alua_state(int state)
+static char print_alua_state(enum scsi_access_state state)
 {
 	switch (state) {
-	case TPGS_STATE_OPTIMIZED:
+	case SCSI_ACCESS_STATE_OPTIMAL:
 		return 'A';
-	case TPGS_STATE_NONOPTIMIZED:
+	case SCSI_ACCESS_STATE_ACTIVE:
 		return 'N';
-	case TPGS_STATE_STANDBY:
+	case SCSI_ACCESS_STATE_STANDBY:
 		return 'S';
-	case TPGS_STATE_UNAVAILABLE:
+	case SCSI_ACCESS_STATE_UNAVAILABLE:
 		return 'U';
-	case TPGS_STATE_LBA_DEPENDENT:
+	case SCSI_ACCESS_STATE_LBA:
 		return 'L';
-	case TPGS_STATE_OFFLINE:
+	case SCSI_ACCESS_STATE_OFFLINE:
 		return 'O';
-	case TPGS_STATE_TRANSITIONING:
+	case SCSI_ACCESS_STATE_TRANSITIONING:
 		return 'T';
 	default:
 		return 'X';

@@ -666,7 +658,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		    valid_states&TPGS_SUPPORT_OPTIMIZED?'A':'a');
 
 	switch (pg->state) {
-	case TPGS_STATE_TRANSITIONING:
+	case SCSI_ACCESS_STATE_TRANSITIONING:
 		if (time_before(jiffies, pg->expiry)) {
 			/* State transition, retry */
 			pg->interval = 2;

@@ -674,11 +666,11 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		} else {
 			/* Transitioning time exceeded, set port to standby */
 			err = SCSI_DH_IO;
-			pg->state = TPGS_STATE_STANDBY;
+			pg->state = SCSI_ACCESS_STATE_STANDBY;
 			pg->expiry = 0;
 		}
 		break;
-	case TPGS_STATE_OFFLINE:
+	case SCSI_ACCESS_STATE_OFFLINE:
 		/* Path unusable */
 		err = SCSI_DH_DEV_OFFLINED;
 		pg->expiry = 0;

@@ -710,21 +702,21 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		return SCSI_DH_RETRY;
 	}
 	switch (pg->state) {
-	case TPGS_STATE_OPTIMIZED:
+	case SCSI_ACCESS_STATE_OPTIMAL:
 		return SCSI_DH_OK;
-	case TPGS_STATE_NONOPTIMIZED:
+	case SCSI_ACCESS_STATE_ACTIVE:
 		if ((pg->flags & ALUA_OPTIMIZE_STPG) &&
 		    !pg->pref &&
 		    (pg->tpgs & TPGS_MODE_IMPLICIT))
 			return SCSI_DH_OK;
 		break;
-	case TPGS_STATE_STANDBY:
-	case TPGS_STATE_UNAVAILABLE:
+	case SCSI_ACCESS_STATE_STANDBY:
+	case SCSI_ACCESS_STATE_UNAVAILABLE:
 		break;
-	case TPGS_STATE_OFFLINE:
+	case SCSI_ACCESS_STATE_OFFLINE:
 		return SCSI_DH_IO;
 		break;
-	case TPGS_STATE_TRANSITIONING:
+	case SCSI_ACCESS_STATE_TRANSITIONING:
 		break;
 	default:
 		sdev_printk(KERN_INFO, sdev,

@@ -734,7 +726,7 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		break;
 	}
 	/* Set state to transitioning */
-	pg->state = TPGS_STATE_TRANSITIONING;
+	pg->state = SCSI_ACCESS_STATE_TRANSITIONING;
 	retval = submit_stpg(sdev, pg->group_id, &sense_hdr);
 
 	if (retval) {

@@ -777,7 +769,7 @@ static void alua_rtpg_work(struct work_struct *work)
 		int state = pg->state;
 
 		spin_unlock_irqrestore(&pg->lock, flags);
-		if (state == TPGS_STATE_TRANSITIONING) {
+		if (state == SCSI_ACCESS_STATE_TRANSITIONING) {
 			if (alua_tur(sdev) == SCSI_DH_RETRY) {
 				spin_lock_irqsave(&pg->lock, flags);
 				pg->flags &= ~ALUA_PG_RUNNING;

@@ -1014,7 +1006,7 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 {
 	struct alua_dh_data *h = sdev->handler_data;
 	struct alua_port_group *pg;
-	int state = TPGS_STATE_OPTIMIZED;
+	enum scsi_access_state state = SCSI_ACCESS_STATE_OPTIMAL;
 	int ret = BLKPREP_OK;
 
 	rcu_read_lock();

@@ -1023,14 +1015,14 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 		state = pg->state;
 		/* Defer I/O while rtpg_work is active */
 		if (pg->rtpg_sdev)
-			state = TPGS_STATE_TRANSITIONING;
+			state = SCSI_ACCESS_STATE_TRANSITIONING;
 	}
 	rcu_read_unlock();
-	if (state == TPGS_STATE_TRANSITIONING)
+	if (state == SCSI_ACCESS_STATE_TRANSITIONING)
 		ret = BLKPREP_DEFER;
-	else if (state != TPGS_STATE_OPTIMIZED &&
-		 state != TPGS_STATE_NONOPTIMIZED &&
-		 state != TPGS_STATE_LBA_DEPENDENT) {
+	else if (state != SCSI_ACCESS_STATE_OPTIMAL &&
+		 state != SCSI_ACCESS_STATE_ACTIVE &&
+		 state != SCSI_ACCESS_STATE_LBA) {
 		ret = BLKPREP_KILL;
 		req->cmd_flags |= REQ_QUIET;
 	}

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:48 UTC #55 lore raw

PATCH v5 35/36 scsi_dh_alua: update 'access_state' field

In reply to Hannes Reinecke

Track attached SCSI devices and update the 'access_state' field whenever an ALUA state change has been detected.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

drivers/scsi/device_handler/scsi_dh_alua.c +29 -0

index d9c7b42..12e6ce9 100644
@@ -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>

@@ -72,6 +73,7 @@ static struct workqueue_struct *kaluad_wq;
 struct alua_port_group {
 	struct kref		kref;
 	struct list_head	node;
+	struct list_head	dh_list;
 	unsigned char		device_id_str[256];
 	int			device_id_len;
 	int			group_id;

@@ -89,6 +91,7 @@ struct alua_port_group {
 };
 
 struct alua_dh_data {
+	struct list_head	node;
 	struct alua_port_group	*pg;
 	int			rel_port;
 	spinlock_t		pg_lock;

@@ -243,6 +246,7 @@ struct alua_port_group *alua_get_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);
 
 	/* Re-check list again to catch concurrent updates */

@@ -371,13 +375,26 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 		old_pg = pg;
 		/* port_group has changed. Update to new port group */
 		if (h->pg != pg) {
+			unsigned long flags;
+
 			old_pg = h->pg;
 			rcu_assign_pointer(h->pg, pg);
+			spin_lock_irqsave(&old_pg->lock, flags);
+			list_del_rcu(&h->node);
+			spin_unlock_irqrestore(&old_pg->lock, flags);
+			spin_lock_irqsave(&pg->lock, flags);
+			list_add_rcu(&h->node, &pg->dh_list);
+			spin_unlock_irqrestore(&pg->lock, flags);
 			h->pg->expiry = 0;
 			pg_found = true;
 		}
 	} else {
+		unsigned long flags;
+
 		rcu_assign_pointer(h->pg, pg);
+		spin_lock_irqsave(&pg->lock, flags);
+		list_add_rcu(&h->node, &pg->dh_list);
+		spin_unlock_irqrestore(&pg->lock, flags);
 		pg_found = true;
 	}
 	alua_rtpg_queue(h->pg, sdev, NULL, true);

@@ -629,6 +646,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 
 		spin_lock_irqsave(&port_group_lock, flags);
 		list_for_each_entry(tmp_pg, &port_group_list, node) {
+			struct alua_dh_data *h;
+
 			if (tmp_pg->group_id != group_id)
 				continue;
 			if (tmp_pg->device_id_len != pg->device_id_len)

@@ -638,6 +657,12 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 				continue;
 			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) {
+				if (h->sdev)
+					h->sdev->access_state = desc[0];
+			}
+			rcu_read_unlock();
 			if (tmp_pg == pg)
 				valid_states = desc[1];
 		}

@@ -1054,6 +1079,7 @@ static int alua_bus_attach(struct scsi_device *sdev)
 	h->rel_port = -1;
 	h->init_error = SCSI_DH_OK;
 	h->sdev = sdev;
+	INIT_LIST_HEAD(&h->node);
 
 	mutex_init(&h->init_mutex);
 	err = alua_initialize(sdev, h);

@@ -1084,6 +1110,9 @@ static void alua_bus_detach(struct scsi_device *sdev)
 	h->sdev = NULL;
 	spin_unlock(&h->pg_lock);
 	if (pg) {
+		spin_lock(&pg->lock);
+		list_del_rcu(&h->node);
+		spin_unlock(&pg->lock);
 		synchronize_rcu();
 		if (pg->rtpg_sdev)
 			flush_delayed_work(&pg->rtpg_work);

Signature · 1 line

1.8.5.6

Hannes Reinecke

author

2015-09-29 10:48 UTC #56 lore raw

PATCH v5 36/36 scsi_dh_alua: Update version to 2.0

In reply to Hannes Reinecke

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/scsi/device_handler/scsi_dh_alua.c +1 -1

index 12e6ce9..98adc11 100644
@@ -30,7 +30,7 @@
 #include <scsi/scsi_dh.h>
 
 #define ALUA_DH_NAME "alua"
-#define ALUA_DH_VER "1.3"
+#define ALUA_DH_VER "2.0"
 
 #define TPGS_SUPPORT_NONE		0x00
 #define TPGS_SUPPORT_OPTIMIZED		0x01

Signature · 1 line

1.8.5.6

Bart Van Assche

2015-09-29 18:29 UTC #57 lore raw

In reply to Hannes Reinecke

Quoted 1 line · Hannes Reinecke

here the next round of my update to the ALUA device handler.

Hello Hannes,

Sorry but this with this version I see an initiator kernel lockup shortly after the initiator system had been booted. I have attached the output of echo t > /proc/sysrq-trigger to this e-mail.

Thanks,

Bart.

Hannes Reinecke

author

2015-09-30 13:21 UTC #58 lore raw

Quoted 9 lines · Bart Van Assche

Quoted 1 line · Hannes Reinecke
here the next round of my update to the ALUA device handler.
Hello Hannes,
Sorry but this with this version I see an initiator kernel lockup shortly after the initiator system had been booted. I have attached the output of echo t > /proc/sysrq-trigger to this e-mail.

Hmm. Weird.
Everything seems to wait for alua_rtpg() to complete:

kworker/4:2 D ffff88045c64c380 0 203 2 0x00000000

Workqueue: kaluad_wq alua_rtpg_work [scsi_dh_alua]
 ffff88045d94f968 0000000000000086 ffff88047fd0dcc0 ffff88047fd15ad8
 ffff88045c64c380 ffff88044fc7c380 ffff88045d950000 ffff88047fd0dcc0
 ffff88047fd0dcc0 000000010001c779 0000000000000004 ffff88045d94f980
Call Trace:
 [<ffffffff814f078a>] schedule+0x3a/0x90
 [<ffffffff814f4b53>] schedule_timeout+0x143/0x290
 [<ffffffff810df1ed>] ? ktime_get+0x7d/0x130
 [<ffffffff810d5b00>] ? init_timer_key+0x140/0x140
 [<ffffffff814efb86>] io_schedule_timeout+0xa6/0x120
 [<ffffffff810ba14d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff814f126f>] wait_for_completion_io_timeout+0xdf/0x120
 [<ffffffff8109ec00>] ? wake_up_q+0x70/0x70
 [<ffffffff8126e46d>] blk_execute_rq+0xad/0x130
 [<ffffffff8125fc39>] ? bio_alloc_bioset+0x179/0x200
 [<ffffffff8125e259>] ? bio_phys_segments+0x19/0x20
 [<ffffffff81269e23>] ? blk_rq_bio_prep+0x63/0x80
 [<ffffffff8126e1c7>] ? blk_rq_map_kern+0xb7/0x130
 [<ffffffffa006b6c3>] scsi_execute+0xd3/0x160 [scsi_mod]
 [<ffffffffa006dafe>] scsi_execute_req_flags+0x8e/0xf0 [scsi_mod]
 [<ffffffffa0281e90>] alua_rtpg_work+0x2d0/0xc10 [scsi_dh_alua]

But this just seems to wait for a command completion, which apparently doesn't arrive. Or not in time.
What's curious, though, is that there are several instances of 'srp_daemon', each trying to allocate/setup a new SRP device:

srp_daemon D ffff88045ca2ad00 0 595 592 0x00000000
ffff88043c3db960 0000000000000082 ffffffff810ba14d ffff88047fd55ad8 ffff88045ca2ad00 ffff88043cf24380 ffff88043c3dc000 ffff880425ef6548 ffff88042d5c3f78 ffff880425ef5968 ffff880425ef4dd0 ffff88043c3db978

Call Trace:
 [<ffffffff810ba14d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff814f078a>] schedule+0x3a/0x90
 [<ffffffff81271e76>] blk_mq_freeze_queue_wait+0x56/0xb0
 [<ffffffff810b4650>] ? prepare_to_wait_event+0xf0/0xf0
 [<ffffffff81273e71>] blk_mq_update_tag_set_depth+0x41/0xb0
 [<ffffffff812746a4>] blk_mq_init_allocated_queue+0x7c4/0x860
 [<ffffffff8127477a>] blk_mq_init_queue+0x3a/0x60
 [<ffffffffa006fa6c>] scsi_mq_alloc_queue+0x1c/0x50 [scsi_mod]
 [<ffffffffa0070c51>] scsi_alloc_sdev+0x331/0x3b0 [scsi_mod]
 [<ffffffffa0071554>] scsi_probe_and_add_lun+0x884/0xd20 [scsi_mod]
 [<ffffffffa00721cb>] __scsi_scan_target+0x52b/0x5f0 [scsi_mod]

Unfortunately I cannot tell from the provided logs whether both refer to the same device; if so this would easily explain the issue. Can you check if there is some line-bouncing involved?
If a device would be setup and teared down several times that would explain things.
However, the main point seems to be that we never get a completion for the RTPG command, Which also might be an issue with the srp driver, as I've never seen this issue during my tests.

Is there a way on how I could be trying to reproduce it?

Cheers,

Hannes

Signature · 9 lines

Dr. Hannes Reinecke		               zSeries & Storage
[email protected]			               +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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bart Van Assche

2015-09-30 21:33 UTC #59 lore raw

Quoted 10 lines · Hannes Reinecke

Quoted 7 lines · Bart Van Assche
Quoted 1 line · Hannes Reinecke
here the next round of my update to the ALUA device handler.
Sorry but this with this version I see an initiator kernel lockup shortly after the initiator system had been booted. I have attached the output of echo t > /proc/sysrq-trigger to this e-mail.
Hmm. Weird.
Everything seems to wait for alua_rtpg() to complete: [ ... ]

Hello Hannes,

Would it be possible to add the patch to your tree that causes scsi_dh_alua to be loaded automatically again
(http://thread.gmane.org/gmane.linux.scsi/105276) ? I might have forgotten to load the scsi_dh_alua driver manually before I ran my test ...

However, even with the scsi_dh_alua driver loaded a kernel lockup is reported. Please note that I do not know whether or not that lockup is related to this patch series or to the changes in v4.3-rc1:

INFO: task srp_daemon:600 blocked for more than 120 seconds.
      Not tainted 4.3.0-rc1-debug+ #1

"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
srp_daemon D ffff88045c6a2d00 0 600 593 0x00000000
ffff88043060b960 0000000000000092 ffffffff810ba0bd ffff88047fc95ad8 ffff88045c6a2d00 ffff880430559680 ffff88043060c000 ffff8804181061c8 ffff88041dbd3cf8 ffff8804181055e8 ffff880418104ad0 ffff88043060b978

Call Trace:
 [<ffffffff810ba0bd>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff814efcda>] schedule+0x3a/0x90
 [<ffffffff81271a46>] blk_mq_freeze_queue_wait+0x56/0xb0
 [<ffffffff810b45c0>] ? prepare_to_wait_event+0xf0/0xf0
 [<ffffffff81273a61>] blk_mq_update_tag_set_depth+0x41/0xb0
 [<ffffffff81274294>] blk_mq_init_allocated_queue+0x7c4/0x860
 [<ffffffff8127436a>] blk_mq_init_queue+0x3a/0x60
 [<ffffffffa0016a6c>] scsi_mq_alloc_queue+0x1c/0x50 [scsi_mod]
 [<ffffffffa0017c51>] scsi_alloc_sdev+0x331/0x3b0 [scsi_mod]
 [<ffffffffa0018554>] scsi_probe_and_add_lun+0x884/0xd20 [scsi_mod]
 [<ffffffffa00191cb>] __scsi_scan_target+0x52b/0x5f0 [scsi_mod]
 [<ffffffff8139198c>] ? __pm_runtime_resume+0x5c/0x80
 [<ffffffffa001936c>] scsi_scan_target+0xdc/0x100 [scsi_mod]
 [<ffffffffa04374ae>] srp_create_target+0xfde/0x1410 [ib_srp]
 [<ffffffff810b8201>] ? match_held_lock+0x1c1/0x200
 [<ffffffff81381b68>] dev_attr_store+0x18/0x30
 [<ffffffff812300f4>] sysfs_kf_write+0x44/0x60
 [<ffffffff8122f724>] kernfs_fop_write+0x144/0x190
 [<ffffffff811b8788>] __vfs_write+0x28/0xe0
 [<ffffffff810b6a4a>] ? percpu_down_read+0x5a/0x90
 [<ffffffff811bba70>] ? __sb_start_write+0xe0/0x100
 [<ffffffff811bba70>] ? __sb_start_write+0xe0/0x100
 [<ffffffff811d7455>] ? __fget+0x5/0x210
 [<ffffffff811b8e19>] vfs_write+0xa9/0x190
 [<ffffffff811b9b19>] SyS_write+0x49/0xa0
 [<ffffffff814f57b6>] entry_SYSCALL_64_fastpath+0x16/0x7a
7 locks held by srp_daemon/600:
 #0:  (&f->f_pos_lock){+.+.+.}, at: [<ffffffff811d84b3>] __fdget_pos+0x43/0x50
 #1:  (sb_writers#3){.+.+.+}, at: [<ffffffff811bba70>] __sb_start_write+0xe0/0x100
 #2:  (&of->mutex){+.+.+.}, at: [<ffffffff8122f646>] kernfs_fop_write+0x66/0x190
 #3:  (s_active#142){.+.+.+}, at: [<ffffffff8122f64e>] kernfs_fop_write+0x6e/0x190
 #4:  (&host->add_target_mutex){+.+.+.}, at: [<ffffffffa043660b>] srp_create_target+0x13b/0x1410 [ib_srp]
 #5:  (&shost->scan_mutex){+.+.+.}, at: [<ffffffffa0019317>] scsi_scan_target+0x87/0x100 [scsi_mod]
 #6:  (&set->tag_list_lock){+.+...}, at: [<ffffffff81274272>] blk_mq_init_allocated_queue+0x7a2/0x860

Thanks,

Bart.

Hannes Reinecke

author

2015-10-13 09:44 UTC #60 lore raw

Quoted 58 lines · Bart Van Assche

Quoted 10 lines · Hannes Reinecke
Quoted 7 lines · Bart Van Assche
Quoted 1 line · Hannes Reinecke
here the next round of my update to the ALUA device handler.
Sorry but this with this version I see an initiator kernel lockup shortly after the initiator system had been booted. I have attached the output of echo t > /proc/sysrq-trigger to this e-mail.
Hmm. Weird.
Everything seems to wait for alua_rtpg() to complete: [ ... ]
Hello Hannes,
Would it be possible to add the patch to your tree that causes scsi_dh_alua to be loaded automatically again
(http://thread.gmane.org/gmane.linux.scsi/105276) ? I might have forgotten to load the scsi_dh_alua driver manually before I ran my test ...
However, even with the scsi_dh_alua driver loaded a kernel lockup is reported. Please note that I do not know whether or not that lockup is related to this patch series or to the changes in v4.3-rc1:
INFO: task srp_daemon:600 blocked for more than 120 seconds.
      Not tainted 4.3.0-rc1-debug+ #1
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
srp_daemon D ffff88045c6a2d00 0 600 593 0x00000000
ffff88043060b960 0000000000000092 ffffffff810ba0bd ffff88047fc95ad8 ffff88045c6a2d00 ffff880430559680 ffff88043060c000 ffff8804181061c8 ffff88041dbd3cf8 ffff8804181055e8 ffff880418104ad0 ffff88043060b978
Call Trace:
 [<ffffffff810ba0bd>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff814efcda>] schedule+0x3a/0x90
 [<ffffffff81271a46>] blk_mq_freeze_queue_wait+0x56/0xb0
 [<ffffffff810b45c0>] ? prepare_to_wait_event+0xf0/0xf0
 [<ffffffff81273a61>] blk_mq_update_tag_set_depth+0x41/0xb0
 [<ffffffff81274294>] blk_mq_init_allocated_queue+0x7c4/0x860
 [<ffffffff8127436a>] blk_mq_init_queue+0x3a/0x60
 [<ffffffffa0016a6c>] scsi_mq_alloc_queue+0x1c/0x50 [scsi_mod]
 [<ffffffffa0017c51>] scsi_alloc_sdev+0x331/0x3b0 [scsi_mod]
 [<ffffffffa0018554>] scsi_probe_and_add_lun+0x884/0xd20 [scsi_mod]
 [<ffffffffa00191cb>] __scsi_scan_target+0x52b/0x5f0 [scsi_mod]
 [<ffffffff8139198c>] ? __pm_runtime_resume+0x5c/0x80
 [<ffffffffa001936c>] scsi_scan_target+0xdc/0x100 [scsi_mod]
 [<ffffffffa04374ae>] srp_create_target+0xfde/0x1410 [ib_srp]
 [<ffffffff810b8201>] ? match_held_lock+0x1c1/0x200
 [<ffffffff81381b68>] dev_attr_store+0x18/0x30
 [<ffffffff812300f4>] sysfs_kf_write+0x44/0x60
 [<ffffffff8122f724>] kernfs_fop_write+0x144/0x190
 [<ffffffff811b8788>] __vfs_write+0x28/0xe0
 [<ffffffff810b6a4a>] ? percpu_down_read+0x5a/0x90
 [<ffffffff811bba70>] ? __sb_start_write+0xe0/0x100
 [<ffffffff811bba70>] ? __sb_start_write+0xe0/0x100
 [<ffffffff811d7455>] ? __fget+0x5/0x210
 [<ffffffff811b8e19>] vfs_write+0xa9/0x190
 [<ffffffff811b9b19>] SyS_write+0x49/0xa0
 [<ffffffff814f57b6>] entry_SYSCALL_64_fastpath+0x16/0x7a

Quoted 9 lines

7 locks held by srp_daemon/600:
 #0:  (&f->f_pos_lock){+.+.+.}, at: [<ffffffff811d84b3>] __fdget_pos+0x43/0x50
 #1:  (sb_writers#3){.+.+.+}, at: [<ffffffff811bba70>] __sb_start_write+0xe0/0x100
 #2:  (&of->mutex){+.+.+.}, at: [<ffffffff8122f646>] kernfs_fop_write+0x66/0x190
 #3:  (s_active#142){.+.+.+}, at: [<ffffffff8122f64e>] kernfs_fop_write+0x6e/0x190
 #4:  (&host->add_target_mutex){+.+.+.}, at: [<ffffffffa043660b>] srp_create_target+0x13b/0x1410 [ib_srp]
 #5:  (&shost->scan_mutex){+.+.+.}, at: [<ffffffffa0019317>] scsi_scan_target+0x87/0x100 [scsi_mod]
 #6:  (&set->tag_list_lock){+.+...}, at: [<ffffffff81274272>] blk_mq_init_allocated_queue+0x7a2/0x860

I have rebased my patches to 4.3.0-rc5 and didn't see any lockups; tested with hpsa and lpfc, both mq enabled and disabled.

Can you check if it's specific to ib_srp? Is there a way to simulate an ib_srp setup?

Cheers,

Hannes

Signature · 9 lines

Dr. Hannes Reinecke		               zSeries & Storage
[email protected]			               +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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bart Van Assche

2015-10-13 15:07 UTC #61 lore raw

Quoted 5 lines · Hannes Reinecke

I have rebased my patches to 4.3.0-rc5 and didn't see any lockups; tested with hpsa and lpfc, both mq enabled and disabled.
Can you check if it's specific to ib_srp? Is there a way to simulate an ib_srp setup?

Hello Hannes,

This infinite loop was due to the scsi_dh_alua handler not being loaded during my test (as mentioned in https://www.mail-archive.com/[email protected]/msg42645.html).

Bart.

Hannes Reinecke

author

2015-10-13 15:13 UTC #62 lore raw

Quoted 4 lines · Bart Van Assche

Quoted 3 lines · Hannes Reinecke
I have rebased my patches to 4.3.0-rc5 and didn't see any lockups; tested with hpsa and lpfc, both mq enabled and disabled.

Quoted 9 lines

Quoted 2 lines
Can you check if it's specific to ib_srp? Is there a way to simulate an ib_srp setup?
Hello Hannes,
This infinite loop was due to the scsi_dh_alua handler not being loaded during my test (as mentioned in
https://www.mail-archive.com/[email protected]/msg42645.html).

So it works with that patch?

Cheers,

Hannes

Signature · 9 lines

Dr. Hannes Reinecke		               zSeries & Storage
[email protected]			               +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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bart Van Assche

2015-10-13 15:40 UTC #63 lore raw

Quoted 5 lines · Hannes Reinecke

Quoted 4 lines · Bart Van Assche
Quoted 3 lines · Hannes Reinecke
I have rebased my patches to 4.3.0-rc5 and didn't see any lockups; tested with hpsa and lpfc, both mq enabled and disabled.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Can you check if it's specific to ib_srp? Is there a way to simulate an ib_srp setup?
This infinite loop was due to the scsi_dh_alua handler not being loaded during my test (as mentioned in
https://www.mail-archive.com/[email protected]/msg42645.html).
So it works with that patch?

Hello Hannes,

Manually loading the scsi_dh_alua handler or adding the patch that loads that handler again automatically indeed fixed the infinite loop. But I'm still very concerned about the scalability issues caused by submitting RTPG commands synchronously instead of asynchronously.

Bart.