[PATCH 0/6] scsi_debug: a few bug fixes and enable clustering support

[PATCH 0/6] scsi_debug: a few bug fixes and enable clustering support

[Akinobu Mita]

This patch set includes a few bug fixes and patches for enabling
clustering support for scsi_debug.  Some of bug fixes were already
sent before.

The reason why I would like to enable clustering is to test commands
with huge transfer length.  Without enabling clustering support, the
transfer length for read and write scsi commands is limited upto 8MB
when page size is 4KB and sg_tablesize is 2048
(= SCSI_MAX_SG_CHAIN_SEGMENTS).

Akinobu Mita (6):
  scsi_debug: fix false positive logical block reference tag check fail
  scsi_debug: make pseudo_primary static
  scsi_debug: fix duplicate dif_errors increment
  scsi_debug: fix resp_xdwriteread() return value when running out of
    memory
  scsi_debug: prepare to enable clustering
  scsi_debug: add ability to enable clustering

 drivers/scsi/scsi_debug.c | 137 +++++++++++++++++++++++++++-------------------
 1 file changed, 80 insertions(+), 57 deletions(-)

Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
-- 
1.8.3.2

[PATCH 1/6] scsi_debug: fix false positive logical block reference tag check fail

[Akinobu Mita]

Reading partially unwritten sectors generates a false positive logical
block reference tag check failure when DIF is enabled.

This bug is caused by missing ei_lba increment in loop of dif_verify()
when unwritten sector is skipped.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/scsi_debug.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 2decc64..bdfb9be 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1832,7 +1832,7 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 	struct sd_dif_tuple *sdt;
 	sector_t sector;
 
-	for (i = 0; i < sectors; i++) {
+	for (i = 0; i < sectors; i++, ei_lba++) {
 		int ret;
 
 		sector = start_sec + i;
@@ -1846,8 +1846,6 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 			dif_errors++;
 			return ret;
 		}
-
-		ei_lba++;
 	}
 
 	dif_copy_prot(SCpnt, start_sec, sectors, true);
-- 
1.8.3.2

Re: [PATCH 1/6] scsi_debug: fix false positive logical block reference tag check fail

[Martin K. Petersen]

>>>>> "Akinobu" == Akinobu Mita <akinobu.mita@gmail.com> writes:

Akinobu> Reading partially unwritten sectors generates a false positive
Akinobu> logical block reference tag check failure when DIF is enabled.

Akinobu> This bug is caused by missing ei_lba increment in loop of
Akinobu> dif_verify() when unwritten sector is skipped.

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 2/6] scsi_debug: make pseudo_primary static

[Akinobu Mita]

As pseudo_primary is only used in scsi_debug.c, it should be static.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/scsi_debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index bdfb9be..9cd211e 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3246,7 +3246,7 @@ static struct attribute *sdebug_drv_attrs[] = {
 };
 ATTRIBUTE_GROUPS(sdebug_drv);
 
-struct device *pseudo_primary;
+static struct device *pseudo_primary;
 
 static int __init scsi_debug_init(void)
 {
-- 
1.8.3.2

[PATCH 3/6] scsi_debug: fix duplicate dif_errors increment

[Akinobu Mita]

It is unnecessary to increase dif_errors in dif_verify(), because the
caller will increment it when dif_verify() detects failure.

This bug was introduced by commit beb40ea42bd6 ("[SCSI] scsi_debug:
reduce duplication between prot_verify_read and prot_verify_write")

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/scsi_debug.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 9cd211e..1a42880 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1780,7 +1780,6 @@ static int dif_verify(struct sd_dif_tuple *sdt, const void *data,
 	    be32_to_cpu(sdt->ref_tag) != ei_lba) {
 		pr_err("%s: REF check failed on sector %lu\n",
 			__func__, (unsigned long)sector);
-			dif_errors++;
 		return 0x03;
 	}
 	return 0;
-- 
1.8.3.2

Re: [PATCH 3/6] scsi_debug: fix duplicate dif_errors increment

[Martin K. Petersen]

>>>>> "Akinobu" == Akinobu Mita <akinobu.mita@gmail.com> writes:

Akinobu> It is unnecessary to increase dif_errors in dif_verify(),
Akinobu> because the caller will increment it when dif_verify() detects
Akinobu> failure.

Akinobu> This bug was introduced by commit beb40ea42bd6 ("[SCSI]
Akinobu> scsi_debug: reduce duplication between prot_verify_read and
Akinobu> prot_verify_write")

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 4/6] scsi_debug: fix resp_xdwriteread() return value when running out of memory

[Akinobu Mita]

When resp_xdwriteread() can't allocate temporary buffer, it returns -1.
But the return value is used as scsi status code and -1 is not
interpreted as correct code.

target_core_mod has similar xdwriteread emulation code. So this mimics
what target_core_mod does for xdwriteread when running out of memory.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/scsi_debug.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 1a42880..a102519 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -64,6 +64,7 @@ static const char * scsi_debug_version_date = "20100324";
 /* Additional Sense Code (ASC) */
 #define NO_ADDITIONAL_SENSE 0x0
 #define LOGICAL_UNIT_NOT_READY 0x4
+#define LOGICAL_UNIT_COMMUNICATION_FAILURE 0x8
 #define UNRECOVERED_READ_ERR 0x11
 #define PARAMETER_LIST_LENGTH_ERR 0x1a
 #define INVALID_OPCODE 0x20
@@ -2318,8 +2319,11 @@ static int resp_xdwriteread(struct scsi_cmnd *scp, unsigned long long lba,
 
 	/* better not to use temporary buffer. */
 	buf = kmalloc(scsi_bufflen(scp), GFP_ATOMIC);
-	if (!buf)
-		return ret;
+	if (!buf) {
+		mk_sense_buffer(devip, NOT_READY,
+				LOGICAL_UNIT_COMMUNICATION_FAILURE, 0);
+		return check_condition_result;
+	}
 
 	scsi_sg_copy_to_buffer(scp, buf, scsi_bufflen(scp));
 
-- 
1.8.3.2

[PATCH 5/6] scsi_debug: prepare to enable clustering

[Akinobu Mita]

Currently, clustering support for scsi_debug is disabled.  This is
because there are for_each_sg() loops which assume that each sg list
element is consisted with a single page.  But enabling clustering
support, each sg list element for scsi commands can be consisted with
multiple pages.

This replaces these for_each_sg() loops with sg mapping iterator which
is capable of handling each sg list element is consisted with multiple
pages.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/scsi_debug.c | 118 ++++++++++++++++++++++++++--------------------
 1 file changed, 68 insertions(+), 50 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index a102519..aca70c1 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1789,23 +1789,29 @@ static int dif_verify(struct sd_dif_tuple *sdt, const void *data,
 static void dif_copy_prot(struct scsi_cmnd *SCpnt, sector_t sector,
 			  unsigned int sectors, bool read)
 {
-	unsigned int i, resid;
-	struct scatterlist *psgl;
+	size_t resid;
 	void *paddr;
 	const void *dif_store_end = dif_storep + sdebug_store_sectors;
+	struct sg_mapping_iter miter;
+	unsigned long flags;
 
 	/* Bytes of protection data to copy into sgl */
 	resid = sectors * sizeof(*dif_storep);
 
-	scsi_for_each_prot_sg(SCpnt, psgl, scsi_prot_sg_count(SCpnt), i) {
-		int len = min(psgl->length, resid);
+	sg_miter_start(&miter, scsi_prot_sglist(SCpnt),
+			scsi_prot_sg_count(SCpnt), SG_MITER_ATOMIC |
+			(read ? SG_MITER_TO_SG : SG_MITER_FROM_SG));
+	local_irq_save(flags);
+
+	while (sg_miter_next(&miter) && resid > 0) {
+		size_t len = min(miter.length, resid);
 		void *start = dif_store(sector);
-		int rest = 0;
+		size_t rest = 0;
 
 		if (dif_store_end < start + len)
 			rest = start + len - dif_store_end;
 
-		paddr = kmap_atomic(sg_page(psgl)) + psgl->offset;
+		paddr = miter.addr;
 
 		if (read)
 			memcpy(paddr, start, len - rest);
@@ -1821,8 +1827,9 @@ static void dif_copy_prot(struct scsi_cmnd *SCpnt, sector_t sector,
 
 		sector += len / sizeof(*dif_storep);
 		resid -= len;
-		kunmap_atomic(paddr);
 	}
+	sg_miter_stop(&miter);
+	local_irq_restore(flags);
 }
 
 static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
@@ -1929,55 +1936,65 @@ void dump_sector(unsigned char *buf, int len)
 static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 			     unsigned int sectors, u32 ei_lba)
 {
-	int i, j, ret;
+	int ret;
 	struct sd_dif_tuple *sdt;
-	struct scatterlist *dsgl;
-	struct scatterlist *psgl = scsi_prot_sglist(SCpnt);
-	void *daddr, *paddr;
+	void *daddr;
 	sector_t sector = start_sec;
 	int ppage_offset;
+	int dpage_offset;
+	struct sg_mapping_iter diter;
+	struct sg_mapping_iter piter;
+	unsigned long flags;
 
 	BUG_ON(scsi_sg_count(SCpnt) == 0);
 	BUG_ON(scsi_prot_sg_count(SCpnt) == 0);
 
-	ppage_offset = 0;
-
-	/* For each data page */
-	scsi_for_each_sg(SCpnt, dsgl, scsi_sg_count(SCpnt), i) {
-		daddr = kmap_atomic(sg_page(dsgl)) + dsgl->offset;
-		paddr = kmap_atomic(sg_page(psgl)) + psgl->offset;
-
-		/* For each sector-sized chunk in data page */
-		for (j = 0; j < dsgl->length; j += scsi_debug_sector_size) {
+	sg_miter_start(&piter, scsi_prot_sglist(SCpnt),
+			scsi_prot_sg_count(SCpnt),
+			SG_MITER_ATOMIC | SG_MITER_FROM_SG);
+	sg_miter_start(&diter, scsi_sglist(SCpnt), scsi_sg_count(SCpnt),
+			SG_MITER_ATOMIC | SG_MITER_FROM_SG);
+	local_irq_save(flags);
+
+	/* For each protection page */
+	while (sg_miter_next(&piter)) {
+		dpage_offset = 0;
+		if (WARN_ON(!sg_miter_next(&diter))) {
+			ret = 0x01;
+			goto out;
+		}
 
+		for (ppage_offset = 0; ppage_offset < piter.length;
+		     ppage_offset += sizeof(struct sd_dif_tuple)) {
 			/* If we're at the end of the current
-			 * protection page advance to the next one
+			 * data page advance to the next one
 			 */
-			if (ppage_offset >= psgl->length) {
-				kunmap_atomic(paddr);
-				psgl = sg_next(psgl);
-				BUG_ON(psgl == NULL);
-				paddr = kmap_atomic(sg_page(psgl))
-					+ psgl->offset;
-				ppage_offset = 0;
+			if (dpage_offset >= diter.length) {
+				if (WARN_ON(!sg_miter_next(&diter))) {
+					ret = 0x01;
+					goto out;
+				}
+				dpage_offset = 0;
 			}
 
-			sdt = paddr + ppage_offset;
+			sdt = piter.addr + ppage_offset;
+			daddr = diter.addr + dpage_offset;
 
-			ret = dif_verify(sdt, daddr + j, sector, ei_lba);
+			ret = dif_verify(sdt, daddr, sector, ei_lba);
 			if (ret) {
-				dump_sector(daddr + j, scsi_debug_sector_size);
+				dump_sector(daddr, scsi_debug_sector_size);
 				goto out;
 			}
 
 			sector++;
 			ei_lba++;
-			ppage_offset += sizeof(struct sd_dif_tuple);
+			dpage_offset += scsi_debug_sector_size;
 		}
-
-		kunmap_atomic(paddr);
-		kunmap_atomic(daddr);
+		diter.consumed = dpage_offset;
+		sg_miter_stop(&diter);
 	}
+	sg_miter_stop(&piter);
+	local_irq_restore(flags);
 
 	dif_copy_prot(SCpnt, start_sec, sectors, false);
 	dix_writes++;
@@ -1986,8 +2003,8 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 
 out:
 	dif_errors++;
-	kunmap_atomic(paddr);
-	kunmap_atomic(daddr);
+	sg_miter_stop(&diter);
+	sg_miter_stop(&piter);
 	return ret;
 }
 
@@ -2311,11 +2328,12 @@ static int resp_report_luns(struct scsi_cmnd * scp,
 static int resp_xdwriteread(struct scsi_cmnd *scp, unsigned long long lba,
 			    unsigned int num, struct sdebug_dev_info *devip)
 {
-	int i, j, ret = -1;
+	int j;
 	unsigned char *kaddr, *buf;
 	unsigned int offset;
-	struct scatterlist *sg;
 	struct scsi_data_buffer *sdb = scsi_in(scp);
+	struct sg_mapping_iter miter;
+	unsigned long flags;
 
 	/* better not to use temporary buffer. */
 	buf = kmalloc(scsi_bufflen(scp), GFP_ATOMIC);
@@ -2328,22 +2346,22 @@ static int resp_xdwriteread(struct scsi_cmnd *scp, unsigned long long lba,
 	scsi_sg_copy_to_buffer(scp, buf, scsi_bufflen(scp));
 
 	offset = 0;
-	for_each_sg(sdb->table.sgl, sg, sdb->table.nents, i) {
-		kaddr = (unsigned char *)kmap_atomic(sg_page(sg));
-		if (!kaddr)
-			goto out;
+	sg_miter_start(&miter, sdb->table.sgl, sdb->table.nents,
+			SG_MITER_ATOMIC | SG_MITER_TO_SG);
+	local_irq_save(flags);
 
-		for (j = 0; j < sg->length; j++)
-			*(kaddr + sg->offset + j) ^= *(buf + offset + j);
+	while (sg_miter_next(&miter)) {
+		kaddr = miter.addr;
+		for (j = 0; j < miter.length; j++)
+			*(kaddr + j) ^= *(buf + offset + j);
 
-		offset += sg->length;
-		kunmap_atomic(kaddr);
+		offset += miter.length;
 	}
-	ret = 0;
-out:
+	sg_miter_stop(&miter);
+	local_irq_restore(flags);
 	kfree(buf);
 
-	return ret;
+	return 0;
 }
 
 /* When timer goes off this function is called. */
-- 
1.8.3.2

Re: [PATCH 5/6] scsi_debug: prepare to enable clustering

[Martin K. Petersen]

>>>>> "Akinobu" == Akinobu Mita <akinobu.mita@gmail.com> writes:

Akinobu> This replaces these for_each_sg() loops with sg mapping
Akinobu> iterator which is capable of handling each sg list element is
Akinobu> consisted with multiple pages.

Looks good in general but what's with disabling interrupts? Would it be
better to move the PI verification in under atomic_rw?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 5/6] scsi_debug: prepare to enable clustering

[Akinobu Mita]

2014/1/11 Martin K. Petersen <martin.petersen@oracle.com>:
>>>>>> "Akinobu" == Akinobu Mita <akinobu.mita@gmail.com> writes:
>
> Akinobu> This replaces these for_each_sg() loops with sg mapping
> Akinobu> iterator which is capable of handling each sg list element is
> Akinobu> consisted with multiple pages.
>
> Looks good in general but what's with disabling interrupts? Would it be

Disabling interrupts look unnecessary and removing these simplies the
change.  I was blindly borrowing the code from sg_copy_{from,to}_buffer().
I'll fix it in the next version.

> better to move the PI verification in under atomic_rw?

It also looks right.  Besides that, I realized that resp_unmap() calls
unmap_region() which also needs atomic_rw write lock, but it doesn't
lock.  I'll do it in another patch.

[PATCH 6/6] scsi_debug: add ability to enable clustering

[Akinobu Mita]

This adds a module parameter to enable clustering.

Without enabling clustering support, the transfer length for read and
write scsi commands is limited upto 8MB when page size is 4KB and
sg_tablesize is 2048 (= SCSI_MAX_SG_CHAIN_SEGMENTS).  I would like to
test commands with more than that transfer length.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/scsi_debug.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index aca70c1..9297688d 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -196,6 +196,7 @@ static unsigned int scsi_debug_unmap_max_blocks = DEF_UNMAP_MAX_BLOCKS;
 static unsigned int scsi_debug_unmap_max_desc = DEF_UNMAP_MAX_DESC;
 static unsigned int scsi_debug_write_same_length = DEF_WRITESAME_LENGTH;
 static bool scsi_debug_removable = DEF_REMOVABLE;
+static bool scsi_debug_clustering;
 
 static int scsi_debug_cmnd_count = 0;
 
@@ -2787,6 +2788,7 @@ module_param_named(opts, scsi_debug_opts, int, S_IRUGO | S_IWUSR);
 module_param_named(physblk_exp, scsi_debug_physblk_exp, int, S_IRUGO);
 module_param_named(ptype, scsi_debug_ptype, int, S_IRUGO | S_IWUSR);
 module_param_named(removable, scsi_debug_removable, bool, S_IRUGO | S_IWUSR);
+module_param_named(clustering, scsi_debug_clustering, bool, S_IRUGO);
 module_param_named(scsi_level, scsi_debug_scsi_level, int, S_IRUGO);
 module_param_named(sector_size, scsi_debug_sector_size, int, S_IRUGO);
 module_param_named(unmap_alignment, scsi_debug_unmap_alignment, int, S_IRUGO);
@@ -3953,6 +3955,8 @@ static int sdebug_driver_probe(struct device * dev)
 	sdbg_host = to_sdebug_host(dev);
 
 	sdebug_driver_template.can_queue = scsi_debug_max_queue;
+	if (scsi_debug_clustering)
+		sdebug_driver_template.use_clustering = ENABLE_CLUSTERING;
 	hpnt = scsi_host_alloc(&sdebug_driver_template, sizeof(sdbg_host));
 	if (NULL == hpnt) {
 		printk(KERN_ERR "%s: scsi_register failed\n", __func__);
-- 
1.8.3.2

Re: [PATCH 6/6] scsi_debug: add ability to enable clustering

[Douglas Gilbert]

On 14-01-05 06:33 PM, Akinobu Mita wrote:
> This adds a module parameter to enable clustering.
>
> Without enabling clustering support, the transfer length for read and
> write scsi commands is limited upto 8MB when page size is 4KB and
> sg_tablesize is 2048 (= SCSI_MAX_SG_CHAIN_SEGMENTS).  I would like to
> test commands with more than that transfer length.
>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
> Cc: Douglas Gilbert <dgilbert@interlog.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: linux-scsi@vger.kernel.org
> ---
>   drivers/scsi/scsi_debug.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index aca70c1..9297688d 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -196,6 +196,7 @@ static unsigned int scsi_debug_unmap_max_blocks = DEF_UNMAP_MAX_BLOCKS;
>   static unsigned int scsi_debug_unmap_max_desc = DEF_UNMAP_MAX_DESC;
>   static unsigned int scsi_debug_write_same_length = DEF_WRITESAME_LENGTH;
>   static bool scsi_debug_removable = DEF_REMOVABLE;
> +static bool scsi_debug_clustering;
>
>   static int scsi_debug_cmnd_count = 0;
>
> @@ -2787,6 +2788,7 @@ module_param_named(opts, scsi_debug_opts, int, S_IRUGO | S_IWUSR);
>   module_param_named(physblk_exp, scsi_debug_physblk_exp, int, S_IRUGO);
>   module_param_named(ptype, scsi_debug_ptype, int, S_IRUGO | S_IWUSR);
>   module_param_named(removable, scsi_debug_removable, bool, S_IRUGO | S_IWUSR);
> +module_param_named(clustering, scsi_debug_clustering, bool, S_IRUGO);

Umm, clustering is writeable, so: S_IRUGO | S_IWUSR

>   module_param_named(scsi_level, scsi_debug_scsi_level, int, S_IRUGO);
>   module_param_named(sector_size, scsi_debug_sector_size, int, S_IRUGO);
>   module_param_named(unmap_alignment, scsi_debug_unmap_alignment, int, S_IRUGO);
> @@ -3953,6 +3955,8 @@ static int sdebug_driver_probe(struct device * dev)
>   	sdbg_host = to_sdebug_host(dev);
>
>   	sdebug_driver_template.can_queue = scsi_debug_max_queue;
> +	if (scsi_debug_clustering)
> +		sdebug_driver_template.use_clustering = ENABLE_CLUSTERING;
>   	hpnt = scsi_host_alloc(&sdebug_driver_template, sizeof(sdbg_host));
>   	if (NULL == hpnt) {
>   		printk(KERN_ERR "%s: scsi_register failed\n", __func__);
>

And please document this extra parameter for modinfo.
Something like:
   MODULE_PARM_DESC(clustering, "when set enables larger transfers (def=0)");

placed in alphabetical order.

Doug Gilbert

Re: [PATCH 6/6] scsi_debug: add ability to enable clustering

[Akinobu Mita]

2014/1/6 Douglas Gilbert <dgilbert@interlog.com>:
>> @@ -2787,6 +2788,7 @@ module_param_named(opts, scsi_debug_opts, int,
>> S_IRUGO | S_IWUSR);
>>   module_param_named(physblk_exp, scsi_debug_physblk_exp, int, S_IRUGO);
>>   module_param_named(ptype, scsi_debug_ptype, int, S_IRUGO | S_IWUSR);
>>   module_param_named(removable, scsi_debug_removable, bool, S_IRUGO |
>> S_IWUSR);
>> +module_param_named(clustering, scsi_debug_clustering, bool, S_IRUGO);
>
>
> Umm, clustering is writeable, so: S_IRUGO | S_IWUSR

OK, it makes sense to make this parameter writable when we adds
scsi_debug hosts dynamically through 'add_host' driver attribute.

>>   module_param_named(scsi_level, scsi_debug_scsi_level, int, S_IRUGO);
>>   module_param_named(sector_size, scsi_debug_sector_size, int, S_IRUGO);
>>   module_param_named(unmap_alignment, scsi_debug_unmap_alignment, int,
>> S_IRUGO);
>> @@ -3953,6 +3955,8 @@ static int sdebug_driver_probe(struct device * dev)
>>         sdbg_host = to_sdebug_host(dev);
>>
>>         sdebug_driver_template.can_queue = scsi_debug_max_queue;
>> +       if (scsi_debug_clustering)
>> +               sdebug_driver_template.use_clustering = ENABLE_CLUSTERING;
>>         hpnt = scsi_host_alloc(&sdebug_driver_template,
>> sizeof(sdbg_host));
>>         if (NULL == hpnt) {
>>                 printk(KERN_ERR "%s: scsi_register failed\n", __func__);
>>
>
> And please document this extra parameter for modinfo.
> Something like:
>   MODULE_PARM_DESC(clustering, "when set enables larger transfers (def=0)");
>
> placed in alphabetical order.

I'll update this patch and resend it.

Thanks for your review.