[RFC 1/3] ata: bump ATA_MAX_SECTORS_LBA48 to 65536

[RFC 1/3] ata: bump ATA_MAX_SECTORS_LBA48 to 65536

[tom.ty89]

From: Tom Yan <tom.ty89@gmail.com>

ATA_MAX_SECTORS_LBA48 is only used for setting the queue limit
"max_hw_sectors", which only serves as the cap for "max_sectors",
which is in turn the actual limit being used. Therefore, it should
be alright for us to bump ATA_MAX_SECTORS_LBA48 to 65536. Also,
lba_48_ok() has been accepting the number of blocks to be 65536.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

diff --git a/include/linux/ata.h b/include/linux/ata.h
index 99346be..24f886c 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -46,7 +46,7 @@ enum {
 	ATA_MAX_SECTORS_128	= 128,
 	ATA_MAX_SECTORS		= 256,
 	ATA_MAX_SECTORS_1024    = 1024,
-	ATA_MAX_SECTORS_LBA48	= 65535,/* TODO: 65536? */
+	ATA_MAX_SECTORS_LBA48	= 65536,
 	ATA_MAX_SECTORS_TAPE	= 65535,
 
 	ATA_ID_WORDS		= 256,
-- 
2.9.0

[RFC 2/3] ata: make lba_{28,48}_ok() use ATA_MAX_SECTORS{,_LBA48}

[tom.ty89]

From: Tom Yan <tom.ty89@gmail.com>

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

diff --git a/include/linux/ata.h b/include/linux/ata.h
index 24f886c..d4bb802 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -1095,13 +1095,13 @@ static inline bool ata_ok(u8 status)
 static inline bool lba_28_ok(u64 block, u32 n_block)
 {
 	/* check the ending block number: must be LESS THAN 0x0fffffff */
-	return ((block + n_block) < ((1 << 28) - 1)) && (n_block <= 256);
+	return ((block + n_block) < ((1 << 28) - 1)) && (n_block <= ATA_MAX_SECTORS);
 }
 
 static inline bool lba_48_ok(u64 block, u32 n_block)
 {
 	/* check the ending block number */
-	return ((block + n_block - 1) < ((u64)1 << 48)) && (n_block <= 65536);
+	return ((block + n_block - 1) < ((u64)1 << 48)) && (n_block <= ATA_MAX_SECTORS_LBA48);
 }
 
 #define sata_pmp_gscr_vendor(gscr)	((gscr)[SATA_PMP_GSCR_PROD_ID] & 0xffff)
-- 
2.9.0

[PATCH] ata: make lba_{28,48}_ok() use ATA_MAX_SECTORS{,_LBA48}

[tom.ty89]

From: Tom Yan <tom.ty89@gmail.com>

Since we set ATA_MAX_SECTORS_LBA48 to 65535 to avoid the corner case
in some drives that commands with "count" set to 0000h (which
reprsents 65536) does not work as expected, lba_48_ok(), which is
used for number-of-blocks checking when libata pack commands, should
use the same limit as well. In fact, there is no reason for the two
functions not to use the macros anyway.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

diff --git a/include/linux/ata.h b/include/linux/ata.h
index 99346be..f87287a 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -46,7 +46,7 @@ enum {
 	ATA_MAX_SECTORS_128	= 128,
 	ATA_MAX_SECTORS		= 256,
 	ATA_MAX_SECTORS_1024    = 1024,
-	ATA_MAX_SECTORS_LBA48	= 65535,/* TODO: 65536? */
+	ATA_MAX_SECTORS_LBA48	= 65535,/* avoid count to be 0000h */
 	ATA_MAX_SECTORS_TAPE	= 65535,
 
 	ATA_ID_WORDS		= 256,
@@ -1095,13 +1095,13 @@ static inline bool ata_ok(u8 status)
 static inline bool lba_28_ok(u64 block, u32 n_block)
 {
 	/* check the ending block number: must be LESS THAN 0x0fffffff */
-	return ((block + n_block) < ((1 << 28) - 1)) && (n_block <= 256);
+	return ((block + n_block) < ((1 << 28) - 1)) && (n_block <= ATA_MAX_SECTORS);
 }
 
 static inline bool lba_48_ok(u64 block, u32 n_block)
 {
 	/* check the ending block number */
-	return ((block + n_block - 1) < ((u64)1 << 48)) && (n_block <= 65536);
+	return ((block + n_block - 1) < ((u64)1 << 48)) && (n_block <= ATA_MAX_SECTORS_LBA48);
 }
 
 #define sata_pmp_gscr_vendor(gscr)	((gscr)[SATA_PMP_GSCR_PROD_ID] & 0xffff)
-- 
2.9.0

Re: [PATCH] ata: make lba_{28,48}_ok() use ATA_MAX_SECTORS{,_LBA48}

[Tejun Heo]

On Fri, Jul 15, 2016 at 05:09:02AM +0800, tom.ty89@gmail.com wrote:
> From: Tom Yan <tom.ty89@gmail.com>
> 
> Since we set ATA_MAX_SECTORS_LBA48 to 65535 to avoid the corner case
> in some drives that commands with "count" set to 0000h (which
> reprsents 65536) does not work as expected, lba_48_ok(), which is
> used for number-of-blocks checking when libata pack commands, should
> use the same limit as well. In fact, there is no reason for the two
> functions not to use the macros anyway.
> 
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>

Applied to libata/for-4.8.

Thanks.

-- 
tejun

[RFC 3/3] libata-scsi: add optimal transfer length to block limits VPD

[tom.ty89]

From: Tom Yan <tom.ty89@gmail.com>

As of commit 6b7e9cde4969 ("sd: Fix rw_max for devices that report
an optimal xfer size"), the scsi disk driver (correctly) derive both
of the queue limits "io_opt" and "max_sectors" from the optimal
transfer length field.

In case we would like the two limits to be derived from a value
other than BLK_DEF_MAX_SECTORS for ATA disks in the future, this
patch has made it easy.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index bfec66f..ab75b5e 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2305,6 +2305,13 @@ static unsigned int ata_scsiop_inq_b0(struct ata_scsi_args *args, u8 *rbuf)
 	put_unaligned_be16(min_io_sectors, &rbuf[6]);
 
 	/*
+	 * Optimal transfer length.
+	 *
+	 * This is used to derive the queue limit "max_sector" and "io_opt".
+	 */
+	put_unaligned_be32(BLK_DEF_MAX_SECTORS, &rbuf[12]);
+
+	/*
 	 * Optimal unmap granularity.
 	 *
 	 * The ATA spec doesn't even know about a granularity or alignment
-- 
2.9.0

Re: [RFC 3/3] libata-scsi: add optimal transfer length to block limits VPD

[Tejun Heo]

On Wed, Jul 13, 2016 at 12:47:08PM +0800, tom.ty89@gmail.com wrote:
> From: Tom Yan <tom.ty89@gmail.com>
> 
> As of commit 6b7e9cde4969 ("sd: Fix rw_max for devices that report
> an optimal xfer size"), the scsi disk driver (correctly) derive both
> of the queue limits "io_opt" and "max_sectors" from the optimal
> transfer length field.
> 
> In case we would like the two limits to be derived from a value
> other than BLK_DEF_MAX_SECTORS for ATA disks in the future, this
> patch has made it easy.

What's the actual impact of this patch at this point?

Thanks.

-- 
tejun

Re: [RFC 3/3] libata-scsi: add optimal transfer length to block limits VPD

[Tom Yan]

Well it's mainly to "pre-de-couple" BLK_DEF_MAX_SECTORS from ATA
drives. If we left the field unset, the scsi disk driver will fall
back to BLK_DEF_MAX_SECTORS when setting the block limit "max_sectors"
(while its cap "max_hw_sectors" is set by libata depending on whether
the drive supports LBA48 or so). So by filling in this field, we can
set an optimal value for the ATA drives specifically, while the block
layer devs will have less to concern when they determine the value of
the macro (which should be hardware-neutural, sort of).

A "side-effect" of this is, the scsi disk driver will also use this
field to set the block limit "io_opt", which we had left it unset all
the time. I am not sure about if there are any pros and cons in
setting the limit, that's why I sent this as an RFC and to the
linux-block mailing list as well. By looking at what the scsi disk
driver do, it appears that it's a sensible thing to set the limit to a
value that matches with the other limit "max_sectors", though.

On 14 July 2016 at 01:04, Tejun Heo <tj@kernel.org> wrote:
> On Wed, Jul 13, 2016 at 12:47:08PM +0800, tom.ty89@gmail.com wrote:
>> From: Tom Yan <tom.ty89@gmail.com>
>>
>> As of commit 6b7e9cde4969 ("sd: Fix rw_max for devices that report
>> an optimal xfer size"), the scsi disk driver (correctly) derive both
>> of the queue limits "io_opt" and "max_sectors" from the optimal
>> transfer length field.
>>
>> In case we would like the two limits to be derived from a value
>> other than BLK_DEF_MAX_SECTORS for ATA disks in the future, this
>> patch has made it easy.
>
> What's the actual impact of this patch at this point?
>
> Thanks.
>
> --
> tejun

Re: [RFC 1/3] ata: bump ATA_MAX_SECTORS_LBA48 to 65536

[Tejun Heo]

Hello,

On Wed, Jul 13, 2016 at 12:47:06PM +0800, tom.ty89@gmail.com wrote:
> From: Tom Yan <tom.ty89@gmail.com>
> 
> ATA_MAX_SECTORS_LBA48 is only used for setting the queue limit
> "max_hw_sectors", which only serves as the cap for "max_sectors",
> which is in turn the actual limit being used. Therefore, it should

It's used to device max_sectors.

> be alright for us to bump ATA_MAX_SECTORS_LBA48 to 65536. Also,
> lba_48_ok() has been accepting the number of blocks to be 65536.

This is way too gratuitous.  There's no rationale for it while it has
actual real-world risks.  max_sectors is staying at 65535.

Thanks.

-- 
tejun

Re: [RFC 1/3] ata: bump ATA_MAX_SECTORS_LBA48 to 65536

[Tom Yan]

On 14 July 2016 at 01:03, Tejun Heo <tj@kernel.org> wrote:
>
> It's used to device max_sectors.
>

Not really. "max_sectors" of ATA drives have been set to
BLK_DEF_MAX_SECTORS, for at least quite some time.

>
> This is way too gratuitous.  There's no rationale for it while it has
> actual real-world risks.  max_sectors is staying at 65535.
>

That's alright. I don't have strong opinion on whether we should bump
this anyway. So I suppose we should replace the TODO comment with
something like "avoid count to be 0000h"? And maybe also change
ATA_MAX_SECTORS to 255 (with comment "avoid count to be 00h")?

Re: [RFC 1/3] ata: bump ATA_MAX_SECTORS_LBA48 to 65536

[Tejun Heo]

On Fri, Jul 15, 2016 at 02:22:52AM +0800, Tom Yan wrote:
> On 14 July 2016 at 01:03, Tejun Heo <tj@kernel.org> wrote:
> >
> > It's used to device max_sectors.
> >
> 
> Not really. "max_sectors" of ATA drives have been set to
> BLK_DEF_MAX_SECTORS, for at least quite some time.
> 
> >
> > This is way too gratuitous.  There's no rationale for it while it has
> > actual real-world risks.  max_sectors is staying at 65535.
> >
> 
> That's alright. I don't have strong opinion on whether we should bump
> this anyway. So I suppose we should replace the TODO comment with
> something like "avoid count to be 0000h"? And maybe also change
> ATA_MAX_SECTORS to 255 (with comment "avoid count to be 00h")?

I'd just leave both alone.  It's one sector difference one way or the
other and those numbers have a pretty long history.  There's no reason
to disturb them at this point.  There's no actual gain whatsoever but
there is some actual risk.

Thanks.

-- 
tejun