Re: [PATCHv4 0/6] Support 64-bit LUNs
On 06/10/2014 07:58 PM, Bart Van Assche wrote:
On 06/03/14 10:58, Hannes Reinecke wrote:
this patchset updates the SCSI stack to support full 64-bit LUNs.
The first patch is a simple fix; the next patch updates
the sequential scan logic to be compliant with SPC.
The third patch addresses a firmware issue with earlier
qla2xxx HBAs.
The next two patches update the SCSI stack and all drivers
to use 64-bit LUNs where appropriate.
And finally we need to update the conversion routines
scsilun_to_int to cope with 64bit LUNs.
Two drivers have issues with 64bit LUNs:
- The qla2xxx driver uses a 32-bit LUN value for TMFs.
But as the driver uses a max_lun value from 0x
we should be safe for the time being.
- The zfcp driver uses a 32-bit LUN for debug records; the
record format would need to be updated to cope with
64-bit LUNs. But again, this driver uses 0x
for max_lun, so it doesn't do any harm.
The other changes have been pretty straightforward.
Hello Hannes,
Many SCSI LLD's use int_to_scsilun() in the hot path (queuecommand()).
This patch series makes the int_to_scsilun() function slightly more
expensive. Has it been considered to cache the result of int_to_scsilun()
such that LLD's can copy the cached int_to_scsilun() result instead of
having to call int_to_scsilun() in the queuecommand() function ?
Something like the (untested) patch below might be sufficient:
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index e02b3aa..9e50d78 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -244,6 +244,7 @@ static struct scsi_device *scsi_alloc_sdev(struct
scsi_target *starget,
sdev-queue_ramp_up_period = SCSI_DEFAULT_RAMP_UP_PERIOD;
sdev-id = starget-id;
sdev-lun = lun;
+ int_to_scsilun(lun, sdev-scsi_lun);
sdev-channel = starget-channel;
sdev-sdev_state = SDEV_CREATED;
INIT_LIST_HEAD(sdev-siblings);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 5853c91..48ea68e 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -100,6 +100,8 @@ struct scsi_device {
unsigned int id, lun, channel;
+ struct scsi_lun scsi_lun; /* int_to_scsilun(lun) */
+
unsigned int manufacturer; /* Manufacturer of device, for using
* vendor-specific cmd's */
unsigned sector_size; /* size in bytes */
Thanks,
Hmm. No, so far it hasn't been considered. Maybe it's even
worthwhile to move the situation around, ie using primarily the
64-bit LUN and only convert it into an integer if so requested.
But yeah, it's definitely something we should look into.
Maybe _after_ the patchset is in?
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 0/6] Support 64-bit LUNs
On 06/11/14 08:34, Hannes Reinecke wrote:
On 06/10/2014 07:58 PM, Bart Van Assche wrote:
Many SCSI LLD's use int_to_scsilun() in the hot path (queuecommand()).
This patch series makes the int_to_scsilun() function slightly more
expensive. Has it been considered to cache the result of int_to_scsilun()
such that LLD's can copy the cached int_to_scsilun() result instead of
having to call int_to_scsilun() in the queuecommand() function ?
Something like the (untested) patch below might be sufficient:
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index e02b3aa..9e50d78 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -244,6 +244,7 @@ static struct scsi_device *scsi_alloc_sdev(struct
scsi_target *starget,
sdev-queue_ramp_up_period = SCSI_DEFAULT_RAMP_UP_PERIOD;
sdev-id = starget-id;
sdev-lun = lun;
+int_to_scsilun(lun, sdev-scsi_lun);
sdev-channel = starget-channel;
sdev-sdev_state = SDEV_CREATED;
INIT_LIST_HEAD(sdev-siblings);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 5853c91..48ea68e 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -100,6 +100,8 @@ struct scsi_device {
unsigned int id, lun, channel;
+struct scsi_lun scsi_lun;/* int_to_scsilun(lun) */
+
unsigned int manufacturer;/* Manufacturer of device, for using
* vendor-specific cmd's */
unsigned sector_size;/* size in bytes */
Thanks,
Hmm. No, so far it hasn't been considered. Maybe it's even worthwhile to
move the situation around, ie using primarily the 64-bit LUN and only
convert it into an integer if so requested.
But yeah, it's definitely something we should look into.
Maybe _after_ the patchset is in?
That's fine with me. I have one more question about this patch series
though: if you mention 64-bit LUNs, are you referring to multi-level
LUNs only or also to so-called extended LUNs ? I think the byte
reordering done by scsilun_to_int() is fine for multi-level LUNs but
unnatural for extended LUNs. As an example, in SAM-5 the format for
eight byte extended LUNs is defined as follows (paragraph 4.7.7.5.3):
byte 0: address method (3), length (2) and extended address method (2)
bytes 1..7: long extended flat space LUN with the MSB in byte 1 and LSB
in byte 7.
Today scsilun_to_int() does not preserve the MSB..LSB byte order for
extended LUNs. I think if we want to preserve the byte order in
scsilun_to_int() that that function will have to be made dependent on
the LUN addressing method. That would make scsilun_to_int() more
complex. Hence the proposal to cache the SCSI LUN such that the
queuecommand() functions are not slowed down by a call into
int_to_scsilun().
Bart.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire
On 6/11/2014 12:17 AM, Quinn Tran wrote: SNIP QT Instead of using existing value within cmd-data_length, can we calculated data_length based on secstors blocksize. cmd-data_length = sectors * dev-dev_attrib.block_size; We can do that I suppose... Although it seems weird that the core discards the transport byte-count... Just wandering if we should recalc on the DIF case only or always. From the QLogic perfective, the cmd-data_length is pull directly from the wire (i.e. FCP header) when cmd is received. In essence, it's whatever the Initiator is set it to. We does not have any indicator to spot DIF vs none-DIF cmd when 1st received, unless the code do a peek. Same for all transports I assume... With that said, the cmd-data_length does not guarantee to contain both data length protection length when cmd is submit to TCM/target_submit_cmd(). In Dif-Insert mode, data_length will only contain the actual data(no Dif). No, in the DOUT_INSERT/DIN_STRIP case, protect bits are off and the core will take the data length as is. This case is covered. It's best that the SBC code re-calculate the actual data length and dif data length based on the number of sectors derived from the cmd. Nic, what's your take on this? Sagi. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-v2 0/6] vhost/scsi: Add T10 PI SGL passthrough support
On Tue, Jun 10, 2014 at 02:20:28PM -0700, Nicholas A. Bellinger wrote: On Tue, 2014-06-10 at 13:56 -0700, Linus Torvalds wrote: On Tue, Jun 10, 2014 at 1:25 PM, Nicholas A. Bellinger n...@linux-iscsi.org wrote: That would work, or I can simply include a pointer to Stephen's patch in the target-pending PULL request after the vhost API changes are merged and Linus can apply himself.. Yes. That way I'll include it in the merge, and everything should just work. nod, sounds good. MST, please drop the target related patches from your tree, and go ahead and send your PULL request now. --nab ack. Doing it right now. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/3] libiscsi, iser: Adjust data_length to include protection information
In case protection information exists over the wire
iscsi header data length is required to include it.
Use protection information aware scsi helpers to set
the correct transfer length.
In order to avoid breakage, remove iser transfer length
checks for each task as they are not always true and
somewhat redundant anyway.
Signed-off-by: Sagi Grimberg sa...@mellanox.com
Reviewed-by: Mike Christie micha...@cs.wisc.edu
---
drivers/infiniband/ulp/iser/iser_initiator.c | 34 +++--
drivers/scsi/libiscsi.c | 18 +++---
2 files changed, 19 insertions(+), 33 deletions(-)
diff --git a/drivers/infiniband/ulp/iser/iser_initiator.c
b/drivers/infiniband/ulp/iser/iser_initiator.c
index 2e2d903..8d44a40 100644
--- a/drivers/infiniband/ulp/iser/iser_initiator.c
+++ b/drivers/infiniband/ulp/iser/iser_initiator.c
@@ -41,11 +41,11 @@
#include iscsi_iser.h
/* Register user buffer memory and initialize passive rdma
- * dto descriptor. Total data size is stored in
- * iser_task-data[ISER_DIR_IN].data_len
+ * dto descriptor. Data size is stored in
+ * task-data[ISER_DIR_IN].data_len, Protection size
+ * os stored in task-prot[ISER_DIR_IN].data_len
*/
-static int iser_prepare_read_cmd(struct iscsi_task *task,
-unsigned int edtl)
+static int iser_prepare_read_cmd(struct iscsi_task *task)
{
struct iscsi_iser_task *iser_task = task-dd_data;
@@ -73,14 +73,6 @@ static int iser_prepare_read_cmd(struct iscsi_task *task,
return err;
}
- if (edtl iser_task-data[ISER_DIR_IN].data_len) {
- iser_err(Total data length: %ld, less than EDTL:
-%d, in READ cmd BHS itt: %d, conn: 0x%p\n,
-iser_task-data[ISER_DIR_IN].data_len, edtl,
-task-itt, iser_task-ib_conn);
- return -EINVAL;
- }
-
err = device-iser_reg_rdma_mem(iser_task, ISER_DIR_IN);
if (err) {
iser_err(Failed to set up Data-IN RDMA\n);
@@ -100,8 +92,9 @@ static int iser_prepare_read_cmd(struct iscsi_task *task,
}
/* Register user buffer memory and initialize passive rdma
- * dto descriptor. Total data size is stored in
- * task-data[ISER_DIR_OUT].data_len
+ * dto descriptor. Data size is stored in
+ * task-data[ISER_DIR_OUT].data_len, Protection size
+ * is stored at task-prot[ISER_DIR_OUT].data_len
*/
static int
iser_prepare_write_cmd(struct iscsi_task *task,
@@ -135,14 +128,6 @@ iser_prepare_write_cmd(struct iscsi_task *task,
return err;
}
- if (edtl iser_task-data[ISER_DIR_OUT].data_len) {
- iser_err(Total data length: %ld, less than EDTL: %d,
-in WRITE cmd BHS itt: %d, conn: 0x%p\n,
-iser_task-data[ISER_DIR_OUT].data_len,
-edtl, task-itt, task-conn);
- return -EINVAL;
- }
-
err = device-iser_reg_rdma_mem(iser_task, ISER_DIR_OUT);
if (err != 0) {
iser_err(Failed to register write cmd RDMA mem\n);
@@ -417,11 +402,12 @@ int iser_send_command(struct iscsi_conn *conn,
if (scsi_prot_sg_count(sc)) {
prot_buf-buf = scsi_prot_sglist(sc);
prot_buf-size = scsi_prot_sg_count(sc);
- prot_buf-data_len = sc-prot_sdb-length;
+ prot_buf-data_len = data_buf-data_len
+ilog2(sc-device-sector_size) * 8;
}
if (hdr-flags ISCSI_FLAG_CMD_READ) {
- err = iser_prepare_read_cmd(task, edtl);
+ err = iser_prepare_read_cmd(task);
if (err)
goto send_command_error;
}
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 26dc005..3f46234 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -338,7 +338,7 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
struct iscsi_session *session = conn-session;
struct scsi_cmnd *sc = task-sc;
struct iscsi_scsi_req *hdr;
- unsigned hdrlength, cmd_len;
+ unsigned hdrlength, cmd_len, transfer_length;
itt_t itt;
int rc;
@@ -391,11 +391,11 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task
*task)
if (scsi_get_prot_op(sc) != SCSI_PROT_NORMAL)
task-protected = true;
+ transfer_length = scsi_transfer_length(sc);
+ hdr-data_length = cpu_to_be32(transfer_length);
if (sc-sc_data_direction == DMA_TO_DEVICE) {
- unsigned out_len = scsi_out(sc)-length;
struct iscsi_r2t_info *r2t = task-unsol_r2t;
- hdr-data_length = cpu_to_be32(out_len);
hdr-flags |= ISCSI_FLAG_CMD_WRITE;
/*
* Write counters:
@@ -414,18 +414,19 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task
*task)
[PATCH v2 0/3] Include protection information in transport header
At the SCSI transport level, there is no distinction between user data and protection information. Thus, iscsi header field expected data transfer length should include protection information. Patch #1 introduces scsi helper scsi_transfer_length which computes wire transfer byte count. Patch #2 modifies iscsi initiator to set correct wire transfer length in iscsi header data_length field (and modifies iser accordingly). Patch #3 modifies target core to re-calculate the pure data length in case of PI presence over the wire (and modifies loopback transport to align with other transports). Changes from v1: - scsi_cmnd: Rewrite scsi_transfer_length as MKP suggested - Target/sbc: re-calculate the data_length in case PI exists on the wire (instead od deacrease data -= prot) Changes from v0: - Introduce scsi helpers to compute correct transfer length in the presence of protection information. - Modify iscsi to set correct transfer length using scsi helpers - Modify loopback transport to set correct transfer length using scsi helpers Sagi Grimberg (3): scsi_cmnd: Introduce scsi_transfer_length helper libiscsi, iser: Adjust data_length to include protection information TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire drivers/infiniband/ulp/iser/iser_initiator.c | 34 +++-- drivers/scsi/libiscsi.c | 18 +++--- drivers/target/loopback/tcm_loop.c | 15 +-- drivers/target/target_core_sbc.c | 15 ++- include/scsi/scsi_cmnd.h | 17 + 5 files changed, 61 insertions(+), 38 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
In case protection information exists on the wire
scsi transports should include it in the transfer
byte count (even if protection information does not
exist in the host memory space). This helper will
compute the total transfer length from the scsi
command data length and protection attributes.
Signed-off-by: Sagi Grimberg sa...@mellanox.com
Signed-off-by: Martin K. Petersen martin.peter...@oracle.com
---
include/scsi/scsi_cmnd.h | 17 +
1 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index dd7c998..a100c6e 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -7,6 +7,7 @@
#include linux/types.h
#include linux/timer.h
#include linux/scatterlist.h
+#include scsi/scsi_device.h
struct Scsi_Host;
struct scsi_device;
@@ -306,4 +307,20 @@ static inline void set_driver_byte(struct scsi_cmnd *cmd,
char status)
cmd-result = (cmd-result 0x00ff) | (status 24);
}
+static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
+{
+ unsigned int xfer_len = blk_rq_bytes(scmd-request);
+ unsigned int prot_op = scsi_get_prot_op(scmd);
+ unsigned int sector_size = scmd-device-sector_size;
+
+ switch (prot_op) {
+ case SCSI_PROT_NORMAL:
+ case SCSI_PROT_WRITE_STRIP:
+ case SCSI_PROT_READ_INSERT:
+ return xfer_len;
+ }
+
+ return xfer_len + (xfer_len ilog2(sector_size)) * 8;
+}
+
#endif /* _SCSI_SCSI_CMND_H */
--
1.7.1
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire
In various areas of the code, it is assumed that
se_cmd-data_length describes pure data. In case
that protection information exists over the wire
(protect bits is are on) the target core re-calculates
the data length from the CDB and the backed device
block size (instead of each transport peeking in the cdb).
Modify loopback device to include protection information
in the transferred data length (like other scsi transports).
Signed-off-by: Sagi Grimberg sa...@mellanox.com
---
drivers/target/loopback/tcm_loop.c | 15 ---
drivers/target/target_core_sbc.c | 15 +--
2 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/drivers/target/loopback/tcm_loop.c
b/drivers/target/loopback/tcm_loop.c
index c886ad1..1f4c015 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -179,7 +179,7 @@ static void tcm_loop_submission_work(struct work_struct
*work)
struct tcm_loop_hba *tl_hba;
struct tcm_loop_tpg *tl_tpg;
struct scatterlist *sgl_bidi = NULL;
- u32 sgl_bidi_count = 0;
+ u32 sgl_bidi_count = 0, transfer_length;
int rc;
tl_hba = *(struct tcm_loop_hba **)shost_priv(sc-device-host);
@@ -213,12 +213,21 @@ static void tcm_loop_submission_work(struct work_struct
*work)
}
- if (!scsi_prot_sg_count(sc) scsi_get_prot_op(sc) != SCSI_PROT_NORMAL)
+ transfer_length = scsi_transfer_length(sc);
+ if (!scsi_prot_sg_count(sc)
+ scsi_get_prot_op(sc) != SCSI_PROT_NORMAL) {
se_cmd-prot_pto = true;
+ /*
+* loopback transport doesn't support
+* WRITE_GENERATE, READ_STRIP protection
+* information operations, go ahead unprotected.
+*/
+ transfer_length = scsi_bufflen(sc);
+ }
rc = target_submit_cmd_map_sgls(se_cmd, tl_nexus-se_sess, sc-cmnd,
tl_cmd-tl_sense_buf[0], tl_cmd-sc-device-lun,
- scsi_bufflen(sc), tcm_loop_sam_attr(sc),
+ transfer_length, tcm_loop_sam_attr(sc),
sc-sc_data_direction, 0,
scsi_sglist(sc), scsi_sg_count(sc),
sgl_bidi, sgl_bidi_count,
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index e022959..4b5716f 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -665,8 +665,19 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd,
unsigned char *cdb,
cmd-prot_type = dev-dev_attrib.pi_prot_type;
cmd-prot_length = dev-prot_length * sectors;
- pr_debug(%s: prot_type=%d, prot_length=%d prot_op=%d prot_checks=%d\n,
-__func__, cmd-prot_type, cmd-prot_length,
+
+ /**
+* In case protection information exists over the wire
+* we modify command data length to describe pure data.
+* The actual transfer length is data length + protection
+* length
+**/
+ if (protect)
+ cmd-data_length = sectors * dev-dev_attrib.block_size;
+
+ pr_debug(%s: prot_type=%d, data_length=%d, prot_length=%d
+prot_op=%d prot_checks=%d\n,
+__func__, cmd-prot_type, cmd-data_length, cmd-prot_length,
cmd-prot_op, cmd-prot_checks);
return true;
--
1.7.1
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] scsi patch queue tree
After this first pull for the 3.16 merge window it seems like this worked out fairly well - we got a large number of patches in, and all reviewed by a second pair of eyes. How should we go on from this? The drivers-for-3.16-2 branch, which had the late lpfs and hpsa updates didn't make it into the pull request for Linus, do you intend to skip them for this window? If not do you still want to have another branch for the other pending smaller updates? -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] notify block layer when using temporary change to cache_type
On Tue, Jun 03, 2014 at 05:37:30PM +0800, Vaughan Cao wrote: This is a fix for commit: 39c60a0948cc06139e2fbfe084f83cb7e7deae3b sd: fix array cache flushing bug causing performance problems We must notify the block layer via q-flush_flags after temporary change the cache_type to write through. If not, SYNCHRONIZE CACHE command will still be generated. This patch factors out a helper that can be called from sd_revalidate_disk and cache_type_store. Signed-off-by: Vaughan Cao vaughan@oracle.com Looks good, Reviewed-by: Christoph Hellwig h...@lst.de -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sd: bad return code of init_sd
Looks good, Reviewed-by: Christoph Hellwig h...@lst.de -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 linux-scsi-ml] linux-firmware: qla2xxx: Update ql2{4,5}00_fw.bin to version 7.03.00
On Tue, Jun 10, 2014 at 12:00:39PM +0200, Xose Vazquez Perez wrote: resent to linux-scsi-ml, without the binary blob(244K). Firmwares were taken from http://ldriver.qlogic.com/firmware/ Looks good, while there's nothing really to review for a firmware blob update, can you get me a review or second signoff instead of all the Ccs? Does this need to go with the updates we merged for 3.16, or can it wait for 3.17? -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] [SCSI] Don't build AdvanSys on ARM
Looks good, Reviewed-by: Christoph Hellwig h...@lst.de -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] [SCSI] pas16: don't call free_dma()
On Thu, Jun 05, 2014 at 11:29:47PM +0200, Arnd Bergmann wrote: The pas16 scsi driver does not use DMA, and the call to free_dma() in its exit function seems to have been copied incorrectly from another driver but never caused trouble. One case where it gets in the way is randconfig builds on ARM, which depending on the configuration does not provide a free_dma() function, causing this build error: Looks good, Reviewed-by: Christoph Hellwig h...@lst.de -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] [SCSI] qlogicfas: don't call free_dma()
On Thu, Jun 05, 2014 at 11:29:48PM +0200, Arnd Bergmann wrote: The qlogicfas scsi driver does not use DMA, and the call to free_dma() in its exit function seems to have been copied incorrectly from another driver but never caused trouble. One case where it gets in the way is randconfig builds on ARM, which depending on the configuration does not provide a free_dma() function, causing this build error: Looks good, Reviewed-by: Christoph Hellwig h...@lst.de -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] [SCSI] NCR53c406a: don't call free_dma() by default
On Thu, Jun 05, 2014 at 11:29:49PM +0200, Arnd Bergmann wrote: The NCR53c406a scsi driver normally does not use DMA, unless the USE_PIO macro is disabled by modifying the source code. The call to free_dma() for some reason uses #ifdef USE_DMA, which does not do the right thing, since USE_DMA is defined as a boolean that is either 0 or 1, but always present. One case where it gets in the way is randconfig builds on ARM, which depending on the configuration does not provide a free_dma() function, causing this build error: Looks good, Reviewed-by: Christoph Hellwig h...@lst.de -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/6] virtio-scsi: avoid cancelling uninitialized work items
Can I get a second review on this one from anyone?
On Wed, Jun 04, 2014 at 01:34:56PM +0200, Paolo Bonzini wrote:
Calling the workqueue interface on uninitialized work items isn't a
good idea even if they're zeroed. It's not failing catastrophically only
through happy accidents.
Signed-off-by: Paolo Bonzini pbonz...@redhat.com
---
drivers/scsi/virtio_scsi.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index f0b4cdbfceb0..d66c4ee2c774 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -253,6 +253,8 @@ static void virtscsi_ctrl_done(struct virtqueue *vq)
virtscsi_vq_done(vscsi, vscsi-ctrl_vq, virtscsi_complete_free);
};
+static void virtscsi_handle_event(struct work_struct *work);
+
static int virtscsi_kick_event(struct virtio_scsi *vscsi,
struct virtio_scsi_event_node *event_node)
{
@@ -260,6 +262,7 @@ static int virtscsi_kick_event(struct virtio_scsi *vscsi,
struct scatterlist sg;
unsigned long flags;
+ INIT_WORK(event_node-work, virtscsi_handle_event);
sg_init_one(sg, event_node-event, sizeof(struct virtio_scsi_event));
spin_lock_irqsave(vscsi-event_vq.vq_lock, flags);
@@ -377,7 +380,6 @@ static void virtscsi_complete_event(struct virtio_scsi
*vscsi, void *buf)
{
struct virtio_scsi_event_node *event_node = buf;
- INIT_WORK(event_node-work, virtscsi_handle_event);
schedule_work(event_node-work);
}
--
1.8.3.1
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
---end quoted text---
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] scsi: Handle power-on reset unit attention
On Thu, Jun 05, 2014 at 09:26:43AM +0200, Hannes Reinecke wrote: As per SAM there is a status precedence, with any sense code 29/XX taking second place just after an ACA ACTIVE status. Additionally, each target might prefer to not queue any unit attention conditions but just report one. Due to the above this will be that one with the highest precedence. This results in the sense code 29/XX effectively overwriting any other unit attention. Hence we should report the power-on reset to userland so that it can take appropriate action. Signed-off-by: Hannes Reinecke h...@suse.de Looks good, Reviewed-by: Christoph Hellwig h...@lst.de -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: Re: [PATCH v3 3/6] virtio-scsi: avoid cancelling uninitialized work items
On Wed, Jun 11, 2014 at 02:53:46PM +0200, Paolo Bonzini wrote:
Messaggio originale
From: Christoph Hellwig h...@infradead.org
To: Paolo Bonzini pbonz...@redhat.com
Cc: linux-ker...@vger.kernel.org, linux-scsi@vger.kernel.org, h...@lst.de,
jbottom...@parallels.com, venkate...@google.com
Subject: Re: [PATCH v3 3/6] virtio-scsi: avoid cancelling uninitialized work
items
Message-ID: 20140611124731.ga16...@infradead.org
In-Reply-To: 1401881699-1456-4-git-send-email-pbonz...@redhat.com
Can I get a second review on this one from anyone?
Reviewed-by: Stefan Hajnoczi stefa...@redhat.com
On Wed, Jun 04, 2014 at 01:34:56PM +0200, Paolo Bonzini wrote:
Calling the workqueue interface on uninitialized work items isn't a
good idea even if they're zeroed. It's not failing catastrophically only
through happy accidents.
Signed-off-by: Paolo Bonzini pbonz...@redhat.com
---
drivers/scsi/virtio_scsi.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index f0b4cdbfceb0..d66c4ee2c774 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -253,6 +253,8 @@ static void virtscsi_ctrl_done(struct virtqueue *vq)
virtscsi_vq_done(vscsi, vscsi-ctrl_vq, virtscsi_complete_free);
};
+static void virtscsi_handle_event(struct work_struct *work);
+
static int virtscsi_kick_event(struct virtio_scsi *vscsi,
struct virtio_scsi_event_node *event_node)
{
@@ -260,6 +262,7 @@ static int virtscsi_kick_event(struct virtio_scsi
*vscsi,
struct scatterlist sg;
unsigned long flags;
+ INIT_WORK(event_node-work, virtscsi_handle_event);
sg_init_one(sg, event_node-event, sizeof(struct virtio_scsi_event));
spin_lock_irqsave(vscsi-event_vq.vq_lock, flags);
@@ -377,7 +380,6 @@ static void virtscsi_complete_event(struct virtio_scsi
*vscsi, void *buf)
{
struct virtio_scsi_event_node *event_node = buf;
- INIT_WORK(event_node-work, virtscsi_handle_event);
schedule_work(event_node-work);
}
--
1.8.3.1
pgpDoUAVNFzxl.pgp
Description: PGP signature
Re: SCSI eats error from flush failure during hot plug
On Mon, Jun 09, 2014 at 10:29:06AM -0700, James Bottomley wrote: I'll do it as a bug fix, but I do need Jens to make sure nothing else breaks first. Best I can tell, the state model for compound commands like flush doesn't expect us to change the request type ... nothing puts it back to REQ_TYPE_FS. In your case, the flush is the last command sent, so there's no problem ... I just worry we will get an obscure problem later on from something that does a BLOCK_PC prepared first command. Yes, I don't think resetting cmd_type is a good idea. I'd much rather see a special case for rq-cmd_flags REQ_FLUSH in the completion handler - we already treat it special during setup anyway. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] vhost: infrastructure changes for 3.16
Hi Linus, Please pull the following. Please note this needs to be merged before merging target-pending PULL which Nicholas will be sending out shortly. Thanks! The following changes since commit 1860e379875dfe7271c649058aeddffe5afd9d0d: Linux 3.15 (2014-06-08 11:19:54 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus for you to fetch changes up to 47283bef7ed356629467d1fac61687756e48f254: vhost: move memory pointer to VQs (2014-06-09 16:21:07 +0300) vhost: infrastructure changes for 3.16 This reworks vhost core dropping unnecessary RCU uses in favor of VQ mutexes which are used on fast path anyway. This fixes worst-case latency for users which change the memory mappings a lot. Memory allocation for vhost-net now supports fallback on vmalloc (same as for vhost-scsi) this makes it possible to create the device on systems where memory is very fragmented, with slightly lower performance. Signed-off-by: Michael S. Tsirkin m...@redhat.com Michael S. Tsirkin (4): vhost-net: extend device allocation to vmalloc vhost: replace rcu with mutex vhost: move acked_features to VQs vhost: move memory pointer to VQs drivers/vhost/vhost.h | 19 -- drivers/vhost/net.c | 35 --- drivers/vhost/scsi.c | 26 -- drivers/vhost/test.c | 11 +++--- drivers/vhost/vhost.c | 97 ++- 5 files changed, 101 insertions(+), 87 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 0/6] Support 64-bit LUNs
On 14-06-11 02:53 AM, Bart Van Assche wrote:
On 06/11/14 08:34, Hannes Reinecke wrote:
On 06/10/2014 07:58 PM, Bart Van Assche wrote:
Many SCSI LLD's use int_to_scsilun() in the hot path (queuecommand()).
This patch series makes the int_to_scsilun() function slightly more
expensive. Has it been considered to cache the result of int_to_scsilun()
such that LLD's can copy the cached int_to_scsilun() result instead of
having to call int_to_scsilun() in the queuecommand() function ?
Something like the (untested) patch below might be sufficient:
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index e02b3aa..9e50d78 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -244,6 +244,7 @@ static struct scsi_device *scsi_alloc_sdev(struct
scsi_target *starget,
sdev-queue_ramp_up_period = SCSI_DEFAULT_RAMP_UP_PERIOD;
sdev-id = starget-id;
sdev-lun = lun;
+int_to_scsilun(lun, sdev-scsi_lun);
sdev-channel = starget-channel;
sdev-sdev_state = SDEV_CREATED;
INIT_LIST_HEAD(sdev-siblings);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 5853c91..48ea68e 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -100,6 +100,8 @@ struct scsi_device {
unsigned int id, lun, channel;
+struct scsi_lun scsi_lun;/* int_to_scsilun(lun) */
+
unsigned int manufacturer;/* Manufacturer of device, for using
* vendor-specific cmd's */
unsigned sector_size;/* size in bytes */
Thanks,
Hmm. No, so far it hasn't been considered. Maybe it's even worthwhile to
move the situation around, ie using primarily the 64-bit LUN and only
convert it into an integer if so requested.
But yeah, it's definitely something we should look into.
Maybe _after_ the patchset is in?
That's fine with me. I have one more question about this patch series
though: if you mention 64-bit LUNs, are you referring to multi-level
LUNs only or also to so-called extended LUNs ? I think the byte
reordering done by scsilun_to_int() is fine for multi-level LUNs but
unnatural for extended LUNs. As an example, in SAM-5 the format for
eight byte extended LUNs is defined as follows (paragraph 4.7.7.5.3):
byte 0: address method (3), length (2) and extended address method (2)
bytes 1..7: long extended flat space LUN with the MSB in byte 1 and LSB
in byte 7.
Today scsilun_to_int() does not preserve the MSB..LSB byte order for
extended LUNs. I think if we want to preserve the byte order in
scsilun_to_int() that that function will have to be made dependent on
the LUN addressing method. That would make scsilun_to_int() more
complex. Hence the proposal to cache the SCSI LUN such that the
queuecommand() functions are not slowed down by a call into
int_to_scsilun().
The only constant with LUNs is that T10 will keep tinkering with
them. The original LUNs were 3 bits long embedded in the cdb.
Today we have 65 bit LUNs (the 64 you have been looking at
and LU_CONG in the INQUIRY response). Decoding LUNs is best
avoided if possible.
int_to_scsilun() was a hack that made 16 bit LUNs look half
reasonable as integers. Beyond 16 bits, it just looks like
a random number generator. As long as the int that is
produced can be mapped to a T10 LUN and vice versa, it doesn't
matter.
Doug Gilbert
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] scsi_scan: Send TEST UNIT READY to the LUN before scanning
On Thu, Jun 05, 2014 at 09:26:42AM +0200, Hannes Reinecke wrote: REPORT_LUN_SCAN does not report any outstanding unit attention condition as per SAM. However, the target might not be fully initialized at that time, so we might end up getting a default entry (or even a partially filled one). But as we're not able to process the REPORT LUN DATA HAS CHANGED unit attention correctly we'll be missing out some LUNs during startup. So it's better to send a TEST UNIT READY for modern implementations and wait until the unit attention condition goes away. Signed-off-by: Hannes Reinecke h...@suse.de --- drivers/scsi/scsi_scan.c | 86 1 file changed, 73 insertions(+), 13 deletions(-) diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index e02b3aa..a8e59c3 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -123,6 +123,13 @@ MODULE_PARM_DESC(inq_timeout, Timeout (in seconds) waiting for devices to answer INQUIRY. Default is 20. Some devices may need more; most need less.); +static unsigned int scsi_scan_timeout = SCSI_TIMEOUT/HZ + 58; + +module_param_named(scan_timeout, scsi_scan_timeout, uint, S_IRUGO|S_IWUSR); +MODULE_PARM_DESC(scan_timeout, + Timeout (in seconds) waiting for devices to become ready + after INQUIRY. Default is 60.); Should this be called tur_timeout, similar to the inq_timeout parameter? Otherwise looks good to me, Reviewed-by: Christoph Hellwig h...@lst.de -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] scsi: Handle power-on reset unit attention
On Thu, 2014-06-05 at 09:26 +0200, Hannes Reinecke wrote:
As per SAM there is a status precedence, with any sense code 29/XX
taking second place just after an ACA ACTIVE status.
Additionally, each target might prefer to not queue any unit
attention conditions but just report one.
Due to the above this will be that one with the highest precedence.
This results in the sense code 29/XX effectively overwriting any
other unit attention.
Hence we should report the power-on reset to userland so that
it can take appropriate action.
Signed-off-by: Hannes Reinecke h...@suse.de
---
drivers/scsi/scsi_error.c | 6 ++
drivers/scsi/scsi_lib.c| 4
include/scsi/scsi_device.h | 3 ++-
3 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 47a1ffc..65ed333 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -420,6 +420,12 @@ static void scsi_report_sense(struct scsi_device *sdev,
threshold.\n);
}
+ if (sshdr-asc == 0x29) {
+ evt_type = SDEV_EVT_POWER_ON_RESET_OCCURRED;
+ sdev_printk(KERN_WARNING, sdev,
+ Power-on or device reset occurred\n);
+ }
+
if (sshdr-asc == 0x2a sshdr-ascq == 0x01) {
evt_type = SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED;
sdev_printk(KERN_WARNING, sdev,
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9f841df..ee158c1 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2183,6 +2183,9 @@ static void scsi_evt_emit(struct scsi_device *sdev,
struct scsi_event *evt)
case SDEV_EVT_LUN_CHANGE_REPORTED:
envp[idx++] = SDEV_UA=REPORTED_LUNS_DATA_HAS_CHANGED;
break;
+ case SDEV_EVT_POWER_ON_RESET_OCCURRED:
+ envp[idx++] = SDEV_UA=POWER_ON_RESET_OCCURRED;
+ break;
default:
/* do nothing */
break;
@@ -2286,6 +2289,7 @@ struct scsi_event *sdev_evt_alloc(enum
scsi_device_event evt_type,
case SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED:
case SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED:
case SDEV_EVT_LUN_CHANGE_REPORTED:
+ case SDEV_EVT_POWER_ON_RESET_OCCURRED:
default:
/* do nothing */
break;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 5853c91..7b9a886 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -57,9 +57,10 @@ enum scsi_device_event {
SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED, /* 38 07 UA reported */
SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED,/* 2A 01 UA reported */
SDEV_EVT_LUN_CHANGE_REPORTED, /* 3F 0E UA reported */
+ SDEV_EVT_POWER_ON_RESET_OCCURRED, /* 29 00 UA reported */
SDEV_EVT_FIRST = SDEV_EVT_MEDIA_CHANGE,
- SDEV_EVT_LAST = SDEV_EVT_LUN_CHANGE_REPORTED,
+ SDEV_EVT_LAST = SDEV_EVT_POWER_ON_RESET_OCCURRED,
SDEV_EVT_MAXBITS= SDEV_EVT_LAST + 1
};
This looks fine to me. We might want to figure out a way
to report the ASCQ in an environment variable on this uevent,
since there are multiple sub-cases:
29 00 D POWER ON, RESET, OR BUS DEVICE RESET OCCURRED
29 01 D POWER ON OCCURRED
29 02 D SCSI BUS RESET OCCURRED
29 03 D BUS DEVICE RESET FUNCTION OCCURRED
29 04 D DEVICE INTERNAL RESET
29 05 D TRANSCEIVER MODE CHANGED TO SINGLE-ENDED
29 06 D TRANSCEIVER MODE CHANGED TO LVD
29 07 D I_T NEXUS LOSS OCCURRED
...but we could add that in a subsequent patch.
A related problem is that when this UA is received, the
device may have changed some of its attributes, so some
of the information that the mid-layer caches may be stale.
The udev rule handling this event should probably rescan
the device.
Reviewed-by: Ewan D. Milne emi...@redhat.com
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] scsi patch queue tree
On Wed, 2014-06-11 at 05:01 -0700, Christoph Hellwig wrote: After this first pull for the 3.16 merge window it seems like this worked out fairly well - we got a large number of patches in, and all reviewed by a second pair of eyes. How should we go on from this? The drivers-for-3.16-2 branch, which had the late lpfs and hpsa updates didn't make it into the pull request for Linus, do you intend to skip them for this window? If not do you still want to have another branch for the other pending smaller updates? No, I was waiting to check if there was any reason to have them split, but I think we've scope today or tomorrow. The only other outstanding thing is the fsync bug fix, which is waiting Jens' investigation of the block issues it may cause, but I'm inclined to send it anyway and fix up block later if there's a problem. James -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] scsi_scan: Send TEST UNIT READY to the LUN before scanning
On Thu, 2014-06-05 at 09:26 +0200, Hannes Reinecke wrote: REPORT_LUN_SCAN does not report any outstanding unit attention condition as per SAM. However, the target might not be fully initialized at that time, so we might end up getting a default entry (or even a partially filled one). But as we're not able to process the REPORT LUN DATA HAS CHANGED unit attention correctly we'll be missing out some LUNs during startup. So it's better to send a TEST UNIT READY for modern implementations and wait until the unit attention condition goes away. Are you sure this is a good idea: we just spent ages tuning SCSI init so we don't slow systems down. This patch, in the event the array is having a power on problem, takes us right back to waiting for init again ... basically the busy wait in scsi_test_lun. Since the array should send us a UA anyway when it's got itself sorted out, what's wrong with just processing the report luns data has changed condition? James -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] scsi patch queue tree
On Wed, Jun 11, 2014 at 07:17:34AM -0700, James Bottomley wrote: No, I was waiting to check if there was any reason to have them split, but I think we've scope today or tomorrow. The only other outstanding thing is the fsync bug fix, which is waiting Jens' investigation of the block issues it may cause, but I'm inclined to send it anyway and fix up block later if there's a problem. There's various fixes marked for stable from Paolos virtio_scsi update, which he asked to be considered for 3.16, including a core one: [PATCH v3 4/6] scsi_error: fix invalid setting of host byte also [PATCH] sd: bad return code of init_sd seems like another candidate for 3.16. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] scsi_scan: Send TEST UNIT READY to the LUN before scanning
On 06/11/2014 04:24 PM, James Bottomley wrote: On Thu, 2014-06-05 at 09:26 +0200, Hannes Reinecke wrote: REPORT_LUN_SCAN does not report any outstanding unit attention condition as per SAM. However, the target might not be fully initialized at that time, so we might end up getting a default entry (or even a partially filled one). But as we're not able to process the REPORT LUN DATA HAS CHANGED unit attention correctly we'll be missing out some LUNs during startup. So it's better to send a TEST UNIT READY for modern implementations and wait until the unit attention condition goes away. Are you sure this is a good idea: we just spent ages tuning SCSI init so we don't slow systems down. This patch, in the event the array is having a power on problem, takes us right back to waiting for init again ... basically the busy wait in scsi_test_lun. Since the array should send us a UA anyway when it's got itself sorted out, what's wrong with just processing the report luns data has changed condition? Because we can't. _If_ we were attempting this we'd run into several issues: a) Boot will fail, as REPORT LUNs will return 0 LUNs (or just LUN 0). So the scanning code will assume everything's fine. Booting will continue, only to figure out that no LUNs are present. As there is _no_ indication that REPORT LUNs should indeed have returned an error (only it can't due to SAM) we wouldn't even now that there _is_ an issue. (In fact, that's what triggered the patchset in the first place.) b) Even _if_ we're able so somehow recover from that we will have to rescan the host and any attached devices. The only way to do this currently is to _remove_ all devices from that host and then do a full rescan. Trying this with any devices which are already part of some complex setup will become ... interesting. So the easy way out here is indeed just to send a TEST UNIT READY. And as we're checking for a reasonably SCSI compliance we should be catching most of the oddballs. Cheers, Hannes -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] scsi_scan: Send TEST UNIT READY to the LUN before scanning
On Wed, 2014-06-11 at 16:33 +0200, Hannes Reinecke wrote: On 06/11/2014 04:24 PM, James Bottomley wrote: On Thu, 2014-06-05 at 09:26 +0200, Hannes Reinecke wrote: REPORT_LUN_SCAN does not report any outstanding unit attention condition as per SAM. However, the target might not be fully initialized at that time, so we might end up getting a default entry (or even a partially filled one). But as we're not able to process the REPORT LUN DATA HAS CHANGED unit attention correctly we'll be missing out some LUNs during startup. So it's better to send a TEST UNIT READY for modern implementations and wait until the unit attention condition goes away. Are you sure this is a good idea: we just spent ages tuning SCSI init so we don't slow systems down. This patch, in the event the array is having a power on problem, takes us right back to waiting for init again ... basically the busy wait in scsi_test_lun. Since the array should send us a UA anyway when it's got itself sorted out, what's wrong with just processing the report luns data has changed condition? Because we can't. _If_ we were attempting this we'd run into several issues: a) Boot will fail, as REPORT LUNs will return 0 LUNs (or just LUN 0). So the scanning code will assume everything's fine. Booting will continue, only to figure out that no LUNs are present. As there is _no_ indication that REPORT LUNs should indeed have returned an error (only it can't due to SAM) we wouldn't even now that there _is_ an issue. (In fact, that's what triggered the patchset in the first place.) b) Even _if_ we're able so somehow recover from that we will have to rescan the host and any attached devices. The only way to do this currently is to _remove_ all devices from that host and then do a full rescan. Trying this with any devices which are already part of some complex setup will become ... interesting. OK, go back to first principles and tell us what the actual problem is, with traces and details. Is this some weird SCSI-3 device with a single LUN that's screwing up report luns ... in which case we can just blacklist it. Or is it boot from an array? So the easy way out here is indeed just to send a TEST UNIT READY. And as we're checking for a reasonably SCSI compliance we should be catching most of the oddballs. I don't object hugely to TUR ... except it binds us to spin up because most devices will respond not ready. I do object to busy waiting in the init thread until we get the right answer. James -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] sg: add SG_FLAG_Q_AT_TAIL flag
On Wed, 2014-06-04 at 10:58 -0400, Douglas Gilbert wrote: When the SG_IO ioctl was copied into the block layer and later into the bsg driver, subtle differences emerged. One difference is the way injected commands are queued through the block layer (i.e. this is not SCSI device queueing nor SATA NCQ). Summarizing: - SG_IO in the block layer: blk_exec*(at_head=false) - sg SG_IO: at_head=true - bsg SG_IO: at_head=true Some time ago Boaz Harrosh introduced a sg v4 flag called BSG_FLAG_Q_AT_TAIL to override the bsg driver default. This patch does the equivalent for the sg driver. ChangeLog: Introduce SG_FLAG_Q_AT_TAIL flag to cause commands to be injected into the block layer with at_head=false. Changes since v1: Make guard condition (only take sg v3 interface or later invocations) clearer. Signed-off-by: Douglas Gilbert dgilb...@interlog.com Looks good. Reviewed-by: Ewan D. Milne emi...@redhat.com -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/14] block: Get rid of bdev_integrity_enabled()
On Wed, May 28, 2014 at 11:28:35PM -0400, Martin K. Petersen wrote: bdev_integrity_enabled() is only used by bio_integrity_enabled(). Combine these two functions. Signed-off-by: Martin K. Petersen martin.peter...@oracle.com Looks good, Reviewed-by: Christoph Hellwig h...@lst.de -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/14] block: Replace bi_integrity with bi_special
On Wed, May 28, 2014 at 11:28:36PM -0400, Martin K. Petersen wrote: For commands like REQ_COPY we need a way to pass extra information along with each bio. Like integrity metadata this information must be available at the bottom of the stack so bi_private does not suffice. Rename the existing bi_integrity field to bi_special and make it a union so we can have different bio extensions for each class of command. We previously used bi_integrity != NULL as a way to identify whether a bio had integrity metadata or not. Introduce a REQ_INTEGRITY to be the indicator now that bi_special can contain different things. In addition, bio_integrity(bio) will now return a pointer to the integrity payload (when applicable). Instead of having a union of pointer just make it a void pointer. I also think special is a terribly generic name, but I don't really have a better idea at hand. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/14] block: Deprecate integrity tagging functions
On Wed, May 28, 2014 at 11:28:37PM -0400, Martin K. Petersen wrote: None of the filesystems appear interested in using the integrity tagging feature. Potentially because very few storage devices actually permit using the application tag space. Deprecate the tagging functions. This patch doesn't just deprecate them but outright removes them. I'm fine with that, but the patch description needs an update. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/14] block: Remove bip_buf
On Wed, May 28, 2014 at 11:28:38PM -0400, Martin K. Petersen wrote: bip_buf is not really needed so we can remove it. Signed-off-by: Martin K. Petersen martin.peter...@oracle.com Looks good, Reviewed-by: Christoph Hellwig h...@lst.de -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/14] block: Deprecate the use of the term sector in the context of block integrity
On Wed, May 28, 2014 at 11:28:39PM -0400, Martin K. Petersen wrote: The protection interval is not necessarily tied to the logical block size of a block device. Stop using the terms sector and sectors. This does more than just renaming symbols, so it needs a better description or even better splitting into separate patches. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/14] block: Add prefix to block integrity profile flags
On Wed, May 28, 2014 at 11:28:41PM -0400, Martin K. Petersen wrote: Add a BLK_ prefix to the integrity profile flags. Also rename the flags to be more consistent with the generate/verify terminology in the rest of the integrity code. Signed-off-by: Martin K. Petersen martin.peter...@oracle.com Looks good, Reviewed-by: Christoph Hellwig h...@lst.de -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/14] block: Relocate integrity flags
On Wed, May 28, 2014 at 11:28:43PM -0400, Martin K. Petersen wrote: Move flags affecting the integrity code out of the bio bi_flags and into the block integrity payload. It seems like bip is guaranteed to be non-NULL in all callers of the getters and setters. I'd recommend just dropping them and opencode the flags manipulation. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/14] block: Integrity checksum flag
On Wed, May 28, 2014 at 11:28:44PM -0400, Martin K. Petersen wrote: Make the choice of checksum a per-I/O property by introducing a flag that can be inspected by the SCSI layer. Why? -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/14] block: Don't merge requests if integrity flags differ
On Wed, May 28, 2014 at 11:28:45PM -0400, Martin K. Petersen wrote: We'd occasionally merge requests with conflicting integrity flags. Introduce a merge helper which checks that the requests have compatible integrity payloads. Looks good, Reviewed-by: Christoph Hellwig h...@lst.de -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/14] block: Add specific data integrity errors
On Wed, May 28, 2014 at 11:28:46PM -0400, Martin K. Petersen wrote: Introduce a set of error codes that can be used by the block integrity subsystem to signal which class of error was encountered by either the I/O controller or the storage device. Signed-off-by: Martin K. Petersen martin.peter...@oracle.com New error code should be run past linux-kernel and linux-api. I'd also love to see something catching these so that they don't leak to userspace. In fact I'd really prefer to not overload the errno space with something so block specific if possible - we use very few errnos in the bio/request errors, and cleaning them up to use a block-specific error type would be the best solution. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/14] block: Add a disk flag to block integrity profile
On Wed, May 28, 2014 at 11:28:42PM -0400, Martin K. Petersen wrote: So far we have relied on the app tag size to determine whether a disk has been formatted with T10 protection information or not. However, not all target devices provide application tag storage. Add a flag to the block integrity profile that indicates whether the disk has been formatted with protection information. I'm totally confused on why the sysfs file and flag are named 'disk'. What does it stand for given that we already have a very established use of the term 'disk' in block I/O land.. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/14] lib: Add T10 Protection Information functions
static struct blk_integrity dif_type1_integrity_crc = {
.name = T10-DIF-TYPE1-CRC,
- .generate_fn= sd_dif_type1_generate_crc,
- .verify_fn = sd_dif_type1_verify_crc,
- .tuple_size = sizeof(struct sd_dif_tuple),
+ .generate_fn= t10_pi_type1_generate_crc,
+ .verify_fn = t10_pi_type1_verify_crc,
+ .tuple_size = sizeof(struct t10_pi_tuple),
.tag_size = 0,
};
Shouldn't the whole profile defintions move to the generic
code as well? Maybe the code also should live in block/ and not lib/.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: add debug flag parameter for SCSI tape driver
On 11.6.2014, at 2.48, Laurence Oberman lober...@redhat.com wrote: Hello Take 2 of this patch, changed module description and subject line. This patch adds a debug_flag parameter that can be set on module load, and allows the DEBUG facility without a module recompile. Usage: mpdprobe st debug_flag=1 Signed-off-by: Laurence Oberman lober...@redhat.com What is wrong with the existing methods to control debugging? You can enable and disable debugging for each device with ioctl() (as described in the driver documentation). You can use mt-st to do this from command line. Your patch just allows one to change the default for all devices. The real problem may be that the distributions don’t compile the debugging code into the drivets but your patch does not change this. Kai -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: add debug flag parameter for SCSI tape driver
Kai, Thank you for considering this. With #define DEBUG 0 We still include #define DEB(a) #define DEBC(a) With the debug_flag we then provide the needed debug I am looking for at module load time. But I agree that it enables it for all devices and that may not be optimal I don't change the default, I just allow the parameter to control it. In the last few issues I have been working I have had to recompile and provide the st module to get what I needed captured for debugging so I decided to try the patch submission. Thank You Laurence - Original Message - From: Kai Mäkisara (Kolumbus) kai.makis...@kolumbus.fi To: Laurence Oberman lober...@redhat.com Cc: linux-scsi@vger.kernel.org Sent: Wednesday, June 11, 2014 2:03:15 PM Subject: Re: [PATCH]: add debug flag parameter for SCSI tape driver On 11.6.2014, at 2.48, Laurence Oberman lober...@redhat.com wrote: Hello Take 2 of this patch, changed module description and subject line. This patch adds a debug_flag parameter that can be set on module load, and allows the DEBUG facility without a module recompile. Usage: mpdprobe st debug_flag=1 Signed-off-by: Laurence Oberman lober...@redhat.com What is wrong with the existing methods to control debugging? You can enable and disable debugging for each device with ioctl() (as described in the driver documentation). You can use mt-st to do this from command line. Your patch just allows one to change the default for all devices. The real problem may be that the distributions don’t compile the debugging code into the drivets but your patch does not change this. Kai -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] drivers/message/i2o/i2o_block.c: remove unnecessary test on unsigned value
unsigned value is never 0 Cc: linux-scsi@vger.kernel.org Signed-off-by: Fabian Frederick f...@skynet.be --- drivers/message/i2o/i2o_block.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/message/i2o/i2o_block.c b/drivers/message/i2o/i2o_block.c index 6fc3866..2b684ec 100644 --- a/drivers/message/i2o/i2o_block.c +++ b/drivers/message/i2o/i2o_block.c @@ -671,7 +671,7 @@ static int i2o_block_ioctl(struct block_device *bdev, fmode_t mode, break; case BLKI2OSRSTRAT: ret = -EINVAL; - if (arg 0 || arg CACHE_SMARTFETCH) + if (arg CACHE_SMARTFETCH) break; dev-rcache = arg; ret = 0; -- 1.8.4.5 -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: add debug flag parameter for SCSI tape driver
This thread leads me to ask: Do people use ANSI-formatted tapes any more? That is, tape volumes with miltiple files, header and trailer labels, etc.? Dale -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: add debug flag parameter for SCSI tape driver
Kai, Its likely not worth doing this, I cross checked and indeed many distros have this compiled out. So lets leave it as is. Thanks Laurence - Original Message - From: Laurence Oberman lober...@redhat.com To: Kai Mäkisara (Kolumbus) kai.makis...@kolumbus.fi Cc: linux-scsi@vger.kernel.org Sent: Wednesday, June 11, 2014 2:24:25 PM Subject: Re: [PATCH]: add debug flag parameter for SCSI tape driver Kai, Thank you for considering this. With #define DEBUG 0 We still include #define DEB(a) #define DEBC(a) With the debug_flag we then provide the needed debug I am looking for at module load time. But I agree that it enables it for all devices and that may not be optimal I don't change the default, I just allow the parameter to control it. In the last few issues I have been working I have had to recompile and provide the st module to get what I needed captured for debugging so I decided to try the patch submission. Thank You Laurence - Original Message - From: Kai Mäkisara (Kolumbus) kai.makis...@kolumbus.fi To: Laurence Oberman lober...@redhat.com Cc: linux-scsi@vger.kernel.org Sent: Wednesday, June 11, 2014 2:03:15 PM Subject: Re: [PATCH]: add debug flag parameter for SCSI tape driver On 11.6.2014, at 2.48, Laurence Oberman lober...@redhat.com wrote: Hello Take 2 of this patch, changed module description and subject line. This patch adds a debug_flag parameter that can be set on module load, and allows the DEBUG facility without a module recompile. Usage: mpdprobe st debug_flag=1 Signed-off-by: Laurence Oberman lober...@redhat.com What is wrong with the existing methods to control debugging? You can enable and disable debugging for each device with ioctl() (as described in the driver documentation). You can use mt-st to do this from command line. Your patch just allows one to change the default for all devices. The real problem may be that the distributions don’t compile the debugging code into the drivets but your patch does not change this. Kai -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] sg: add SG_FLAG_Q_AT_TAIL flag
On 06/04/2014 09:58 AM, Douglas Gilbert wrote: When the SG_IO ioctl was copied into the block layer and later into the bsg driver, subtle differences emerged. One difference is the way injected commands are queued through the block layer (i.e. this is not SCSI device queueing nor SATA NCQ). Summarizing: - SG_IO in the block layer: blk_exec*(at_head=false) - sg SG_IO: at_head=true - bsg SG_IO: at_head=true Some time ago Boaz Harrosh introduced a sg v4 flag called BSG_FLAG_Q_AT_TAIL to override the bsg driver default. This patch does the equivalent for the sg driver. ChangeLog: Introduce SG_FLAG_Q_AT_TAIL flag to cause commands to be injected into the block layer with at_head=false. Changes since v1: Make guard condition (only take sg v3 interface or later invocations) clearer. Signed-off-by: Douglas Gilbert dgilb...@interlog.com Looks ok to me. Reviewed-by: Mike Christie micha...@cs.wisc.edu -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire
On Wed, 2014-06-11 at 10:24 +0300, Sagi Grimberg wrote: On 6/11/2014 12:17 AM, Quinn Tran wrote: SNIP QT Instead of using existing value within cmd-data_length, can we calculated data_length based on secstors blocksize. cmd-data_length = sectors * dev-dev_attrib.block_size; We can do that I suppose... Although it seems weird that the core discards the transport byte-count... Just wandering if we should recalc on the DIF case only or always. Yeah, same concern here wrt to discarding the transport length. This effectively makes the residual handling further down in sbc_parse_cdb() - target_cmd_size_check() always match size == cmd-data_length, because the latter as been recalculated by the former. Honestly, I don't know if this is a problem for normal READ + WRITE with prot=1 as I've never seen size != cmd-data_length for I/O related commands, but it does seem potentially dangerous. From the QLogic perfective, the cmd-data_length is pull directly from the wire (i.e. FCP header) when cmd is received. In essence, it's whatever the Initiator is set it to. We does not have any indicator to spot DIF vs none-DIF cmd when 1st received, unless the code do a peek. Same for all transports I assume... So just to confirm Quinn, the Qlogic the initiator includes the PI bytes into the FCP header data_length for the target-side *_PASS and DOUT_STRIP / DIN_INSERT, that is currently passed into qla_tgt_ops-handle_cmd(), right..? If that is the case, Sagi's v1 to cmd-data_length -= cmd-prot_length seems like it would still do right thing for *_PASS and DOUT_STRIP / DIN_INSERT, given that cmd-prot_length is calculated in sbc_check_prot() based upon dev-prot_length * sectors.. With that said, the cmd-data_length does not guarantee to contain both data length protection length when cmd is submit to TCM/target_submit_cmd(). In Dif-Insert mode, data_length will only contain the actual data(no Dif). No, in the DOUT_INSERT/DIN_STRIP case, protect bits are off and the core will take the data length as is. This case is covered. nod It's best that the SBC code re-calculate the actual data length and dif data length based on the number of sectors derived from the cmd. Nic, what's your take on this? Hard to say. Discarding the transport length in v2 doesn't seem like a good idea, but subtracting from cmd-prot_length in v1 is using the sector count from the CDB anyways, so it's essentially the same tradeoff of recalculating the transport's cmd-data_length from values within the CDB w/ prot=1. MKP, WDYT..? --nab -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] block SG_IO: add SG_FLAG_Q_AT_HEAD flag
On 06/05/2014 09:02 AM, Douglas Gilbert wrote: After the SG_IO ioctl was copied into the block layer and later into the bsg driver, subtle differences emerged. One difference is the way injected commands are queued through the block layer (i.e. this is not SCSI device queueing nor SATA NCQ). Summarizing: - SG_IO on block layer device: blk_exec*(at_head=false) - sg device SG_IO: at_head=true - bsg device SG_IO: at_head=true Some time ago Boaz Harrosh introduced a sg v4 flag called BSG_FLAG_Q_AT_TAIL to override the bsg driver default. A recent patch titled: sg: add SG_FLAG_Q_AT_TAIL flag allowed the sg driver default to be overridden. This patch allows a SG_IO ioctl sent to a block layer device to have its default overridden. ChangeLog: - introduce SG_FLAG_Q_AT_HEAD flag in sg.h to cause commands that are injected via a block layer device SG_IO ioctl to set at_head=true - make comments clearer about queueing in sg.h since the header is used both by the sg device and block layer device implementations of the SG_IO ioctl. - introduce BSG_FLAG_Q_AT_HEAD in bsg.h for compatibility (it does nothing) and update comments. Signed-off-by: Douglas Gilbert dgilb...@interlog.com Looks ok to me. Reviewed-by: Mike Christie micha...@cs.wisc.edu -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] Include protection information in transport header
On Wed, 2014-06-11 at 12:09 +0300, Sagi Grimberg wrote: At the SCSI transport level, there is no distinction between user data and protection information. Thus, iscsi header field expected data transfer length should include protection information. Patch #1 introduces scsi helper scsi_transfer_length which computes wire transfer byte count. Patch #2 modifies iscsi initiator to set correct wire transfer length in iscsi header data_length field (and modifies iser accordingly). Patch #3 modifies target core to re-calculate the pure data length in case of PI presence over the wire (and modifies loopback transport to align with other transports). Changes from v1: - scsi_cmnd: Rewrite scsi_transfer_length as MKP suggested - Target/sbc: re-calculate the data_length in case PI exists on the wire (instead od deacrease data -= prot) Changes from v0: - Introduce scsi helpers to compute correct transfer length in the presence of protection information. - Modify iscsi to set correct transfer length using scsi helpers - Modify loopback transport to set correct transfer length using scsi helpers Sagi Grimberg (3): scsi_cmnd: Introduce scsi_transfer_length helper libiscsi, iser: Adjust data_length to include protection information TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire drivers/infiniband/ulp/iser/iser_initiator.c | 34 +++-- drivers/scsi/libiscsi.c | 18 +++--- drivers/target/loopback/tcm_loop.c | 15 +-- drivers/target/target_core_sbc.c | 15 ++- include/scsi/scsi_cmnd.h | 17 + 5 files changed, 61 insertions(+), 38 deletions(-) Ok, I've applied these to for-next, but let's see what MKP recommends for patch #3 wrt to recalculating cmd-data_length. Thanks Sagi! --nab -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] sg: relax 16 byte cdb restriction
On 06/03/2014 12:18 PM, Douglas Gilbert wrote: v4 of this patch was sent 20131201. ChangeLog: - remove the 16 byte CDB (SCSI command) length limit from the sg driver by handling longer CDBs the same way as the bsg driver. Remove comment from sg.h public interface about the cmd_len field being limited to 16 bytes. - remove some dead code caused by this change - cleanup comment block at the top of sg.h, fix urls Signed-off-by: Douglas Gilbert dgilb...@interlog.com Looks ok to me. Reviewed-by: Mike Christie micha...@cs.wisc.edu -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire
On 6/11/14 2:30 PM, Nicholas A. Bellinger n...@linux-iscsi.org wrote: On Wed, 2014-06-11 at 10:24 +0300, Sagi Grimberg wrote: On 6/11/2014 12:17 AM, Quinn Tran wrote: SNIP QT Instead of using existing value within cmd-data_length, can we calculated data_length based on secstors blocksize. cmd-data_length = sectors * dev-dev_attrib.block_size; We can do that I suppose... Although it seems weird that the core discards the transport byte-count... Just wandering if we should recalc on the DIF case only or always. Yeah, same concern here wrt to discarding the transport length. This effectively makes the residual handling further down in sbc_parse_cdb() - target_cmd_size_check() always match size == cmd-data_length, because the latter as been recalculated by the former. Honestly, I don't know if this is a problem for normal READ + WRITE with prot=1 as I've never seen size != cmd-data_length for I/O related commands, but it does seem potentially dangerous. From the QLogic perfective, the cmd-data_length is pull directly from the wire (i.e. FCP header) when cmd is received. In essence, it's whatever the Initiator is set it to. We does not have any indicator to spot DIF vs none-DIF cmd when 1st received, unless the code do a peek. Same for all transports I assume... So just to confirm Quinn, the Qlogic the initiator includes the PI bytes into the FCP header data_length for the target-side *_PASS and DOUT_STRIP / DIN_INSERT, that is currently passed into qla_tgt_ops-handle_cmd(), right..? QT Initiator: DOUT_STRIP/DIN_Insert: FCP_DL = data length only (no dif length) Dif PASS: FCP_DL = data length + Dif length. Target: DOUT_strip/DIN_Insert: qla_tgt_ops-handle_cmd(data length only) sbc_check_prot() If (protect) cmd-data_length -= cmd-prot_length; truncation --- DIF_PASS: qla_tgt_ops-handle_cmd(data length + Dif length) If that is the case, Sagi's v1 to cmd-data_length -= cmd-prot_length seems like it would still do right thing for *_PASS and DOUT_STRIP / DIN_INSERT, given that cmd-prot_length is calculated in sbc_check_prot() based upon dev-prot_length * sectors.. With that said, the cmd-data_length does not guarantee to contain both data length protection length when cmd is submit to TCM/target_submit_cmd(). In Dif-Insert mode, data_length will only contain the actual data(no Dif). No, in the DOUT_INSERT/DIN_STRIP case, protect bits are off and the core will take the data length as is. This case is covered. nod QT agree. It's best that the SBC code re-calculate the actual data length and dif data length based on the number of sectors derived from the cmd. Nic, what's your take on this? Hard to say. Discarding the transport length in v2 doesn't seem like a good idea, but subtracting from cmd-prot_length in v1 is using the sector count from the CDB anyways, so it's essentially the same tradeoff of recalculating the transport's cmd-data_length from values within the CDB w/ prot=1. MKP, WDYT..? --nab This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message. attachment: winmail.dat
Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire
nab == Nicholas A Bellinger n...@linux-iscsi.org writes:
nab Hard to say. Discarding the transport length in v2 doesn't seem
nab like a good idea, but subtracting from cmd-prot_length in v1 is
nab using the sector count from the CDB anyways, so it's essentially
nab the same tradeoff of recalculating the transport's cmd-data_length
nab from values within the CDB w/ prot=1.
nab MKP, WDYT..?
My general feeling is that once sbc on the target sees {rd,wr}protect
0 we're in the territory where you should start separating data and
PI internally.
--
Martin K. Petersen Oracle Linux Engineering
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper
Sagi == Sagi Grimberg sa...@mellanox.com writes: Sagi In case protection information exists on the wire scsi transports Sagi should include it in the transfer byte count (even if protection Sagi information does not exist in the host memory space). This helper Sagi will compute the total transfer length from the scsi command data Sagi length and protection attributes. Looks good! -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/14] block: Deprecate integrity tagging functions
Christoph == Christoph Hellwig h...@infradead.org writes: Deprecate the tagging functions. Christoph This patch doesn't just deprecate them but outright removes Christoph them. Fixed. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/14] block: Replace bi_integrity with bi_special
Christoph == Christoph Hellwig h...@infradead.org writes:
Christoph Instead of having a union of pointer just make it a void
Christoph pointer. I also think special is a terribly generic name, but
Christoph I don't really have a better idea at hand.
I needed something that could encompass additional information to be
passed for integrity, copy offload and discard requests.
Another option is that we forgo the union name:
union {
#if defined(CONFIG_BLK_DEV_INTEGRITY)
struct bio_integrity_payload *bi_integrity;
#endif
struct bio_copy *bi_copy;
};
That's the way Jens has done it in struct request. I think I like that
better and it doesn't send the same up-for-grabs signal that a void
pointer might.
Jens: Any preference?
--
Martin K. Petersen Oracle Linux Engineering
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/14] block: Deprecate the use of the term sector in the context of block integrity
Christoph == Christoph Hellwig h...@infradead.org writes: Christoph On Wed, May 28, 2014 at 11:28:39PM -0400, Martin K. Petersen wrote: The protection interval is not necessarily tied to the logical block size of a block device. Stop using the terms sector and sectors. Christoph This does more than just renaming symbols, so it needs a Christoph better description or even better splitting into separate Christoph patches. The only thing I see that does more than a rename is the interval calculation tweak. I'll put that in a separate patch. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/14] block: Relocate integrity flags
Christoph == Christoph Hellwig h...@infradead.org writes: Christoph On Wed, May 28, 2014 at 11:28:43PM -0400, Martin K. Petersen wrote: Move flags affecting the integrity code out of the bio bi_flags and into the block integrity payload. Christoph It seems like bip is guaranteed to be non-NULL in all callers Christoph of the getters and setters. Yeah, check removed. Christoph I'd recommend just dropping them and opencode the flags Christoph manipulation. The reason I don't use test_bit() and set_bit() is that bip_flags is just a short. And to-shift-or-not-to-shift is a crappy, error prone interface, IMHO. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/14] block: Integrity checksum flag
Christoph == Christoph Hellwig h...@infradead.org writes: Make the choice of checksum a per-I/O property by introducing a flag that can be inspected by the SCSI layer. Christoph Why? First of all it's desirable to be able to use both IP and CRC checksums without having to unload the HBA driver. Also, there are some corner cases where you have to force the use of CRC guard tag even when the HBA supports IP checksums. If you are doing recovery and need to read a disk block with bad PI, for instance, you must be able to tell the HBA to pass the CRC through verbatim instead of attempting to convert it to IP checksum. Another example is qualification tooling that needs to be able to write a block with bad PI out to storage. In that case we also have to tell the HBA to ignore checking the PI and that it shouldn't attempt a checksum conversion. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/14] block: Add specific data integrity errors
Christoph == Christoph Hellwig h...@infradead.org writes: Introduce a set of error codes that can be used by the block integrity subsystem to signal which class of error was encountered by either the I/O controller or the storage device. Christoph I'd also love to see something catching these so that they Christoph don't leak to userspace. This patch was really meant as an RFC. But it is absolutely my intent to expose these to userspace. Albeit only to applications that supply or request protection information via Darrick's aio extensions. I also use these errors extensively in my test utilities to verify that the correct problem gets detected by the correct entity when I inject an error. I should add that in the past I had a separate error status inside the bip that contained the data integrity specific errors. But that involved all sorts of evil hacks when bios were cloned, split and stacked. After talking to nab about his needs for target I figured it was better to just define new error codes and handle them like Hannes did for the extended SCSI errors. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/14] lib: Add T10 Protection Information functions
Christoph == Christoph Hellwig h...@infradead.org writes:
static struct blk_integrity dif_type1_integrity_crc = {
Christoph Shouldn't the whole profile defintions move to the generic
Christoph code as well?
Yeah, good idea.
Christoph Maybe the code also should live in block/ and not lib/.
Fine by me. Jens?
--
Martin K. Petersen Oracle Linux Engineering
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire
On Wed, 2014-06-11 at 22:32 +, Quinn Tran wrote: On 6/11/14 2:30 PM, Nicholas A. Bellinger n...@linux-iscsi.org wrote: On Wed, 2014-06-11 at 10:24 +0300, Sagi Grimberg wrote: On 6/11/2014 12:17 AM, Quinn Tran wrote: SNIP QT Instead of using existing value within cmd-data_length, can we calculated data_length based on secstors blocksize. cmd-data_length = sectors * dev-dev_attrib.block_size; We can do that I suppose... Although it seems weird that the core discards the transport byte-count... Just wandering if we should recalc on the DIF case only or always. Yeah, same concern here wrt to discarding the transport length. This effectively makes the residual handling further down in sbc_parse_cdb() - target_cmd_size_check() always match size == cmd-data_length, because the latter as been recalculated by the former. Honestly, I don't know if this is a problem for normal READ + WRITE with prot=1 as I've never seen size != cmd-data_length for I/O related commands, but it does seem potentially dangerous. From the QLogic perfective, the cmd-data_length is pull directly from the wire (i.e. FCP header) when cmd is received. In essence, it's whatever the Initiator is set it to. We does not have any indicator to spot DIF vs none-DIF cmd when 1st received, unless the code do a peek. Same for all transports I assume... So just to confirm Quinn, the Qlogic the initiator includes the PI bytes into the FCP header data_length for the target-side *_PASS and DOUT_STRIP / DIN_INSERT, that is currently passed into qla_tgt_ops-handle_cmd(), right..? QT Initiator: DOUT_STRIP/DIN_Insert: FCP_DL = data length only (no dif length) Dif PASS: FCP_DL = data length + Dif length. Target: DOUT_strip/DIN_Insert: qla_tgt_ops-handle_cmd(data length only) sbc_check_prot() If (protect) cmd-data_length -= cmd-prot_length; truncation nod --- DIF_PASS: qla_tgt_ops-handle_cmd(data length + Dif length) Ok, so Sagi's patches for legacy fabric - PI backend and PI fabric - PI backend cases look OK, but not for the PI fabric - legacy backend case as noted above.. Given DOUT_STRIP + DIN_INSERT have not been enabled in sbc_check_prot() just yet, I'll go ahead and merge his v2 series to address the two existing cases in -rc1 + v3.15.y stable code. From there, enabling PI fabric - legacy backend support in = rc2 with target-core will be easy, as it's just a matter of updating sbc_set_prot_op_checks() to assign prot_ops, avoid sbc_check_prot() cmd-data_length recalculation, and making sure PROTECTION control feature bits are still exposed for legacy - unprotected backends. Thanks! --nab -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
