Hello, Tom.

On Wed, Aug 10, 2016 at 04:32:39PM +0800, Tom Yan wrote:

I have to admit that libata may not be the right place to deal with my
concern over the current BLK_DEF_MAX_SECTORS, which seems non-sensical
to me. In the original commit message:

(d2be537c3ba3, "block: bump BLK_DEF_MAX_SECTORS to 2560")
"A value of 2560 (1280k) will accommodate a 10-data-disk stripe write
with chunk size 128k...a value of 1280 does not show a big performance
difference from 512, but will hopefully help software RAID setups
using SATA disks, as reported by Christoph."

So I have no idea at all why the bump was allowed. What so special

In general, people are okay with upping the limits as long as there
are actual use cases which can benefit.  The devices are getting
constantly faster and the hardware limits are going up along with it
after all.

about "10-data-disk stripe write with chunk size 128k", that we would
want to make a block layer general default base on that?

It's not special.  It's just an actual use case which can benefit from
the raised limit.

The macro appeared to be used by the aoeblk driver only. Yet since a
later commit (ca369d51b3e1, "block/sd: Fix device-imposed transfer
length limits"), the scsi disk driver will use it to set the effective
max_sectors(_kb) for devices that does not report Optimal Transfer
Length in the Block Limit VPD (which is the case of libata's SATL).

So the consequence is, ATA drives with max_hw_sectors(_kb) (i.e.
dev->max_sectors set in libata-core.c) set to higher than
BLK_DEF_MAX_SECTORS will end up having it as the effective
max_sectors. Not only the value is non-sensical, but also the logic.
(Btw, ATA_HORKAGE_MAX_SEC_LBA48 will sort of become ineffective
because of this.)

Now let's just come back to libata. I've thought of reporting
dev->max_sectors as Optimal Transfer Length in the SATL. However, I am
not sure if it is a safe thing to do, because we set it as high as
65535 for devices with LBA48 devices. Does such a high max_sectors
ever make sense in libata's case?

libata is just exposing whatever the underlying hardware supports.
Whether that should be exposed as optimal transfer length is a
different matter.

That's why I took the approach of the USB Attached SCSI driver. That
is, we only explicitly set max_hw_sectors in cases that is strictly
necessary (e.g. no LBA48 support, horkages). Otherwise we'll leave
request queue as-is.

I see.  Alright, let's do that.  Can you please respin the patch with
more detailed description?

Thanks.

-- 
tejun