On Tue, 2012-05-08 at 04:11 -0400, Christoph Hellwig wrote:
> On Sun, May 06, 2012 at 09:54:48PM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
> >
> > This patch changes the received sectors vs. se_dev_attrib.max_sectors
> > check to fail only when checking against passthrough (pSCSI) backends,
> > instead of against all existing max_sectors that may be reflecting a
> > smaller reported value due to underlying HW limitations.
> >
> > It addresses a potential case where se_dev_attrib.max_sectors for IBLOCK
> > backends has already been set via queue_max_sectors() to something small
> > like max_sectors=32 (LVM, DRBD may do this), resulting typically sized
> > SCF_SCSI_DATA_SG_IO_CDB to be incorrectly rejected with invalid_cdb_field
> > in transport_generic_cmd_sequencer().
>
> max_sectors for iblock now always is UINT_MAX. We still need to limit
> the request size for the file backend as well for now, so I think this
> patch is incorrect.
>
So the issue of concern here is with existing userspace code that is
using this value to explicitly set max_sectors at target restart time
based upon the original blkdev queue limits (or smaller) with IBLOCK,
and can possibly end up rejecting incoming I/O if the max_sectors is set
too small.
Considering we are still enforcing an fabric_max_sectors default value
of 8192 ahead of the max_sectors check, I think it makes sense to just
use max_hw_sectors instead here for all backends, and eventually turn
max_sectors into read-only attr and/or drop it's usage entirely in the
future..
How about the following patch to address the smaller 1024 max_sectors
enforced with FILEIO..?
--nab
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index b463561..17582df 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2981,12 +2981,11 @@ static int transport_generic_cmd_sequencer(
su_dev->se_dev_attrib.fabric_max_sectors);
goto out_invalid_cdb_field;
}
- if (passthrough &&
- sectors > su_dev->se_dev_attrib.max_sectors) {
+ if (sectors > su_dev->se_dev_attrib.max_hw_sectors) {
printk_ratelimited(KERN_ERR "SCSI OP %02xh with too"
- " big sectors %u exceeds backend max_sectors:"
+ " big sectors %u exceeds backend max_hw_sectors:"
" %u\n", cdb[0], sectors,
- su_dev->se_dev_attrib.max_sectors);
+ su_dev->se_dev_attrib.max_hw_sectors);
goto out_invalid_cdb_field;
}
}
--
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
