[PATCH V4 0/5] scsi: Configure number of LUs reported by 'report-luns'

[PATCH V4 0/5] scsi: Configure number of LUs reported by 'report-luns'

[Rob Evers]

This patch set retrieves the number of LUs available on a target
using the report-luns command.  The initial size of the report-luns
command is 512 entries, as the previous default initial number was.
If more LUs than 511 are present on a target, the report-luns is
re-issued with the size indicated in the result of the original
report-luns, up to max_report_luns.

A few minor bug fixes and enhancements are included as well.

The default value of max_report_luns is increased to 16k-1 from 512-1.

4rth version changes from 3rd posting:

 - Fix return value on first kmalloc failing in scsi_report_lun_scan

 - Fix bug in put_unaligned_be32 usage

 - Re-order patches to put bug fixes in first

 - Enhance comments

 - Fix checkpatch warnings

3rd version changes from 2nd posting:

 - Add a patch to use get/put_unaligned_be32() in report-luns code

2nd version changes from first posting:

 - Minor tweak added in 2nd patch to use the number of luns
   reported in the 2nd report-luns command, if it is executed.
   There is a chance that the number changed between the
   1st and 2nd report-luns.

 - Add 3rd patch changing kmalloc flag in report luns from
   GFP_ATOMIC to GFP_KERNEL, as this is more consistent with
   the allocation flag in blk_alloc_queue_node()

Rob Evers (5):
  scsi: Fix scsi_report_lun_scan kmalloc flag
  scsi: Fix scsi_report_lun_scan return when kmalloc fails
  scsi: Encapsulate scsi_do_report_luns
  scsi: Change default value of max_report_luns to 16k-1
  scsi: Use set/get_unaligned_be32 in report_luns

 drivers/scsi/scsi_scan.c | 202 +++++++++++++++++++++++++++++------------------
 1 file changed, 125 insertions(+), 77 deletions(-)

-- 
1.8.3.1

[PATCH V4 1/5] scsi: Fix scsi_report_lun_scan kmalloc flag

[Rob Evers]

===================================================================

Signed-off-by: Rob Evers <revers@redhat.com>
---
 drivers/scsi/scsi_scan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index ba3f1e8..e460b35 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1408,7 +1408,7 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 	 * prevent us from finding any LUNs on this target.
 	 */
 	length = (max_scsi_report_luns + 1) * sizeof(struct scsi_lun);
-	lun_data = kmalloc(length, GFP_ATOMIC |
+	lun_data = kmalloc(length, GFP_KERNEL |
 			   (sdev->host->unchecked_isa_dma ? __GFP_DMA : 0));
 	if (!lun_data) {
 		printk(ALLOC_FAILURE_MSG, __func__);
-- 
1.8.3.1

Re: [PATCH V4 1/5] scsi: Fix scsi_report_lun_scan kmalloc flag

[Christoph Hellwig]

This could use a better description like:

	scsi: avoid unessecary GFP_ATOMIC allocation in scsi_report_lun_scan

On Mon, Nov 17, 2014 at 10:22:06AM -0500, Rob Evers wrote:
> ===================================================================

And what's this supposed to be?

[PATCH V4 2/5] scsi: Fix scsi_report_lun_scan return when kmalloc fails

[Rob Evers]

Signed-off-by: Rob Evers <revers@redhat.com>
---
 drivers/scsi/scsi_scan.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index e460b35..69291bf3 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1412,6 +1412,7 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 			   (sdev->host->unchecked_isa_dma ? __GFP_DMA : 0));
 	if (!lun_data) {
 		printk(ALLOC_FAILURE_MSG, __func__);
+		ret = 1;
 		goto out;
 	}
 
-- 
1.8.3.1

Re: [PATCH V4 2/5] scsi: Fix scsi_report_lun_scan return when kmalloc fails

[Christoph Hellwig]

What are the chances that the sequential scan will succeed when we
failed to allocate memory for report luns?

[PATCH V4 3/5] scsi: Encapsulate scsi_do_report_luns

[Rob Evers]

Report luns functionality is encapsulated as it will be re-used in the next
patch in this series if a large number of LUs are present.

Signed-off-by: Rob Evers <revers@redhat.com>
---
 drivers/scsi/scsi_scan.c | 114 ++++++++++++++++++++++++++---------------------
 1 file changed, 62 insertions(+), 52 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 69291bf3..193c20c 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1328,6 +1328,67 @@ void int_to_scsilun(u64 lun, struct scsi_lun *scsilun)
 }
 EXPORT_SYMBOL(int_to_scsilun);
 
+int scsi_do_report_luns(struct scsi_device *sdev, int length,
+			struct scsi_lun *lun_data, char *devname)
+{
+	unsigned int retries;
+	unsigned char scsi_cmd[MAX_COMMAND_SIZE];
+	struct scsi_sense_hdr sshdr;
+	int result = 0;
+
+	scsi_cmd[0] = REPORT_LUNS;
+
+	/*
+	 * bytes 1 - 5: reserved, set to zero.
+	 */
+	memset(&scsi_cmd[1], 0, 5);
+
+	/*
+	 * bytes 6 - 9: length of the command.
+	 */
+	scsi_cmd[6] = (unsigned char) (length >> 24) & 0xff;
+	scsi_cmd[7] = (unsigned char) (length >> 16) & 0xff;
+	scsi_cmd[8] = (unsigned char) (length >> 8) & 0xff;
+	scsi_cmd[9] = (unsigned char) length & 0xff;
+
+	scsi_cmd[10] = 0;	/* reserved */
+	scsi_cmd[11] = 0;	/* control */
+
+	/*
+	 * We can get a UNIT ATTENTION, for example a power on/reset, so
+	 * retry a few times (like sd.c does for TEST UNIT READY).
+	 * Experience shows some combinations of adapter/devices get at
+	 * least two power on/resets.
+	 *
+	 * Illegal requests (for devices that do not support REPORT LUNS)
+	 * should come through as a check condition, and will not generate
+	 * a retry.
+	 */
+	for (retries = 0; retries < 3; retries++) {
+		SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
+				"scsi scan: Sending REPORT LUNS to (try %d)\n",
+				retries));
+
+		result = scsi_execute_req(sdev, scsi_cmd, DMA_FROM_DEVICE,
+					  lun_data, length, &sshdr,
+					  SCSI_TIMEOUT + 4 * HZ, 3, NULL);
+
+		SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
+				  "scsi scan: REPORT LUNS"
+				  " %s (try %d) result 0x%x\n",
+				  result ?  "failed" : "successful",
+				  retries, result));
+		if (result == 0)
+			break;
+		else if (scsi_sense_valid(&sshdr)) {
+			if (sshdr.sense_key != UNIT_ATTENTION)
+				break;
+		}
+	}
+
+	return result;
+}
+
 /**
  * scsi_report_lun_scan - Scan using SCSI REPORT LUN results
  * @starget: which target
@@ -1352,15 +1413,12 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 				int rescan)
 {
 	char devname[64];
-	unsigned char scsi_cmd[MAX_COMMAND_SIZE];
 	unsigned int length;
 	u64 lun;
 	unsigned int num_luns;
-	unsigned int retries;
 	int result;
 	struct scsi_lun *lunp, *lun_data;
 	u8 *data;
-	struct scsi_sense_hdr sshdr;
 	struct scsi_device *sdev;
 	struct Scsi_Host *shost = dev_to_shost(&starget->dev);
 	int ret = 0;
@@ -1416,55 +1474,7 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 		goto out;
 	}
 
-	scsi_cmd[0] = REPORT_LUNS;
-
-	/*
-	 * bytes 1 - 5: reserved, set to zero.
-	 */
-	memset(&scsi_cmd[1], 0, 5);
-
-	/*
-	 * bytes 6 - 9: length of the command.
-	 */
-	scsi_cmd[6] = (unsigned char) (length >> 24) & 0xff;
-	scsi_cmd[7] = (unsigned char) (length >> 16) & 0xff;
-	scsi_cmd[8] = (unsigned char) (length >> 8) & 0xff;
-	scsi_cmd[9] = (unsigned char) length & 0xff;
-
-	scsi_cmd[10] = 0;	/* reserved */
-	scsi_cmd[11] = 0;	/* control */
-
-	/*
-	 * We can get a UNIT ATTENTION, for example a power on/reset, so
-	 * retry a few times (like sd.c does for TEST UNIT READY).
-	 * Experience shows some combinations of adapter/devices get at
-	 * least two power on/resets.
-	 *
-	 * Illegal requests (for devices that do not support REPORT LUNS)
-	 * should come through as a check condition, and will not generate
-	 * a retry.
-	 */
-	for (retries = 0; retries < 3; retries++) {
-		SCSI_LOG_SCAN_BUS(3, sdev_printk (KERN_INFO, sdev,
-				"scsi scan: Sending REPORT LUNS to (try %d)\n",
-				retries));
-
-		result = scsi_execute_req(sdev, scsi_cmd, DMA_FROM_DEVICE,
-					  lun_data, length, &sshdr,
-					  SCSI_TIMEOUT + 4 * HZ, 3, NULL);
-
-		SCSI_LOG_SCAN_BUS(3, sdev_printk (KERN_INFO, sdev,
-				"scsi scan: REPORT LUNS"
-				" %s (try %d) result 0x%x\n",
-				result ?  "failed" : "successful",
-				retries, result));
-		if (result == 0)
-			break;
-		else if (scsi_sense_valid(&sshdr)) {
-			if (sshdr.sense_key != UNIT_ATTENTION)
-				break;
-		}
-	}
+	result = scsi_do_report_luns(sdev, length, lun_data, devname);
 
 	if (result) {
 		/*
-- 
1.8.3.1

Re: [PATCH V4 3/5] scsi: Encapsulate scsi_do_report_luns

[Christoph Hellwig]

> +int scsi_do_report_luns(struct scsi_device *sdev, int length,
> +			struct scsi_lun *lun_data, char *devname)

I would skip the do and call this scsi_report_luns.  Also shouldn't this
function be marked static?

[PATCH V4 4/5] scsi: Change default value of max_report_luns to 16k-1

[Rob Evers]

First, allocate a buffer large enough for report luns command to report
on 511 LUs and issue a report luns command.  This buffer size matches
the previous report lun behavior based on the original default
max_report_luns value.

If more that 511 LUs are reported in the first report luns, allocate a
new buffer with enough space, up to max_report_luns, for all the LUs
reported and re-issue the report luns command.

If the allocation of the 2nd buffer fails, use the result of the first
report luns to configure the LUs reported there.

According to T10 SPC-4 'REPORT LUNS' and 'Allocation length' specifications,
the 'REPORT LUNS parameter data format' will return normally when more space
is required to report all luns, and indicate how much total space is required
in the LUN LIST LENGTH field.

Signed-off-by: Rob Evers <revers@redhat.com>
---
 drivers/scsi/scsi_scan.c | 90 ++++++++++++++++++++++++++++++++++++------------
 1 file changed, 68 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 193c20c..0d9108e 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -105,12 +105,13 @@ MODULE_PARM_DESC(scan, "sync, async or none");
  * in practice, the maximum number of LUNs suppored by any device
  * is about 16k.
  */
-static unsigned int max_scsi_report_luns = 511;
+static unsigned int max_scsi_report_luns = 16383;
 
 module_param_named(max_report_luns, max_scsi_report_luns, uint, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(max_report_luns,
 		 "REPORT LUNS maximum number of LUNS received (should be"
-		 " between 1 and 16384)");
+		 " between 1 and 16383)");
+#define INITIAL_MAX_REPORT_LUNS 511
 
 static unsigned int scsi_inq_timeout = SCSI_TIMEOUT/HZ + 18;
 
@@ -1415,9 +1416,10 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 	char devname[64];
 	unsigned int length;
 	u64 lun;
-	unsigned int num_luns;
+	unsigned int num_luns, num_luns_reported;
 	int result;
 	struct scsi_lun *lunp, *lun_data;
+	struct scsi_lun *first_lun_data, *second_lun_data;
 	u8 *data;
 	struct scsi_device *sdev;
 	struct Scsi_Host *shost = dev_to_shost(&starget->dev);
@@ -1458,48 +1460,92 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 	/*
 	 * Allocate enough to hold the header (the same size as one scsi_lun)
 	 * plus the max number of luns we are requesting.
-	 *
-	 * Reallocating and trying again (with the exact amount we need)
-	 * would be nice, but then we need to somehow limit the size
-	 * allocated based on the available memory and the limits of
-	 * kmalloc - we don't want a kmalloc() failure of a huge value to
-	 * prevent us from finding any LUNs on this target.
 	 */
-	length = (max_scsi_report_luns + 1) * sizeof(struct scsi_lun);
-	lun_data = kmalloc(length, GFP_KERNEL |
-			   (sdev->host->unchecked_isa_dma ? __GFP_DMA : 0));
-	if (!lun_data) {
+
+	if (max_scsi_report_luns > INITIAL_MAX_REPORT_LUNS)
+		length = (INITIAL_MAX_REPORT_LUNS + 1) *
+			  sizeof(struct scsi_lun);
+	else
+		length = (max_scsi_report_luns + 1) *
+			  sizeof(struct scsi_lun);
+
+	first_lun_data = kmalloc(length, GFP_KERNEL |
+				 (sdev->host->unchecked_isa_dma ?
+				 __GFP_DMA : 0));
+	if (!first_lun_data) {
 		printk(ALLOC_FAILURE_MSG, __func__);
 		ret = 1;
 		goto out;
 	}
 
-	result = scsi_do_report_luns(sdev, length, lun_data, devname);
+	result = scsi_do_report_luns(sdev, length, first_lun_data, devname);
 
 	if (result) {
 		/*
 		 * The device probably does not support a REPORT LUN command
 		 */
+		lun_data = first_lun_data;
 		ret = 1;
 		goto out_err;
 	}
 
 	/*
-	 * Get the length from the first four bytes of lun_data.
+	 * Get the length from the first four bytes of first_lun_data.
 	 */
-	data = (u8 *) lun_data->scsi_lun;
+	data = (u8 *) first_lun_data->scsi_lun;
 	length = ((data[0] << 24) | (data[1] << 16) |
 		  (data[2] << 8) | (data[3] << 0));
 
-	num_luns = (length / sizeof(struct scsi_lun));
-	if (num_luns > max_scsi_report_luns) {
+	num_luns_reported = (length / sizeof(struct scsi_lun));
+
+	if (num_luns_reported > max_scsi_report_luns) {
+		num_luns = max_scsi_report_luns;
+		length = num_luns * sizeof(struct scsi_lun);
 		sdev_printk(KERN_WARNING, sdev,
 			    "Only %d (max_scsi_report_luns)"
 			    " of %d luns reported, try increasing"
-			    " max_scsi_report_luns.\n",
-			    max_scsi_report_luns, num_luns);
-		num_luns = max_scsi_report_luns;
-	}
+			    " max_report_luns parameter\n",
+			    max_scsi_report_luns, num_luns_reported);
+	} else
+		num_luns = num_luns_reported;
+
+	if (num_luns > INITIAL_MAX_REPORT_LUNS) {
+		/*
+		 * add one for the header
+		 */
+		length = length + sizeof(struct scsi_lun);
+		second_lun_data = kmalloc(length, GFP_KERNEL |
+					  (sdev->host->unchecked_isa_dma ?
+					  __GFP_DMA : 0));
+		if (!second_lun_data) {
+			num_luns = INITIAL_MAX_REPORT_LUNS;
+			lun_data = first_lun_data;
+			printk(ALLOC_FAILURE_MSG, __func__);
+			sdev_printk(KERN_WARNING, sdev,
+				    "Configuring %d of %d luns reported\n",
+				    num_luns, num_luns_reported);
+		} else {
+			kfree(first_lun_data);
+			lun_data = second_lun_data;
+			result = scsi_do_report_luns(sdev, length,
+						     lun_data, devname);
+			if (result) {
+				ret = 1;
+				goto out_err;
+			} else {
+				/*
+				 * Get the length from the first four bytes
+				 * of second_lun_data.
+				 */
+				data = (u8 *) lun_data->scsi_lun;
+				length = ((data[0] << 24) | (data[1] << 16) |
+					  (data[2] << 8) | (data[3] << 0));
+
+				num_luns = (length / sizeof(struct scsi_lun));
+			}
+		}
+	} else
+		lun_data = first_lun_data;
 
 	SCSI_LOG_SCAN_BUS(3, sdev_printk (KERN_INFO, sdev,
 		"scsi scan: REPORT LUN scan\n"));
-- 
1.8.3.1

Re: [PATCH V4 4/5] scsi: Change default value of max_report_luns to 16k-1

[Christoph Hellwig]

> +++ b/drivers/scsi/scsi_scan.c
> @@ -105,12 +105,13 @@ MODULE_PARM_DESC(scan, "sync, async or none");
>   * in practice, the maximum number of LUNs suppored by any device
>   * is about 16k.
>   */
> -static unsigned int max_scsi_report_luns = 511;
> +static unsigned int max_scsi_report_luns = 16383;
>  
>  module_param_named(max_report_luns, max_scsi_report_luns, uint, S_IRUGO|S_IWUSR);

What's the point of keepign this parameter?  From looking at ancient
history we initially supported just 128 LUNs by default when Doug added
support for REPORT_LUNS, and we later increased it to 511 when Kurt
enabled REPORT_LUNS support by default.

I can't really see any reason to limit the size except for avoiding
large allocations, and with the two step process we take care of that
easily.

Any reason to keep two responses around in the file instead of a
"goto retry" after updating the length?

[PATCH V4 5/5] scsi: Use set/get_unaligned_be32 in report_luns

[Rob Evers]

Signed-off-by: Rob Evers <revers@redhat.com>
---
 drivers/scsi/scsi_scan.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 0d9108e..fa6ac51 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -34,6 +34,7 @@
 #include <linux/spinlock.h>
 #include <linux/async.h>
 #include <linux/slab.h>
+#include <asm/unaligned.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
@@ -1329,7 +1330,7 @@ void int_to_scsilun(u64 lun, struct scsi_lun *scsilun)
 }
 EXPORT_SYMBOL(int_to_scsilun);
 
-int scsi_do_report_luns(struct scsi_device *sdev, int length,
+int scsi_do_report_luns(struct scsi_device *sdev, u32 length,
 			struct scsi_lun *lun_data, char *devname)
 {
 	unsigned int retries;
@@ -1347,10 +1348,7 @@ int scsi_do_report_luns(struct scsi_device *sdev, int length,
 	/*
 	 * bytes 6 - 9: length of the command.
 	 */
-	scsi_cmd[6] = (unsigned char) (length >> 24) & 0xff;
-	scsi_cmd[7] = (unsigned char) (length >> 16) & 0xff;
-	scsi_cmd[8] = (unsigned char) (length >> 8) & 0xff;
-	scsi_cmd[9] = (unsigned char) length & 0xff;
+	put_unaligned_be32(length, &scsi_cmd[6]);
 
 	scsi_cmd[10] = 0;	/* reserved */
 	scsi_cmd[11] = 0;	/* control */
@@ -1414,13 +1412,12 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 				int rescan)
 {
 	char devname[64];
-	unsigned int length;
+	u32 length;
 	u64 lun;
 	unsigned int num_luns, num_luns_reported;
 	int result;
 	struct scsi_lun *lunp, *lun_data;
 	struct scsi_lun *first_lun_data, *second_lun_data;
-	u8 *data;
 	struct scsi_device *sdev;
 	struct Scsi_Host *shost = dev_to_shost(&starget->dev);
 	int ret = 0;
@@ -1492,10 +1489,7 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 	/*
 	 * Get the length from the first four bytes of first_lun_data.
 	 */
-	data = (u8 *) first_lun_data->scsi_lun;
-	length = ((data[0] << 24) | (data[1] << 16) |
-		  (data[2] << 8) | (data[3] << 0));
-
+	length = get_unaligned_be32(first_lun_data->scsi_lun);
 	num_luns_reported = (length / sizeof(struct scsi_lun));
 
 	if (num_luns_reported > max_scsi_report_luns) {
@@ -1537,10 +1531,7 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 				 * Get the length from the first four bytes
 				 * of second_lun_data.
 				 */
-				data = (u8 *) lun_data->scsi_lun;
-				length = ((data[0] << 24) | (data[1] << 16) |
-					  (data[2] << 8) | (data[3] << 0));
-
+				length = get_unaligned_be32(lun_data->scsi_lun);
 				num_luns = (length / sizeof(struct scsi_lun));
 			}
 		}
-- 
1.8.3.1

Re: [PATCH V4 0/5] scsi: Configure number of LUs reported by 'report-luns'

[Rob Evers]

On 11/17/2014 10:22 AM, Rob Evers wrote:
> This patch set retrieves the number of LUs available on a target
> using the report-luns command.  The initial size of the report-luns
> command is 512 entries, as the previous default initial number was.
> If more LUs than 511 are present on a target, the report-luns is
> re-issued with the size indicated in the result of the original
> report-luns, up to max_report_luns.

Thanks for your feedback. I'll work on V5.

Rob