headers
Nicholas A. Bellinger | 1 Oct 2011 14:55

[PATCH] tcm_qla2xxx: Wait for LUN_RESET aborted WRITEs to post via CTIO interrupt

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

This patch addresses a bug where LUN_RESET aborted WRITEs could be released
before a CTIO interrupt was able to post from hardware to acknowledge the
aborted descriptor on the fabric.

The fix adds a check in tcm_qla2xxx_write_pending_status for TRANSPORT_WRITE_PENDING
status to wait for completion timeout in processing thread context while LUN_RESET
is performed, and adds the inverse check within tcm_qla2xxx_handle_data() to
determine when to finish completion when qla_tgt_cmd->write_data_transferred == 0
and se_cmd->t_transport_aborted != 0 once the CTIO interrupt has been triggered.

This was first noticed with the following OOPs on .32 backports for the following
descriptor: cmd: ffff88016709c300

[  609.414494] LUN_RESET:  cmd: ffff88016709c300 task: ffff8801dea81b60 ITT/CmdSN:
0x00116e20/0x00000000, i_state: 0, t_state/def_t_state: 3/0 cdb: 0x2a
[  609.414497] LUN_RESET: ITT[0x00116e20] - pr_res_key: 0x0000000000000000 t_task_cdbs: 8
t_task_cdbs_left: 8 t_task_cdbs_sent: 0 -- t_transport_active: 0 t_transport_stop: 0
t_transport_sent: 0
[  609.414500] LUN_RESET: Got t_transport_active = 0 for task: ffff8801dea81b60, t_fe_count: 1 dev: ffff8801d33b66c0
[  609.670008] LUN_RESET:  from Device Queue: cmd: ffff8801df199c40 t_state: 9 t_fe_count: 0
[  609.670017] LUN_RESET:  from Device Queue: cmd: ffff8801df199440 t_state: 9 t_fe_count: 0
[  609.670021] LUN_RESET:  from Device Queue: cmd: ffff8801df199840 t_state: 9 t_fe_count: 0
[  609.670025] LUN_RESET:  from Device Queue: cmd: ffff8801df19a040 t_state: 9 t_fe_count: 0
[  609.670030] LUN_RESET: TMR for [iblock] Complete
[  609.670032] queue_tm_rsp: mcmd: ffff8801df199000 func: 0x05 response: 0x00
[  609.676015] ------------[ cut here ]------------
[  609.676017] kernel BUG at /usr/src/lio-core-backport.git/kernel/drivers/scsi/qla2xxx/qla_target.c:2818!
[  609.676019] invalid opcode: 0000 [#1] SMP
(Continue reading)

Permalink | Reply | 0 comments |
headers
Nicholas A. Bellinger | 1 Oct 2011 19:13

[PATCH] target: Remove non-active tasks from execute list during LUN_RESET

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

This patch fixes a piece of left-over breakage after the re-org of
core_tmr_lun_reset() in core_tmr_drain_task_list() in patch:

     target: Re-org of core_tmr_lun_reset + FREE_CMD_INTR bugfix

that was preventing non-active tasks from being properly removed from
task->t_execute_list (following the original logic) before moving the
selected descriptor onto the drain_task_list for final processing.

This was causing the following OOPs where left-over descriptors with
TRANSPORT_PROCESSING status (as cmd: ffff880012882b40 below) where
causing se_tasks to be incorrectly executed after an LUN_RESET task
drain in core_tmr_drain_task_list() had completed.

This patch will be folded into the original re-org before heading
into v3.2 mainline + Cc: stable <at> kernel.org.

[ 2351.515219] qla2xxx 0000:03:00.1: LIP occurred (0).
[ 2351.520694] qla_target(0): session for port 21:00:00:24:ff:31:4c:4c (loop ID 0) scheduled for
deletion in 35 secs
[ 2351.536315] qla_target(0): local session for port 21:00:00:24:ff:31:4c:4c (loop ID 0) reappeared
[ 2351.546113] qla_target(0): local session for port 21:00:00:24:ff:31:4c:4c (loop ID 0) became global
[ 2365.482752] LUN_RESET: TMR caller fabric: qla2xxx initiator port 21:00:00:24:ff:31:4c:4d
[ 2365.491785] LUN_RESET: TMR starting for [iblock], tas: 1

<SNIP>

[ 2376.689805] LUN_RESET: ITT[0x00128f40] - pr_res_key: 0x0000000000000000 t_task_cdbs: 8
(Continue reading)

Permalink | Reply | 0 comments |
headers
AHMMAD YOUNIS | 4 Oct 2011 13:21
Picon
Favicon

Go through this:

Go through this:
http://en.wikipedia.org/wiki/Abdul_Fatah_Younis
Pls help me 
safegaurd this $42M USD currently loged in a security company in Europe by my 
late father Abdul Fatah Younis, who was arrested and killed by rebel force 
(NFSL). I will give you more detail upon ur acceptance.
Ahmmad Younis
Libya
Permalink | Reply | 0 comments |
headers
Paolo Bonzini | 5 Oct 2011 14:23
Picon
Favicon
Gravatar

oops with tcm_loop

When I do this:

modprobe tcm_loop
mount -t configfs configfs /sys/kernel/config/

cd /sys/kernel/config/target/core
mkdir fileio_0
mkdir fileio_0/fileio
echo 'fd_dev_name='$HOME/test2.img',fd_dev_size=5368709120' > fileio_0/fileio/control
echo 1 > fileio_0/fileio/enable

cd /sys/kernel/config/target
mkdir -p loopback/naa.600140554cf3a18e/tpgt_1
mkdir -p loopback/naa.600140554cf3a18e/tpgt_1/lun/lun_1
cd loopback/naa.600140554cf3a18e/tpgt_1
# "Obvious" error here: need to set up nexus before virtual_scsi_port!
ln -sf ../../../core/fileio_0/fileio lun/lun_1/virtual_scsi_port
echo naa.60014053226f0388 > nexus
echo '- - -' > /sys/bus/tcm_loop_bus/drivers/tcm_loop/tcm_loop_adapter_0/host*/scsi_host/host*/scan

... I get the following two OOPSes:

[  264.135309] BUG: unable to handle kernel NULL pointer dereference at 0000000000000090
[  264.136279] IP: [<ffffffffa0510498>] transport_processing_thread+0x61b/0xc8b [target_core_mod]
[  264.136279] PGD 0
[  264.136279] Oops: 0000 [#1] SMP
[  264.136279] CPU 0
[  264.136279] Modules linked in: target_core_pscsi target_core_file target_core_iblock tcm_loop
target_core_mod configfs ecryptfs sha256_generic encrypted ecryptfs_format trusted ebtable_nat
ebtables xt_CHECKSUM iptable_mangle bridge rfcomm btusb stp llc bnep lockd bluetooth
(Continue reading)

Permalink | Reply | 1 comment |
headers
Roland Dreier | 6 Oct 2011 18:56
Favicon
Gravatar

[PATCH] target: Have core_tmr_alloc_req() take an explicit GFP_xxx flag

Testing in_interrupt() to know when sleeping is allowed is not really
reliable (since eg it won't be true if the caller is holding a spinlock).
Instead have the caller tell core_tmr_alloc_req() what GFP_xxx to use;
every caller except tcm_qla2xxx can use GFP_KERNEL.

Signed-off-by: Roland Dreier <roland <at> purestorage.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c           |    2 +-
 drivers/target/iscsi/iscsi_target_util.c        |    3 ++-
 drivers/target/loopback/tcm_loop.c              |    2 +-
 drivers/target/target_core_tmr.c                |    6 +++---
 drivers/target/tcm_fc/tfc_cmd.c                 |    2 +-
 drivers/target/tcm_qla2xxx/tcm_qla2xxx_fabric.c |    2 +-
 include/target/target_core_tmr.h                |    2 +-
 7 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 60415e1..385acf2 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
 <at>  <at>  -1899,7 +1899,7  <at>  <at>  static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
 			TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
 		goto process_tmr;
 	}
-	cmd->se_tmr_req = core_tmr_alloc_req(cmd, NULL, tcm_tmr);
+	cmd->se_tmr_req = core_tmr_alloc_req(cmd, NULL, tcm_tmr, GFP_KERNEL);
 	if (!cmd->se_tmr_req) {
 		send_ioctx->cmd.se_cmd_flags |= SCF_SCSI_CDB_EXCEPTION;
 		send_ioctx->cmd.se_tmr_req->response = TMR_FUNCTION_REJECTED;
diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
(Continue reading)

Permalink | Reply | 2 comments |
headers
Andy Grover | 7 Oct 2011 02:48
Picon
Favicon
Gravatar

targetcli package name

Hi Ritesh,

It may be a dumb question, but were you planning on packaging targetcli
as "targetcli" or something else?

Thanks -- Regards -- Andy
Permalink | Reply | 0 comments |
headers
Andy Grover | 7 Oct 2011 18:45
Picon
Favicon
Gravatar

Re: targetcli package name

On 10/07/2011 01:31 AM, Ritesh Raj Sarraf wrote:
> It may be a dumb question, but were you planning on packaging
> targetcli as "targetcli" or something else?
> As targetcli only.

Great, just wanted to check. Same here. One less cross-distro issue for
users to worry about :)

Thanks -- Regards -- Andy
Permalink | Reply | 0 comments |
headers
Nicholas A. Bellinger | 8 Oct 2011 23:11

Re: [PATCH 4/6] target: simplify transport_put_cmd

On Tue, 2011-09-13 at 23:08 +0200, Christoph Hellwig wrote:
> Inline two simple functions only used by it, and replace a goto
> with a simple if else construct.
> 
> Note that the code moved from transport_dec_and_check seems fairly
> buggy - the atomic_read check on a variable where we'd do an
> atomic_dec_and_test looks racy if we'll ever get someone increment
> it without the lock held around them (which it looks like we do),
> and not decrementing the second counter if the first one doesn't
> hit zero also at least needs an explanation.
> 

Hey Christoph,

Sorry for the long delay here.  I've finally been reviewing this code.
Comments are below..

> Signed-off-by: Christoph Hellwig <hch <at> lst.de>
> 
> Index: lio-core/drivers/target/target_core_transport.c
> ===================================================================
> --- lio-core.orig/drivers/target/target_core_transport.c	2011-09-13 14:54:21.892088669 -0400
> +++ lio-core/drivers/target/target_core_transport.c	2011-09-13 14:54:25.125089859 -0400
>  <at>  <at>  -3754,36 +3754,6  <at>  <at>  static inline void transport_free_pages(
>  	cmd->t_bidi_data_nents = 0;
>  }
>  
> -static inline void transport_release_tasks(struct se_cmd *cmd)
> -{
> -	transport_free_dev_tasks(cmd);
(Continue reading)

Permalink | Reply | 2 comments |
headers
Nicholas A. Bellinger | 8 Oct 2011 23:17

Re: [PATCH 6/6] target: push session reinstatement out of transport_generic_free_cmd

On Tue, 2011-09-13 at 23:09 +0200, Christoph Hellwig wrote:
> Push session reinstatement out of transport_generic_free_cmd into the only
> caller that actually needs it.  Clean up transport_generic_free_cmd a bit,
> and remove the useless comment.  I'd love to add a more useful kerneldoc
> comment for it, but as this point I'm still a bit confused in where it
> stands in the command release stack.
> 
> Signed-off-by: Christoph Hellwig <hch <at> lst.de>
> 

After taking another look at the 'session_reinstatement' bit, I think
it's legacy cruft for iscsi-target, and it's safe to drop it entirely.

Along with your original series, i'm including the following patch.

Thanks,

commit a10d32e2a2d86557de9ed88a78f5b537fac3bc9f
Author: Nicholas Bellinger <nab <at> linux-iscsi.org>
Date:   Sat Oct 8 13:59:52 2011 -0700

    target: Remove session_reinstatement parameter from ->transport_wait_for_tasks

    This patch removes the unnecessary session_reinstatement parameter from
    se_cmd->transport_wait_for_tasks(), logic in transport_generic_wait_for_tasks,
    and usage within iscsi-target code.

    Signed-off-by: Nicholas Bellinger <nab <at> linux-iscsi.org>

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
(Continue reading)

Permalink | Reply | 8 comments |
headers
Nicholas A. Bellinger | 8 Oct 2011 23:38

Re: [PATCH 0/6] command freeing cleanups

On Tue, 2011-09-13 at 23:08 +0200, Christoph Hellwig wrote:
> This series cleans up various lose ends when freeing struct se_cmds.
> 
> There are two things I stumbled upon when doing this which rather confuse
> me:
> 
>  (1) what exactly is the purpose of the check_stop_free fabric method.
>      Generally this ends up as a transport_generic_free_cmd with just
>      a few notable differences:
> 
>        - srpt decrements an internal reference count, which ends
> 	 up calling transport_generic_free_cmd once it hits zero
>        - tcm_fc does not ignore TMR requests like all others
>        - qla2xxxx cals transport_generic_free_cmd for tmr request,
> 	 but does not do it for normal requests
> 

->check_stop_free() was originally added for tcm_loop, which is able to
release it's descriptor in the main callback path, eg:
scsi_cmnd->scsi_done is called directly within ->queue_data_in() and
->queue_status, and we don't have to wait for a fabric acknowledge
before releasing..

tcm_fc is using ->check_stop_free() in the same manner as tcm_loop, to
directly release se_cmd immediately via transport_generic_free_cmd()

>      I seems rather counter productive to not have a common model
>      here, and I don't quite understand what qla2xxx and the srp
>      target are trying to archive
>
(Continue reading)

Permalink | Reply | 2 comments |

Gmane