[PATCH] sd: fixup capacity calculation for 4k drives

[PATCH] sd: fixup capacity calculation for 4k drives

[Hannes Reinecke]

in sd_read_capacity() the sdkp->capacity field changes its meaning:
after the call to read_capacity_XX() it carries the _unscaled_ values,
making the comparison between the original value and the new value
always false for drives with a sector size != 512.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/sd.c | 69 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 39 insertions(+), 30 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5a5457a..0afe113 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2046,7 +2046,7 @@ static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device *sdp,
 #define READ_CAPACITY_RETRIES_ON_RESET	10
 
 static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
-						unsigned char *buffer)
+			    unsigned char *buffer, u64 *capacity)
 {
 	unsigned char cmd[16];
 	struct scsi_sense_hdr sshdr;
@@ -2054,8 +2054,8 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 	int the_result;
 	int retries = 3, reset_retries = READ_CAPACITY_RETRIES_ON_RESET;
 	unsigned int alignment;
-	unsigned long long lba;
-	unsigned sector_size;
+	u64 lba;
+	u32 sector_size;
 
 	if (sdp->no_read_capacity_16)
 		return -EINVAL;
@@ -2114,7 +2114,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
 			"kernel compiled with support for large block "
 			"devices.\n");
-		sdkp->capacity = 0;
+		*capacity = 0;
 		return -EOVERFLOW;
 	}
 
@@ -2137,20 +2137,20 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 		sd_config_discard(sdkp, SD_LBP_WS16);
 	}
 
-	sdkp->capacity = lba + 1;
+	*capacity = lba + 1;
 	return sector_size;
 }
 
 static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
-						unsigned char *buffer)
+			    unsigned char *buffer, u64 *capacity)
 {
 	unsigned char cmd[16];
 	struct scsi_sense_hdr sshdr;
 	int sense_valid = 0;
 	int the_result;
 	int retries = 3, reset_retries = READ_CAPACITY_RETRIES_ON_RESET;
-	sector_t lba;
-	unsigned sector_size;
+	u32 lba;
+	u32 sector_size;
 
 	do {
 		cmd[0] = READ_CAPACITY;
@@ -2191,7 +2191,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
 		/* Some buggy (usb cardreader) devices return an lba of
 		   0xffffffff when the want to report a size of 0 (with
 		   which they really mean no media is present) */
-		sdkp->capacity = 0;
+		*capacity = 0;
 		sdkp->physical_block_size = sector_size;
 		return sector_size;
 	}
@@ -2200,11 +2200,11 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
 		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
 			"kernel compiled with support for large block "
 			"devices.\n");
-		sdkp->capacity = 0;
+		*capacity = 0;
 		return -EOVERFLOW;
 	}
 
-	sdkp->capacity = lba + 1;
+	*capacity = (u64)lba + 1;
 	sdkp->physical_block_size = sector_size;
 	return sector_size;
 }
@@ -2230,34 +2230,38 @@ sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer)
 {
 	int sector_size;
 	struct scsi_device *sdp = sdkp->device;
-	sector_t old_capacity = sdkp->capacity;
+	u64 old_capacity = sdkp->capacity, new_capacity;
 
 	if (sd_try_rc16_first(sdp)) {
-		sector_size = read_capacity_16(sdkp, sdp, buffer);
+		sector_size = read_capacity_16(sdkp, sdp, buffer,
+					       &new_capacity);
 		if (sector_size == -EOVERFLOW)
 			goto got_data;
 		if (sector_size == -ENODEV)
 			return;
 		if (sector_size < 0)
-			sector_size = read_capacity_10(sdkp, sdp, buffer);
+			sector_size = read_capacity_10(sdkp, sdp, buffer,
+						       &new_capacity);
 		if (sector_size < 0)
 			return;
 	} else {
-		sector_size = read_capacity_10(sdkp, sdp, buffer);
+		sector_size = read_capacity_10(sdkp, sdp, buffer,
+					       &new_capacity);
 		if (sector_size == -EOVERFLOW)
 			goto got_data;
 		if (sector_size < 0)
 			return;
 		if ((sizeof(sdkp->capacity) > 4) &&
-		    (sdkp->capacity > 0xffffffffULL)) {
+		    (new_capacity > 0xffffffffULL)) {
 			int old_sector_size = sector_size;
 			sd_printk(KERN_NOTICE, sdkp, "Very big device. "
 					"Trying to use READ CAPACITY(16).\n");
-			sector_size = read_capacity_16(sdkp, sdp, buffer);
+			sector_size = read_capacity_16(sdkp, sdp, buffer,
+						       &new_capacity);
 			if (sector_size < 0) {
 				sd_printk(KERN_NOTICE, sdkp,
 					"Using 0xffffffff as device size\n");
-				sdkp->capacity = 1 + (sector_t) 0xffffffff;
+				new_capacity = 1 + (u64) 0xffffffff;
 				sector_size = old_sector_size;
 				goto got_data;
 			}
@@ -2275,11 +2279,11 @@ sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer)
 	 * the capacity.
 	 */
 	if (sdp->fix_capacity ||
-	    (sdp->guess_capacity && (sdkp->capacity & 0x01))) {
+	    (sdp->guess_capacity && (new_capacity & 0x01))) {
 		sd_printk(KERN_INFO, sdkp, "Adjusting the sector count "
 				"from its reported value: %llu\n",
-				(unsigned long long) sdkp->capacity);
-		--sdkp->capacity;
+				(unsigned long long) new_capacity);
+		--new_capacity;
 	}
 
 got_data:
@@ -2301,7 +2305,7 @@ got_data:
 		 * would be relatively trivial to set the thing up.
 		 * For this reason, we leave the thing in the table.
 		 */
-		sdkp->capacity = 0;
+		new_capacity = 0;
 		/*
 		 * set a bogus sector size so the normal read/write
 		 * logic in the block layer will eventually refuse any
@@ -2312,6 +2316,19 @@ got_data:
 	}
 	blk_queue_logical_block_size(sdp->request_queue, sector_size);
 
+	/*
+	 * Note: up to this point new_capacity carries the
+	 * _unscaled_ capacity. So rescale capacity to 512-byte units.
+	 */
+	if (sector_size == 4096)
+		sdkp->capacity = new_capacity << 3;
+	else if (sector_size == 2048)
+		sdkp->capacity = new_capacity << 2;
+	else if (sector_size == 1024)
+		sdkp->capacity = new_capacity << 1;
+	else
+		sdkp->capacity = new_capacity;
+
 	{
 		char cap_str_2[10], cap_str_10[10];
 
@@ -2337,14 +2354,6 @@ got_data:
 	if (sdkp->capacity > 0xffffffff)
 		sdp->use_16_for_rw = 1;
 
-	/* Rescale capacity to 512-byte units */
-	if (sector_size == 4096)
-		sdkp->capacity <<= 3;
-	else if (sector_size == 2048)
-		sdkp->capacity <<= 2;
-	else if (sector_size == 1024)
-		sdkp->capacity <<= 1;
-
 	blk_queue_physical_block_size(sdp->request_queue,
 				      sdkp->physical_block_size);
 	sdkp->device->sector_size = sector_size;
-- 
1.8.5.6

Re: [PATCH] sd: fixup capacity calculation for 4k drives

[Lee Duncan]

On 03/29/2016 01:06 AM, Hannes Reinecke wrote:
> in sd_read_capacity() the sdkp->capacity field changes its meaning:
> after the call to read_capacity_XX() it carries the _unscaled_ values,
> making the comparison between the original value and the new value
> always false for drives with a sector size != 512.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/sd.c | 69 +++++++++++++++++++++++++++++++------------------------
>  1 file changed, 39 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 5a5457a..0afe113 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2046,7 +2046,7 @@ static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  #define READ_CAPACITY_RETRIES_ON_RESET	10
>  
>  static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
> -						unsigned char *buffer)
> +			    unsigned char *buffer, u64 *capacity)
>  {
>  	unsigned char cmd[16];
>  	struct scsi_sense_hdr sshdr;
> @@ -2054,8 +2054,8 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  	int the_result;
>  	int retries = 3, reset_retries = READ_CAPACITY_RETRIES_ON_RESET;
>  	unsigned int alignment;
> -	unsigned long long lba;
> -	unsigned sector_size;
> +	u64 lba;
> +	u32 sector_size;
>  
>  	if (sdp->no_read_capacity_16)
>  		return -EINVAL;
> @@ -2114,7 +2114,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
>  			"kernel compiled with support for large block "
>  			"devices.\n");
> -		sdkp->capacity = 0;
> +		*capacity = 0;
>  		return -EOVERFLOW;
>  	}
>  
> @@ -2137,20 +2137,20 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  		sd_config_discard(sdkp, SD_LBP_WS16);
>  	}
>  
> -	sdkp->capacity = lba + 1;
> +	*capacity = lba + 1;
>  	return sector_size;
>  }
>  
>  static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
> -						unsigned char *buffer)
> +			    unsigned char *buffer, u64 *capacity)
>  {
>  	unsigned char cmd[16];
>  	struct scsi_sense_hdr sshdr;
>  	int sense_valid = 0;
>  	int the_result;
>  	int retries = 3, reset_retries = READ_CAPACITY_RETRIES_ON_RESET;
> -	sector_t lba;
> -	unsigned sector_size;
> +	u32 lba;
> +	u32 sector_size;
>  
>  	do {
>  		cmd[0] = READ_CAPACITY;
> @@ -2191,7 +2191,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  		/* Some buggy (usb cardreader) devices return an lba of
>  		   0xffffffff when the want to report a size of 0 (with
>  		   which they really mean no media is present) */
> -		sdkp->capacity = 0;
> +		*capacity = 0;
>  		sdkp->physical_block_size = sector_size;
>  		return sector_size;
>  	}
> @@ -2200,11 +2200,11 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
>  		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
>  			"kernel compiled with support for large block "
>  			"devices.\n");
> -		sdkp->capacity = 0;
> +		*capacity = 0;
>  		return -EOVERFLOW;
>  	}
>  
> -	sdkp->capacity = lba + 1;
> +	*capacity = (u64)lba + 1;
>  	sdkp->physical_block_size = sector_size;
>  	return sector_size;
>  }
> @@ -2230,34 +2230,38 @@ sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer)
>  {
>  	int sector_size;
>  	struct scsi_device *sdp = sdkp->device;
> -	sector_t old_capacity = sdkp->capacity;
> +	u64 old_capacity = sdkp->capacity, new_capacity;
>  
>  	if (sd_try_rc16_first(sdp)) {
> -		sector_size = read_capacity_16(sdkp, sdp, buffer);
> +		sector_size = read_capacity_16(sdkp, sdp, buffer,
> +					       &new_capacity);
>  		if (sector_size == -EOVERFLOW)
>  			goto got_data;
>  		if (sector_size == -ENODEV)
>  			return;
>  		if (sector_size < 0)
> -			sector_size = read_capacity_10(sdkp, sdp, buffer);
> +			sector_size = read_capacity_10(sdkp, sdp, buffer,
> +						       &new_capacity);
>  		if (sector_size < 0)
>  			return;
>  	} else {
> -		sector_size = read_capacity_10(sdkp, sdp, buffer);
> +		sector_size = read_capacity_10(sdkp, sdp, buffer,
> +					       &new_capacity);
>  		if (sector_size == -EOVERFLOW)
>  			goto got_data;
>  		if (sector_size < 0)
>  			return;
>  		if ((sizeof(sdkp->capacity) > 4) &&
> -		    (sdkp->capacity > 0xffffffffULL)) {
> +		    (new_capacity > 0xffffffffULL)) {
>  			int old_sector_size = sector_size;
>  			sd_printk(KERN_NOTICE, sdkp, "Very big device. "
>  					"Trying to use READ CAPACITY(16).\n");
> -			sector_size = read_capacity_16(sdkp, sdp, buffer);
> +			sector_size = read_capacity_16(sdkp, sdp, buffer,
> +						       &new_capacity);
>  			if (sector_size < 0) {
>  				sd_printk(KERN_NOTICE, sdkp,
>  					"Using 0xffffffff as device size\n");
> -				sdkp->capacity = 1 + (sector_t) 0xffffffff;
> +				new_capacity = 1 + (u64) 0xffffffff;
>  				sector_size = old_sector_size;
>  				goto got_data;
>  			}
> @@ -2275,11 +2279,11 @@ sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer)
>  	 * the capacity.
>  	 */
>  	if (sdp->fix_capacity ||
> -	    (sdp->guess_capacity && (sdkp->capacity & 0x01))) {
> +	    (sdp->guess_capacity && (new_capacity & 0x01))) {
>  		sd_printk(KERN_INFO, sdkp, "Adjusting the sector count "
>  				"from its reported value: %llu\n",
> -				(unsigned long long) sdkp->capacity);
> -		--sdkp->capacity;
> +				(unsigned long long) new_capacity);
> +		--new_capacity;
>  	}
>  
>  got_data:
> @@ -2301,7 +2305,7 @@ got_data:
>  		 * would be relatively trivial to set the thing up.
>  		 * For this reason, we leave the thing in the table.
>  		 */
> -		sdkp->capacity = 0;
> +		new_capacity = 0;
>  		/*
>  		 * set a bogus sector size so the normal read/write
>  		 * logic in the block layer will eventually refuse any
> @@ -2312,6 +2316,19 @@ got_data:
>  	}
>  	blk_queue_logical_block_size(sdp->request_queue, sector_size);
>  
> +	/*
> +	 * Note: up to this point new_capacity carries the
> +	 * _unscaled_ capacity. So rescale capacity to 512-byte units.
> +	 */
> +	if (sector_size == 4096)
> +		sdkp->capacity = new_capacity << 3;
> +	else if (sector_size == 2048)
> +		sdkp->capacity = new_capacity << 2;
> +	else if (sector_size == 1024)
> +		sdkp->capacity = new_capacity << 1;
> +	else
> +		sdkp->capacity = new_capacity;
> +
>  	{
>  		char cap_str_2[10], cap_str_10[10];
>  
> @@ -2337,14 +2354,6 @@ got_data:
>  	if (sdkp->capacity > 0xffffffff)
>  		sdp->use_16_for_rw = 1;
>  
> -	/* Rescale capacity to 512-byte units */
> -	if (sector_size == 4096)
> -		sdkp->capacity <<= 3;
> -	else if (sector_size == 2048)
> -		sdkp->capacity <<= 2;
> -	else if (sector_size == 1024)
> -		sdkp->capacity <<= 1;
> -
>  	blk_queue_physical_block_size(sdp->request_queue,
>  				      sdkp->physical_block_size);
>  	sdkp->device->sector_size = sector_size;
> 

Reviewed-by: Lee Duncan <lduncan@suse.com>

[PATCH] sd: fixup capacity calculation for 4k drives

[Hannes Reinecke]

in sd_read_capacity() the sdkp->capacity field changes its meaning:
after the call to read_capacity_XX() it carries the _unscaled_ values,
making the comparison between the original value and the new value
always false for drives with a sector size != 512.
So introduce a 'new_capacity' carrying the new, scaled, capacity.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/sd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5a5457a..fbb8daa 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2312,8 +2312,13 @@ got_data:
 	}
 	blk_queue_logical_block_size(sdp->request_queue, sector_size);
 
+	/*
+	 * Note: up to this point sdkp->capacity carries the
+	 * _unscaled_ capacity (cf the scaling after this block).
+	 */
 	{
 		char cap_str_2[10], cap_str_10[10];
+		size_t new_capacity = sdkp->capacity >> (ilog2(sector_size) - 9);
 
 		string_get_size(sdkp->capacity, sector_size,
 				STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
@@ -2321,7 +2326,7 @@ got_data:
 				STRING_UNITS_10, cap_str_10,
 				sizeof(cap_str_10));
 
-		if (sdkp->first_scan || old_capacity != sdkp->capacity) {
+		if (sdkp->first_scan || old_capacity != new_capacity) {
 			sd_printk(KERN_NOTICE, sdkp,
 				  "%llu %d-byte logical blocks: (%s/%s)\n",
 				  (unsigned long long)sdkp->capacity,
-- 
1.8.5.6

Re: [PATCH] sd: fixup capacity calculation for 4k drives

[Christoph Hellwig]

On Mon, Mar 21, 2016 at 01:27:29PM +0100, Hannes Reinecke wrote:
> in sd_read_capacity() the sdkp->capacity field changes its meaning:
> after the call to read_capacity_XX() it carries the _unscaled_ values,
> making the comparison between the original value and the new value
> always false for drives with a sector size != 512.
> So introduce a 'new_capacity' carrying the new, scaled, capacity.

While this fixes a bug and adds a comment to clarify things I think
the whole function is still a mess.  And the way how your first
calculate new_capacity but then keep the duplicated scaling on
sdkp->capacity a littler later isn't really helpful either.

Is there any chance to rewrite it so that the unscaled capacity has its
own local variable, and sdkp->capacity is always either the correct
old or new capacity?

Re: [PATCH] sd: fixup capacity calculation for 4k drives

[Martin K. Petersen]

>>>>> "Hannes" == Hannes Reinecke <hare@suse.de> writes:

Hannes> in sd_read_capacity() the sdkp->capacity field changes its
Hannes> meaning: after the call to read_capacity_XX() it carries the
Hannes> _unscaled_ values, making the comparison between the original
Hannes> value and the new value always false for drives with a sector
Hannes> size != 512.  So introduce a 'new_capacity' carrying the new,
Hannes> scaled, capacity.

I agree with Christoph.

How about something like this instead?

-- 
Martin K. Petersen	Oracle Linux Engineering

commit 2049b38f7de6dd073acee98230e7d735ceced8c9
Author: Martin K. Petersen <martin.petersen@oracle.com>
Date:   Mon Mar 21 20:49:53 2016 -0400

    sd: Fix excessive capacity printing on devices with blocks bigger than 512 bytes
    
    During revalidate we check whether device capacity has changed before we
    decide whether to output disk information or not.
    
    The check for old capacity failed to take into account that we scaled
    sdkp->capacity based on the reported logical block size. And therefore
    the capacity test would always fail for devices with sectors bigger than
    512 bytes and we would print several copies of the same discovery
    information.
    
    Avoid scaling sdkp->capacity and instead adjust the value on the fly
    when setting the block device capacity.
    
    Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
    Reported-by: Hannes Reinecke <hare@suse.de>

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5a5457ac9cdb..d8c672d4ae1d 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2337,14 +2337,6 @@ got_data:
 	if (sdkp->capacity > 0xffffffff)
 		sdp->use_16_for_rw = 1;
 
-	/* Rescale capacity to 512-byte units */
-	if (sector_size == 4096)
-		sdkp->capacity <<= 3;
-	else if (sector_size == 2048)
-		sdkp->capacity <<= 2;
-	else if (sector_size == 1024)
-		sdkp->capacity <<= 1;
-
 	blk_queue_physical_block_size(sdp->request_queue,
 				      sdkp->physical_block_size);
 	sdkp->device->sector_size = sector_size;
@@ -2812,7 +2804,7 @@ static int sd_try_extended_inquiry(struct scsi_device *sdp)
 	return 0;
 }
 
-static inline u32 logical_to_sectors(struct scsi_device *sdev, u32 blocks)
+static inline sector_t logical_to_sectors(struct scsi_device *sdev, sector_t blocks)
 {
 	return blocks << (ilog2(sdev->sector_size) - 9);
 }
@@ -2900,7 +2892,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	/* Combine with controller limits */
 	q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));
 
-	set_capacity(disk, sdkp->capacity);
+	set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity));
 	sd_config_write_same(sdkp);
 	kfree(buffer);
 

Re: [PATCH] sd: fixup capacity calculation for 4k drives

[Christoph Hellwig]

Looks fine,

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

Re: [PATCH] sd: fixup capacity calculation for 4k drives

[Hannes Reinecke]

On 03/22/2016 02:16 AM, Martin K. Petersen wrote:
>>>>>> "Hannes" == Hannes Reinecke <hare@suse.de> writes:
> 
> Hannes> in sd_read_capacity() the sdkp->capacity field changes its
> Hannes> meaning: after the call to read_capacity_XX() it carries the
> Hannes> _unscaled_ values, making the comparison between the original
> Hannes> value and the new value always false for drives with a sector
> Hannes> size != 512.  So introduce a 'new_capacity' carrying the new,
> Hannes> scaled, capacity.
> 
> I agree with Christoph.
> 
> How about something like this instead?
> 
I've coded it somewhat different, but this one works as well.
But please modify the description in sd.h, as with this patch
'sdkp->capacity' is the _unscaled_ value.
Might lead to confusion otherwise.

diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 5f2a84a..5ed7434 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -65,7 +65,7 @@ struct scsi_disk {
        struct device   dev;
        struct gendisk  *disk;
        atomic_t        openers;
-       sector_t        capacity;       /* size in 512-byte sectors */
+       sector_t        capacity;       /* size in logical sectors */
        u32             max_xfer_blocks;
        u32             opt_xfer_blocks;
        u32             max_ws_blocks;

(Apologies for the mangled patch)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sd: fixup capacity calculation for 4k drives

[Ewan D. Milne]

On Tue, 2016-03-22 at 08:14 +0100, Hannes Reinecke wrote:
> On 03/22/2016 02:16 AM, Martin K. Petersen wrote:
> >>>>>> "Hannes" == Hannes Reinecke <hare@suse.de> writes:
> > 
> > Hannes> in sd_read_capacity() the sdkp->capacity field changes its
> > Hannes> meaning: after the call to read_capacity_XX() it carries the
> > Hannes> _unscaled_ values, making the comparison between the original
> > Hannes> value and the new value always false for drives with a sector
> > Hannes> size != 512.  So introduce a 'new_capacity' carrying the new,
> > Hannes> scaled, capacity.
> > 
> > I agree with Christoph.
> > 
> > How about something like this instead?
> > 
> I've coded it somewhat different, but this one works as well.
> But please modify the description in sd.h, as with this patch
> 'sdkp->capacity' is the _unscaled_ value.
> Might lead to confusion otherwise.
> 
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index 5f2a84a..5ed7434 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -65,7 +65,7 @@ struct scsi_disk {
>         struct device   dev;
>         struct gendisk  *disk;
>         atomic_t        openers;
> -       sector_t        capacity;       /* size in 512-byte sectors */
> +       sector_t        capacity;       /* size in logical sectors */
>         u32             max_xfer_blocks;
>         u32             opt_xfer_blocks;
>         u32             max_ws_blocks;
> 
> (Apologies for the mangled patch)
> 
> Cheers,
> 
> Hannes

If we change the meaning of the scsi_disk->capacity field to be logical
sectors, we also have to change sd_getgeo() in sd.c, do we not?

The ->capacity field is also accessed by last_sector_hacks() in
drivers/usb/storage/transport.c, although it kind of looks like
that code might not have been correct for 4K sectors with the
scaled value.

-Ewan

Re: [PATCH] sd: fixup capacity calculation for 4k drives

[Martin K. Petersen]

>>>>> "Ewan" == Ewan D Milne <emilne@redhat.com> writes:

Ewan/Hannes,

Sorry I dropped the ball on this. My eyes started bleeding.

Ewan> If we change the meaning of the scsi_disk->capacity field to be
Ewan> logical sectors, we also have to change sd_getgeo() in sd.c, do we
Ewan> not?

I did consider that before sending the original patch but didn't see a
problem. In looking closer, however, it does appear that we'd
inadvertently change the reported geometry for 4Kn devices that are
sufficiently small. So I fixed that up.

Ewan> The ->capacity field is also accessed by last_sector_hacks() in
Ewan> drivers/usb/storage/transport.c, although it kind of looks like
Ewan> that code might not have been correct for 4K sectors with the
Ewan> scaled value.

That case I had missed. Ew, gross! But yes, that was broken to begin
with.

I have to admit I was going back and forth how to fix this. But in the
end I find it really ugly that capacity suddenly changes from logical
blocks to block layer sectors. And I prefer to make that conversion
explicit when it is necessary.

Amended patch follows...

-- 
Martin K. Petersen	Oracle Linux Engineering