[PATCH 0/4] scsi: increase upper limit for max_sectors

[PATCH 0/4] scsi: increase upper limit for max_sectors

[Akinobu Mita]

This patchset increases the SCSI mid level's limitation for max_sectors
upto the block layer's limitation for max_hw_sectors, and enables the
scsi_debug driver to request read/write commands with huge transfer
length.  This also includes a few fixes for the problems which can be
caused by huge max_sectors in the block layer.

Akinobu Mita (4):
  block: fix BLKSECTGET ioctl when max_sectors is greater than USHRT_MAX
  block: fix SG_[GS]ET_RESERVED_SIZE ioctl when max_sectors is huge
  scsi: increase upper limit for max_sectors
  scsi_debug: allow huge transfer length for read/write commands

 block/compat_ioctl.c      |  6 ++++--
 block/ioctl.c             |  5 ++++-
 block/scsi_ioctl.c        | 15 +++++++++++----
 drivers/scsi/scsi_debug.c |  6 +++---
 drivers/scsi/sd.c         |  5 +----
 drivers/scsi/sg.c         | 17 +++++++++++++----
 include/scsi/scsi_host.h  |  4 ++--
 7 files changed, 38 insertions(+), 20 deletions(-)

Cc: Jens Axboe <axboe@kernel.dk>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: linux-scsi@vger.kernel.org
-- 
1.9.1

[PATCH 1/4] block: fix BLKSECTGET ioctl when max_sectors is greater than USHRT_MAX

[Akinobu Mita]

BLKSECTGET ioctl loads the request queue's max_sectors as unsigned
short value to the argument pointer.  So if the max_sector is greater
than USHRT_MAX, the upper 16 bits of that is just discarded.

In such case, USHRT_MAX is more preferable than the lower 16 bits of
max_sectors.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: linux-scsi@vger.kernel.org
---
 block/compat_ioctl.c | 6 ++++--
 block/ioctl.c        | 5 ++++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/block/compat_ioctl.c b/block/compat_ioctl.c
index fbd5a67..e0393cd 100644
--- a/block/compat_ioctl.c
+++ b/block/compat_ioctl.c
@@ -663,6 +663,7 @@ long compat_blkdev_ioctl(struct file *file, unsigned cmd, unsigned long arg)
 	fmode_t mode = file->f_mode;
 	struct backing_dev_info *bdi;
 	loff_t size;
+	unsigned int max_sectors;
 
 	/*
 	 * O_NDELAY can be altered using fcntl(.., F_SETFL, ..), so we have
@@ -718,8 +719,9 @@ long compat_blkdev_ioctl(struct file *file, unsigned cmd, unsigned long arg)
 	case BLKSSZGET: /* get block device hardware sector size */
 		return compat_put_int(arg, bdev_logical_block_size(bdev));
 	case BLKSECTGET:
-		return compat_put_ushort(arg,
-					 queue_max_sectors(bdev_get_queue(bdev)));
+		max_sectors = min_t(unsigned int, USHRT_MAX,
+				    queue_max_sectors(bdev_get_queue(bdev)));
+		return compat_put_ushort(arg, max_sectors);
 	case BLKROTATIONAL:
 		return compat_put_ushort(arg,
 					 !blk_queue_nonrot(bdev_get_queue(bdev)));
diff --git a/block/ioctl.c b/block/ioctl.c
index 7d5c3b2..d6cda81 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -278,6 +278,7 @@ int blkdev_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 	struct backing_dev_info *bdi;
 	loff_t size;
 	int ret, n;
+	unsigned int max_sectors;
 
 	switch(cmd) {
 	case BLKFLSBUF:
@@ -375,7 +376,9 @@ int blkdev_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 	case BLKDISCARDZEROES:
 		return put_uint(arg, bdev_discard_zeroes_data(bdev));
 	case BLKSECTGET:
-		return put_ushort(arg, queue_max_sectors(bdev_get_queue(bdev)));
+		max_sectors = min_t(unsigned int, USHRT_MAX,
+				    queue_max_sectors(bdev_get_queue(bdev)));
+		return put_ushort(arg, max_sectors);
 	case BLKROTATIONAL:
 		return put_ushort(arg, !blk_queue_nonrot(bdev_get_queue(bdev)));
 	case BLKRASET:
-- 
1.9.1

Re: [PATCH 1/4] block: fix BLKSECTGET ioctl when max_sectors is greater than USHRT_MAX

[Christoph Hellwig]

On Sun, May 25, 2014 at 09:43:33PM +0900, Akinobu Mita wrote:
> BLKSECTGET ioctl loads the request queue's max_sectors as unsigned
> short value to the argument pointer.  So if the max_sector is greater
> than USHRT_MAX, the upper 16 bits of that is just discarded.
> 
> In such case, USHRT_MAX is more preferable than the lower 16 bits of
> max_sectors.

Looks good,

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

[PATCH 2/4] block: fix SG_[GS]ET_RESERVED_SIZE ioctl when max_sectors is huge

[Akinobu Mita]

SG_GET_RESERVED_SIZE and SG_SET_RESERVED_SIZE ioctls access a reserved
buffer in bytes as int type.  The value needs to be capped at the request
queue's max_sectors.  But integer overflow is not correctly handled in
the calculation when converting max_sectors from sectors to bytes.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: linux-scsi@vger.kernel.org
---
 block/scsi_ioctl.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 2648797..6174bba 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -82,9 +82,18 @@ static int sg_set_timeout(struct request_queue *q, int __user *p)
 	return err;
 }
 
+static int max_sectors_bytes(struct request_queue *q)
+{
+	unsigned int max_sectors = queue_max_sectors(q);
+
+	max_sectors = min_t(unsigned int, max_sectors, INT_MAX >> 9);
+
+	return max_sectors << 9;
+}
+
 static int sg_get_reserved_size(struct request_queue *q, int __user *p)
 {
-	unsigned val = min(q->sg_reserved_size, queue_max_sectors(q) << 9);
+	int val = min_t(int, q->sg_reserved_size, max_sectors_bytes(q));
 
 	return put_user(val, p);
 }
@@ -98,10 +107,8 @@ static int sg_set_reserved_size(struct request_queue *q, int __user *p)
 
 	if (size < 0)
 		return -EINVAL;
-	if (size > (queue_max_sectors(q) << 9))
-		size = queue_max_sectors(q) << 9;
 
-	q->sg_reserved_size = size;
+	q->sg_reserved_size = min(size, max_sectors_bytes(q));
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH 2/4] block: fix SG_[GS]ET_RESERVED_SIZE ioctl when max_sectors is huge

[Christoph Hellwig]

On Sun, May 25, 2014 at 09:43:34PM +0900, Akinobu Mita wrote:
> SG_GET_RESERVED_SIZE and SG_SET_RESERVED_SIZE ioctls access a reserved
> buffer in bytes as int type.  The value needs to be capped at the request
> queue's max_sectors.  But integer overflow is not correctly handled in
> the calculation when converting max_sectors from sectors to bytes.

Looks good,

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

[PATCH 3/4] scsi: increase upper limit for max_sectors

[Akinobu Mita]

max_sectors in struct Scsi_Host specifies maximum number of sectors
allowed in a single SCSI command.  The data type of max_sectors is
unsigned short, so the maximum transfer length per SCSI command is
limited to less than 256MB in 4096-bytes sector size. (0xffff * 4096)

This commit increases the SCSI mid level's limitation for max_sectors
upto the block layer's limitation for max_hw_sectors by extending the
data type of max_sectors in struct Scsi_Host and scsi_host_template,
so that SCSI lower level drivers can specify more than 0xffff.

This change requires the scsi disk (sd) driver to handle the requests
whose transfer length is more than 0xffff with READ_16 or WRITE_16.
Also, this needs to prevent SG_GET_RESERVED_SIZE and SG_SET_RESERVED_SIZE
ioctls for the scsi generic (sg) driver from integer overflow when
converting max_sectors to bytes.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/sd.c        |  5 +----
 drivers/scsi/sg.c        | 17 +++++++++++++----
 include/scsi/scsi_host.h |  4 ++--
 3 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index efcbcd1..4508115 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1056,7 +1056,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 		SCpnt->cmnd[29] = (unsigned char) (this_count >> 16) & 0xff;
 		SCpnt->cmnd[30] = (unsigned char) (this_count >> 8) & 0xff;
 		SCpnt->cmnd[31] = (unsigned char) this_count & 0xff;
-	} else if (sdp->use_16_for_rw) {
+	} else if ((this_count > 0xffff) || sdp->use_16_for_rw) {
 		SCpnt->cmnd[0] += READ_16 - READ_6;
 		SCpnt->cmnd[1] = protect | ((rq->cmd_flags & REQ_FUA) ? 0x8 : 0);
 		SCpnt->cmnd[2] = sizeof(block) > 4 ? (unsigned char) (block >> 56) & 0xff : 0;
@@ -1075,9 +1075,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	} else if ((this_count > 0xff) || (block > 0x1fffff) ||
 		   scsi_device_protection(SCpnt->device) ||
 		   SCpnt->device->use_10_for_rw) {
-		if (this_count > 0xffff)
-			this_count = 0xffff;
-
 		SCpnt->cmnd[0] += READ_10 - READ_6;
 		SCpnt->cmnd[1] = protect | ((rq->cmd_flags & REQ_FUA) ? 0x8 : 0);
 		SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index df5e961..e3404d2 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -806,6 +806,15 @@ static int srp_done(Sg_fd *sfp, Sg_request *srp)
 	return ret;
 }
 
+static int max_sectors_bytes(struct request_queue *q)
+{
+	unsigned int max_sectors = queue_max_sectors(q);
+
+	max_sectors = min_t(unsigned int, max_sectors, INT_MAX >> 9);
+
+	return max_sectors << 9;
+}
+
 static long
 sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 {
@@ -945,7 +954,7 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
                 if (val < 0)
                         return -EINVAL;
 		val = min_t(int, val,
-			    queue_max_sectors(sdp->device->request_queue) * 512);
+			    max_sectors_bytes(sdp->device->request_queue));
 		if (val != sfp->reserve.bufflen) {
 			if (sg_res_in_use(sfp) || sfp->mmap_called)
 				return -EBUSY;
@@ -955,7 +964,7 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 		return 0;
 	case SG_GET_RESERVED_SIZE:
 		val = min_t(int, sfp->reserve.bufflen,
-			    queue_max_sectors(sdp->device->request_queue) * 512);
+			    max_sectors_bytes(sdp->device->request_queue));
 		return put_user(val, ip);
 	case SG_SET_COMMAND_Q:
 		result = get_user(val, ip);
@@ -1095,7 +1104,7 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 			return -ENODEV;
 		return scsi_ioctl(sdp->device, cmd_in, p);
 	case BLKSECTGET:
-		return put_user(queue_max_sectors(sdp->device->request_queue) * 512,
+		return put_user(max_sectors_bytes(sdp->device->request_queue),
 				ip);
 	case BLKTRACESETUP:
 		return blk_trace_setup(sdp->device->request_queue,
@@ -2086,7 +2095,7 @@ sg_add_sfp(Sg_device * sdp, int dev)
 		sg_big_buff = def_reserved_size;
 
 	bufflen = min_t(int, sg_big_buff,
-			queue_max_sectors(sdp->device->request_queue) * 512);
+			max_sectors_bytes(sdp->device->request_queue));
 	sg_build_reserve(sfp, bufflen);
 	SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp:   bufflen=%d, k_use_sg=%d\n",
 			   sfp->reserve.bufflen, sfp->reserve.k_use_sg));
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 94844fc..db7d8bd 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -408,7 +408,7 @@ struct scsi_host_template {
 	/*
 	 * Set this if the host adapter has limitations beside segment count.
 	 */
-	unsigned short max_sectors;
+	unsigned int max_sectors;
 
 	/*
 	 * DMA scatter gather segment boundary limit. A segment crossing this
@@ -652,7 +652,7 @@ struct Scsi_Host {
 	short cmd_per_lun;
 	short unsigned int sg_tablesize;
 	short unsigned int sg_prot_tablesize;
-	short unsigned int max_sectors;
+	unsigned int max_sectors;
 	unsigned long dma_boundary;
 	/* 
 	 * Used to assign serial numbers to the cmds.
-- 
1.9.1

Re: [PATCH 3/4] scsi: increase upper limit for max_sectors

[Christoph Hellwig]

On Sun, May 25, 2014 at 09:43:35PM +0900, Akinobu Mita wrote:
> max_sectors in struct Scsi_Host specifies maximum number of sectors
> allowed in a single SCSI command.  The data type of max_sectors is
> unsigned short, so the maximum transfer length per SCSI command is
> limited to less than 256MB in 4096-bytes sector size. (0xffff * 4096)
> 
> This commit increases the SCSI mid level's limitation for max_sectors
> upto the block layer's limitation for max_hw_sectors by extending the
> data type of max_sectors in struct Scsi_Host and scsi_host_template,
> so that SCSI lower level drivers can specify more than 0xffff.
> 
> This change requires the scsi disk (sd) driver to handle the requests
> whose transfer length is more than 0xffff with READ_16 or WRITE_16.
> Also, this needs to prevent SG_GET_RESERVED_SIZE and SG_SET_RESERVED_SIZE
> ioctls for the scsi generic (sg) driver from integer overflow when
> converting max_sectors to bytes.
> 
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

Looks good, but I think this should be three patches, one for the ioctl
in sg, one for the command selection in sd, and one to change the field
with in the scsi core.

Re: [PATCH 3/4] scsi: increase upper limit for max_sectors

[Akinobu Mita]

Hi Christoph,

Thanks for your review.

2014-05-28 19:39 GMT+09:00 Christoph Hellwig <hch@infradead.org>:
> Looks good, but I think this should be three patches, one for the ioctl
> in sg, one for the command selection in sd, and one to change the field
> with in the scsi core.

OK, I'll break this into three patches and rearrange the patchset for
next submission.

RE: [PATCH 3/4] scsi: increase upper limit for max_sectors

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Akinobu Mita
> Sent: Sunday, 25 May, 2014 7:44 AM
> To: linux-scsi@vger.kernel.org
> Cc: Akinobu Mita; Jens Axboe; James E.J. Bottomley; Douglas Gilbert
> Subject: [PATCH 3/4] scsi: increase upper limit for max_sectors
> 
...
> -	} else if (sdp->use_16_for_rw) {
> +	} else if ((this_count > 0xffff) || sdp->use_16_for_rw) {

Suggestion: reorder those so the unlikely case of a huge 
this_count is on the right side of the || operator.

Re: [PATCH 3/4] scsi: increase upper limit for max_sectors

[Akinobu Mita]

2014-05-29 1:18 GMT+09:00 Elliott, Robert (Server Storage) <Elliott@hp.com>:
>
>> -----Original Message-----
>> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
>> owner@vger.kernel.org] On Behalf Of Akinobu Mita
>> Sent: Sunday, 25 May, 2014 7:44 AM
>> To: linux-scsi@vger.kernel.org
>> Cc: Akinobu Mita; Jens Axboe; James E.J. Bottomley; Douglas Gilbert
>> Subject: [PATCH 3/4] scsi: increase upper limit for max_sectors
>>
> ...
>> -     } else if (sdp->use_16_for_rw) {
>> +     } else if ((this_count > 0xffff) || sdp->use_16_for_rw) {
>
> Suggestion: reorder those so the unlikely case of a huge
> this_count is on the right side of the || operator.

No problem.  I'll make this change in the next version.
Thanks for your suggestion.

[PATCH 4/4] scsi_debug: allow huge transfer length for read/write commands

[Akinobu Mita]

This change enables to test read/write commands with huge transfer
length such as 1GB.  For example:

	# modprobe scsi_debug dev_size_mb=1024 clustering=1 opts=1
	# cat /sys/block/$DEV/queue/max_hw_sectors_kb > \
		/sys/block/$DEV/queue/max_sectors_kb
	# fio --name=test --rw=write --bs=1g --size=1g --filename=/dev/$DEV \
		--mem=mmaphuge  --direct=1

The data type of max_sectors in scsi_host_template has been extended
to unsigned int by the previous change.  So we can increase it from
0xffff to 0xffffffff to allow such huge transfer length.

Also, this increases sg_tablesize and max_segment_size, otherwise the
maximum transfer length is limited to 64MB.
(sg_tablesize * max_segment_size = 256 * 256KB)

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Jens Axboe <axboe@kernel.dk>
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 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index f3e9cc0..35ce1fa 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -2483,7 +2483,7 @@ static int scsi_debug_slave_configure(struct scsi_device *sdp)
 	if (sdp->host->cmd_per_lun)
 		scsi_adjust_queue_depth(sdp, SDEBUG_TAGGED_QUEUING,
 					sdp->host->cmd_per_lun);
-	blk_queue_max_segment_size(sdp->request_queue, 256 * 1024);
+	blk_queue_max_segment_size(sdp->request_queue, -1U);
 	if (scsi_debug_no_uld)
 		sdp->no_uld_attach = 1;
 	return 0;
@@ -3938,9 +3938,9 @@ static struct scsi_host_template sdebug_driver_template = {
 	.bios_param =		scsi_debug_biosparam,
 	.can_queue =		SCSI_DEBUG_CANQUEUE,
 	.this_id =		7,
-	.sg_tablesize =		256,
+	.sg_tablesize =		SCSI_MAX_SG_CHAIN_SEGMENTS,
 	.cmd_per_lun =		16,
-	.max_sectors =		0xffff,
+	.max_sectors =		-1U,
 	.use_clustering = 	DISABLE_CLUSTERING,
 	.module =		THIS_MODULE,
 };
-- 
1.9.1

Re: [PATCH 4/4] scsi_debug: allow huge transfer length for read/write commands

[Christoph Hellwig]

On Sun, May 25, 2014 at 09:43:36PM +0900, Akinobu Mita wrote:
> This change enables to test read/write commands with huge transfer
> length such as 1GB.  For example:
> 
> 	# modprobe scsi_debug dev_size_mb=1024 clustering=1 opts=1
> 	# cat /sys/block/$DEV/queue/max_hw_sectors_kb > \
> 		/sys/block/$DEV/queue/max_sectors_kb
> 	# fio --name=test --rw=write --bs=1g --size=1g --filename=/dev/$DEV \
> 		--mem=mmaphuge  --direct=1
> 
> The data type of max_sectors in scsi_host_template has been extended
> to unsigned int by the previous change.  So we can increase it from
> 0xffff to 0xffffffff to allow such huge transfer length.
> 
> Also, this increases sg_tablesize and max_segment_size, otherwise the
> maximum transfer length is limited to 64MB.
> (sg_tablesize * max_segment_size = 256 * 256KB)
> 
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

Looks good,

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