Re: [PATCH] sd: fixup capacity calculation for 4k drives
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
> ---
> 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
> 0x 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_capacit
[PATCH] sd: fixup capacity calculation for 4k drives
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
---
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
0x 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)
Re: [PATCH] sd: fixup capacity calculation for 4k drives
> "Ewan" == Ewan D Milne 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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sd: fixup capacity calculation for 4k drives
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 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_topeners;
> - sector_tcapacity; /* size in 512-byte sectors */
> + sector_tcapacity; /* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sd: fixup capacity calculation for 4k drives
On 03/22/2016 02:16 AM, Martin K. Petersen wrote:
>> "Hannes" == Hannes Reinecke 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_topeners;
- sector_tcapacity; /* size in 512-byte sectors */
+ sector_tcapacity; /* 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 ReineckeTeamlead Storage & Networking
h...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sd: fixup capacity calculation for 4k drives
Looks fine, Reviewed-by: Christoph Hellwig -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sd: fixup capacity calculation for 4k drives
> "Hannes" == Hannes Reinecke 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
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
Reported-by: Hannes Reinecke
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 > 0x)
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);
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sd: fixup capacity calculation for 4k drives
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? -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] sd: fixup capacity calculation for 4k drives
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
