[PATCH 0/2] configure number of LUs reported by 'report-luns'

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

Some warnings were detected by checkpatch.pl.  Requested feedback
from the maintainer but none provided.

Rob Evers (2):
  Encapsulate scsi_do_report_luns
  Configure reported luns

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

-- 
1.7.11.7

[PATCH 1/2] 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

[PATCH 2/2] 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 | 79 +++++++++++++++++++++++++++++++++++-------------
 1 file changed, 58 insertions(+), 21 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index b2abf22..0f7ce45 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,80 @@ 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 {
+		lun_data = first_lun_data;
 	}
 
 	SCSI_LOG_SCAN_BUS(3, sdev_printk (KERN_INFO, sdev,
-- 
1.7.11.7

Re: [PATCH 2/2] Configure reported luns

[Mike Christie]

On 01/28/2013 02:27 PM, Rob Evers wrote:
> 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 | 79 +++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 58 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index b2abf22..0f7ce45 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,80 @@ 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;


I think it seems like a good idea from the user perspective. They do not
have to worry about setting the modparam. Maybe this is not done already
because of bad targets? I do not know. Maybe someone else does.

One question about the memory allocation here. I know it was already
like this, but why are we using GFP_ATOMIC here and in some of the other
places in this code path? We sleep in some of these paths and some
functions we call use GFP_KERNEL, so it seems like we could use
something that is a little safer from allocation failures like GFP_NOIO?

If we change it, that could be another patch that modifies all the scan
code since you are are just carrying over the existing gfp flag in this
patch and this patch is just doing the one change to the report luns code.

Re: [PATCH 2/2] Configure reported luns

[Rob Evers]

Thanks for looking Mike.

I agree on changing the GFP_ATOMIC to something less
restrictive but wonder if NOIO is the right choice or not.

See below.

  02/01/2013 02:44 AM, Mike Christie wrote:
> On 01/28/2013 02:27 PM, Rob Evers wrote:
>> 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 | 79 +++++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 58 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>> index b2abf22..0f7ce45 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,80 @@ 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;
> I think it seems like a good idea from the user perspective. They do not
> have to worry about setting the modparam. Maybe this is not done already
> because of bad targets? I do not know. Maybe someone else does.
>
> One question about the memory allocation here. I know it was already
> like this, but why are we using GFP_ATOMIC here and in some of the other
> places in this code path? We sleep in some of these paths and some
> functions we call use GFP_KERNEL,

As you say...

Followed this call chain and noticed blk_alloc_queue_node(GFP_KERNEL, ...)

scsi_report_lun_scan->
   scsi_probe_and_add_lun->scsi_alloc_sdev->
     scsi_alloc_queue->
       __scsi_alloc_queue->
         blk_init_queue->blk_init_queue_node->
           blk_alloc_queue_node(GFP_KERNEL...)->
             kmem_cache_alloc_node

> so it seems like we could use
> something that is a little safer from allocation failures like GFP_NOIO?

Seems GFP_NOIO would be better than GFP_ATOMIC for
scsi_report_lun_scan() and scsi_probe_and_add_lun()

The question in my mind is whether it is ok to use GFP_KERNEL
in scsi_report_lun_scan() and scsi_probe_and_add_lun() or
should the block_alloc_queue_node() be changed to use GFP_NOIO?

> If we change it, that could be another patch that modifies all the scan
> code since you are are just carrying over the existing gfp flag in this
> patch and this patch is just doing the one change to the report luns code.
>

The kmalloc in scsi_complete_async_scans() uses GFP_KERNEL
which looks ok to me.  Agree?