[PATCH 1/2] scsi: fix max discard sectors calculation

[PATCH 1/2] scsi: fix max discard sectors calculation

[mchristi]

From: Mike Christie <mchristi@redhat.com>

logical_block_size and max_blocks are 32 bits, so we can
overflow when trying to convert SCSI device blocks to linux
block layer sectors.

Signed-off-by: Mike Christie <mchristi@redhat.com>
Cc: stable@vger.kernel.org

---
 drivers/scsi/sd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index f52b74c..1668e1d 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -690,7 +690,8 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
 		break;
 	}
 
-	blk_queue_max_discard_sectors(q, max_blocks * (logical_block_size >> 9));
+	blk_queue_max_discard_sectors(q, (u64)max_blocks *
+				      (logical_block_size >> 9));
 	queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
 }
 
-- 
2.7.2

Re: [PATCH 1/2] scsi: fix max discard sectors calculation

[Bart Van Assche]

On 05/16/2016 11:46 AM, mchristi@redhat.com wrote:
> logical_block_size and max_blocks are 32 bits, so we can
> overflow when trying to convert SCSI device blocks to linux
> block layer sectors.
>
> Signed-off-by: Mike Christie <mchristi@redhat.com>
> Cc: stable@vger.kernel.org
>
> ---
>   drivers/scsi/sd.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index f52b74c..1668e1d 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -690,7 +690,8 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
> -	blk_queue_max_discard_sectors(q, max_blocks * (logical_block_size >> 9));
> +	blk_queue_max_discard_sectors(q, (u64)max_blocks *
> +				      (logical_block_size >> 9));

Hello Mike,

As far as I can see max_blocks <= 0xffff (SD_MAX_WS10_BLOCKS) and 512 <= 
logical_block_size <= 4096. How can max_blocks * (logical_block_size >> 
9) be too large for a 32 bit integer?

Thanks,

Bart.

Re: [PATCH 1/2] scsi: fix max discard sectors calculation

[Mike Christie]

On 05/16/2016 05:44 PM, Bart Van Assche wrote:
> On 05/16/2016 11:46 AM, mchristi@redhat.com wrote:
>> logical_block_size and max_blocks are 32 bits, so we can
>> overflow when trying to convert SCSI device blocks to linux
>> block layer sectors.
>>
>> Signed-off-by: Mike Christie <mchristi@redhat.com>
>> Cc: stable@vger.kernel.org
>>
>> ---
>>   drivers/scsi/sd.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index f52b74c..1668e1d 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -690,7 +690,8 @@ static void sd_config_discard(struct scsi_disk
>> *sdkp, unsigned int mode)
>> -    blk_queue_max_discard_sectors(q, max_blocks * (logical_block_size
>> >> 9));
>> +    blk_queue_max_discard_sectors(q, (u64)max_blocks *
>> +                      (logical_block_size >> 9));
> 
> Hello Mike,
> 
> As far as I can see max_blocks <= 0xffff (SD_MAX_WS10_BLOCKS) and 512 <=
> logical_block_size <= 4096. How can max_blocks * (logical_block_size >>
> 9) be too large for a 32 bit integer?
> 

Oops yeah, you are right. I forgot I modified the code to test the
target patch. The patch is not needed. Thanks.

[PATCH 2/2] target: fix max discard sectors calculation

[mchristi]

From: Mike Christie <mchristi@redhat.com>

max_discard_sectors and block_size are 32 bits, so we can
overflow when trying to convert between the linux block
layer max discard sectors and the LIO value.

This fixes a regression caused by this patch:

commit 8a9ebe717a133ba7bc90b06047f43cc6b8bcb8b3
Author: Mike Christie <mchristi@redhat.com>
Date:   Mon Jan 18 14:09:27 2016 -0600

    target: Fix WRITE_SAME/DISCARD conversion to linux 512b sectors

where you will get smaller and extra discards due to the max size
being shortened.


Signed-off-by: Mike Christie <mchristi@redhat.com>
Cc: stable@vger.kernel.org

---
 drivers/target/target_core_device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index a4046ca..3f9f304 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -826,8 +826,8 @@ bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
 	if (!blk_queue_discard(q))
 		return false;
 
-	attrib->max_unmap_lba_count = (q->limits.max_discard_sectors << 9) /
-								block_size;
+	attrib->max_unmap_lba_count =
+			((u64)q->limits.max_discard_sectors << 9) / block_size;
 	/*
 	 * Currently hardcoded to 1 in Linux/SCSI code..
 	 */
-- 
2.7.2

Re: [PATCH 2/2] target: fix max discard sectors calculation

[Bart Van Assche]

On 05/16/2016 11:46 AM, mchristi@redhat.com wrote:
> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> index a4046ca..3f9f304 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -826,8 +826,8 @@ bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
> -	attrib->max_unmap_lba_count = (q->limits.max_discard_sectors << 9) /
> -								block_size;
> +	attrib->max_unmap_lba_count =
> +			((u64)q->limits.max_discard_sectors << 9) / block_size;

Hello Mike,

That's a good catch. But seeing this patch makes me wonder whether this 
patch introduces a 64-bit division? If so, I'm afraid this patch will 
make 32-bit users unhappy. Have you considered to use do_div() or >> 
(ilog2(block_size) - 9) instead? For the latter alternative no 64-bit 
cast is needed.

Thanks,

Bart.

Re: [PATCH 2/2] target: fix max discard sectors calculation

[Mike Christie]

On 05/16/2016 01:57 PM, Bart Van Assche wrote:
> On 05/16/2016 11:46 AM, mchristi@redhat.com wrote:
>> diff --git a/drivers/target/target_core_device.c
>> b/drivers/target/target_core_device.c
>> index a4046ca..3f9f304 100644
>> --- a/drivers/target/target_core_device.c
>> +++ b/drivers/target/target_core_device.c
>> @@ -826,8 +826,8 @@ bool target_configure_unmap_from_queue(struct
>> se_dev_attrib *attrib,
>> -    attrib->max_unmap_lba_count = (q->limits.max_discard_sectors << 9) /
>> -                                block_size;
>> +    attrib->max_unmap_lba_count =
>> +            ((u64)q->limits.max_discard_sectors << 9) / block_size;
> 
> Hello Mike,
> 
> That's a good catch. But seeing this patch makes me wonder whether this
> patch introduces a 64-bit division? If so, I'm afraid this patch will
> make 32-bit users unhappy. Have you considered to use do_div() or >>
> (ilog2(block_size) - 9) instead? For the latter alternative no 64-bit
> cast is needed.
> 

That is better. Will fix. Thanks.

Re: [PATCH 2/2] target: fix max discard sectors calculation

[Martin K. Petersen]

>>>>> "Bart" == Bart Van Assche <bart.vanassche@sandisk.com> writes:

Bart> That's a good catch. But seeing this patch makes me wonder whether
Bart> this patch introduces a 64-bit division? If so, I'm afraid this
Bart> patch will make 32-bit users unhappy. Have you considered to use
Bart> do_div() or >> (ilog2(block_size) - 9) instead? For the latter
Bart> alternative no 64-bit cast is needed.

Please use logical_to_sectors().

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 2/2] target: fix max discard sectors calculation

[Bart Van Assche]

On 05/16/2016 12:02 PM, Martin K. Petersen wrote:
>>>>>> "Bart" == Bart Van Assche <bart.vanassche@sandisk.com> writes:
>
> Bart> That's a good catch. But seeing this patch makes me wonder whether
> Bart> this patch introduces a 64-bit division? If so, I'm afraid this
> Bart> patch will make 32-bit users unhappy. Have you considered to use
> Bart> do_div() or >> (ilog2(block_size) - 9) instead? For the latter
> Bart> alternative no 64-bit cast is needed.
>
> Please use logical_to_sectors().

Hello Martin,

For SCSI initiator devices the logical block size is stored in struct 
scsi_device. logical_to_sectors() gets the logical block size from 
struct scsi_device. LIO however stores the logical block size in struct 
se_dev_attrib. If we want to keep SCSI initiator and SCSI target headers 
separate I think we will need two logical_to_sectors() functions - one 
for SCSI initiator devices and one for SCSI target devices.

Bart.

Re: [PATCH 2/2] target: fix max discard sectors calculation

[Mike Christie]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

On 05/17/2016 01:16 PM, Bart Van Assche wrote:
> On 05/16/2016 12:02 PM, Martin K. Petersen wrote:
>>>>>>> "Bart" == Bart Van Assche <bart.vanassche@sandisk.com> writes:
>>
>> Bart> That's a good catch. But seeing this patch makes me wonder whether
>> Bart> this patch introduces a 64-bit division? If so, I'm afraid this
>> Bart> patch will make 32-bit users unhappy. Have you considered to use
>> Bart> do_div() or >> (ilog2(block_size) - 9) instead? For the latter
>> Bart> alternative no 64-bit cast is needed.
>>
>> Please use logical_to_sectors().
> 
> Hello Martin,
> 
> For SCSI initiator devices the logical block size is stored in struct
> scsi_device. logical_to_sectors() gets the logical block size from
> struct scsi_device. LIO however stores the logical block size in struct
> se_dev_attrib. If we want to keep SCSI initiator and SCSI target headers
> separate I think we will need two logical_to_sectors() functions - one
> for SCSI initiator devices and one for SCSI target devices.
> 

Or block layer helpers? Something like the attached patch. It is a
compile only tested patch (just for show and not for submission as I
would break it up and test, etc) that would move the scsi converter to
the block layer, and convert the target layer and scsi to use it.

[-- Attachment #2: blk-logical-sector-helpers.patch --]
[-- Type: text/x-patch, Size: 4682 bytes --]

diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 654630b..d405400 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -148,7 +148,7 @@ static inline int scsi_medium_access_command(struct scsi_cmnd *scmd)
 
 static inline sector_t logical_to_sectors(struct scsi_device *sdev, sector_t blocks)
 {
-	return blocks << (ilog2(sdev->sector_size) - 9);
+	return blk_logical_to_sector(blocks, sdev->sector_size);
 }
 
 /*
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index a4046ca..95d6112 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -821,13 +821,15 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
  * in ATA and we need to set TPE=1
  */
 bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
-				       struct request_queue *q, int block_size)
+				       struct request_queue *q)
 {
+	unsigned short block_size = queue_logical_block_size(q);
+
 	if (!blk_queue_discard(q))
 		return false;
 
-	attrib->max_unmap_lba_count = (q->limits.max_discard_sectors << 9) /
-								block_size;
+	attrib->max_unmap_lba_count = queue_sector_to_logical(q,
+						q->limits.max_discard_sectors);
 	/*
 	 * Currently hardcoded to 1 in Linux/SCSI code..
 	 */
@@ -846,16 +848,7 @@ EXPORT_SYMBOL(target_configure_unmap_from_queue);
  */
 sector_t target_to_linux_sector(struct se_device *dev, sector_t lb)
 {
-	switch (dev->dev_attrib.block_size) {
-	case 4096:
-		return lb << 3;
-	case 2048:
-		return lb << 2;
-	case 1024:
-		return lb << 1;
-	default:
-		return lb;
-	}
+	return blk_logical_to_sector(lb, dev->dev_attrib.block_size);
 }
 EXPORT_SYMBOL(target_to_linux_sector);
 
diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 75f0f08..7929186 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -161,8 +161,7 @@ static int fd_configure_device(struct se_device *dev)
 			dev_size, div_u64(dev_size, fd_dev->fd_block_size),
 			fd_dev->fd_block_size);
 
-		if (target_configure_unmap_from_queue(&dev->dev_attrib, q,
-						      fd_dev->fd_block_size))
+		if (target_configure_unmap_from_queue(&dev->dev_attrib, q))
 			pr_debug("IFILE: BLOCK Discard support available,"
 				 " disabled by default\n");
 		/*
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 026a758..3cbe060 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -121,8 +121,7 @@ static int iblock_configure_device(struct se_device *dev)
 	dev->dev_attrib.hw_max_sectors = queue_max_hw_sectors(q);
 	dev->dev_attrib.hw_queue_depth = q->nr_requests;
 
-	if (target_configure_unmap_from_queue(&dev->dev_attrib, q,
-					      dev->dev_attrib.hw_block_size))
+	if (target_configure_unmap_from_queue(&dev->dev_attrib, q))
 		pr_debug("IBLOCK: BLOCK Discard support available,"
 			 " disabled by default\n");
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 669e419..55cb34b 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1210,6 +1210,30 @@ static inline unsigned short bdev_logical_block_size(struct block_device *bdev)
 	return queue_logical_block_size(bdev_get_queue(bdev));
 }
 
+static inline sector_t blk_logical_to_sector(sector_t blocks,
+					     unsigned short block_size)
+{
+	return blocks << (ilog2(block_size) - 9);
+}
+
+static inline sector_t queue_logical_to_sector(struct request_queue *q,
+					       sector_t blocks)
+{
+	return blk_logical_to_sector(blocks, queue_logical_block_size(q));
+}
+
+static inline sector_t blk_sector_to_logical(sector_t sectors,
+					     unsigned short block_size)
+{
+	return sectors >> (ilog2(block_size) - 9);
+}
+
+static inline sector_t queue_sector_to_logical(struct request_queue *q,
+					       sector_t sectors)
+{
+	return blk_sector_to_logical(sectors, queue_logical_block_size(q));
+}
+
 static inline unsigned int queue_physical_block_size(struct request_queue *q)
 {
 	return q->limits.physical_block_size;
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index 28ee5c2..711322a 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -96,6 +96,6 @@ sense_reason_t passthrough_parse_cdb(struct se_cmd *cmd,
 bool target_sense_desc_format(struct se_device *dev);
 sector_t target_to_linux_sector(struct se_device *dev, sector_t lb);
 bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
-				       struct request_queue *q, int block_size);
+				       struct request_queue *q);
 
 #endif /* TARGET_CORE_BACKEND_H */

Re: [PATCH 2/2] target: fix max discard sectors calculation

[Bart Van Assche]

On 05/17/2016 02:49 PM, Mike Christie wrote:
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -821,13 +821,15 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
>    * in ATA and we need to set TPE=1
>    */
>   bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
> -				       struct request_queue *q, int block_size)
> +				       struct request_queue *q)
>   {
> +	unsigned short block_size = queue_logical_block_size(q);
> +
>   	if (!blk_queue_discard(q))
>   		return false;
>
> -	attrib->max_unmap_lba_count = (q->limits.max_discard_sectors << 9) /
> -								block_size;
> +	attrib->max_unmap_lba_count = queue_sector_to_logical(q,
> +						q->limits.max_discard_sectors);
>   	/*
>   	 * Currently hardcoded to 1 in Linux/SCSI code..
>   	 */
> diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
> index 75f0f08..7929186 100644
> --- a/drivers/target/target_core_file.c
> +++ b/drivers/target/target_core_file.c
> @@ -161,8 +161,7 @@ static int fd_configure_device(struct se_device *dev)
>   			dev_size, div_u64(dev_size, fd_dev->fd_block_size),
>   			fd_dev->fd_block_size);
>
> -		if (target_configure_unmap_from_queue(&dev->dev_attrib, q,
> -						      fd_dev->fd_block_size))
> +		if (target_configure_unmap_from_queue(&dev->dev_attrib, q))
>   			pr_debug("IFILE: BLOCK Discard support available,"
>   				 " disabled by default\n");
>   		/*
> diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
> index 026a758..3cbe060 100644
> --- a/drivers/target/target_core_iblock.c
> +++ b/drivers/target/target_core_iblock.c
> @@ -121,8 +121,7 @@ static int iblock_configure_device(struct se_device *dev)
>   	dev->dev_attrib.hw_max_sectors = queue_max_hw_sectors(q);
>   	dev->dev_attrib.hw_queue_depth = q->nr_requests;
>
> -	if (target_configure_unmap_from_queue(&dev->dev_attrib, q,
> -					      dev->dev_attrib.hw_block_size))
> +	if (target_configure_unmap_from_queue(&dev->dev_attrib, q))
>   		pr_debug("IBLOCK: BLOCK Discard support available,"
>   			 " disabled by default\n");
>
 > [ ... ]

Hello Mike,

Are you sure that LIO guarantees that dev_attrib.hw_block_size is 
identical to queue_logical_block_size(q)? The other changes in this 
patch look like a nice improvement to me.

Bart.

Re: [PATCH 2/2] target: fix max discard sectors calculation

[Mike Christie]

On 05/18/2016 11:56 AM, Bart Van Assche wrote:
> On 05/17/2016 02:49 PM, Mike Christie wrote:
>> --- a/drivers/target/target_core_device.c
>> +++ b/drivers/target/target_core_device.c
>> @@ -821,13 +821,15 @@ struct se_device *target_alloc_device(struct
>> se_hba *hba, const char *name)
>>    * in ATA and we need to set TPE=1
>>    */
>>   bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
>> -                       struct request_queue *q, int block_size)
>> +                       struct request_queue *q)
>>   {
>> +    unsigned short block_size = queue_logical_block_size(q);
>> +
>>       if (!blk_queue_discard(q))
>>           return false;
>>
>> -    attrib->max_unmap_lba_count = (q->limits.max_discard_sectors << 9) /
>> -                                block_size;
>> +    attrib->max_unmap_lba_count = queue_sector_to_logical(q,
>> +                        q->limits.max_discard_sectors);
>>       /*
>>        * Currently hardcoded to 1 in Linux/SCSI code..
>>        */
>> diff --git a/drivers/target/target_core_file.c
>> b/drivers/target/target_core_file.c
>> index 75f0f08..7929186 100644
>> --- a/drivers/target/target_core_file.c
>> +++ b/drivers/target/target_core_file.c
>> @@ -161,8 +161,7 @@ static int fd_configure_device(struct se_device *dev)
>>               dev_size, div_u64(dev_size, fd_dev->fd_block_size),
>>               fd_dev->fd_block_size);
>>
>> -        if (target_configure_unmap_from_queue(&dev->dev_attrib, q,
>> -                              fd_dev->fd_block_size))
>> +        if (target_configure_unmap_from_queue(&dev->dev_attrib, q))
>>               pr_debug("IFILE: BLOCK Discard support available,"
>>                    " disabled by default\n");
>>           /*
>> diff --git a/drivers/target/target_core_iblock.c
>> b/drivers/target/target_core_iblock.c
>> index 026a758..3cbe060 100644
>> --- a/drivers/target/target_core_iblock.c
>> +++ b/drivers/target/target_core_iblock.c
>> @@ -121,8 +121,7 @@ static int iblock_configure_device(struct
>> se_device *dev)
>>       dev->dev_attrib.hw_max_sectors = queue_max_hw_sectors(q);
>>       dev->dev_attrib.hw_queue_depth = q->nr_requests;
>>
>> -    if (target_configure_unmap_from_queue(&dev->dev_attrib, q,
>> -                          dev->dev_attrib.hw_block_size))
>> +    if (target_configure_unmap_from_queue(&dev->dev_attrib, q))
>>           pr_debug("IBLOCK: BLOCK Discard support available,"
>>                " disabled by default\n");
>>
>> [ ... ]
> 
> Hello Mike,
> 
> Are you sure that LIO guarantees that dev_attrib.hw_block_size is
> identical to queue_logical_block_size(q)? The other changes in this
> patch look like a nice improvement to me.
> 

Yeah. In iblock a line above where patch/diff started from above we do:

        dev->dev_attrib.hw_block_size = bdev_logical_block_size(bd);


For the file setup code path we do:

fd_dev->fd_block_size = bdev_logical_block_size(inode->i_bdev);

and then a couple lines later we did:

if (target_configure_unmap_from_queue(&dev->dev_attrib, q,
				fd_dev->fd_block_size))