[PATCH 0/3] target: Fixes for COMPARE_AND_WRITE backend I/O failure cases

[PATCH 0/3] target: Fixes for COMPARE_AND_WRITE backend I/O failure cases

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Hi folks,

This series addresses a handful of COMPARE_AND_WRITE backend I/O failure
handling bugs reported recently by Thomas Glanzmann.

This includes a data_length handling bug that was specific to FILEIO
backends, and recursive callback bug that ended up triggered a double
complete OOPs, and another compare_and_write_callback() specific check
to avoid a bug with asychronous IBLOCK bio completion failures.

With these fixes in place, COMPARE_AND_WRITE emulation should now be
handling all backend specific I/O failure cases correctly.

Thomas, these are all fairly obvious fixes to me, and I've been able
to confirm them on my setup.  Please confirm on your end, and I'll
plan to push them for v3.12-rc4 by the end of the week.

Thanks!

--nab

Nicholas Bellinger (3):
  target: Reset data_length for COMPARE_AND_WRITE to NoLB * block_size
  target: Fix recursive COMPARE_AND_WRITE callback failure
  target: Fail on non zero scsi_status in compare_and_write_callback

 drivers/target/target_core_sbc.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

-- 
1.8.4.rc3

[PATCH 1/3] target: Reset data_length for COMPARE_AND_WRITE to NoLB * block_size

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch resets se_cmd->data_length for COMPARE_AND_WRITE emulation
within sbc_compare_and_write() to NoLB * block_size in order to address
a bug with FILEIO backends where a I/O failure will occur when data_length
does not match the I/O size being actually dispatched for the individual
per block READs + WRITEs.

This is done late enough in sbc_compare_and_write() after the memory
allocations have occured in transport_generic_new_cmd() to not cause
any unwanted side-effects.

Reported-by: Thomas Glanzmann <thomas@glanzmann.de>
Cc: Thomas Glanzmann <thomas@glanzmann.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_sbc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 6c17295..a9dca11 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -508,6 +508,12 @@ sbc_compare_and_write(struct se_cmd *cmd)
 		cmd->transport_complete_callback = NULL;
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 	}
+	/*
+	 * Reset cmd->data_length to individual block_size in order to not
+	 * confuse backend drivers that depend on this value matching the
+	 * size of the I/O being submitted.
+	 */
+	cmd->data_length = cmd->t_task_nolb * dev->dev_attrib.block_size;
 
 	ret = cmd->execute_rw(cmd, cmd->t_bidi_data_sg, cmd->t_bidi_data_nents,
 			      DMA_FROM_DEVICE);
-- 
1.8.4.rc3

[PATCH 2/3] target: Fix recursive COMPARE_AND_WRITE callback failure

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch addresses a bug when compare_and_write_callback() invoked from
target_complete_ok_work() hits an failure from __target_execute_cmd() ->
cmd->execute_cmd(), that ends up calling transport_generic_request_failure()
-> compare_and_write_post(), thus causing SCF_COMPARE_AND_WRITE_POST to
incorrectly be set.

The result of this bug is that target_complete_ok_work() no longer hits
the if (!rc && !(cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE_POST) check
that forces an immediate return, and instead double completes the se_cmd
in question, triggering an OOPs in the process.

This patch changes compare_and_write_post() to only set this bit when a
failure has not already occured to ensure the immediate return from within
target_complete_ok_work(), and thus allow transport_generic_request_failure()
to handle the sending of the CHECK_CONDITION exception status.

Reported-by: Thomas Glanzmann <thomas@glanzmann.de>
Cc: Thomas Glanzmann <thomas@glanzmann.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_sbc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index a9dca11..1393d0e 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -349,7 +349,16 @@ static sense_reason_t compare_and_write_post(struct se_cmd *cmd)
 {
 	struct se_device *dev = cmd->se_dev;
 
-	cmd->se_cmd_flags |= SCF_COMPARE_AND_WRITE_POST;
+	/*
+	 * Only set SCF_COMPARE_AND_WRITE_POST to force a response fall-through
+	 * within target_complete_ok_work() if the command was successfully
+	 * sent to the backend driver.
+	 */
+	spin_lock_irq(&cmd->t_state_lock);
+	if ((cmd->transport_state & CMD_T_SENT) && !cmd->scsi_status)
+		cmd->se_cmd_flags |= SCF_COMPARE_AND_WRITE_POST;
+	spin_unlock_irq(&cmd->t_state_lock);
+
 	/*
 	 * Unlock ->caw_sem originally obtained during sbc_compare_and_write()
 	 * before the original READ I/O submission.
-- 
1.8.4.rc3

[PATCH 3/3] target: Fail on non zero scsi_status in compare_and_write_callback

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch addresses a bug for backends such as IBLOCK that perform
asynchronous completion via transport_complete_cmd(), that will call
target_complete_failure_work() -> transport_generic_request_failure(),
upon exception status and invoke cmd->transport_complete_callback()
-> compare_and_write_callback() incorrectly during the failure case.

It adds a check for a non zero se_cmd->scsi_status within the first
invocation of compare_and_write_callback(), and will jump to out plus
up se_device->caw_sem before exiting the callback.

Reported-by: Thomas Glanzmann <thomas@glanzmann.de>
Cc: Thomas Glanzmann <thomas@glanzmann.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_sbc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 1393d0e..4714c6f 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -372,7 +372,7 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd)
 {
 	struct se_device *dev = cmd->se_dev;
 	struct scatterlist *write_sg = NULL, *sg;
-	unsigned char *buf, *addr;
+	unsigned char *buf = NULL, *addr;
 	struct sg_mapping_iter m;
 	unsigned int offset = 0, len;
 	unsigned int nlbas = cmd->t_task_nolb;
@@ -387,6 +387,15 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd)
 	 */
 	if (!cmd->t_data_sg || !cmd->t_bidi_data_sg)
 		return TCM_NO_SENSE;
+	/*
+	 * Immediately exit + release dev->caw_sem if command has already
+	 * been failed with a non-zero SCSI status.
+	 */
+	if (cmd->scsi_status) {
+		pr_err("compare_and_write_callback: non zero scsi_status:"
+			" 0x%02x\n", cmd->scsi_status);
+		goto out;
+	}
 
 	buf = kzalloc(cmd->data_length, GFP_KERNEL);
 	if (!buf) {
-- 
1.8.4.rc3

Re: [PATCH 0/3] target: Fixes for COMPARE_AND_WRITE backend I/O failure cases

[Thomas Glanzmann]

Hello Nab,

> Thomas, these are all fairly obvious fixes to me, and I've been able
> to confirm them on my setup.  Please confirm on your end, and I'll
> plan to push them for v3.12-rc4 by the end of the week.

I confirm that the fixes work. Thank you for fixing this. I did the
following:

        - Applied the patches on top of your for-next, recompiled,
          installed modules and kernel got rid of
          '/lib/modules/3.11.0-rc5+/extra/' which contained target
          modules with an unknown symbol __dynamic_pr_debug (from the
          debug build, I assume)

        - Started the target, discovered from two esx servers, created
          VMFS (no error), deployed one VM, used XCOPY to clone it three
          times, powered it on and did 4 svmotion in parallel.

So the fix works. I also have the feeling that the xcopy code became
faster, but I first have to meassure it.

I'll today some more testing with rescans from 12 ESX and svmotion
(XCOPY) of 24 VMs in parallel. But everything is looking good from my
site. I'm also working on the discovery patch.

Cheers,
        Thomas

Re: [PATCH 0/3] target: Fixes for COMPARE_AND_WRITE backend I/O failure cases

[Nicholas A. Bellinger]

On Wed, 2013-10-02 at 06:31 +0200, Thomas Glanzmann wrote:
> Hello Nab,
> 
> > Thomas, these are all fairly obvious fixes to me, and I've been able
> > to confirm them on my setup.  Please confirm on your end, and I'll
> > plan to push them for v3.12-rc4 by the end of the week.
> 
> I confirm that the fixes work. Thank you for fixing this. I did the
> following:
> 
>         - Applied the patches on top of your for-next, recompiled,
>           installed modules and kernel got rid of
>           '/lib/modules/3.11.0-rc5+/extra/' which contained target
>           modules with an unknown symbol __dynamic_pr_debug (from the
>           debug build, I assume)
> 
>         - Started the target, discovered from two esx servers, created
>           VMFS (no error), deployed one VM, used XCOPY to clone it three
>           times, powered it on and did 4 svmotion in parallel.
> 
> So the fix works. I also have the feeling that the xcopy code became
> faster, but I first have to meassure it.
> 

Great, thanks for the confirmation here.

> I'll today some more testing with rescans from 12 ESX and svmotion
> (XCOPY) of 24 VMs in parallel. But everything is looking good from my
> site. I'm also working on the discovery patch.
> 

There is one other regression wrt to the struct iscsi_cmd pre-allocation
changes in v3.12-rc1 code that I'm still tracking down, that you may end
up encountering during stress testing.

It's currently caused by tag starvation under heavy (remotely generated)
I/O load, and manifests itself as percpu_ida_alloc() sleeping
indefinitely in iscsit_allocate_cmd() waiting for new tag(s) to become
available while StatSN acknowledgement is (AFAICT) being blocked
elsewhere.

FYI, the output of 'echo w > /proc/sysrq-trigger', and/or the hung task
checker should produce a stack-trace to this effect.

Please let me know if you encounter any issues, and a patch to address
this bit should be along in the next 24-48 hours.

--nab

Re: [PATCH 0/3] target: Fixes for COMPARE_AND_WRITE backend I/O failure cases

[Thomas Glanzmann]

Hallo Nab,

> Please let me know if you encounter any issues, and a patch to address
> this bit should be along in the next 24-48 hours.

I just did a little bit of stress testing:

        - Rescan from 12 Initiators at the same time - PASS
        - Deployed 24 VMs over 12 Initiators at the same time 200 MB/s
          frontend traffic - PASS
        - Booted 24 VMs on 12 ESX servers at the same time - 120 MB/s
          frontend traffic - PASS
        - Did 24 svMotion at the same time - 400 MB/s backend traffic -
          10 MB/s frontend traffic using XCOPY. - PASS
        - Unloaded the target in production, ran 'grep se_tmr
          /proc/slabinfo' and loaded it again. And made sure that the
          VMs continued to run. - PASS

It looks pretty stable to me. I'll use it next week in a class and
report back if I had any issues whatsoever. This was with:

v3.12-rc3-4-g8a77fe9

Cheers,
        Thomas

Re: [PATCH 0/3] target: Fixes for COMPARE_AND_WRITE backend I/O failure cases

[Nicholas A. Bellinger]

On Thu, 2013-10-03 at 13:38 +0200, Thomas Glanzmann wrote:
> Hallo Nab,
> 
> > Please let me know if you encounter any issues, and a patch to address
> > this bit should be along in the next 24-48 hours.
> 
> I just did a little bit of stress testing:
> 
>         - Rescan from 12 Initiators at the same time - PASS
>         - Deployed 24 VMs over 12 Initiators at the same time 200 MB/s
>           frontend traffic - PASS
>         - Booted 24 VMs on 12 ESX servers at the same time - 120 MB/s
>           frontend traffic - PASS
>         - Did 24 svMotion at the same time - 400 MB/s backend traffic -
>           10 MB/s frontend traffic using XCOPY. - PASS
>         - Unloaded the target in production, ran 'grep se_tmr
>           /proc/slabinfo' and loaded it again. And made sure that the
>           VMs continued to run. - PASS
> 
> It looks pretty stable to me. I'll use it next week in a class and
> report back if I had any issues whatsoever. This was with:
> 
> v3.12-rc3-4-g8a77fe9
> 

Excellent, thanks for the additional testing feedback.  :-)

Btw, the tag starvation regression mentioned in the last mail only seems
to be triggered on 10 Gb/sec links, and the patches that I'm testing now
seem to address this issue.

These patches will be going out to the list later today, and I'd very
much appreciate if you could include these in your ESX client testing.

Thanks again!

--nab