* [PATCH 10/14] target: Simplify the code for waiting for command completion
@ 2018-03-01 22:26 Bart Van Assche
2018-06-11 19:02 ` Mike Christie
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Bart Van Assche @ 2018-03-01 22:26 UTC (permalink / raw)
To: target-devel
Instead of embedding the completion that is used for waiting for
command completion in struct se_cmd, let the context that waits for
command completion allocate it. This makes it possible to have a
single code path for non-aborted and aborted commands in
target_release_cmd_kref() and avoids that transport_generic_free_cmd()
has to call cmd->se_tfo->release_cmd() directly. This patch does not
change any functionality. Note: transport_generic_free_cmd() only
waits until the se_cmd reference count has reached zero after it has
set both CMD_T_FABRIC_STOP and CMD_T_ABORTED.
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Mike Christie <mchristi@redhat.com>
---
drivers/target/target_core_transport.c | 24 ++++++++----------------
include/target/target_core_base.h | 2 +-
2 files changed, 9 insertions(+), 17 deletions(-)
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index c8f19a143f5c..d7b0b2f4261e 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1276,7 +1276,7 @@ void transport_init_se_cmd(
INIT_LIST_HEAD(&cmd->se_cmd_list);
INIT_LIST_HEAD(&cmd->state_list);
init_completion(&cmd->t_transport_stop_comp);
- init_completion(&cmd->cmd_wait_comp);
+ cmd->compl = NULL;
spin_lock_init(&cmd->t_state_lock);
INIT_WORK(&cmd->work, NULL);
kref_init(&cmd->cmd_kref);
@@ -2596,6 +2596,7 @@ static void target_wait_free_cmd(struct se_cmd *cmd, bool *aborted, bool *tas)
*/
int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
{
+ DECLARE_COMPLETION_ONSTACK(compl);
int ret = 0;
bool aborted = false, tas = false;
@@ -2614,12 +2615,13 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
if (cmd->se_lun)
transport_lun_remove_cmd(cmd);
}
+ if (aborted)
+ cmd->compl = &compl;
if (!aborted || tas)
ret = target_put_sess_cmd(cmd);
if (aborted) {
pr_debug("Detected CMD_T_ABORTED for ITT: %llu\n", cmd->tag);
- wait_for_completion(&cmd->cmd_wait_comp);
- cmd->se_tfo->release_cmd(cmd);
+ wait_for_completion(&compl);
ret = 1;
}
return ret;
@@ -2679,8 +2681,8 @@ static void target_release_cmd_kref(struct kref *kref)
{
struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
struct se_session *se_sess = se_cmd->se_sess;
+ struct completion *compl = se_cmd->compl;
unsigned long flags;
- bool fabric_stop;
if (se_sess) {
spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
@@ -2689,23 +2691,13 @@ static void target_release_cmd_kref(struct kref *kref)
if (list_empty(&se_sess->sess_cmd_list))
wake_up(&se_sess->cmd_list_wq);
}
-
- spin_lock(&se_cmd->t_state_lock);
- fabric_stop = (se_cmd->transport_state & CMD_T_FABRIC_STOP) &&
- (se_cmd->transport_state & CMD_T_ABORTED);
- spin_unlock(&se_cmd->t_state_lock);
-
- if (fabric_stop) {
- spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
- target_free_cmd_mem(se_cmd);
- complete(&se_cmd->cmd_wait_comp);
- return;
- }
spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
}
target_free_cmd_mem(se_cmd);
se_cmd->se_tfo->release_cmd(se_cmd);
+ if (compl)
+ complete(compl);
}
/**
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index b9fb73f2dcbb..7248965609b5 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -473,7 +473,7 @@ struct se_cmd {
struct se_session *se_sess;
struct se_tmr_req *se_tmr_req;
struct list_head se_cmd_list;
- struct completion cmd_wait_comp;
+ struct completion *compl;
const struct target_core_fabric_ops *se_tfo;
sense_reason_t (*execute_cmd)(struct se_cmd *);
sense_reason_t (*transport_complete_callback)(struct se_cmd *, bool, int *);
--
2.16.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 10/14] target: Simplify the code for waiting for command completion
2018-03-01 22:26 [PATCH 10/14] target: Simplify the code for waiting for command completion Bart Van Assche
@ 2018-06-11 19:02 ` Mike Christie
2018-06-11 20:37 ` Bart Van Assche
2018-06-11 21:05 ` Mike Christie
2 siblings, 0 replies; 4+ messages in thread
From: Mike Christie @ 2018-06-11 19:02 UTC (permalink / raw)
To: target-devel
On 03/01/2018 04:26 PM, Bart Van Assche wrote:
> Instead of embedding the completion that is used for waiting for
> command completion in struct se_cmd, let the context that waits for
> command completion allocate it. This makes it possible to have a
> single code path for non-aborted and aborted commands in
> target_release_cmd_kref() and avoids that transport_generic_free_cmd()
> has to call cmd->se_tfo->release_cmd() directly. This patch does not
> change any functionality. Note: transport_generic_free_cmd() only
> waits until the se_cmd reference count has reached zero after it has
> set both CMD_T_FABRIC_STOP and CMD_T_ABORTED.
>
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Mike Christie <mchristi@redhat.com>
> ---
> drivers/target/target_core_transport.c | 24 ++++++++----------------
> include/target/target_core_base.h | 2 +-
> 2 files changed, 9 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index c8f19a143f5c..d7b0b2f4261e 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1276,7 +1276,7 @@ void transport_init_se_cmd(
> INIT_LIST_HEAD(&cmd->se_cmd_list);
> INIT_LIST_HEAD(&cmd->state_list);
> init_completion(&cmd->t_transport_stop_comp);
> - init_completion(&cmd->cmd_wait_comp);
> + cmd->compl = NULL;
> spin_lock_init(&cmd->t_state_lock);
> INIT_WORK(&cmd->work, NULL);
> kref_init(&cmd->cmd_kref);
> @@ -2596,6 +2596,7 @@ static void target_wait_free_cmd(struct se_cmd *cmd, bool *aborted, bool *tas)
> */
> int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
> {
> + DECLARE_COMPLETION_ONSTACK(compl);
> int ret = 0;
> bool aborted = false, tas = false;
>
> @@ -2614,12 +2615,13 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
> if (cmd->se_lun)
> transport_lun_remove_cmd(cmd);
> }
> + if (aborted)
I see that in the current code we always do a wait if the CMD_T_ABORTED
bit was set, so your patch matches that. However, is the original code
correct? In target_release_cmd_kref below we only do a wake up if both
CMD_T_FABRIC_STOP and CMD_T_ABORTED is set. If wait_for_tasksúlse and
CMD_T_ABORTED was set, we would end up waiting forever. Is that right or
can it never happen?
........
> }
> -
> - spin_lock(&se_cmd->t_state_lock);
> - fabric_stop = (se_cmd->transport_state & CMD_T_FABRIC_STOP) &&
> - (se_cmd->transport_state & CMD_T_ABORTED);
> - spin_unlock(&se_cmd->t_state_lock);
> -
> - if (fabric_stop) {
> - spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> - target_free_cmd_mem(se_cmd);
> - complete(&se_cmd->cmd_wait_comp);
> - return;
> - }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 10/14] target: Simplify the code for waiting for command completion
2018-03-01 22:26 [PATCH 10/14] target: Simplify the code for waiting for command completion Bart Van Assche
2018-06-11 19:02 ` Mike Christie
@ 2018-06-11 20:37 ` Bart Van Assche
2018-06-11 21:05 ` Mike Christie
2 siblings, 0 replies; 4+ messages in thread
From: Bart Van Assche @ 2018-06-11 20:37 UTC (permalink / raw)
To: target-devel
T24gTW9uLCAyMDE4LTA2LTExIGF0IDE0OjAyIC0wNTAwLCBNaWtlIENocmlzdGllIHdyb3RlOg0K
PiBJIHNlZSB0aGF0IGluIHRoZSBjdXJyZW50IGNvZGUgd2UgYWx3YXlzIGRvIGEgd2FpdCBpZiB0
aGUgQ01EX1RfQUJPUlRFRA0KPiBiaXQgd2FzIHNldCwgc28geW91ciBwYXRjaCBtYXRjaGVzIHRo
YXQuIEhvd2V2ZXIsIGlzIHRoZSBvcmlnaW5hbCBjb2RlDQo+IGNvcnJlY3Q/IEluIHRhcmdldF9y
ZWxlYXNlX2NtZF9rcmVmIGJlbG93IHdlIG9ubHkgZG8gYSB3YWtlIHVwIGlmIGJvdGgNCj4gQ01E
X1RfRkFCUklDX1NUT1AgYW5kIENNRF9UX0FCT1JURUQgaXMgc2V0LiBJZiB3YWl0X2Zvcl90YXNr
cz1mYWxzZSBhbmQNCj4gQ01EX1RfQUJPUlRFRCB3YXMgc2V0LCB3ZSB3b3VsZCBlbmQgdXAgd2Fp
dGluZyBmb3JldmVyLiBJcyB0aGF0IHJpZ2h0IG9yDQo+IGNhbiBpdCBuZXZlciBoYXBwZW4/DQoN
CkhlbGxvIE1pa2UsDQoNCnRyYW5zcG9ydF9nZW5lcmljX2ZyZWVfY21kKCkgY2FsbHMgdGFyZ2V0
X3dhaXRfZnJlZV9jbWQoKSBiZWZvcmUgd2FpdGluZw0Kb24gY21kLT5jbWRfd2FpdF9jb21wIGZv
ciBhYm9ydGVkIGNvbW1hbmRzLiB0YXJnZXRfd2FpdF9mcmVlX2NtZCgpIGNhbGxzDQpfX3RyYW5z
cG9ydF93YWl0X2Zvcl90YXNrcygpIHdpdGggdGhlIGZhYnJpY19zdG9wIGFyZ3VtZW50IHNldCB0
byB0cnVlDQphbmQgdGhhdCBjYXVzZXMgdGhlIENNRF9UX0ZBQlJJQ19TVE9QIGZsYWcgdG8gYmUg
c2V0Lg0KDQpCYXJ0Lg0KDQoNCg0K
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 10/14] target: Simplify the code for waiting for command completion
2018-03-01 22:26 [PATCH 10/14] target: Simplify the code for waiting for command completion Bart Van Assche
2018-06-11 19:02 ` Mike Christie
2018-06-11 20:37 ` Bart Van Assche
@ 2018-06-11 21:05 ` Mike Christie
2 siblings, 0 replies; 4+ messages in thread
From: Mike Christie @ 2018-06-11 21:05 UTC (permalink / raw)
To: target-devel
On 06/11/2018 03:37 PM, Bart Van Assche wrote:
> On Mon, 2018-06-11 at 14:02 -0500, Mike Christie wrote:
>> I see that in the current code we always do a wait if the CMD_T_ABORTED
>> bit was set, so your patch matches that. However, is the original code
>> correct? In target_release_cmd_kref below we only do a wake up if both
>> CMD_T_FABRIC_STOP and CMD_T_ABORTED is set. If wait_for_tasksúlse and
>> CMD_T_ABORTED was set, we would end up waiting forever. Is that right or
>> can it never happen?
>
> Hello Mike,
>
> transport_generic_free_cmd() calls target_wait_free_cmd() before waiting
> on cmd->cmd_wait_comp for aborted commands. target_wait_free_cmd() calls
> __transport_wait_for_tasks() with the fabric_stop argument set to true
> and that causes the CMD_T_FABRIC_STOP flag to be set.
>
Yeah, I see it's always set together now. Just misread the code.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-06-11 21:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-01 22:26 [PATCH 10/14] target: Simplify the code for waiting for command completion Bart Van Assche
2018-06-11 19:02 ` Mike Christie
2018-06-11 20:37 ` Bart Van Assche
2018-06-11 21:05 ` Mike Christie
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.