Hi Roland + Stefan,
On Wed, 2012-03-21 at 10:11 -0700, Roland Dreier wrote:
> On Wed, Mar 21, 2012 at 4:18 AM, Stefan Gourguis <brockz@xxxxxxxxxxxx> wrote:
> > And also my relevant demsg output:
> > [ 1936.317399] Rounding down aligned max_sectors from 4294967295 to 8388600
>
> Looks like a bug exposed with hch's change to iblock in c1775c389556:
>
> - limits->max_hw_sectors = queue_max_hw_sectors(q);
> - limits->max_sectors = queue_max_sectors(q);
> + limits->max_hw_sectors = UINT_MAX;
> + limits->max_sectors = UINT_MAX;
>
> now the code in target_core_device.c is broken:
>
> u32 se_dev_align_max_sectors(u32 max_sectors, u32 block_size)
> {
> u32 tmp, aligned_max_sectors;
> /*
> * Limit max_sectors to a PAGE_SIZE aligned value for modern
> * transport_allocate_data_tasks() operation.
> */
> tmp = rounddown((max_sectors * block_size), PAGE_SIZE);
> --> max_sectors is UINT_MAX, so max_sectors * block_size overflows.
>
<SNIP from Roland's patch>
- tmp = rounddown((max_sectors * block_size), PAGE_SIZE);
- aligned_max_sectors = (tmp / block_size);
+ aligned_max_sectors = rounddown(max_sectors, PAGE_SIZE / block_size);
The PAGE_SIZE / block_size calculation looks bogus to me here..
if (max_sectors != aligned_max_sectors) {
printk(KERN_INFO "Rounding down aligned max_sectors from %u"
" to %u\n", max_sectors, aligned_max_sectors);
To address the overflow with a large rounddown() calculation for
max_sectors, I think it should be unsigned long long tmp with do_div()
to avoid the unwanted __udivdi3() usage on 32-bit..
How about the following patch instead..?
--nab
commit 62d0f392884c5e712abb986182bde307cc110c7e
Author: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
Date: Wed Mar 21 17:03:47 2012 -0700
target: Fix max_sectors alignment handling
Makes se_dev_align_max_sectors() use do_div() to avoid overflow from a
large max_sectors when checking if the passed max_sectors needs to be
rounded-down to be PAGE_SIZE aligned for transport_allocate_data_tasks().
Also bump the constant DA_STATUS_MAX_SECTORS_MAX used by the configfs
device attribute max_sectors store function, that was limiting this
value to 8192.
Reported-by: Stefan Gourguis <brockz@xxxxxxxxxxxx>
Cc: Roland Dreier <roland@xxxxxxxxxxxxxxx>
Cc: Christoph Hellwig <hch@xxxxxx>
Signed-off-by: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index a374cb5..fe95965 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -827,17 +827,17 @@ int se_dev_check_shutdown(struct se_device *dev)
u32 se_dev_align_max_sectors(u32 max_sectors, u32 block_size)
{
- u32 tmp, aligned_max_sectors;
+ unsigned long long tmp;
/*
* Limit max_sectors to a PAGE_SIZE aligned value for modern
* transport_allocate_data_tasks() operation.
*/
tmp = rounddown((max_sectors * block_size), PAGE_SIZE);
- aligned_max_sectors = (tmp / block_size);
- if (max_sectors != aligned_max_sectors) {
- printk(KERN_INFO "Rounding down aligned max_sectors from %u"
- " to %u\n", max_sectors, aligned_max_sectors);
- return aligned_max_sectors;
+ do_div(tmp, block_size);
+ if (max_sectors != tmp) {
+ pr_debug("Rounding down aligned max_sectors from %u"
+ " to %llu\n", max_sectors, tmp);
+ return (u32)tmp;
}
return max_sectors;
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index b571556..d27bd4d 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -112,7 +112,8 @@
/* Enforce SCSI Initiator Port TransportID with 'ISID' for PR */
#define DA_ENFORCE_PR_ISIDS 1
#define DA_STATUS_MAX_SECTORS_MIN 16
-#define DA_STATUS_MAX_SECTORS_MAX 8192
+/* Match value returned by se_dev_align_max_sectors with UINT_MAX passed from IBLOCK */
+#define DA_STATUS_MAX_SECTORS_MAX 8388600
/* By default don't report non-rotating (solid state) medium */
#define DA_IS_NONROT 0
/* Queue Algorithm Modifier default for restricted reordering in control mode page */
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
