re: uas: Use GFP_NOIO rather then GFP_ATOMIC where possible
Hello Hans de Goede,
The patch e36e64930cff: uas: Use GFP_NOIO rather then GFP_ATOMIC
where possible from Nov 7, 2013, leads to the following static
checker warning:
drivers/usb/storage/uas.c:806 uas_eh_task_mgmt()
error: scheduling with locks held: 'spin_lock:lock'
drivers/usb/storage/uas.c
789 spin_lock_irqsave(devinfo-lock, flags);
790
791 if (devinfo-resetting) {
792 spin_unlock_irqrestore(devinfo-lock, flags);
793 return FAILED;
794 }
795
796 if (devinfo-running_task) {
797 shost_printk(KERN_INFO, shost,
798 %s: %s: error already running a task\n,
799 __func__, fname);
800 spin_unlock_irqrestore(devinfo-lock, flags);
801 return FAILED;
802 }
803
804 devinfo-running_task = 1;
805 memset(devinfo-response, 0, sizeof(devinfo-response));
806 sense_urb = uas_submit_sense_urb(cmnd, GFP_NOIO,
The original code had this as GFP_ATOMIC because we can't sleep while
we have spin_lock_irqsave() held.
807 devinfo-use_streams ? tag :
0);
808 if (!sense_urb) {
809 shost_printk(KERN_INFO, shost,
810 %s: %s: submit sense urb failed\n,
811 __func__, fname);
812 devinfo-running_task = 0;
813 spin_unlock_irqrestore(devinfo-lock, flags);
814 return FAILED;
815 }
816 if (uas_submit_task_urb(cmnd, GFP_NOIO, function, tag)) {
Same.
817 shost_printk(KERN_INFO, shost,
818 %s: %s: submit task mgmt urb failed\n,
819 __func__, fname);
820 devinfo-running_task = 0;
821 spin_unlock_irqrestore(devinfo-lock, flags);
822 usb_kill_urb(sense_urb);
823 return FAILED;
824 }
825 spin_unlock_irqrestore(devinfo-lock, flags);
regards,
dan carpenter
--
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/5] scsi_scan: Restrict sequential scan to 256 LUNs
On 03/27/2014 07:49 AM, Christoph Hellwig wrote: On Tue, Dec 10, 2013 at 12:05:12PM +0100, Hannes Reinecke wrote: Sequential scan for more than 256 LUNs is very fragile as LUNs might not be numbered sequentially after that point. SAM revisions later than SCSI-3 impose a structure on LUNs larger than 256, making LUN numbers between 256 and 16384 illegal. SCSI-3, however allows for plain 64-bit numbers with no internal structure. So restrict sequential LUN scan to 256 LUNs and add a new blacklist flag 'BLIST_SCSI3LUN' to scan up to max_lun devices. What do you need the blacklist flag for? There's no user of it, and supporting that many LUNs without REPORT LUNS support doesn't sound very practical anyway. Because there is no guarantee that pre-SCSI-3 devices (or devices announcing to be pre-SCSI-3) will not allow to scan more than 256 devices. Thinking of older Symmetrix here with their weird 'SPC-3 masking as SCSI-2' habit. Also currently we're allowing to scan beyond 256 without any restrictions there might be installations out there which rely on this behaviour. Hence this flag. 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
[patch] [SCSI] mpt3sas: tidy up output slightly
The indenting here for pr_info(\n); is not correct. It's not part of the if condition. Also using pr_info() would put extra characters in the middle of the line. I suppose that people could complain that pr_cont() is racy but at least it's better than the original code. Signed-off-by: Dan Carpenter dan.carpen...@oracle.com diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c index 0cf4f70..aa0e042 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_base.c +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c @@ -585,9 +585,9 @@ _base_display_event_data(struct MPT3SAS_ADAPTER *ioc, (event_data-ReasonCode == MPI2_EVENT_SAS_DISC_RC_STARTED) ? start : stop); if (event_data-DiscoveryStatus) - pr_info(discovery_status(0x%08x), + pr_cont(discovery_status(0x%08x), le32_to_cpu(event_data-DiscoveryStatus)); - pr_info(\n); + pr_cont(\n); return; } case MPI2_EVENT_SAS_BROADCAST_PRIMITIVE: -- 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: [SCSI] be2iscsi: Fix handling timed out MBX completion from FW
Hello Jayamohan Kallickal,
The patch 1957aa7f6246: [SCSI] be2iscsi: Fix handling timed out MBX
completion from FW from Jan 29, 2014, leads to the following static
checker warning:
drivers/scsi/be2iscsi/be_main.c:5581 beiscsi_dev_probe()
error: memset() 'phba-ctrl.ptag_state[i]-tag_mem_state' too small
(24 vs 32)
drivers/scsi/be2iscsi/be_main.c
5576 for (i = 0; i MAX_MCC_CMD; i++) {
5577 init_waitqueue_head(phba-ctrl.mcc_wait[i + 1]);
5578 phba-ctrl.mcc_tag[i] = i + 1;
5579 phba-ctrl.mcc_numtag[i + 1] = 0;
5580 phba-ctrl.mcc_tag_available++;
5581 memset(phba-ctrl.ptag_state[i].tag_mem_state, 0,
5582 sizeof(struct beiscsi_mcc_tag_state));
Probably this this be change to sizeof(struct be_dma_mem struct)? It
looks like we are corrupting memory a bit here.
5583 }
regards,
dan carpenter
--
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/5] scsi_scan: Restrict sequential scan to 256 LUNs
On Fri, Mar 28, 2014 at 01:22:15AM -0700, Hannes Reinecke wrote: Because there is no guarantee that pre-SCSI-3 devices (or devices announcing to be pre-SCSI-3) will not allow to scan more than 256 devices. Thinking of older Symmetrix here with their weird 'SPC-3 masking as SCSI-2' habit. Don't we have a blist entry for those to use REPORT LUNS anyway? Also currently we're allowing to scan beyond 256 without any restrictions there might be installations out there which rely on this behaviour. Hence this flag. Which doesn't really have an effect as-is as it's never set. -- 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/5] scsi_scan: Restrict sequential scan to 256 LUNs
On 03/28/2014 05:47 AM, Christoph Hellwig wrote: On Fri, Mar 28, 2014 at 01:22:15AM -0700, Hannes Reinecke wrote: Because there is no guarantee that pre-SCSI-3 devices (or devices announcing to be pre-SCSI-3) will not allow to scan more than 256 devices. Thinking of older Symmetrix here with their weird 'SPC-3 masking as SCSI-2' habit. Don't we have a blist entry for those to use REPORT LUNS anyway? Symmetrix was just an example of weirdness ... Also currently we're allowing to scan beyond 256 without any restrictions there might be installations out there which rely on this behaviour. Hence this flag. Which doesn't really have an effect as-is as it's never set. Because currently there are no target which would require them. Obviously. But that doesn't mean there won't be any. Note, this is just a safety net for backwards compability. If you think we won't need it of course we can remove it. 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
[PATCH 3/3] [SCSI] Fix command result state propagation
From: Alan Stern st...@rowland.harvard.edu
We're seeing a case where the contents of scmd-result isn't being reset after
a SCSI command encounters an error, is resubmitted, times out and then gets
handled. The error handler acts on the stale result of the previous error
instead of the timeout. Fix this by properly zeroing the scmd-status before
the command is resubmitted.
Signed-off-by: Alan Stern st...@rowland.harvard.edu
Signed-off-by: James Bottomley jbottom...@parallels.com
---
drivers/scsi/scsi_error.c | 1 +
drivers/scsi/scsi_lib.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 221a5bc..335eaf4 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -927,6 +927,7 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct
scsi_eh_save *ses,
memset(scmd-cmnd, 0, BLK_MAX_CDB);
memset(scmd-sdb, 0, sizeof(scmd-sdb));
scmd-request-next_rq = NULL;
+ scmd-result = 0;
if (sense_bytes) {
scmd-sdb.length = min_t(unsigned, SCSI_SENSE_BUFFERSIZE,
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 5681c05..b1f4867 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -137,6 +137,7 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int
reason, int unbusy)
* lock such that the kblockd_schedule_work() call happens
* before blk_cleanup_queue() finishes.
*/
+ cmd-result = 0;
spin_lock_irqsave(q-queue_lock, flags);
blk_requeue_request(q, cmd-request);
kblockd_schedule_work(q, device-requeue_work);
--
1.9.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 1/3] [SCSI] Fix abort state memory problem
The abort state is being stored persistently across commands, meaning if the
command times out a second time, the error handler thinks an abort is still
pending. Fix this by properly resetting the abort flag after the abort is
finished.
Signed-off-by: James Bottomley jbottom...@parallels.com
---
drivers/scsi/scsi_error.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 771c16b..b9f3b07 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -869,10 +869,13 @@ static int scsi_try_bus_device_reset(struct scsi_cmnd
*scmd)
static int scsi_try_to_abort_cmd(struct scsi_host_template *hostt, struct
scsi_cmnd *scmd)
{
- if (!hostt-eh_abort_handler)
- return FAILED;
+ int retval = FAILED;
+
+ if (hostt-eh_abort_handler)
+ retval = hostt-eh_abort_handler(scmd);
- return hostt-eh_abort_handler(scmd);
+ scmd-eh_eflags = ~SCSI_EH_ABORT_SCHEDULED;
+ return retval;
}
static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
--
1.9.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 0/3] Fix USB deadlock caused by SCSI error handling
This is a set of three patches we agreed to a while ago to eliminate a USB deadlock. I did rewrite the first patch, if it could be reviewed and tested. Thanks, James --- Alan Stern (1): [SCSI] Fix command result state propagation James Bottomley (2): [SCSI] Fix abort state memory problem [SCSI] Fix spurious request sense in error handling drivers/scsi/scsi_error.c | 19 --- drivers/scsi/scsi_lib.c | 1 + 2 files changed, 17 insertions(+), 3 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 2/3] [SCSI] Fix spurious request sense in error handling
We unconditionally execute scsi_eh_get_sense() to make sure all failed commands that should have sense attached, do. However, the routine forgets that some commands, because of the way they fail, will not have any sense code ... we should not bother them with a REQUEST_SENSE command. Fix this by testing to see if we actually got a CHECK_CONDITION return and skip asking for sense if we don't. Tested-by: Alan Stern st...@rowland.harvard.edu Signed-off-by: James Bottomley jbottom...@parallels.com --- drivers/scsi/scsi_error.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index b9f3b07..221a5bc 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -1160,6 +1160,15 @@ int scsi_eh_get_sense(struct list_head *work_q, __func__)); break; } + if (status_byte(scmd-result) != CHECK_CONDITION) + /* +* don't request sense if there's no check condition +* status because the error we're processing isn't one +* that has a sense code (and some devices get +* confused by sense requests out of the blue) +*/ + continue; + SCSI_LOG_ERROR_RECOVERY(2, scmd_printk(KERN_INFO, scmd, %s: requesting sense\n, current-comm)); -- 1.9.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: [Scst-devel] OSS target - VMware SCSI reservation bug conformity.
From: Dr. Greg Wettstein g...@wind.enjellic.com If there is an issue it would seem to be in the best interests of those of us committed to open-source storage solutions to understand and protect ourselves from the situation. There is a third saying which is important as well: If the question is a legitimate question of interpretation, then the better course is probably be liberal in what you receive and be conservative in what you transmit. Practically speaking, I remember the story of US Robotics modems. I'm told they captured the market for server modems in the days of dialup services by testing their modems against every make of modem on the market and tweaking its behavior so that it could successfully interwork with basically any modem the consumer purchased, no matter how crappy. This level of reliability was very valuable to the operators of online services. 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 0/3] Fix USB deadlock caused by SCSI error handling
On Fri, 28 Mar 2014, James Bottomley wrote: This is a set of three patches we agreed to a while ago to eliminate a USB deadlock. I did rewrite the first patch, if it could be reviewed and tested. I tested all three patches under the same conditions as before, and they all worked correctly. In the revised patch 1, the meaning of SCSI_EH_ABORT_SCHEDULED isn't entirely clear. This boils down to two questions, which I don't know the answers to: What should happen in scmd_eh_abort_handler() if scsi_host_eh_past_deadline() returns true and thereby prevents scsi_try_to_abort_cmd() from being called? The flag wouldn't get cleared then. What should happen if some other pathway manages to call scsi_try_to_abort_cmd() while scmd-abort_work is still sitting on the work queue? The command would be aborted and the flag would be cleared, but the queued work would remain. Can this ever happen? Maybe scmd_eh_abort_handler() should check the flag before doing anything. Is there any sort of sychronization to prevent the same incarnation of a command from being aborted twice (or by two different threads at the same time)? If there is, it isn't obvious. (Also, what's going on at the start of scsi_abort_command()? Contrary to what one might expect, the first part of the function _cancels_ a scheduled abort. And it does so without clearing the SCSI_EH_ABORT_SCHEDULED flag.) Maybe in all these scenarios, the command is doomed to fail anyway so these questions don't make any real difference. I don't know... Alan Stern -- 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] Block/iopoll: Remove redundant export variable blk_iopoll_enabled
No need for this variable anymore - blk_iopoll should always
be enabled. Also remove the user references to it.
I just removed the blk_iopoll_enabled condition from the user logic
but I don't have the facilities to test that I didn't break be2iscsi
or ipr users, So I was hoping that Jayamohan Wen can confirm.
Signed-off-by: Sagi Grimberg sa...@mellanox.com
Cc: Jayamohan Kallickal jayamohan.kallic...@emulex.com
Cc: Wen Xiong wenxi...@linux.vnet.ibm.com
Cc: Brian King brk...@us.ibm.com
---
block/blk-iopoll.c |3 -
drivers/scsi/be2iscsi/be_main.c | 206 ---
drivers/scsi/ipr.c | 20 ++--
include/linux/blk-iopoll.h |2 -
kernel/sysctl.c | 12 ---
5 files changed, 73 insertions(+), 170 deletions(-)
diff --git a/block/blk-iopoll.c b/block/blk-iopoll.c
index 1855bf5..c11d24e 100644
--- a/block/blk-iopoll.c
+++ b/block/blk-iopoll.c
@@ -14,9 +14,6 @@
#include blk.h
-int blk_iopoll_enabled = 1;
-EXPORT_SYMBOL(blk_iopoll_enabled);
-
static unsigned int blk_iopoll_budget __read_mostly = 256;
static DEFINE_PER_CPU(struct list_head, blk_cpu_iopoll);
diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index 1f37505..a929c3c 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -873,7 +873,6 @@ static irqreturn_t be_isr_msix(int irq, void *dev_id)
struct be_queue_info *cq;
unsigned int num_eq_processed;
struct be_eq_obj *pbe_eq;
- unsigned long flags;
pbe_eq = dev_id;
eq = pbe_eq-q;
@@ -882,31 +881,15 @@ static irqreturn_t be_isr_msix(int irq, void *dev_id)
phba = pbe_eq-phba;
num_eq_processed = 0;
- if (blk_iopoll_enabled) {
- while (eqe-dw[offsetof(struct amap_eq_entry, valid) / 32]
-EQE_VALID_MASK) {
- if (!blk_iopoll_sched_prep(pbe_eq-iopoll))
- blk_iopoll_sched(pbe_eq-iopoll);
-
- AMAP_SET_BITS(struct amap_eq_entry, valid, eqe, 0);
- queue_tail_inc(eq);
- eqe = queue_tail_node(eq);
- num_eq_processed++;
- }
- } else {
- while (eqe-dw[offsetof(struct amap_eq_entry, valid) / 32]
-EQE_VALID_MASK) {
- spin_lock_irqsave(phba-isr_lock, flags);
- pbe_eq-todo_cq = true;
- spin_unlock_irqrestore(phba-isr_lock, flags);
- AMAP_SET_BITS(struct amap_eq_entry, valid, eqe, 0);
- queue_tail_inc(eq);
- eqe = queue_tail_node(eq);
- num_eq_processed++;
- }
+ while (eqe-dw[offsetof(struct amap_eq_entry, valid) / 32]
+EQE_VALID_MASK) {
+ if (!blk_iopoll_sched_prep(pbe_eq-iopoll))
+ blk_iopoll_sched(pbe_eq-iopoll);
- if (pbe_eq-todo_cq)
- queue_work(phba-wq, pbe_eq-work_cqs);
+ AMAP_SET_BITS(struct amap_eq_entry, valid, eqe, 0);
+ queue_tail_inc(eq);
+ eqe = queue_tail_node(eq);
+ num_eq_processed++;
}
if (num_eq_processed)
@@ -927,7 +910,6 @@ static irqreturn_t be_isr(int irq, void *dev_id)
struct hwi_context_memory *phwi_context;
struct be_eq_entry *eqe = NULL;
struct be_queue_info *eq;
- struct be_queue_info *cq;
struct be_queue_info *mcc;
unsigned long flags, index;
unsigned int num_mcceq_processed, num_ioeq_processed;
@@ -953,72 +935,40 @@ static irqreturn_t be_isr(int irq, void *dev_id)
num_ioeq_processed = 0;
num_mcceq_processed = 0;
- if (blk_iopoll_enabled) {
- while (eqe-dw[offsetof(struct amap_eq_entry, valid) / 32]
-EQE_VALID_MASK) {
- if (((eqe-dw[offsetof(struct amap_eq_entry,
-resource_id) / 32]
-EQE_RESID_MASK) 16) == mcc-id) {
- spin_lock_irqsave(phba-isr_lock, flags);
- pbe_eq-todo_mcc_cq = true;
- spin_unlock_irqrestore(phba-isr_lock, flags);
- num_mcceq_processed++;
- } else {
- if (!blk_iopoll_sched_prep(pbe_eq-iopoll))
- blk_iopoll_sched(pbe_eq-iopoll);
- num_ioeq_processed++;
- }
- AMAP_SET_BITS(struct amap_eq_entry, valid, eqe, 0);
- queue_tail_inc(eq);
- eqe = queue_tail_node(eq);
- }
- if
Re: [PATCH] Block/iopoll: Remove redundant export variable blk_iopoll_enabled
On 03/28/2014 02:01 PM, Sagi Grimberg wrote: No need for this variable anymore - blk_iopoll should always be enabled. Also remove the user references to it. I just removed the blk_iopoll_enabled condition from the user logic but I don't have the facilities to test that I didn't break be2iscsi or ipr users, So I was hoping that Jayamohan Wen can confirm. Signed-off-by: Sagi Grimberg sa...@mellanox.com Cc: Jayamohan Kallickal jayamohan.kallic...@emulex.com Cc: Wen Xiong wenxi...@linux.vnet.ibm.com Cc: Brian King brk...@us.ibm.com This is sitting in my for-3.15/core branch since about two weeks ago: http://git.kernel.dk/?p=linux-block.git;a=commit;h=89f8b33ca1ea881d1d84542282cb85d07d02e78d -- Jens Axboe -- 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/iopoll: Remove redundant export variable blk_iopoll_enabled
On 3/28/2014 11:05 PM, Jens Axboe wrote: On 03/28/2014 02:01 PM, Sagi Grimberg wrote: No need for this variable anymore - blk_iopoll should always be enabled. Also remove the user references to it. I just removed the blk_iopoll_enabled condition from the user logic but I don't have the facilities to test that I didn't break be2iscsi or ipr users, So I was hoping that Jayamohan Wen can confirm. Signed-off-by: Sagi Grimberg sa...@mellanox.com Cc: Jayamohan Kallickal jayamohan.kallic...@emulex.com Cc: Wen Xiong wenxi...@linux.vnet.ibm.com Cc: Brian King brk...@us.ibm.com This is sitting in my for-3.15/core branch since about two weeks ago: http://git.kernel.dk/?p=linux-block.git;a=commit;h=89f8b33ca1ea881d1d84542282cb85d07d02e78d U, I must it... Discard it then, sorry for the spam. 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: Deadlock in usb-storage error handling
On Thu, 2014-03-20 at 15:49 -0400, Alan Stern wrote:
On Thu, 20 Mar 2014, James Bottomley wrote:
On Thu, 2014-03-20 at 12:34 -0400, Alan Stern wrote:
On Thu, 20 Mar 2014, James Bottomley wrote:
OK, so I think we have three things to do
1. Investigate SCSI and fix it's abort state problem that's causing
it not to send the abort second time around
2. Fix usb-storage to fail a reset it can't do (i.e. device reset
with outstanding commands)
3. Find out why we're sending a spurious request sense.
I can look at 1 and 3 if you want to take 2.
It's a deal! Thanks for your help.
OK, I think this is the fix for 1, if you could try it out.
Thanks,
James
---
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 771c16b..c52bfb2 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -145,14 +145,14 @@ scmd_eh_abort_handler(struct work_struct *work)
scmd %p retry
aborted command\n, scmd));
scsi_queue_insert(scmd, SCSI_MLQUEUE_EH_RETRY);
- return;
+ goto out;
} else {
SCSI_LOG_ERROR_RECOVERY(3,
scmd_printk(KERN_WARNING, scmd,
scmd %p finish
aborted command\n, scmd));
scsi_finish_command(scmd);
- return;
+ goto out;
}
} else {
SCSI_LOG_ERROR_RECOVERY(3,
@@ -162,6 +162,8 @@ scmd_eh_abort_handler(struct work_struct *work)
}
}
+ scmd-eh_eflags = ~SCSI_EH_ABORT_SCHEDULED;
+
if (!scsi_eh_scmd_add(scmd, 0)) {
SCSI_LOG_ERROR_RECOVERY(3,
scmd_printk(KERN_WARNING, scmd,
@@ -170,6 +172,10 @@ scmd_eh_abort_handler(struct work_struct *work)
scmd-result |= DID_TIME_OUT 16;
scsi_finish_command(scmd);
}
+ return;
+ out:
+ scmd-eh_eflags = ~SCSI_EH_ABORT_SCHEDULED;
+ return;
}
/**
This worked the first time. :-)
But I wonder, is it safe to access scmd after calling
scsi_finish_command()?
Agree, I've redone the patch integrated into the try_to_abort call
instead. Will post shortly.
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
[PATCH 1/4] target/core: T10-Dif: check HW support capabilities
Check lower layer/HW support of T10-dif capability.
When the LUN is linked between the underlining fabric
and back end device, the Protection Type(1,2,3) is
check against each other to make sure both side are
capable of supporting the same protection setting.
ln -s /sys/kernel/config/target/core/rd_mcp_0/q_tcm_mcp.0
/sys/kernel/config/target/qla2xxx/$L_WWPN/tpgt_1/lun/lun_0/tcm_123
Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
Signed-off-by: Giridhar Malavali giridhar.malav...@qlogic.com
---
drivers/target/target_core_fabric_configfs.c | 20
drivers/target/target_core_tpg.c | 9 +
include/target/target_core_base.h| 14 ++
include/target/target_core_fabric.h | 1 +
4 files changed, 44 insertions(+)
diff --git a/drivers/target/target_core_fabric_configfs.c
b/drivers/target/target_core_fabric_configfs.c
index 7de9f04..3ce07ec 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -777,6 +777,26 @@ static int target_fabric_port_link(
struct se_portal_group, tpg_group);
tf = se_tpg-se_tpg_wwn-wwn_tf;
+
+ if (dev-dev_attrib.pi_prot_type) {
+ uint32_t cap[] = { 0,
+ TARGET_DIF_TYPE1_PROTECTION,
+ TARGET_DIF_TYPE2_PROTECTION,
+ TARGET_DIF_TYPE3_PROTECTION};
+ uint32_t pt_bits = cap[dev-dev_attrib.pi_prot_type];
+ pt_bits = se_tpg-fabric_sup_prot_type;
+
+ /* cross check with fabric to see if t10dif is supported */
+ if (!pt_bits) {
+ //dev-dev_attrib.pi_prot_type = 0;
+ pr_err(dev[%p]: DIF protection mismatch with fabric
+ (%s). Transport capability bits[0x%x]\n,
+ dev,
se_tpg-se_tpg_wwn-wwn_group.cg_item.ci_name,
+ se_tpg-fabric_sup_prot_type);
+ return -EFAULT;
+ }
+ }
+
if (lun-lun_se_dev != NULL) {
pr_err(Port Symlink already exists\n);
return -EEXIST;
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index c036595..9279971 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -632,6 +632,15 @@ int core_tpg_set_initiator_node_tag(
}
EXPORT_SYMBOL(core_tpg_set_initiator_node_tag);
+void core_tpg_set_fabric_t10dif(
+ struct se_portal_group *tpg,
+ uint32_t fabric_t10dif_force_on)
+{
+ tpg-fabric_t10dif_force_on = fabric_t10dif_force_on;
+}
+EXPORT_SYMBOL(core_tpg_set_fabric_t10dif);
+
+
static void core_tpg_lun_ref_release(struct percpu_ref *ref)
{
struct se_lun *lun = container_of(ref, struct se_lun, lun_ref);
diff --git a/include/target/target_core_base.h
b/include/target/target_core_base.h
index ec3e3a3..fc2c0ef 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -241,6 +241,17 @@ enum tcm_tmrsp_table {
TMR_FUNCTION_REJECTED = 5,
};
+enum target_guard_type_cap {
+ TARGET_GUARD_CRC = 1 0,
+ TARGET_GUARD_IP = 1 1,
+};
+
+enum target_prot_type_cap {
+ TARGET_DIF_TYPE1_PROTECTION = 1 0, /* T10 DIF Type 1 */
+ TARGET_DIF_TYPE2_PROTECTION = 1 1, /* T10 DIF Type 2 */
+ TARGET_DIF_TYPE3_PROTECTION = 1 2, /* T10 DIF Type 3 */
+};
+
/*
* Used for target SCSI statistics
*/
@@ -862,6 +873,9 @@ struct se_portal_group {
enum transport_tpg_type_table se_tpg_type;
/* Number of ACLed Initiator Nodes for this TPG */
u32 num_node_acls;
+ u32 fabric_t10dif_force_on;
+ u32 fabric_sup_guard_type;
+ u32 fabric_sup_prot_type;
/* Used for PR SPEC_I_PT=1 and REGISTER_AND_MOVE */
atomic_ttpg_pr_ref_count;
/* Spinlock for adding/removing ACLed Nodes */
diff --git a/include/target/target_core_fabric.h
b/include/target/target_core_fabric.h
index 1d10436..4c3dab5 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -152,6 +152,7 @@ int core_tpg_set_initiator_node_queue_depth(struct
se_portal_group *,
unsigned char *, u32, int);
intcore_tpg_set_initiator_node_tag(struct se_portal_group *,
struct se_node_acl *, const char *);
+void core_tpg_set_fabric_t10dif(struct se_portal_group *, uint32_t);
intcore_tpg_register(struct target_core_fabric_ops *, struct se_wwn *,
struct se_portal_group *, void *, int);
intcore_tpg_deregister(struct se_portal_group *);
--
1.8.4.GIT
--
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
[PATCH 3/4] target/rd: T10-Dif: Add init/format support
This patch is borrow code from
commit 0f5e2ec46dd64579c0770f3822764f94db4fa465
Author: Nicholas Bellinger n...@linux-iscsi.org
Date: Sat Jan 18 09:32:56 2014 +
target/file: Add DIF protection init/format support
This patch adds support for DIF protection init/format support into
the FILEIO backend.
It involves using a seperate $FILE.protection for storing PI that is
opened via fd_init_prot() using the common pi_prot_type attribute.
The actual formatting of the protection is done via fd_format_prot()
using the common pi_prot_format attribute, that will populate the
initial PI data based upon the currently configured pi_prot_type.
Based on original FILEIO code from Sagi.
v1 changes:
- Fix sparse warnings in fd_init_format_buf (Fengguang)
Cc: Martin K. Petersen martin.peter...@oracle.com
Cc: Christoph Hellwig h...@lst.de
Cc: Hannes Reinecke h...@suse.de
Cc: Sagi Grimberg sa...@mellanox.com
Cc: Or Gerlitz ogerl...@mellanox.com
Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
---
drivers/target/target_core_rd.c | 64 -
1 file changed, 63 insertions(+), 1 deletion(-)
diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
index 66a5aba..01dda0b 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -603,7 +603,7 @@ static int rd_init_prot(struct se_device *dev)
{
struct rd_dev *rd_dev = RD_DEV(dev);
-if (!dev-dev_attrib.pi_prot_type)
+ if (!dev-dev_attrib.pi_prot_type)
return 0;
return rd_build_prot_space(rd_dev, dev-prot_length);
@@ -616,6 +616,67 @@ static void rd_free_prot(struct se_device *dev)
rd_release_prot_space(rd_dev);
}
+static void rd_init_format_buf(struct se_device *dev, unsigned char *buf,
+ u32 unit_size, u32 *ref_tag, u16 app_tag,
+ bool inc_reftag)
+{
+ unsigned char *p = buf;
+ int i;
+
+ for (i = 0; i unit_size; i += dev-prot_length) {
+ *((u16 *)p[0]) = 0x;
+ *((__be16 *)p[2]) = cpu_to_be16(app_tag);
+ *((__be32 *)p[4]) = cpu_to_be32(*ref_tag);
+
+ if (inc_reftag)
+ (*ref_tag)++;
+
+ p += dev-prot_length;
+ }
+}
+
+static int rd_format_prot(struct se_device *dev)
+{
+ struct rd_dev *rd_dev = RD_DEV(dev);
+ u32 ref_tag = 0;
+ int i,j;
+ bool inc_reftag = false;
+ struct rd_dev_sg_table *pt;
+ struct scatterlist *sg;
+ void *paddr;
+
+ if (!dev-dev_attrib.pi_prot_type) {
+ pr_err(Unable to format_prot while pi_prot_type == 0\n);
+ return -ENODEV;
+ }
+
+ switch (dev-dev_attrib.pi_prot_type) {
+ case TARGET_DIF_TYPE3_PROT:
+ ref_tag = 0x;
+ break;
+ case TARGET_DIF_TYPE2_PROT:
+ case TARGET_DIF_TYPE1_PROT:
+ inc_reftag = true;
+ break;
+ default:
+ break;
+ }
+
+ for (i=0; i rd_dev-sg_prot_count; i++) {
+ pt= rd_dev-sg_prot_array[i];
+ for_each_sg(pt-sg_table, sg, pt-rd_sg_count, j) {
+ paddr = kmap(sg_page(sg)) + sg-offset;
+
+ rd_init_format_buf(dev, paddr, sg-length, ref_tag,
+ 0x, inc_reftag);
+ kunmap(paddr);
+ }
+ }
+
+ return 0;
+}
+
+
static struct sbc_ops rd_sbc_ops = {
.execute_rw = rd_execute_rw,
};
@@ -642,6 +703,7 @@ static struct se_subsystem_api rd_mcp_template = {
.get_device_type= sbc_get_device_type,
.get_blocks = rd_get_blocks,
.init_prot = rd_init_prot,
+ .format_prot= rd_format_prot,
.free_prot = rd_free_prot,
};
--
1.8.4.GIT
--
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 4/4] target/rd: T10-Dif: RAM disk is allocating more space than required.
Ram disk is allocating 8x more space than required for diff data. For large RAM disk test, there is small potential for memory starvation. Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org Signed-off-by: Giridhar Malavali giridhar.malav...@qlogic.com --- drivers/target/target_core_rd.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c index 01dda0b..6df177a 100644 --- a/drivers/target/target_core_rd.c +++ b/drivers/target/target_core_rd.c @@ -253,7 +253,11 @@ static int rd_build_prot_space(struct rd_dev *rd_dev, int prot_length) if (rd_dev-rd_flags RDF_NULLIO) return 0; - total_sg_needed = rd_dev-rd_page_count / prot_length; + /* prot_length=8byte dif data +* tot sg needed = rd_page_count * (PGSZ/512) * (prot_length/PGSZ) + pad +* PGSZ canceled each other. +*/ + total_sg_needed = (rd_dev-rd_page_count * prot_length / 512) +1; sg_tables = (total_sg_needed / max_sg_per_table) + 1; -- 1.8.4.GIT -- 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 2/4] tcm_qla2xxx: T10-Dif set harware capability
Set Protection Type(1,2,3) capabilities, Guarg type (CRC/IPchksm)
capabilities bits to let TCM core knows of HW/fabric capabilities.
Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
Signed-off-by: Giridhar Malavali giridhar.malav...@qlogic.com
---
drivers/scsi/qla2xxx/tcm_qla2xxx.c | 23 +++
drivers/scsi/qla2xxx/tcm_qla2xxx.h | 1 +
2 files changed, 24 insertions(+)
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index b23a0ff..4d93081 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -910,12 +910,20 @@ DEF_QLA_TPG_ATTR_BOOL(demo_mode_login_only);
DEF_QLA_TPG_ATTRIB(demo_mode_login_only);
QLA_TPG_ATTR(demo_mode_login_only, S_IRUGO | S_IWUSR);
+/*
+ * Define tcm_qla2xxx_tpg_attrib_s_t10dif_force_on
+ */
+DEF_QLA_TPG_ATTR_BOOL(t10dif_force_on);
+DEF_QLA_TPG_ATTRIB(t10dif_force_on);
+QLA_TPG_ATTR(t10dif_force_on, S_IRUGO | S_IWUSR);
+
static struct configfs_attribute *tcm_qla2xxx_tpg_attrib_attrs[] = {
tcm_qla2xxx_tpg_attrib_generate_node_acls.attr,
tcm_qla2xxx_tpg_attrib_cache_dynamic_acls.attr,
tcm_qla2xxx_tpg_attrib_demo_mode_write_protect.attr,
tcm_qla2xxx_tpg_attrib_prod_mode_write_protect.attr,
tcm_qla2xxx_tpg_attrib_demo_mode_login_only.attr,
+ tcm_qla2xxx_tpg_attrib_t10dif_force_on.attr,
NULL,
};
@@ -1049,6 +1057,18 @@ static struct se_portal_group *tcm_qla2xxx_make_tpg(
tpg-tpg_attrib.demo_mode_write_protect = 1;
tpg-tpg_attrib.cache_dynamic_acls = 1;
tpg-tpg_attrib.demo_mode_login_only = 1;
+ tpg-tpg_attrib.t10dif_force_on = 0;
+ tpg-se_tpg.fabric_sup_prot_type = 0;
+ tpg-se_tpg.fabric_sup_guard_type = 0;
+
+ if (scsi_host_get_prot(lport-qla_vha-host)) {
+ tpg-se_tpg.fabric_sup_prot_type = (TARGET_DIF_TYPE0_PROT|
+ TARGET_DIF_TYPE1_PROT|TARGET_DIF_TYPE2_PROT|
+ TARGET_DIF_TYPE3_PROT);
+
+ tpg-se_tpg.fabric_sup_guard_type = TARGET_GUARD_CRC|
+ TARGET_GUARD_IP;
+ }
ret = core_tpg_register(tcm_qla2xxx_fabric_configfs-tf_ops, wwn,
tpg-se_tpg, tpg, TRANSPORT_TPG_TYPE_NORMAL);
@@ -1127,6 +1147,8 @@ static ssize_t tcm_qla2xxx_npiv_tpg_store_enable(
qlt_stop_phase1(vha-vha_tgt.qla_tgt);
}
+ core_tpg_set_fabric_t10dif(se_tpg, tpg-tpg_attrib.t10dif_force_on);
+
return count;
}
@@ -1169,6 +1191,7 @@ static struct se_portal_group *tcm_qla2xxx_npiv_make_tpg(
tpg-tpg_attrib.demo_mode_write_protect = 1;
tpg-tpg_attrib.cache_dynamic_acls = 1;
tpg-tpg_attrib.demo_mode_login_only = 1;
+ tpg-tpg_attrib.t10dif_force_on = 0;
ret = core_tpg_register(tcm_qla2xxx_npiv_fabric_configfs-tf_ops, wwn,
tpg-se_tpg, tpg, TRANSPORT_TPG_TYPE_NORMAL);
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.h
b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
index 33aaac8..62fdce3 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.h
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
@@ -28,6 +28,7 @@ struct tcm_qla2xxx_tpg_attrib {
int demo_mode_write_protect;
int prod_mode_write_protect;
int demo_mode_login_only;
+ int t10dif_force_on;
};
struct tcm_qla2xxx_tpg {
--
1.8.4.GIT
--
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 RFC 0/4] add T10-Dif registration for tcm_qla2xxx
Nicholas, Per our conversation at LSF, the following are the patch set of bug fix/tweak found during T10-Dif testing and add ability for QLogic FC driver to register with TCM its T10-PI capabilities. As for the rest of the other patches that does the actual T10-Dif data movement, I will submit them to target-devel and stable kernel. Quinn - Quinn Tran (4): target/core: T10-Dif: check HW support capabilities tcm_qla2xxx: T10-Dif set harware capability target/rd: T10-Dif: Add init/format support target/rd: T10-Dif: RAM disk is allocating more space than required. drivers/scsi/qla2xxx/tcm_qla2xxx.c | 23 + drivers/scsi/qla2xxx/tcm_qla2xxx.h | 1 + drivers/target/target_core_fabric_configfs.c | 20 drivers/target/target_core_rd.c | 70 +++- drivers/target/target_core_tpg.c | 9 include/target/target_core_base.h| 14 ++ include/target/target_core_fabric.h | 1 + 7 files changed, 136 insertions(+), 2 deletions(-) -- 1.8.4.GIT -- 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 RFC 0/4] add T10-Dif registration for tcm_qla2xxx
+Sagi +Martin Regards, Quinn Tran On 3/28/14 4:05 PM, Quinn Tran quinn.t...@qlogic.com wrote: Nicholas, Per our conversation at LSF, the following are the patch set of bug fix/tweak found during T10-Dif testing and add ability for QLogic FC driver to register with TCM its T10-PI capabilities. As for the rest of the other patches that does the actual T10-Dif data movement, I will submit them to target-devel and stable kernel. Quinn - Quinn Tran (4): target/core: T10-Dif: check HW support capabilities tcm_qla2xxx: T10-Dif set harware capability target/rd: T10-Dif: Add init/format support target/rd: T10-Dif: RAM disk is allocating more space than required. drivers/scsi/qla2xxx/tcm_qla2xxx.c | 23 + drivers/scsi/qla2xxx/tcm_qla2xxx.h | 1 + drivers/target/target_core_fabric_configfs.c | 20 drivers/target/target_core_rd.c | 70 +++- drivers/target/target_core_tpg.c | 9 include/target/target_core_base.h| 14 ++ include/target/target_core_fabric.h | 1 + 7 files changed, 136 insertions(+), 2 deletions(-) -- 1.8.4.GIT -- 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 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 1/4] target/core: T10-Dif: check HW support capabilities
On 3/29/2014 2:05 AM, Quinn Tran wrote:
Check lower layer/HW support of T10-dif capability.
When the LUN is linked between the underlining fabric
and back end device, the Protection Type(1,2,3) is
check against each other to make sure both side are
capable of supporting the same protection setting.
ln -s /sys/kernel/config/target/core/rd_mcp_0/q_tcm_mcp.0
/sys/kernel/config/target/qla2xxx/$L_WWPN/tpgt_1/lun/lun_0/tcm_123
Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
Signed-off-by: Giridhar Malavali giridhar.malav...@qlogic.com
---
drivers/target/target_core_fabric_configfs.c | 20
drivers/target/target_core_tpg.c | 9 +
include/target/target_core_base.h| 14 ++
include/target/target_core_fabric.h | 1 +
4 files changed, 44 insertions(+)
diff --git a/drivers/target/target_core_fabric_configfs.c
b/drivers/target/target_core_fabric_configfs.c
index 7de9f04..3ce07ec 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -777,6 +777,26 @@ static int target_fabric_port_link(
struct se_portal_group, tpg_group);
tf = se_tpg-se_tpg_wwn-wwn_tf;
+
+ if (dev-dev_attrib.pi_prot_type) {
+ uint32_t cap[] = { 0,
+ TARGET_DIF_TYPE1_PROTECTION,
+ TARGET_DIF_TYPE2_PROTECTION,
+ TARGET_DIF_TYPE3_PROTECTION};
+ uint32_t pt_bits = cap[dev-dev_attrib.pi_prot_type];
+ pt_bits = se_tpg-fabric_sup_prot_type;
At what point should the fabric fill that? it can vary between portals
right?
I would prefer to do that as a function pointer to explicitly ask the
fabric it's support.
+
+ /* cross check with fabric to see if t10dif is supported */
+ if (!pt_bits) {
+ //dev-dev_attrib.pi_prot_type = 0;
Probably should lose the comment.
+ pr_err(dev[%p]: DIF protection mismatch with fabric
+ (%s). Transport capability bits[0x%x]\n,
+ dev,
se_tpg-se_tpg_wwn-wwn_group.cg_item.ci_name,
+ se_tpg-fabric_sup_prot_type);
+ return -EFAULT;
Didn't we agree that this is allowed and the target core should
compensate on the lack fabric support?
+ }
+ }
+
if (lun-lun_se_dev != NULL) {
pr_err(Port Symlink already exists\n);
return -EEXIST;
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index c036595..9279971 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -632,6 +632,15 @@ int core_tpg_set_initiator_node_tag(
}
EXPORT_SYMBOL(core_tpg_set_initiator_node_tag);
+void core_tpg_set_fabric_t10dif(
+ struct se_portal_group *tpg,
+ uint32_t fabric_t10dif_force_on)
+{
+ tpg-fabric_t10dif_force_on = fabric_t10dif_force_on;
+}
+EXPORT_SYMBOL(core_tpg_set_fabric_t10dif);
+
Is there a user for this function in this patch?
+
static void core_tpg_lun_ref_release(struct percpu_ref *ref)
{
struct se_lun *lun = container_of(ref, struct se_lun, lun_ref);
diff --git a/include/target/target_core_base.h
b/include/target/target_core_base.h
index ec3e3a3..fc2c0ef 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -241,6 +241,17 @@ enum tcm_tmrsp_table {
TMR_FUNCTION_REJECTED = 5,
};
+enum target_guard_type_cap {
+ TARGET_GUARD_CRC = 1 0,
+ TARGET_GUARD_IP = 1 1,
+};
+
So this was dropped in previous rounds, this is more of an initiator
thing, the target will always
receive CRC from the wire and will pass it to the backing device so no
need to convert to IP_CSUM.
+enum target_prot_type_cap {
+ TARGET_DIF_TYPE1_PROTECTION = 1 0, /* T10 DIF Type 1 */
+ TARGET_DIF_TYPE2_PROTECTION = 1 1, /* T10 DIF Type 2 */
+ TARGET_DIF_TYPE3_PROTECTION = 1 2, /* T10 DIF Type 3 */
+};
+
/*
* Used for target SCSI statistics
*/
@@ -862,6 +873,9 @@ struct se_portal_group {
enum transport_tpg_type_table se_tpg_type;
/* Number of ACLed Initiator Nodes for this TPG */
u32 num_node_acls;
+ u32 fabric_t10dif_force_on;
+ u32 fabric_sup_guard_type;
+ u32 fabric_sup_prot_type;
/* Used for PR SPEC_I_PT=1 and REGISTER_AND_MOVE */
atomic_ttpg_pr_ref_count;
/* Spinlock for adding/removing ACLed Nodes */
diff --git a/include/target/target_core_fabric.h
b/include/target/target_core_fabric.h
index 1d10436..4c3dab5 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -152,6 +152,7 @@ int
Re: [PATCH 2/4] tcm_qla2xxx: T10-Dif set harware capability
On 3/29/2014 2:05 AM, Quinn Tran wrote:
Set Protection Type(1,2,3) capabilities, Guarg type (CRC/IPchksm)
capabilities bits to let TCM core knows of HW/fabric capabilities.
Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
Signed-off-by: Giridhar Malavali giridhar.malav...@qlogic.com
---
drivers/scsi/qla2xxx/tcm_qla2xxx.c | 23 +++
drivers/scsi/qla2xxx/tcm_qla2xxx.h | 1 +
2 files changed, 24 insertions(+)
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index b23a0ff..4d93081 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -910,12 +910,20 @@ DEF_QLA_TPG_ATTR_BOOL(demo_mode_login_only);
DEF_QLA_TPG_ATTRIB(demo_mode_login_only);
QLA_TPG_ATTR(demo_mode_login_only, S_IRUGO | S_IWUSR);
+/*
+ * Define tcm_qla2xxx_tpg_attrib_s_t10dif_force_on
+ */
+DEF_QLA_TPG_ATTR_BOOL(t10dif_force_on);
+DEF_QLA_TPG_ATTRIB(t10dif_force_on);
+QLA_TPG_ATTR(t10dif_force_on, S_IRUGO | S_IWUSR);
+
static struct configfs_attribute *tcm_qla2xxx_tpg_attrib_attrs[] = {
tcm_qla2xxx_tpg_attrib_generate_node_acls.attr,
tcm_qla2xxx_tpg_attrib_cache_dynamic_acls.attr,
tcm_qla2xxx_tpg_attrib_demo_mode_write_protect.attr,
tcm_qla2xxx_tpg_attrib_prod_mode_write_protect.attr,
tcm_qla2xxx_tpg_attrib_demo_mode_login_only.attr,
+ tcm_qla2xxx_tpg_attrib_t10dif_force_on.attr,
NULL,
};
@@ -1049,6 +1057,18 @@ static struct se_portal_group *tcm_qla2xxx_make_tpg(
tpg-tpg_attrib.demo_mode_write_protect = 1;
tpg-tpg_attrib.cache_dynamic_acls = 1;
tpg-tpg_attrib.demo_mode_login_only = 1;
+ tpg-tpg_attrib.t10dif_force_on = 0;
+ tpg-se_tpg.fabric_sup_prot_type = 0;
+ tpg-se_tpg.fabric_sup_guard_type = 0;
You can lose guard_type - this is more relevant to the initiator side.
+
+ if (scsi_host_get_prot(lport-qla_vha-host)) {
+ tpg-se_tpg.fabric_sup_prot_type = (TARGET_DIF_TYPE0_PROT|
+ TARGET_DIF_TYPE1_PROT|TARGET_DIF_TYPE2_PROT|
+ TARGET_DIF_TYPE3_PROT);
+
+ tpg-se_tpg.fabric_sup_guard_type = TARGET_GUARD_CRC|
+ TARGET_GUARD_IP;
+ }
ret = core_tpg_register(tcm_qla2xxx_fabric_configfs-tf_ops, wwn,
tpg-se_tpg, tpg, TRANSPORT_TPG_TYPE_NORMAL);
@@ -1127,6 +1147,8 @@ static ssize_t tcm_qla2xxx_npiv_tpg_store_enable(
qlt_stop_phase1(vha-vha_tgt.qla_tgt);
}
+ core_tpg_set_fabric_t10dif(se_tpg, tpg-tpg_attrib.t10dif_force_on);
+
Any way we can get this logic to be shared also with iscsi, srp, etc...
all fabrics should
be set with t10dif right? so I would imagine it would be better to
centralize it right?
return count;
}
@@ -1169,6 +1191,7 @@ static struct se_portal_group *tcm_qla2xxx_npiv_make_tpg(
tpg-tpg_attrib.demo_mode_write_protect = 1;
tpg-tpg_attrib.cache_dynamic_acls = 1;
tpg-tpg_attrib.demo_mode_login_only = 1;
+ tpg-tpg_attrib.t10dif_force_on = 0;
ret = core_tpg_register(tcm_qla2xxx_npiv_fabric_configfs-tf_ops, wwn,
tpg-se_tpg, tpg, TRANSPORT_TPG_TYPE_NORMAL);
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.h
b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
index 33aaac8..62fdce3 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.h
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
@@ -28,6 +28,7 @@ struct tcm_qla2xxx_tpg_attrib {
int demo_mode_write_protect;
int prod_mode_write_protect;
int demo_mode_login_only;
+ int t10dif_force_on;
};
struct tcm_qla2xxx_tpg {
--
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] target/rd: T10-Dif: Add init/format support
On 3/29/2014 2:05 AM, Quinn Tran wrote:
This patch is borrow code from
commit 0f5e2ec46dd64579c0770f3822764f94db4fa465
Author: Nicholas Bellinger n...@linux-iscsi.org
Date: Sat Jan 18 09:32:56 2014 +
target/file: Add DIF protection init/format support
This patch adds support for DIF protection init/format support into
the FILEIO backend.
It involves using a seperate $FILE.protection for storing PI that is
opened via fd_init_prot() using the common pi_prot_type attribute.
The actual formatting of the protection is done via fd_format_prot()
using the common pi_prot_format attribute, that will populate the
initial PI data based upon the currently configured pi_prot_type.
Based on original FILEIO code from Sagi.
v1 changes:
- Fix sparse warnings in fd_init_format_buf (Fengguang)
Cc: Martin K. Petersen martin.peter...@oracle.com
Cc: Christoph Hellwig h...@lst.de
Cc: Hannes Reinecke h...@suse.de
Cc: Sagi Grimberg sa...@mellanox.com
Cc: Or Gerlitz ogerl...@mellanox.com
Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
---
drivers/target/target_core_rd.c | 64 -
1 file changed, 63 insertions(+), 1 deletion(-)
diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
index 66a5aba..01dda0b 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -603,7 +603,7 @@ static int rd_init_prot(struct se_device *dev)
{
struct rd_dev *rd_dev = RD_DEV(dev);
-if (!dev-dev_attrib.pi_prot_type)
+ if (!dev-dev_attrib.pi_prot_type)
return 0;
return rd_build_prot_space(rd_dev, dev-prot_length);
@@ -616,6 +616,67 @@ static void rd_free_prot(struct se_device *dev)
rd_release_prot_space(rd_dev);
}
+static void rd_init_format_buf(struct se_device *dev, unsigned char *buf,
+ u32 unit_size, u32 *ref_tag, u16 app_tag,
+ bool inc_reftag)
+{
+ unsigned char *p = buf;
+ int i;
+
+ for (i = 0; i unit_size; i += dev-prot_length) {
+ *((u16 *)p[0]) = 0x;
+ *((__be16 *)p[2]) = cpu_to_be16(app_tag);
+ *((__be32 *)p[4]) = cpu_to_be32(*ref_tag);
+
+ if (inc_reftag)
+ (*ref_tag)++;
+
+ p += dev-prot_length;
+ }
+}
+
+static int rd_format_prot(struct se_device *dev)
+{
+ struct rd_dev *rd_dev = RD_DEV(dev);
+ u32 ref_tag = 0;
+ int i,j;
+ bool inc_reftag = false;
+ struct rd_dev_sg_table *pt;
+ struct scatterlist *sg;
+ void *paddr;
+
+ if (!dev-dev_attrib.pi_prot_type) {
+ pr_err(Unable to format_prot while pi_prot_type == 0\n);
+ return -ENODEV;
+ }
+
+ switch (dev-dev_attrib.pi_prot_type) {
+ case TARGET_DIF_TYPE3_PROT:
+ ref_tag = 0x;
+ break;
+ case TARGET_DIF_TYPE2_PROT:
+ case TARGET_DIF_TYPE1_PROT:
+ inc_reftag = true;
+ break;
+ default:
+ break;
+ }
+
+ for (i=0; i rd_dev-sg_prot_count; i++) {
+ pt= rd_dev-sg_prot_array[i];
+ for_each_sg(pt-sg_table, sg, pt-rd_sg_count, j) {
+ paddr = kmap(sg_page(sg)) + sg-offset;
+
+ rd_init_format_buf(dev, paddr, sg-length, ref_tag,
+ 0x, inc_reftag);
+ kunmap(paddr);
+ }
+ }
+
+ return 0;
+}
+
+
static struct sbc_ops rd_sbc_ops = {
.execute_rw = rd_execute_rw,
};
@@ -642,6 +703,7 @@ static struct se_subsystem_api rd_mcp_template = {
.get_device_type= sbc_get_device_type,
.get_blocks = rd_get_blocks,
.init_prot = rd_init_prot,
+ .format_prot= rd_format_prot,
.free_prot = rd_free_prot,
};
This patch is redundant altogether. rd_mcp already init protection with
escape values:
rc = rd_allocate_sgl_table(rd_dev, sg_table, total_sg_needed, 0xff);
--
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] target/rd: T10-Dif: RAM disk is allocating more space than required.
On 3/29/2014 2:05 AM, Quinn Tran wrote: Ram disk is allocating 8x more space than required for diff data. For large RAM disk test, there is small potential for memory starvation. Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org Signed-off-by: Giridhar Malavali giridhar.malav...@qlogic.com --- drivers/target/target_core_rd.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c index 01dda0b..6df177a 100644 --- a/drivers/target/target_core_rd.c +++ b/drivers/target/target_core_rd.c @@ -253,7 +253,11 @@ static int rd_build_prot_space(struct rd_dev *rd_dev, int prot_length) if (rd_dev-rd_flags RDF_NULLIO) return 0; - total_sg_needed = rd_dev-rd_page_count / prot_length; + /* prot_length=8byte dif data +* tot sg needed = rd_page_count * (PGSZ/512) * (prot_length/PGSZ) + pad +* PGSZ canceled each other. +*/ + total_sg_needed = (rd_dev-rd_page_count * prot_length / 512) +1; You probably will want to use block_size instead of hard-coding 512. Other then that this makes sense. sg_tables = (total_sg_needed / max_sg_per_table) + 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: [PATCH RFC 0/4] add T10-Dif registration for tcm_qla2xxx
On 3/29/2014 2:48 AM, Quinn Tran wrote: +Sagi +Martin Thanks Quinn! Already had a look. Regards, Quinn Tran -- 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] target/core: T10-Dif: check HW support capabilities
Answer in line.
Regards,
Quinn Tran
On 3/28/14 5:05 PM, sagi grimberg sa...@mellanox.com wrote:
On 3/29/2014 2:05 AM, Quinn Tran wrote:
Check lower layer/HW support of T10-dif capability.
When the LUN is linked between the underlining fabric
and back end device, the Protection Type(1,2,3) is
check against each other to make sure both side are
capable of supporting the same protection setting.
ln -s /sys/kernel/config/target/core/rd_mcp_0/q_tcm_mcp.0
/sys/kernel/config/target/qla2xxx/$L_WWPN/tpgt_1/lun/lun_0/tcm_123
Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
Signed-off-by: Giridhar Malavali giridhar.malav...@qlogic.com
---
drivers/target/target_core_fabric_configfs.c | 20
drivers/target/target_core_tpg.c | 9 +
include/target/target_core_base.h| 14 ++
include/target/target_core_fabric.h | 1 +
4 files changed, 44 insertions(+)
diff --git a/drivers/target/target_core_fabric_configfs.c
b/drivers/target/target_core_fabric_configfs.c
index 7de9f04..3ce07ec 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -777,6 +777,26 @@ static int target_fabric_port_link(
struct se_portal_group, tpg_group);
tf = se_tpg-se_tpg_wwn-wwn_tf;
+
+if (dev-dev_attrib.pi_prot_type) {
+uint32_t cap[] = { 0,
+ TARGET_DIF_TYPE1_PROTECTION,
+ TARGET_DIF_TYPE2_PROTECTION,
+ TARGET_DIF_TYPE3_PROTECTION};
+uint32_t pt_bits = cap[dev-dev_attrib.pi_prot_type];
+pt_bits = se_tpg-fabric_sup_prot_type;
At what point should the fabric fill that? it can vary between portals
right?
QT Yes, protection mode can vary between portals. When user select the
physical function (via fabric_make_tpg), you know the specific portal
based on user input and its capability. This is where Qlogic register its
capabilities based on specific hardware.
I would prefer to do that as a function pointer to explicitly ask the
fabric it's support.
QT is it still require with previous answer ?
+
+/* cross check with fabric to see if t10dif is supported */
+if (!pt_bits) {
+//dev-dev_attrib.pi_prot_type = 0;
Probably should lose the comment.
nod
+pr_err(dev[%p]: DIF protection mismatch with fabric
+(%s). Transport capability bits[0x%x]\n,
+dev,
se_tpg-se_tpg_wwn-wwn_group.cg_item.ci_name,
+se_tpg-fabric_sup_prot_type);
+return -EFAULT;
Didn't we agree that this is allowed and the target core should
compensate on the lack fabric support?
QT My recollection was that it's not recommended to have different
portals with different supported feature. Meaning a SCSI Write without Dif
down a none-T10PI portal update the data. The Guard on the disk is now
mismatch with the data. A SCSI Read down a T10PI path will run into a
Guard failure.
By introducing this option, Disk vendor require additional logic to
compensate for this. I think it's cheaper to have supported hardware
rather than support nightmare.
+}
+}
+
if (lun-lun_se_dev != NULL) {
pr_err(Port Symlink already exists\n);
return -EEXIST;
diff --git a/drivers/target/target_core_tpg.c
b/drivers/target/target_core_tpg.c
index c036595..9279971 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -632,6 +632,15 @@ int core_tpg_set_initiator_node_tag(
}
EXPORT_SYMBOL(core_tpg_set_initiator_node_tag);
+void core_tpg_set_fabric_t10dif(
+struct se_portal_group *tpg,
+uint32_t fabric_t10dif_force_on)
+{
+tpg-fabric_t10dif_force_on = fabric_t10dif_force_on;
+}
+EXPORT_SYMBOL(core_tpg_set_fabric_t10dif);
+
Is there a user for this function in this patch?
QT I'm on the fence with this piece. Just thinking of a case where DIX
is not available for initiator side, but user wants to turn on protection
at the link layer. Our test folks would like to cover this case in the
future.
+
static void core_tpg_lun_ref_release(struct percpu_ref *ref)
{
struct se_lun *lun = container_of(ref, struct se_lun, lun_ref);
diff --git a/include/target/target_core_base.h
b/include/target/target_core_base.h
index ec3e3a3..fc2c0ef 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -241,6 +241,17 @@ enum tcm_tmrsp_table {
TMR_FUNCTION_REJECTED = 5,
};
+enum target_guard_type_cap {
+TARGET_GUARD_CRC = 1 0,
+TARGET_GUARD_IP = 1 1,
+};
+
So this was dropped in previous rounds, this is more of an initiator
thing, the target will always
receive CRC from the wire and will pass it to the backing device so no
Re: [PATCH 1/4] target/core: T10-Dif: check HW support capabilities
On 3/29/2014 3:53 AM, Quinn Tran wrote:
+
+if (dev-dev_attrib.pi_prot_type) {
+uint32_t cap[] = { 0,
+ TARGET_DIF_TYPE1_PROTECTION,
+ TARGET_DIF_TYPE2_PROTECTION,
+ TARGET_DIF_TYPE3_PROTECTION};
+uint32_t pt_bits = cap[dev-dev_attrib.pi_prot_type];
+pt_bits = se_tpg-fabric_sup_prot_type;
At what point should the fabric fill that? it can vary between portals
right?
QT Yes, protection mode can vary between portals. When user select the
physical function (via fabric_make_tpg), you know the specific portal
based on user input and its capability. This is where Qlogic register its
capabilities based on specific hardware.
I would prefer to do that as a function pointer to explicitly ask the
fabric it's support.
QT is it still require with previous answer ?
Well, I think it is nicer to explicitly ask the fabric at that point
instead of checking what it previously set.
By the way, I think this patch breaks existing iSER support as iSER
doesn't set these bits.
Thats why I think it would be a good idea to *explicitly* ask.
+pr_err(dev[%p]: DIF protection mismatch with fabric
+(%s). Transport capability bits[0x%x]\n,
+dev, se_tpg-se_tpg_wwn-wwn_group.cg_item.ci_name,
+se_tpg-fabric_sup_prot_type);
+return -EFAULT;
Didn't we agree that this is allowed and the target core should
compensate on the lack fabric support?
QT My recollection was that it's not recommended to have different
portals with different supported feature.
Well we seem to remember different things...
Anyway I think it is better to compensate that in backstore/target-core
level, that would be better
than silently turn off protection. Martin? Nic? your takes?
Also I don't know what rats are hiding here if the backstore is handling
IOs in this time...
What about cleaning up all the protection resources the backstore driver
might be managing?
Meaning a SCSI Write without Dif
down a none-T10PI portal update the data. The Guard on the disk is now
mismatch with the data. A SCSI Read down a T10PI path will run into a
Guard failure.
By introducing this option, Disk vendor require additional logic to
compensate for this. I think it's cheaper to have supported hardware
rather than support nightmare.
+}
+}
+
if (lun-lun_se_dev != NULL) {
pr_err(Port Symlink already exists\n);
return -EEXIST;
diff --git a/drivers/target/target_core_tpg.c
b/drivers/target/target_core_tpg.c
index c036595..9279971 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -632,6 +632,15 @@ int core_tpg_set_initiator_node_tag(
}
EXPORT_SYMBOL(core_tpg_set_initiator_node_tag);
+void core_tpg_set_fabric_t10dif(
+struct se_portal_group *tpg,
+uint32_t fabric_t10dif_force_on)
+{
+tpg-fabric_t10dif_force_on = fabric_t10dif_force_on;
+}
+EXPORT_SYMBOL(core_tpg_set_fabric_t10dif);
+
Is there a user for this function in this patch?
QT I'm on the fence with this piece. Just thinking of a case where DIX
is not available for initiator side, but user wants to turn on protection
at the link layer. Our test folks would like to cover this case in the
future.
Not sure I understand. Initiator will send the target data+protection
(DIX disabled HBA does INSERT), why does this help?
Why should the target fabric care who generated the protection (OS or HBA)?
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
