libata: fix ata_id_logical_per_physical_sectors
PATCH 1/1 libata: fix ata_id_logical_per_physical_sectors
The value we get from the low byte of the ATA_ID_SECTOR_SIZE word is not not a plain multiple, but the log of it, so fix the helper to give the correct answer. Without this we'll get an incorrect minimal I/O size in the block limits VPD page for 4k sector drives.
include/linux/ata.h +1 -1
@@ -649,7 +649,7 @@ static inline int ata_id_has_large_logicstatic inline u8 ata_id_logical_per_physical_sectors (const u16 *id) { -return id[ATA_ID_SECTOR_SIZE ] &0xf ; +return 1 << (id[ATA_ID_SECTOR_SIZE ] &0xf ); }static inline int ata_id_has_lba48 (const u16 *id)
Hello.
Quoted 12 lines · Christoph Hellwig
The value we get from the low byte of the
ATA_ID_SECTOR_SIZEword is not not a plain multiple, but the log of it, so fix the helper to give the correct answer. Without this we'll get an incorrect minimal I/O size in the block limits VPD page for 4k sector drives.Signed-off-by Christoph Hellwig <[email protected]>Index:
linux-2.6/include/linux/ata.h
===================================================================
include/linux/ata.h static inline int ata_id_has_large_logic · line 653
Quoted 6 lines
include/linux/ata.h +1
-1@@ -649,7 +649,7 @@ static inline int ata_id_has_large_logicstatic inline u8 ata_id_logical_per_physical_sectors (const u16 *id) { -return id[ATA_ID_SECTOR_SIZE ] &0xf ; +return 1 << (id[ATA_ID_SECTOR_SIZE ] &0xf );
Will this still fit into u8?
WBR, Sergei
Quoted 7 lines · Sergei Shtylyov
include/linux/ata.h static inline int ata_id_has_large_logic · line 652
Quoted 5 lines
static inline u8
ata_id_logical_per_physical_sectors(const u16 *id){include/linux/ata.h +1
-1-return id[ATA_ID_SECTOR_SIZE ] &0xf ; +return 1 << (id[ATA_ID_SECTOR_SIZE ] &0xf );Will this still fit into u8?
Right now the only value we get in practice is 8 for 4k sector drivers which still fits fine into an u8. But it might indeed be safer to use a large value.
PATCH v2 1/1 libata: fix ata_id_logical_per_physical_sectors
In reply to Christoph HellwigThe value we get from the low byte of the ATA_ID_SECTOR_SIZE word is not not a plain multiple, but the log of it, so fix the helper to give the correct answer. Without this we'll get an incorrect minimal I/O size in the block limits VPD page for 4k sector drives.
Also change the return value of ata_id_logical_per_physical_sectors to u16 for the unlikely case of very large logical sectors.
include/linux/ata.h +2 -2
@@ -647,9 +647,9 @@ static inline int ata_id_has_large_logicreturn id[ATA_ID_SECTOR_SIZE ] & (1 <<13 ); } -static inline u8 ata_id_logical_per_physical_sectors (const u16 *id) +static inline u16 ata_id_logical_per_physical_sectors (const u16 *id) { -return id[ATA_ID_SECTOR_SIZE ] &0xf ; +return 1 << (id[ATA_ID_SECTOR_SIZE ] &0xf ); }static inline int ata_id_has_lba48 (const u16 *id)
Jeff, ping? Without this patch the minimum I/O size for 4k drives will be reported incorrectly, which will make paritions misaligned on modern distros.
Quoted 16 lines · Christoph Hellwig
The value we get from the low byte of the
ATA_ID_SECTOR_SIZEword is not not a plain multiple, but the log of it, so fix the helper to give the correct answer. Without this we'll get an incorrect minimal I/O size in the block limits VPD page for 4k sector drives.Also change the return value of
ata_id_logical_per_physical_sectorsto u16 for the unlikely case of very large logical sectors.Signed-off-by Christoph Hellwig <[email protected]>Index:
linux-2.6/include/linux/ata.h
===================================================================include/linux/ata.h +0
-0@@ -647,9 +647,9 @@ static inline int ata_id_has_large_logicreturn id[ATA_ID_SECTOR_SIZE ] & (1 <<13 );
include/linux/ata.h static inline int ata_id_has_large_logic · line 655
Quoted 10 lines
include/linux/ata.h +2
-2} -static inline u8 ata_id_logical_per_physical_sectors (const u16 *id) +static inline u16 ata_id_logical_per_physical_sectors (const u16 *id) { -return id[ATA_ID_SECTOR_SIZE ] &0xf ; +return 1 << (id[ATA_ID_SECTOR_SIZE ] &0xf ); }static inline int ata_id_has_lba48 (const u16 *id)
---end quoted text---
Quoted 3 lines · Christoph Hellwig
Jeff, ping? Without this patch the minimum I/O size for 4k drives will be reported incorrectly, which will make paritions misaligned on modern distros.
It's queued locally. I'm waiting to push on the flush_dache_page() discussion for 2.6.33, but I'll make sure your patch gets pushed today one way or the other.
Jeff
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Christoph" == Christoph Hellwig <[email protected]> writes:
Christoph> Without this patch the minimum I/O size for 4k drives will be Christoph> reported incorrectly, which will make paritions misaligned on Christoph> modern distros.
I'm in total agreement with the patch (feel free to add my Acked-by:).
However, the minimum I/O size should still be reported correctly. We don't allow min_io to be smaller than physical_block_size...
[root@10 ~]# grep . /sys/block/sdc/queue/{logical_block,physical_block,minimum_io}_size /sys/block/sdc/queue/logical_block_size:512 /sys/block/sdc/queue/physical_block_size:4096 /sys/block/sdc/queue/minimum_io_size:4096
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 10 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Christoph" == Christoph Hellwig <[email protected]> writes:
Christoph> Without this patch the minimum I/O size for 4k drives will be Christoph> reported incorrectly, which will make paritions misaligned on Christoph> modern distros.
I'm in total agreement with the patch (feel free to add my Acked-by:).
However, the minimum I/O size should still be reported correctly. We don't allow
min_ioto be smaller thanphysical_block_size...
Indeed, I only verified it using sg_inq, not the sysfs files.