| Message ID | 20180301222632.31507-11-bart.vanassche@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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=false 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; > - } -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
T24gTW9uLCAyMDE4LTA2LTExIGF0IDE0OjAyIC0wNTAwLCBNaWtlIENocmlzdGllIHdyb3RlOg0K PiBJIHNlZSB0aGF0IGluIHRoZSBjdXJyZW50IGNvZGUgd2UgYWx3YXlzIGRvIGEgd2FpdCBpZiB0 aGUgQ01EX1RfQUJPUlRFRA0KPiBiaXQgd2FzIHNldCwgc28geW91ciBwYXRjaCBtYXRjaGVzIHRo YXQuIEhvd2V2ZXIsIGlzIHRoZSBvcmlnaW5hbCBjb2RlDQo+IGNvcnJlY3Q/IEluIHRhcmdldF9y ZWxlYXNlX2NtZF9rcmVmIGJlbG93IHdlIG9ubHkgZG8gYSB3YWtlIHVwIGlmIGJvdGgNCj4gQ01E X1RfRkFCUklDX1NUT1AgYW5kIENNRF9UX0FCT1JURUQgaXMgc2V0LiBJZiB3YWl0X2Zvcl90YXNr cz1mYWxzZSBhbmQNCj4gQ01EX1RfQUJPUlRFRCB3YXMgc2V0LCB3ZSB3b3VsZCBlbmQgdXAgd2Fp dGluZyBmb3JldmVyLiBJcyB0aGF0IHJpZ2h0IG9yDQo+IGNhbiBpdCBuZXZlciBoYXBwZW4/DQoN CkhlbGxvIE1pa2UsDQoNCnRyYW5zcG9ydF9nZW5lcmljX2ZyZWVfY21kKCkgY2FsbHMgdGFyZ2V0 X3dhaXRfZnJlZV9jbWQoKSBiZWZvcmUgd2FpdGluZw0Kb24gY21kLT5jbWRfd2FpdF9jb21wIGZv ciBhYm9ydGVkIGNvbW1hbmRzLiB0YXJnZXRfd2FpdF9mcmVlX2NtZCgpIGNhbGxzDQpfX3RyYW5z cG9ydF93YWl0X2Zvcl90YXNrcygpIHdpdGggdGhlIGZhYnJpY19zdG9wIGFyZ3VtZW50IHNldCB0 byB0cnVlDQphbmQgdGhhdCBjYXVzZXMgdGhlIENNRF9UX0ZBQlJJQ19TVE9QIGZsYWcgdG8gYmUg c2V0Lg0KDQpCYXJ0Lg0KDQoNCg0K -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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=false 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. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 *);
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(-)