[PATCH v2 0/3] Configure number of LUs reported by 'report-luns'

[PATCH v2 0/3] 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.

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

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 (3):
  Encapsulate scsi_do_report_luns
  Configure reported luns
  Change kmallocs in report_luns to use GFP_KERNEL

 drivers/scsi/scsi_scan.c | 196 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 126 insertions(+), 70 deletions(-)

-- 
1.7.11.7

[PATCH v2 1/3] Encapsulate scsi_do_report_luns

[Rob Evers]

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

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 3e58b22..b2abf22 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1282,6 +1282,64 @@ void int_to_scsilun(unsigned int 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;
+
+	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, printk(KERN_INFO "scsi scan: Sending"
+				  " REPORT LUNS to %s (try %d)\n", devname,
+				  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, printk(KERN_INFO "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
@@ -1306,15 +1364,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;
 	unsigned int 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;
@@ -1369,53 +1424,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, printk (KERN_INFO "scsi scan: Sending"
-				" REPORT LUNS to %s (try %d)\n", devname,
-				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, printk (KERN_INFO "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.7.11.7

Re: [PATCH v2 1/3] Encapsulate scsi_do_report_luns

[Bart Van Assche]

On 02/18/13 20:15, Rob Evers wrote:
> +	/*
> +	 * 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;

Have you considered using put_unaligned_be32() here ?

Bart.

Re: [PATCH v2 1/3] Encapsulate scsi_do_report_luns

[Rob Evers]

On 02/19/2013 02:15 AM, Bart Van Assche wrote:
> On 02/18/13 20:15, Rob Evers wrote:
>> +    /*
>> +     * 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;
>
> Have you considered using put_unaligned_be32() here ?
>
> Bart.

Thanks for the suggestion Bart.

James,

Are you in favor of the type of change
Bart is suggesting?

I thought I saw some email a while back where
you disagreed with stylistic only changes to existing
code.  Is that true and does it apply here?

Rob

[PATCH v2 2/3] Configure reported luns

[Rob Evers]

Change default value of max_report_luns to 16k-1.

Use data returned from max report luns command to configure the number
of logical units present if previous default of 511 isn't enough.

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

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index b2abf22..671ff58 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -109,12 +109,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;
 
@@ -1366,9 +1367,10 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 	char devname[64];
 	unsigned int length;
 	unsigned int 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);
@@ -1409,45 +1411,90 @@ 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_ATOMIC |
-			   (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_ATOMIC |
+				 (sdev->host->unchecked_isa_dma ?
+				 __GFP_DMA : 0));
+	if (!first_lun_data) {
 		printk(ALLOC_FAILURE_MSG, __func__);
 		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);
 		printk(KERN_WARNING "scsi: On %s only %d (max_scsi_report_luns)"
 		       " of %d luns reported, try increasing"
-		       " max_scsi_report_luns.\n", devname,
-		       max_scsi_report_luns, num_luns);
-		num_luns = max_scsi_report_luns;
+		       " max_report_luns parameter.\n", devname,
+		       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_ATOMIC |
+					  (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__);
+			printk(KERN_WARNING "scsi: On %s %d of %d luns reported.\n",
+			       devname, 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,
-- 
1.7.11.7

Re: [PATCH v2 2/3] Configure reported luns

[Bart Van Assche]

On 02/18/13 20:15, Rob Evers wrote:
>   	length = ((data[0] << 24) | (data[1] << 16) |
>   		  (data[2] << 8) | (data[3] << 0));
[ ... ]
> +				length = ((data[0] << 24) | (data[1] << 16) |
> +				  (data[2] << 8) | (data[3] << 0));

Please consider using get_unaligned_be32() for these two statements.

Thanks,

Bart.

[PATCH v2 3/3] Change kmallocs in report_luns to use GFP_KERNEL

[Rob Evers]

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

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 671ff58..1d41730 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1419,7 +1419,7 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 		length = (max_scsi_report_luns + 1) *
 			sizeof(struct scsi_lun);
 
-	first_lun_data = kmalloc(length, GFP_ATOMIC |
+	first_lun_data = kmalloc(length, GFP_KERNEL |
 				 (sdev->host->unchecked_isa_dma ?
 				 __GFP_DMA : 0));
 	if (!first_lun_data) {
@@ -1463,7 +1463,7 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 		 * add one for the header
 		 */
 		length = length + sizeof(struct scsi_lun);
-		second_lun_data = kmalloc(length, GFP_ATOMIC |
+		second_lun_data = kmalloc(length, GFP_KERNEL |
 					  (sdev->host->unchecked_isa_dma ?
 					  __GFP_DMA : 0));
 		if (!second_lun_data) {
-- 
1.7.11.7