[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.
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

[PATCH v2] sd: Fix excessive capacity printing on devices with blocks bigger than 512 bytes

[Martin K. Petersen]

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 and generating fake C/H/S
geometry.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Reported-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/sd.c | 28 ++++++++--------------------
 drivers/scsi/sd.h |  7 ++++++-
 2 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5a5457ac9cdb..8401697ff6aa 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1275,18 +1275,19 @@ static int sd_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 	struct scsi_disk *sdkp = scsi_disk(bdev->bd_disk);
 	struct scsi_device *sdp = sdkp->device;
 	struct Scsi_Host *host = sdp->host;
+	sector_t capacity = logical_to_sectors(sdp, sdkp->capacity);
 	int diskinfo[4];
 
 	/* default to most commonly used values */
-        diskinfo[0] = 0x40;	/* 1 << 6 */
-       	diskinfo[1] = 0x20;	/* 1 << 5 */
-       	diskinfo[2] = sdkp->capacity >> 11;
-	
+	diskinfo[0] = 0x40;	/* 1 << 6 */
+	diskinfo[1] = 0x20;	/* 1 << 5 */
+	diskinfo[2] = capacity >> 11;
+
 	/* override with calculated, extended default, or driver values */
 	if (host->hostt->bios_param)
-		host->hostt->bios_param(sdp, bdev, sdkp->capacity, diskinfo);
+		host->hostt->bios_param(sdp, bdev, capacity, diskinfo);
 	else
-		scsicam_bios_param(bdev, sdkp->capacity, diskinfo);
+		scsicam_bios_param(bdev, capacity, diskinfo);
 
 	geo->heads = diskinfo[0];
 	geo->sectors = diskinfo[1];
@@ -2337,14 +2338,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,11 +2805,6 @@ static int sd_try_extended_inquiry(struct scsi_device *sdp)
 	return 0;
 }
 
-static inline u32 logical_to_sectors(struct scsi_device *sdev, u32 blocks)
-{
-	return blocks << (ilog2(sdev->sector_size) - 9);
-}
-
 /**
  *	sd_revalidate_disk - called the first time a new disk is seen,
  *	performs disk spin up, read_capacity, etc.
@@ -2900,7 +2888,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);
 
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 5f2a84aff29f..654630bb7d0e 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 blocks */
 	u32		max_xfer_blocks;
 	u32		opt_xfer_blocks;
 	u32		max_ws_blocks;
@@ -146,6 +146,11 @@ static inline int scsi_medium_access_command(struct scsi_cmnd *scmd)
 	return 0;
 }
 
+static inline sector_t logical_to_sectors(struct scsi_device *sdev, sector_t blocks)
+{
+	return blocks << (ilog2(sdev->sector_size) - 9);
+}
+
 /*
  * A DIF-capable target device can be formatted with different
  * protection schemes.  Currently 0 through 3 are defined:
-- 
2.7.4

Re: [PATCH v2] sd: Fix excessive capacity printing on devices with blocks bigger than 512 bytes

[Hannes Reinecke]

On 03/29/2016 03:18 AM, Martin K. Petersen wrote:
> 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 and generating fake C/H/S
> geometry.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> Reported-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/sd.c | 28 ++++++++--------------------
>  drivers/scsi/sd.h |  7 ++++++-
>  2 files changed, 14 insertions(+), 21 deletions(-)
> 
After some testing I do prefer this version over mine.
Especially as I need the exported version of logical_to_sectors for
my SMR patchset :-)

So:

Reviewed-by: Hannes Reinecke <hare@suse.com>

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 v2] sd: Fix excessive capacity printing on devices with blocks bigger than 512 bytes

[Ewan D. Milne]

On Mon, 2016-03-28 at 21:18 -0400, Martin K. Petersen wrote:
> 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 and generating fake C/H/S
> geometry.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> Reported-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/sd.c | 28 ++++++++--------------------
>  drivers/scsi/sd.h |  7 ++++++-
>  2 files changed, 14 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 5a5457ac9cdb..8401697ff6aa 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1275,18 +1275,19 @@ static int sd_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>  	struct scsi_disk *sdkp = scsi_disk(bdev->bd_disk);
>  	struct scsi_device *sdp = sdkp->device;
>  	struct Scsi_Host *host = sdp->host;
> +	sector_t capacity = logical_to_sectors(sdp, sdkp->capacity);
>  	int diskinfo[4];
>  
>  	/* default to most commonly used values */
> -        diskinfo[0] = 0x40;	/* 1 << 6 */
> -       	diskinfo[1] = 0x20;	/* 1 << 5 */
> -       	diskinfo[2] = sdkp->capacity >> 11;
> -	
> +	diskinfo[0] = 0x40;	/* 1 << 6 */
> +	diskinfo[1] = 0x20;	/* 1 << 5 */
> +	diskinfo[2] = capacity >> 11;
> +
>  	/* override with calculated, extended default, or driver values */
>  	if (host->hostt->bios_param)
> -		host->hostt->bios_param(sdp, bdev, sdkp->capacity, diskinfo);
> +		host->hostt->bios_param(sdp, bdev, capacity, diskinfo);
>  	else
> -		scsicam_bios_param(bdev, sdkp->capacity, diskinfo);
> +		scsicam_bios_param(bdev, capacity, diskinfo);
>  
>  	geo->heads = diskinfo[0];
>  	geo->sectors = diskinfo[1];
> @@ -2337,14 +2338,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,11 +2805,6 @@ static int sd_try_extended_inquiry(struct scsi_device *sdp)
>  	return 0;
>  }
>  
> -static inline u32 logical_to_sectors(struct scsi_device *sdev, u32 blocks)
> -{
> -	return blocks << (ilog2(sdev->sector_size) - 9);
> -}
> -
>  /**
>   *	sd_revalidate_disk - called the first time a new disk is seen,
>   *	performs disk spin up, read_capacity, etc.
> @@ -2900,7 +2888,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);
>  
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index 5f2a84aff29f..654630bb7d0e 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 blocks */
>  	u32		max_xfer_blocks;
>  	u32		opt_xfer_blocks;
>  	u32		max_ws_blocks;
> @@ -146,6 +146,11 @@ static inline int scsi_medium_access_command(struct scsi_cmnd *scmd)
>  	return 0;
>  }
>  
> +static inline sector_t logical_to_sectors(struct scsi_device *sdev, sector_t blocks)
> +{
> +	return blocks << (ilog2(sdev->sector_size) - 9);
> +}
> +
>  /*
>   * A DIF-capable target device can be formatted with different
>   * protection schemes.  Currently 0 through 3 are defined:

Reviewed-by: Ewan D. Milne <emilne@redhat.com>