From: "Nicholas A. Bellinger" <nab@linux-iscsi.org>
To: Michael Cyr <mikecyr@linux.vnet.ibm.com>
Cc: rjui@broadcom.com, sbranden@broadcom.com, jonmason@broadcom.com,
linux-scsi@vger.kernel.org, target-devel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
bcm-kernel-feedback-list@broadcom.com,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Fix for hang of Ordered task in TCM
Date: Mon, 23 May 2016 22:31:44 -0700 [thread overview]
Message-ID: <1464067904.22249.53.camel@haakon3.risingtidesystems.com> (raw)
In-Reply-To: <82ddb84d-1a3c-f7ad-3f91-0a9506317bdf@linux.vnet.ibm.com>
On Wed, 2016-05-18 at 14:35 -0500, Michael Cyr wrote:
> On 5/18/16 12:53 AM, Nicholas A. Bellinger wrote:
> > Hi Michael,
> >
> > On Fri, 2016-05-13 at 17:15 -0500, Michael Cyr wrote:
> >> If a command with a Simple task attribute is failed due to a Unit
> >> Attention, then a subsequent command with an Ordered task attribute will
> >> hang forever. The reason for this is that the Unit Attention status is
> >> checked for in target_setup_cmd_from_cdb, before the call to
> >> target_execute_cmd, which calls target_handle_task_attr, which in turn
> >> increments dev->simple_cmds. However, transport_generic_request_failure
> >> still calls transport_complete_task_attr, which will decrement
> >> dev->simple_cmds. In this case, simple_cmds is now -1. So when a
> >> command with the Ordered task attribute is sent, target_handle_task_attr
> >> sees that dev->simple_cmds is not 0, so it decides it can't execute the
> >> command until all the (nonexistent) Simple commands have completed.
> >>
> > Thanks for reporting this bug. Comments below.
<SNIP>
> > So AFAICT for delayed commands, the above patch ends up skipping these
> > three checks subsequently when doing __target_execute_cmd() directly
> > from target_restart_delayed_cmds(), no..?
> >
> > After pondering this some more, what about moving these checks into
> > __target_execute_cmd() to handle both target_core_transport.c cases
> > instead..?
>>
> You're right, __target_execute_cmd is clearly the right place for the
> checks.
>
> > We'll also need a parameter for internal COMPARE_AND_WRITE usage
> > within compare_and_write_callback(), to bypass checks upon secondary
> > ->execute_cmd() WRITE payload submission after READ + COMPARE has
> > completed successfully.
>>
> I'm still learning about the target code, and I was unaware of
> COMPARE_AND_WRITE. Thanks for pointing this out to me. You're right
> here too, we need to make sure we only make the tests when the Compare
> and Write is first executed (to do the read), and not later when the
> write is done.
> >
> > WDYT..?
>>
> You've covered all the places that call __target_execute_cmd, so I think
> it's good.
Thanks for the feedback.
Given the nature of the change, it needs some more Reviewed-by +
Tested-By (across different drivers) ahead of pushing this patch into
mainline.
So for the moment it's in target-pending/queue as a post v4.7-rc1 item
here:
https://git.kernel.org/cgit/linux/kernel/git/nab/target-pending.git/commit/?h=queue&id=0c141c33b667b6538e38b87db87232523bcd4f5b
next prev parent reply other threads:[~2016-05-24 5:31 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-13 22:15 [PATCH] Fix for hang of Ordered task in TCM Michael Cyr
2016-05-18 5:53 ` Nicholas A. Bellinger
2016-05-18 19:35 ` Michael Cyr
2016-05-24 5:31 ` Nicholas A. Bellinger [this message]
2016-06-08 19:43 ` [PATCH] target: " Bryant G. Ly
2016-06-09 5:38 ` Nicholas A. Bellinger
2016-05-23 23:17 ` [PATCH] " Bryant G Ly
2016-05-24 5:47 ` Nicholas A. Bellinger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1464067904.22249.53.camel@haakon3.risingtidesystems.com \
--to=nab@linux-iscsi.org \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=jonmason@broadcom.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=mikecyr@linux.vnet.ibm.com \
--cc=rjui@broadcom.com \
--cc=sbranden@broadcom.com \
--cc=target-devel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).