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

[PATCH V5 0/3] 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 by re-sizing the returned data
buffer and retrying report luns.

A minor bug fix is included.

scsi_mod parameter max_report_luns is no longer used and is removed.

Changes from previous posting:

 - simplify implementation by removing max_report_luns parameter and
   removing 2nd kmalloc failure handling

 - remove patch to fix return value on first kmalloc failing

 - drop do_scsi_report_luns encapsulation as it isn't required
   with new implementation

Rob Evers (3):
  scsi: Avoid unnecessary GFP_ATOMIC allocation in scsi_report_lun_scan
  scsi: Use set/get_unaligned_be32 in report_luns
  scsi: Retry report-luns when reported LU count requres more memory

 drivers/scsi/scsi_scan.c | 54 ++++++++++++++----------------------------------
 1 file changed, 15 insertions(+), 39 deletions(-)

-- 
1.8.3.1

[PATCH V5 1/3] scsi: Avoid unnecessary GFP_ATOMIC allocation in scsi_report_lun_scan

[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

[PATCH V5 2/3] scsi: Use set/get_unaligned_be32 in report_luns

[Rob Evers]

---
 drivers/scsi/scsi_scan.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index e460b35..8db1f6f 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>
@@ -1359,7 +1360,6 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 	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);
@@ -1425,10 +1425,7 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 	/*
 	 * 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 */
@@ -1476,9 +1473,7 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 	/*
 	 * Get the length from the first four bytes of 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));
 	if (num_luns > max_scsi_report_luns) {
-- 
1.8.3.1

[PATCH V5 3/3] scsi: Retry report-luns when reported LU count requres more memory

[Rob Evers]

Update scsi_report_lun_scan to initially always report up to 511 LUs,
as the previous default max_report_luns did.  Retry in a loop if not
enough memory is available for the number of LUs reported.  Parameter
max_report_luns is removed as it is no longer used.
---
 drivers/scsi/scsi_scan.c | 43 ++++++++++++-------------------------------
 1 file changed, 12 insertions(+), 31 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 8db1f6f..ecd8703 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -80,6 +80,7 @@
 
 static const char *scsi_null_device_strs = "nullnullnullnull";
 
+#define INITIAL_MAX_SCSI_REPORT_LUNS	511
 #define MAX_SCSI_LUNS	512
 
 static u64 max_scsi_luns = MAX_SCSI_LUNS;
@@ -99,20 +100,6 @@ char scsi_scan_type[6] = SCSI_SCAN_TYPE_DEFAULT;
 module_param_string(scan, scsi_scan_type, sizeof(scsi_scan_type), S_IRUGO);
 MODULE_PARM_DESC(scan, "sync, async or none");
 
-/*
- * max_scsi_report_luns: the maximum number of LUNS that will be
- * returned from the REPORT LUNS command. 8 times this value must
- * be allocated. In theory this could be up to an 8 byte value, but
- * in practice, the maximum number of LUNs suppored by any device
- * is about 16k.
- */
-static unsigned int max_scsi_report_luns = 511;
-
-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)");
-
 static unsigned int scsi_inq_timeout = SCSI_TIMEOUT/HZ + 18;
 
 module_param_named(inq_timeout, scsi_inq_timeout, uint, S_IRUGO|S_IWUSR);
@@ -1399,15 +1386,10 @@ 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.
+	 * plus the number of luns we are requesting.
 	 */
-	length = (max_scsi_report_luns + 1) * sizeof(struct scsi_lun);
+	length = (INITIAL_MAX_SCSI_REPORT_LUNS + 1) * sizeof(struct scsi_lun);
+retry:
 	lun_data = kmalloc(length, GFP_KERNEL |
 			   (sdev->host->unchecked_isa_dma ? __GFP_DMA : 0));
 	if (!lun_data) {
@@ -1473,17 +1455,16 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 	/*
 	 * Get the length from the first four bytes of lun_data.
 	 */
-	length = get_unaligned_be32(lun_data->scsi_lun);
+	if (get_unaligned_be32(lun_data->scsi_lun) +
+	    sizeof(struct scsi_lun) > length) {
+		length = get_unaligned_be32(lun_data->scsi_lun) +
+			 sizeof(struct scsi_lun);
+		kfree(lun_data);
+		goto retry;
+	} else
+		length = get_unaligned_be32(lun_data->scsi_lun);
 
 	num_luns = (length / sizeof(struct scsi_lun));
-	if (num_luns > max_scsi_report_luns) {
-		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;
-	}
 
 	SCSI_LOG_SCAN_BUS(3, sdev_printk (KERN_INFO, sdev,
 		"scsi scan: REPORT LUN scan\n"));
-- 
1.8.3.1

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

[Rob Evers]

On 12/05/2014 02:37 PM, Rob Evers wrote:
> This patch set retrieves the number of LUs available on a target
> using the report-luns command by re-sizing the returned data
> buffer and retrying report luns.
>
> A minor bug fix is included.
>
> scsi_mod parameter max_report_luns is no longer used and is removed.
>
> Changes from previous posting:
>
>   - simplify implementation by removing max_report_luns parameter and
>     removing 2nd kmalloc failure handling
>
>   - remove patch to fix return value on first kmalloc failing
>
>   - drop do_scsi_report_luns encapsulation as it isn't required
>     with new implementation
>
> Rob Evers (3):
>    scsi: Avoid unnecessary GFP_ATOMIC allocation in scsi_report_lun_scan
>    scsi: Use set/get_unaligned_be32 in report_luns
>    scsi: Retry report-luns when reported LU count requres more memory
>
>   drivers/scsi/scsi_scan.c | 54 ++++++++++++++----------------------------------
>   1 file changed, 15 insertions(+), 39 deletions(-)
>

Patch set Signed-off-by: Rob Evers <revers@redhat.com>

Re: [PATCH V5 1/3] scsi: Avoid unnecessary GFP_ATOMIC allocation in scsi_report_lun_scan

[Christoph Hellwig]

On Fri, Dec 05, 2014 at 02:37:41PM -0500, Rob Evers wrote:
> Signed-off-by: Rob Evers <revers@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH V5 2/3] scsi: Use set/get_unaligned_be32 in report_luns

[Christoph Hellwig]

Looks good, but I still need your signoff.

Re: [PATCH V5 3/3] scsi: Retry report-luns when reported LU count requres more memory

[Christoph Hellwig]

On Fri, Dec 05, 2014 at 02:37:43PM -0500, Rob Evers wrote:
> Update scsi_report_lun_scan to initially always report up to 511 LUs,
> as the previous default max_report_luns did.  Retry in a loop if not
> enough memory is available for the number of LUs reported.  Parameter
> max_report_luns is removed as it is no longer used.
> ---
>  drivers/scsi/scsi_scan.c | 43 ++++++++++++-------------------------------
>  1 file changed, 12 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 8db1f6f..ecd8703 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -80,6 +80,7 @@
>  
>  static const char *scsi_null_device_strs = "nullnullnullnull";
>  
> +#define INITIAL_MAX_SCSI_REPORT_LUNS	511

The name seems very verbose.  Given that it's only used once we might
as well just remove it and add a comment about why this number is chosen
instead.


> -	length = get_unaligned_be32(lun_data->scsi_lun);
> +	if (get_unaligned_be32(lun_data->scsi_lun) +
> +	    sizeof(struct scsi_lun) > length) {
> +		length = get_unaligned_be32(lun_data->scsi_lun) +
> +			 sizeof(struct scsi_lun);
> +		kfree(lun_data);
> +		goto retry;
> +	} else
> +		length = get_unaligned_be32(lun_data->scsi_lun);

The else won't be reached due to the goto.  Just remove it and one
level of indentation for the next line.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>