[RFC] sd: dynamically adjust SD_MAX_WS16_BLOCKS as per the actual logical block size

Commit Message

Tom Yan Aug. 11, 2016, 10:29 p.m. UTC

From: Tom Yan <[e-mail]>

WRITE SAME (16) command can technically handle up to 32-bit
number of blocks. However, since 32-bit is also the limitation of
the maximum number of bytes that can be represented in the block
layer, the current SD_MAX_WS16_BLOCKS was hence derived from the
technical limit devided by 512.

However, SD_MAX_WS16_BLOCKS is used to check values that are, for
example, orignated from Maximum Write Same Length field on the
Block Limit VPD. Such field expresses the number of blocks in
terms of the actual logical sector size of the specific drive
instead of the block size that the block layer is based on (512).

Therefore, the original hack would work fine for drives with
512-byte logical sectors. However, for drives with larger logical
sector size (e.g. AF 4Kn drives), the hack would be in vain.

So let's bump the macro set in sd.h back to the technical limit,
and adjust it as per the actual logical block size when it is used.

Signed-off-by: Tom Yan <[e-mail]>

Comments

Martin K. Petersen Aug. 12, 2016, 3:10 a.m. UTC | #1

>>>>> "Tom" == tom ty89 <[e-mail]> writes:

Tom,

Tom> However, SD_MAX_WS16_BLOCKS is used to check values that are, for
Tom> example, orignated from Maximum Write Same Length field on the
Tom> Block Limit VPD. Such field expresses the number of blocks in terms
Tom> of the actual logical sector size of the specific drive instead of
Tom> the block size that the block layer is based on (512).

I agree the SD_MAX_WS16_BLOCKS is ugly. I think it started out as a
limit expressly set in block layer sector units and then when the
discard code got expanded it got re-purposed into being something
else. So let's get that cleaned up.

However, the CDB transfer length limit is really not the main issue
here, it's bi_size that we need to enforce.

After contemplating a bit I think it would be cleanest to add
BLK_MAX_BIO_SECTORS and clamp on that in blk_queue_max_foobar() like we
do with some of the other queue limits. Move the enforcement to block
where the actual limit originates rather than code around it in sd.

Also, please use ilog2() instead of division for things like this.

Tom Yan Aug. 12, 2016, 4:52 a.m. UTC | #2

On 12 August 2016 at 11:10, Martin K. Petersen
<[e-mail]> wrote:
>
> However, the CDB transfer length limit is really not the main issue
> here, it's bi_size that we need to enforce.
>
> After contemplating a bit I think it would be cleanest to add
> BLK_MAX_BIO_SECTORS and clamp on that in blk_queue_max_foobar() like we
> do with some of the other queue limits. Move the enforcement to block
> where the actual limit originates rather than code around it in sd.

I don't really follow. What would this BLK_MAX_BIO_SECTORS be? It
doesn't appear to me that a static value is going to address the
problem I am addressing in this patch.

>
> Also, please use ilog2() instead of division for things like this.
>
> --
> Martin K. Petersen      Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Martin K. Petersen Aug. 12, 2016, 8:26 p.m. UTC | #3

>>>>> "Tom" == Tom Yan <[e-mail]> writes:

Tom,

Tom> I don't really follow. What would this BLK_MAX_BIO_SECTORS be? It
Tom> doesn't appear to me that a static value is going to address the
Tom> problem I am addressing in this patch.

0x7fffff, the maximum number of block layer sectors that can be
expressed in a single bio.

Tom Yan Aug. 14, 2016, 8:46 a.m. UTC | #4

On 13 August 2016 at 04:26, Martin K. Petersen
<[e-mail]> wrote:
>>>>>> "Tom" == Tom Yan <[e-mail]> writes:
>
> Tom,
>
> Tom> I don't really follow. What would this BLK_MAX_BIO_SECTORS be? It
> Tom> doesn't appear to me that a static value is going to address the
> Tom> problem I am addressing in this patch.
>
> 0x7fffff, the maximum number of block layer sectors that can be
> expressed in a single bio.

Hmm, so when we queue any of the limits, we convert a certain maximum
number of physical sectors (which we has already been doing) to its
corresponding maximum number of block layer sectors, and then make
sure that number does not exceed 0x7fffff, right?

>
> --
> Martin K. Petersen      Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Martin K. Petersen Aug. 16, 2016, 4:02 a.m. UTC | #5

>>>>> "Tom" == Tom Yan <[e-mail]> writes:

Tom,

>> 0x7fffff, the maximum number of block layer sectors that can be
>> expressed in a single bio.

Tom> Hmm, so when we queue any of the limits, we convert a certain
Tom> maximum number of physical sectors (which we has already been
Tom> doing)

logical sectors

Tom> to its corresponding maximum number of block layer sectors, and
Tom> then make sure that number does not exceed 0x7fffff, right?

Yep.

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d3e852a..601afd6 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -452,6 +452,8 @@  max_write_same_blocks_store(struct device *dev, struct device_attribute *attr,
 {
 	struct scsi_disk *sdkp = to_scsi_disk(dev);
 	struct scsi_device *sdp = sdkp->device;
+	unsigned int logical_block_size = sdp->sector_size;
+	unsigned int max_ws16_blocks = SD_MAX_WS16_BLOCKS / logical_block_size;
 	unsigned long max;
 	int err;
 
@@ -468,7 +470,7 @@  max_write_same_blocks_store(struct device *dev, struct device_attribute *attr,
 
 	if (max == 0)
 		sdp->no_write_same = 1;
-	else if (max <= SD_MAX_WS16_BLOCKS) {
+	else if (max <= max_ws16_blocks) {
 		sdp->no_write_same = 0;
 		sdkp->max_ws_blocks = max;
 	}
@@ -635,6 +637,7 @@  static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
 {
 	struct request_queue *q = sdkp->disk->queue;
 	unsigned int logical_block_size = sdkp->device->sector_size;
+	unsigned int max_ws16_blocks = SD_MAX_WS16_BLOCKS / logical_block_size;
 	unsigned int max_blocks = 0;
 
 	q->limits.discard_zeroes_data = 0;
@@ -668,12 +671,12 @@  static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
 
 	case SD_LBP_UNMAP:
 		max_blocks = min_not_zero(sdkp->max_unmap_blocks,
-					  (u32)SD_MAX_WS16_BLOCKS);
+					  (u32)max_ws16_blocks);
 		break;
 
 	case SD_LBP_WS16:
 		max_blocks = min_not_zero(sdkp->max_ws_blocks,
-					  (u32)SD_MAX_WS16_BLOCKS);
+					  (u32)max_ws16_blocks);
 		q->limits.discard_zeroes_data = sdkp->lbprz;
 		break;
 
@@ -793,6 +796,7 @@  static void sd_config_write_same(struct scsi_disk *sdkp)
 {
 	struct request_queue *q = sdkp->disk->queue;
 	unsigned int logical_block_size = sdkp->device->sector_size;
+	unsigned int max_ws16_blocks = SD_MAX_WS16_BLOCKS / logical_block_size;
 
 	if (sdkp->device->no_write_same) {
 		sdkp->max_ws_blocks = 0;
@@ -806,7 +810,7 @@  static void sd_config_write_same(struct scsi_disk *sdkp)
 	 */
 	if (sdkp->max_ws_blocks > SD_MAX_WS10_BLOCKS)
 		sdkp->max_ws_blocks = min_not_zero(sdkp->max_ws_blocks,
-						   (u32)SD_MAX_WS16_BLOCKS);
+						   (u32)max_ws16_blocks);
 	else if (sdkp->ws16 || sdkp->ws10 || sdkp->device->no_report_opcodes)
 		sdkp->max_ws_blocks = min_not_zero(sdkp->max_ws_blocks,
 						   (u32)SD_MAX_WS10_BLOCKS);
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 765a6f1..56ff88c 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -47,7 +47,7 @@  enum {
 	SD_DEF_XFER_BLOCKS = 0xffff,
 	SD_MAX_XFER_BLOCKS = 0xffffffff,
 	SD_MAX_WS10_BLOCKS = 0xffff,
-	SD_MAX_WS16_BLOCKS = 0x7fffff,
+	SD_MAX_WS16_BLOCKS = 0xffffffff,
 };
 
 enum {