[v2,2/2] sd: check BLK_DEF_MAX_SECTORS against max_dev_sectors

Commit Message

Tom Yan Aug. 12, 2016, 2:27 p.m. UTC

From: Tom Yan <[e-mail]>

The SCSI disk driver sets max_sectors to BLK_DEF_MAX_SECTORS when
the device does not report Optimal Transfer Length. However, it
checks only whether it is smaller than max_hw_sectors, but not
max_dev_sectors.

Signed-off-by: Tom Yan <[e-mail]>

Comments

Martin K. Petersen Aug. 12, 2016, 9:37 p.m. UTC | #1

>>>>> "Tom" == tom ty89 <[e-mail]> writes:

Tom,

Tom> The SCSI disk driver sets max_sectors to BLK_DEF_MAX_SECTORS when
Tom> the device does not report Optimal Transfer Length. However, it
Tom> checks only whether it is smaller than max_hw_sectors, but not
Tom> max_dev_sectors.

It would be pretty unusual for a device that is smart enough to report a
transfer length limit to be constrained to 1 MB and change.

@@ -2875,8 +2875,11 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	    logical_to_bytes(sdp, sdkp->opt_xfer_blocks) >= PAGE_SIZE) {
 		q->limits.io_opt = logical_to_bytes(sdp, sdkp->opt_xfer_blocks);
 		rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
-	} else
+	} else {
 		rw_max = BLK_DEF_MAX_SECTORS;
+		/* Combine with device limits */
+		rw_max = min(rw_max, q->limits.max_dev_sectors);
+	}
 
 	/* Combine with controller limits */
 	q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));

If you want to fix this please drop the braces and do:

		rw_max = min(BLK_DEF_MAX_SECTORS, q->limits.max_dev_sectors);

Tom Yan Aug. 14, 2016, 9 a.m. UTC | #2

On 13 August 2016 at 05:37, Martin K. Petersen
<[e-mail]> wrote:
>
> It would be pretty unusual for a device that is smart enough to report a
> transfer length limit to be constrained to 1 MB and change.
>

Well, it is done pretty much for libata's SATL. Also since
opt_xfer_blocks is checked against dev_max, I think it makes sense in
logic that we do the equivalence for BLK_DEF_MAX_SECTORS. Not to
mention that since we check rw_max against the controller limit, no
reason not to make sure we check it against the device limit in both
cases.

>
> If you want to fix this please drop the braces and do:
>
>                 rw_max = min(BLK_DEF_MAX_SECTORS, q->limits.max_dev_sectors);

That won't really work. min_t() would, though the line is gonna be a
bit long; not sure if I can/should simply cast the type (unsigned int)
to BLK_DEF_MAX_SECTORS. And which braces are you referring to?

>
> --
> Martin K. Petersen      Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tom Yan Aug. 14, 2016, 9:40 a.m. UTC | #3

On 14 August 2016 at 17:00, Tom Yan <[e-mail]> wrote:
>
> That won't really work. min_t() would, though the line is gonna be a
> bit long; not sure if I can/should simply cast the type (unsigned int)
> to BLK_DEF_MAX_SECTORS. And which braces are you referring to?
>
Oh you mean the else-clause braces. Hmm I thought the coding style was
that if you needed braces in one clause, you should use braces for
others in the condition even if it consists of only one line of
statement. At least that is what I was told when I send patches for
libata.

So, should I use min_t() or just do a type casting in front to
BLK_DEF_MAX_SECTORS? Also do I need to break the line at a certain
point when it exceeds certain length?
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Martin K. Petersen Aug. 16, 2016, 4:10 a.m. UTC | #4

>>>>> "Tom" == Tom Yan <[e-mail]> writes:

Tom,

>> It would be pretty unusual for a device that is smart enough to
>> report a transfer length limit to be constrained to 1 MB and change.

Tom> Well, it is done pretty much for libata's SATL.

But why?

>> rw_max = min(BLK_DEF_MAX_SECTORS, q->limits.max_dev_sectors);

Tom> That won't really work. min_t() would, though the line is gonna be
Tom> a bit long; not sure if I can/should simply cast the type (unsigned
Tom> int) to BLK_DEF_MAX_SECTORS. And which braces are you referring to?

I'd rather have a split line than double lines with braces.

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d3e852a..2b6da13 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2875,8 +2875,11 @@  static int sd_revalidate_disk(struct gendisk *disk)
 	    logical_to_bytes(sdp, sdkp->opt_xfer_blocks) >= PAGE_SIZE) {
 		q->limits.io_opt = logical_to_bytes(sdp, sdkp->opt_xfer_blocks);
 		rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
-	} else
+	} else {
 		rw_max = BLK_DEF_MAX_SECTORS;
+		/* Combine with device limits */
+		rw_max = min(rw_max, q->limits.max_dev_sectors);
+	}
 
 	/* Combine with controller limits */
 	q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));