| Message ID | 20170330171244.8346-3-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2017-03-30 at 10:12 -0700, Bart Van Assche wrote: > While releasing a command __iscsit_free_cmd() can be called multiple > times but .iscsit_release_cmd() must be called only once. Hence move > the .iscsit_release_cmd() call into iscsit_release_cmd(). The latter > function is only called once per command. The only driver that defines > the .iscsit_release_cmd() callback is the cxgbit driver so this change > only affects the cxgbit driver. > > Fixes: 7ec811a8e9c3 ("iscsi-target: add void (*iscsit_release_cmd)()") > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > Cc: Varun Prakash <varun@chelsio.com> > Cc: Nicholas Bellinger <nab@linux-iscsi.org> > Cc: <stable@vger.kernel.org> > --- > drivers/target/iscsi/iscsi_target_util.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > Applied to target-pending/for-next, but dropping the stable CC' because the single caller in cxgbit_release_cmd() is already checking to ensure resources are only released on the first invocation. So it's not a bug-fix. > diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c > index 5041a9c8bdcb..8a022b5b2317 100644 > --- a/drivers/target/iscsi/iscsi_target_util.c > +++ b/drivers/target/iscsi/iscsi_target_util.c > @@ -691,11 +691,17 @@ void iscsit_release_cmd(struct iscsi_cmd *cmd) > { > struct iscsi_session *sess; > struct se_cmd *se_cmd = &cmd->se_cmd; > + struct iscsi_conn *conn = cmd->conn; > + void (*release)(struct iscsi_conn *, struct iscsi_cmd *); > > - if (cmd->conn) > - sess = cmd->conn->sess; > - else > + if (conn) { > + sess = conn->sess; > + release = conn->conn_transport->iscsit_release_cmd; > + if (release) > + release(conn, cmd); > + } else { > sess = cmd->sess; > + } > > BUG_ON(!sess || !sess->se_sess); Also, no need for a local (*release) function pointer and extra assignment. Dropping that part now, and squashing into the original patch. -- 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
Hi Nicholas and Bart, On Sun, Apr 02, 2017 at 03:59:05PM -0700, Nicholas A. Bellinger wrote: > On Thu, 2017-03-30 at 10:12 -0700, Bart Van Assche wrote: > > While releasing a command __iscsit_free_cmd() can be called multiple > > times but .iscsit_release_cmd() must be called only once. Hence move > > the .iscsit_release_cmd() call into iscsit_release_cmd(). The latter > > function is only called once per command. The only driver that defines > > the .iscsit_release_cmd() callback is the cxgbit driver so this change > > only affects the cxgbit driver. > > > > Fixes: 7ec811a8e9c3 ("iscsi-target: add void (*iscsit_release_cmd)()") > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > > Cc: Varun Prakash <varun@chelsio.com> > > Cc: Nicholas Bellinger <nab@linux-iscsi.org> > > Cc: <stable@vger.kernel.org> > > --- > > drivers/target/iscsi/iscsi_target_util.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > Applied to target-pending/for-next, but dropping the stable CC' because > the single caller in cxgbit_release_cmd() is already checking to ensure > resources are only released on the first invocation. > > So it's not a bug-fix. In case of DDP cxgbit driver assigns cmd->se_cmd.t_data_sg to ttinfo->sgl and calls dma_map_sg(), cxgbit_release_cmd() calls dma_unmap_sg(), it needs a valid sg(ttinfo->sgl), before calling iscsit_release_cmd() cmd->se_cmd.t_data_sg gets freed so ttinfo->sgl will not be valid if we move ->iscsit_release_cmd() to iscsit_release_cmd(). Thanks Varun -- 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
Hi Nicholas, On Tue, Apr 04, 2017 at 10:36:50AM +0530, Varun Prakash wrote: > Hi Nicholas and Bart, > > On Sun, Apr 02, 2017 at 03:59:05PM -0700, Nicholas A. Bellinger wrote: > > On Thu, 2017-03-30 at 10:12 -0700, Bart Van Assche wrote: > > > While releasing a command __iscsit_free_cmd() can be called multiple > > > times but .iscsit_release_cmd() must be called only once. Hence move > > > the .iscsit_release_cmd() call into iscsit_release_cmd(). The latter > > > function is only called once per command. The only driver that defines > > > the .iscsit_release_cmd() callback is the cxgbit driver so this change > > > only affects the cxgbit driver. > > > > > > Fixes: 7ec811a8e9c3 ("iscsi-target: add void (*iscsit_release_cmd)()") > > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > > > Cc: Varun Prakash <varun@chelsio.com> > > > Cc: Nicholas Bellinger <nab@linux-iscsi.org> > > > Cc: <stable@vger.kernel.org> > > > --- > > > drivers/target/iscsi/iscsi_target_util.c | 15 +++++++++------ > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > > Applied to target-pending/for-next, but dropping the stable CC' because > > the single caller in cxgbit_release_cmd() is already checking to ensure > > resources are only released on the first invocation. > > > > So it's not a bug-fix. > > In case of DDP cxgbit driver assigns cmd->se_cmd.t_data_sg to ttinfo->sgl > and calls dma_map_sg(), cxgbit_release_cmd() calls dma_unmap_sg(), it needs > a valid sg(ttinfo->sgl), before calling iscsit_release_cmd() > cmd->se_cmd.t_data_sg gets freed so ttinfo->sgl will not be valid if we move > ->iscsit_release_cmd() to iscsit_release_cmd(). Please drop this patch, it will regress cxgbit driver, with this patch scatterlist is freed before calling ->iscsit_release_cmd(), cxgbit calls dma_unmap_sg() so scatterlist should not be freed before calling ->iscst_release_cmd(). Thanks Varun -- 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 Thu, 2017-04-13 at 13:14 +0530, Varun Prakash wrote: > Hi Nicholas, > > On Tue, Apr 04, 2017 at 10:36:50AM +0530, Varun Prakash wrote: > > Hi Nicholas and Bart, > > > > On Sun, Apr 02, 2017 at 03:59:05PM -0700, Nicholas A. Bellinger wrote: > > > On Thu, 2017-03-30 at 10:12 -0700, Bart Van Assche wrote: > > > > While releasing a command __iscsit_free_cmd() can be called multiple > > > > times but .iscsit_release_cmd() must be called only once. Hence move > > > > the .iscsit_release_cmd() call into iscsit_release_cmd(). The latter > > > > function is only called once per command. The only driver that defines > > > > the .iscsit_release_cmd() callback is the cxgbit driver so this change > > > > only affects the cxgbit driver. > > > > > > > > Fixes: 7ec811a8e9c3 ("iscsi-target: add void (*iscsit_release_cmd)()") > > > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > > > > Cc: Varun Prakash <varun@chelsio.com> > > > > Cc: Nicholas Bellinger <nab@linux-iscsi.org> > > > > Cc: <stable@vger.kernel.org> > > > > --- > > > > drivers/target/iscsi/iscsi_target_util.c | 15 +++++++++------ > > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > > > > > Applied to target-pending/for-next, but dropping the stable CC' because > > > the single caller in cxgbit_release_cmd() is already checking to ensure > > > resources are only released on the first invocation. > > > > > > So it's not a bug-fix. > > > > In case of DDP cxgbit driver assigns cmd->se_cmd.t_data_sg to ttinfo->sgl > > and calls dma_map_sg(), cxgbit_release_cmd() calls dma_unmap_sg(), it needs > > a valid sg(ttinfo->sgl), before calling iscsit_release_cmd() > > cmd->se_cmd.t_data_sg gets freed so ttinfo->sgl will not be valid if we move > > ->iscsit_release_cmd() to iscsit_release_cmd(). > > Please drop this patch, it will regress cxgbit driver, with this patch > scatterlist is freed before calling ->iscsit_release_cmd(), cxgbit > calls dma_unmap_sg() so scatterlist should not be freed before calling > ->iscst_release_cmd(). > Thanks for the heads up. Dropping this patch for now. To address this, AFAICT the cleanest approach is to simply do the unmapping of DDP SGLs currently done by cxgbit_release_cmd(), directly from a cxgbit specific iscsit_transport->iscsit_queue_rsp() callback. That way, we can unmap the DDP SGLs immediately before invoking iscsit_queue_rsp() from a cxgbit specific handler, and drop the iscsi_transport->iscsit_release_cmd() callback alltogether. WDYT..? -- 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 Mon, May 01, 2017 at 09:33:31PM -0700, Nicholas A. Bellinger wrote: > On Thu, 2017-04-13 at 13:14 +0530, Varun Prakash wrote: > > On Tue, Apr 04, 2017 at 10:36:50AM +0530, Varun Prakash wrote: > > > On Sun, Apr 02, 2017 at 03:59:05PM -0700, Nicholas A. Bellinger wrote: > > > > On Thu, 2017-03-30 at 10:12 -0700, Bart Van Assche wrote: > > > > > While releasing a command __iscsit_free_cmd() can be called multiple > > > > > times but .iscsit_release_cmd() must be called only once. Hence move > > > > > the .iscsit_release_cmd() call into iscsit_release_cmd(). The latter > > > > > function is only called once per command. The only driver that defines > > > > > the .iscsit_release_cmd() callback is the cxgbit driver so this change > > > > > only affects the cxgbit driver. > > > > > > > > > > Fixes: 7ec811a8e9c3 ("iscsi-target: add void (*iscsit_release_cmd)()") > > > > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > > > > > Cc: Varun Prakash <varun@chelsio.com> > > > > > Cc: Nicholas Bellinger <nab@linux-iscsi.org> > > > > > Cc: <stable@vger.kernel.org> > > > > > --- > > > > > drivers/target/iscsi/iscsi_target_util.c | 15 +++++++++------ > > > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > > > > > > > > Applied to target-pending/for-next, but dropping the stable CC' because > > > > the single caller in cxgbit_release_cmd() is already checking to ensure > > > > resources are only released on the first invocation. > > > > > > > > So it's not a bug-fix. > > > > > > In case of DDP cxgbit driver assigns cmd->se_cmd.t_data_sg to ttinfo->sgl > > > and calls dma_map_sg(), cxgbit_release_cmd() calls dma_unmap_sg(), it needs > > > a valid sg(ttinfo->sgl), before calling iscsit_release_cmd() > > > cmd->se_cmd.t_data_sg gets freed so ttinfo->sgl will not be valid if we move > > > ->iscsit_release_cmd() to iscsit_release_cmd(). > > > > Please drop this patch, it will regress cxgbit driver, with this patch > > scatterlist is freed before calling ->iscsit_release_cmd(), cxgbit > > calls dma_unmap_sg() so scatterlist should not be freed before calling > > ->iscst_release_cmd(). > > > > Thanks for the heads up. Dropping this patch for now. > > To address this, AFAICT the cleanest approach is to simply do the > unmapping of DDP SGLs currently done by cxgbit_release_cmd(), directly > from a cxgbit specific iscsit_transport->iscsit_queue_rsp() callback. > > That way, we can unmap the DDP SGLs immediately before invoking > iscsit_queue_rsp() from a cxgbit specific handler, and drop the > iscsi_transport->iscsit_release_cmd() callback alltogether. > > WDYT..? This approach will work in success case, but in failure cases(digest errors, ...) ->iscsit_queue_status() is not called so dma umap will not happen. -- 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
Hi Varun, On Sun, 2017-05-07 at 18:22 +0530, Varun Prakash wrote: > On Mon, May 01, 2017 at 09:33:31PM -0700, Nicholas A. Bellinger wrote: <SNIP> > > Thanks for the heads up. Dropping this patch for now. > > > > To address this, AFAICT the cleanest approach is to simply do the > > unmapping of DDP SGLs currently done by cxgbit_release_cmd(), directly > > from a cxgbit specific iscsit_transport->iscsit_queue_rsp() callback. > > > > That way, we can unmap the DDP SGLs immediately before invoking > > iscsit_queue_rsp() from a cxgbit specific handler, and drop the > > iscsi_transport->iscsit_release_cmd() callback alltogether. > > > > WDYT..? > > This approach will work in success case, but in failure > cases(digest errors, ...) ->iscsit_queue_status() is not called so dma umap > will not happen. Indeed. Looking at other hw drivers like qla2xxx that have this same requirement, they do *_unmap_sg() once a completion interrupt has triggered, but before target_core_fabric_ops->release_cmd() is invoked and se_cmd->t_task_sg has already been freed. So I'm open to adding a target_core_fabric_ops->unmap_sg() to do this ahead of target_core_transport.c:transport_free_pages(). That said, snother option is to perform *_unmap_sg() internally in cxgbit once DDP completion for WRITEs has completed, but before it's submitted into iscsi_target -> target_core. AFAICT from a quick scan of cxgbit code, the two scenarios for this would be: *) For ISCSI_OP_SCSI_CMD with immediate data, once cxgbit_get_immediate_data() is invoked. Either for all cases when this is invoked (eg: does the DDP SGLs both immediate, unsolicited and solicited data when mapped..?), or only when iscsi_cmd->immediate_data = 1 && iscsi_cmd_flags & ICF_GOT_LAST_DATAOUT is true. *) For ISCSI_OP_SCSI_DATA_OUT with FINAL_BIT set, before iscsit_check_dataout_payload() is called to invoke target_execute_cmd() WDYT..? -- 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 Tue, May 09, 2017 at 12:49:26AM -0700, Nicholas A. Bellinger wrote: > Hi Varun, > > On Sun, 2017-05-07 at 18:22 +0530, Varun Prakash wrote: > > On Mon, May 01, 2017 at 09:33:31PM -0700, Nicholas A. Bellinger wrote: > > <SNIP> > > Indeed. > > Looking at other hw drivers like qla2xxx that have this same > requirement, they do *_unmap_sg() once a completion interrupt has > triggered, but before target_core_fabric_ops->release_cmd() is invoked > and se_cmd->t_task_sg has already been freed. > > So I'm open to adding a target_core_fabric_ops->unmap_sg() to do this > ahead of target_core_transport.c:transport_free_pages(). > > That said, snother option is to perform *_unmap_sg() internally in > cxgbit once DDP completion for WRITEs has completed, but before it's > submitted into iscsi_target -> target_core. > > AFAICT from a quick scan of cxgbit code, the two scenarios for this > would be: > > *) For ISCSI_OP_SCSI_CMD with immediate data, once > cxgbit_get_immediate_data() is invoked. Either for all cases when this > is invoked (eg: does the DDP SGLs both immediate, unsolicited and > solicited data when mapped..?), or only when iscsi_cmd->immediate_data = > 1 && iscsi_cmd_flags & ICF_GOT_LAST_DATAOUT is true. > > *) For ISCSI_OP_SCSI_DATA_OUT with FINAL_BIT set, before > iscsit_check_dataout_payload() is called to invoke target_execute_cmd() > > WDYT..? > Current cxgbit code does following in cxgbit_release_cmd() 1. put_page() in case of PASSTHROUGH_SG_TO_MEM_NOALLOC. 2. dma_unmap_sg() DDP SGL. 3. free hw DDP resource. If cxgbit does cleanup internally then lot of error handling code is required in cxgbit driver, eg: - PDU with F bit set never arrives, header digest errors etc. Another approach to add target_core_fabrics_ops->unmap_sg() will not work for ERL 2 case because for calling ->iscsit_unmap_sg() valid cmd->conn pointer is required, in case of ERL 2 cmd->conn can be NULL, but we can use this approach because in current cxgbit code I enable DDP only for ERL 0 case, similiary I can add code to enable PASSTHROUGH_SG_TO_MEM_NOALLOC only for ERL 0 case. -- 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
T24gVHVlLCAyMDE3LTA0LTA0IGF0IDEwOjM2ICswNTMwLCBWYXJ1biBQcmFrYXNoIHdyb3RlOg0K PiBPbiBTdW4sIEFwciAwMiwgMjAxNyBhdCAwMzo1OTowNVBNIC0wNzAwLCBOaWNob2xhcyBBLiBC ZWxsaW5nZXIgd3JvdGU6DQo+ID4gT24gVGh1LCAyMDE3LTAzLTMwIGF0IDEwOjEyIC0wNzAwLCBC YXJ0IFZhbiBBc3NjaGUgd3JvdGU6DQo+ID4gPiBXaGlsZSByZWxlYXNpbmcgYSBjb21tYW5kIF9f aXNjc2l0X2ZyZWVfY21kKCkgY2FuIGJlIGNhbGxlZCBtdWx0aXBsZQ0KPiA+ID4gdGltZXMgYnV0 IC5pc2NzaXRfcmVsZWFzZV9jbWQoKSBtdXN0IGJlIGNhbGxlZCBvbmx5IG9uY2UuIEhlbmNlIG1v dmUNCj4gPiA+IHRoZSAuaXNjc2l0X3JlbGVhc2VfY21kKCkgY2FsbCBpbnRvIGlzY3NpdF9yZWxl YXNlX2NtZCgpLiBUaGUgbGF0dGVyDQo+ID4gPiBmdW5jdGlvbiBpcyBvbmx5IGNhbGxlZCBvbmNl IHBlciBjb21tYW5kLiBUaGUgb25seSBkcml2ZXIgdGhhdCBkZWZpbmVzDQo+ID4gPiB0aGUgLmlz Y3NpdF9yZWxlYXNlX2NtZCgpIGNhbGxiYWNrIGlzIHRoZSBjeGdiaXQgZHJpdmVyIHNvIHRoaXMg Y2hhbmdlDQo+ID4gPiBvbmx5IGFmZmVjdHMgdGhlIGN4Z2JpdCBkcml2ZXIuDQo+ID4gPiANCj4g PiA+IEZpeGVzOiA3ZWM4MTFhOGU5YzMgKCJpc2NzaS10YXJnZXQ6IGFkZCB2b2lkICgqaXNjc2l0 X3JlbGVhc2VfY21kKSgpIikNCj4gPiA+IFNpZ25lZC1vZmYtYnk6IEJhcnQgVmFuIEFzc2NoZSA8 YmFydC52YW5hc3NjaGVAc2FuZGlzay5jb20+DQo+ID4gPiBDYzogVmFydW4gUHJha2FzaCA8dmFy dW5AY2hlbHNpby5jb20+DQo+ID4gPiBDYzogTmljaG9sYXMgQmVsbGluZ2VyIDxuYWJAbGludXgt aXNjc2kub3JnPg0KPiA+ID4gQ2M6IDxzdGFibGVAdmdlci5rZXJuZWwub3JnPg0KPiA+ID4gLS0t DQo+ID4gPiAgZHJpdmVycy90YXJnZXQvaXNjc2kvaXNjc2lfdGFyZ2V0X3V0aWwuYyB8IDE1ICsr KysrKysrKy0tLS0tLQ0KPiA+ID4gIDEgZmlsZSBjaGFuZ2VkLCA5IGluc2VydGlvbnMoKyksIDYg ZGVsZXRpb25zKC0pDQo+ID4gPiANCj4gPiANCj4gPiBBcHBsaWVkIHRvIHRhcmdldC1wZW5kaW5n L2Zvci1uZXh0LCBidXQgZHJvcHBpbmcgdGhlIHN0YWJsZSBDQycgYmVjYXVzZQ0KPiA+IHRoZSBz aW5nbGUgY2FsbGVyIGluIGN4Z2JpdF9yZWxlYXNlX2NtZCgpIGlzIGFscmVhZHkgY2hlY2tpbmcg dG8gZW5zdXJlDQo+ID4gcmVzb3VyY2VzIGFyZSBvbmx5IHJlbGVhc2VkIG9uIHRoZSBmaXJzdCBp bnZvY2F0aW9uLg0KPiA+IA0KPiA+IFNvIGl0J3Mgbm90IGEgYnVnLWZpeC4NCj4gDQo+IEluIGNh c2Ugb2YgRERQIGN4Z2JpdCBkcml2ZXIgYXNzaWducyBjbWQtPnNlX2NtZC50X2RhdGFfc2cgdG8g dHRpbmZvLT5zZ2wNCj4gYW5kIGNhbGxzIGRtYV9tYXBfc2coKSwgY3hnYml0X3JlbGVhc2VfY21k KCkgY2FsbHMgZG1hX3VubWFwX3NnKCksIGl0IG5lZWRzDQo+IGEgdmFsaWQgc2codHRpbmZvLT5z Z2wpLCBiZWZvcmUgY2FsbGluZyBpc2NzaXRfcmVsZWFzZV9jbWQoKQ0KPiBjbWQtPnNlX2NtZC50 X2RhdGFfc2cgZ2V0cyBmcmVlZCBzbyB0dGluZm8tPnNnbCB3aWxsIG5vdCBiZSB2YWxpZCBpZiB3 ZSBtb3ZlDQo+IC0+aXNjc2l0X3JlbGVhc2VfY21kKCkgdG8gaXNjc2l0X3JlbGVhc2VfY21kKCku DQoNCihyZXBseWluZyB0byBhbiBlLW1haWwgb2Ygc2l4IG1vbnRocyBhZ28pDQoNCkhlbGxvIFZh cnVuLA0KDQpIYXZlIHlvdSBub3RpY2VkIHRoYXQgY29tbWl0IGZlYmU1NjJjMjBkZiAodGFyZ2V0 OiBGaXggTFVOX1JFU0VUIGFjdGl2ZSBJL08NCmhhbmRsaW5nIGZvciBBQ0tfS1JFRjsgSmFudWFy eSAyMDE2KSBtb3ZlZCB0aGUgdHJhbnNwb3J0X2ZyZWVfcGFnZXMoKSBjYWxsDQpmcm9tIHRyYW5z cG9ydF9wdXRfY21kKCkgdG8gdGFyZ2V0X3JlbGVhc2VfY21kX2tyZWYoKT8gSSB0aGluayB0aGF0 IG1lYW5zDQp0aGF0IGl0IGlzIG5vdyBzYWZlIHRvIGNhbGwgLmlzY3NpdF9yZWxlYXNlX2NtZCgp IGFmdGVyDQp0cmFuc3BvcnRfZ2VuZXJpY19mcmVlX2NtZCgpLg0KDQpUaGFua3MsDQoNCkJhcnQu -- 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 Wed, Nov 01, 2017 at 12:07:52AM +0000, Bart Van Assche wrote: > On Tue, 2017-04-04 at 10:36 +0530, Varun Prakash wrote: > > On Sun, Apr 02, 2017 at 03:59:05PM -0700, Nicholas A. Bellinger wrote: > > > On Thu, 2017-03-30 at 10:12 -0700, Bart Van Assche wrote: > > > > While releasing a command __iscsit_free_cmd() can be called multiple > > > > times but .iscsit_release_cmd() must be called only once. Hence move > > > > the .iscsit_release_cmd() call into iscsit_release_cmd(). The latter > > > > function is only called once per command. The only driver that defines > > > > the .iscsit_release_cmd() callback is the cxgbit driver so this change > > > > only affects the cxgbit driver. > > > > > > > > Fixes: 7ec811a8e9c3 ("iscsi-target: add void (*iscsit_release_cmd)()") > > > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > > > > Cc: Varun Prakash <varun@chelsio.com> > > > > Cc: Nicholas Bellinger <nab@linux-iscsi.org> > > > > Cc: <stable@vger.kernel.org> > > > > --- > > > > drivers/target/iscsi/iscsi_target_util.c | 15 +++++++++------ > > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > > > > (replying to an e-mail of six months ago) > > Hello Varun, > > Have you noticed that commit febe562c20df (target: Fix LUN_RESET active I/O > handling for ACK_KREF; January 2016) moved the transport_free_pages() call > from transport_put_cmd() to target_release_cmd_kref()? I think that means > that it is now safe to call .iscsit_release_cmd() after > transport_generic_free_cmd(). > Hello Bart, The requirement here is to call .iscsit_release_cmd() before target free the pages so that cxgbit driver can call dma_unmap_sg() and free the pages in case of PASSTHROUGH_SG_TO_MEM_NOALLOC. Currently .iscsit_release_cmd() is called from two functions - iscsit_free_cmd() -> __iscsit_free_cmd() -> .iscsit_release_cmd() iscsit_aborted_task() -> __iscsit_free_cmd() -> .iscsit_release_cmd() If we move .iscsit_release_cmd() after transport_generic_free_cmd(), will it handle all the error cases(abort etc)? In case of abort currently it is called from iscsit_aborted_task(), if we move then in case of abort .iscsit_release_cmd() will not be called. If we can confirm that moving .iscsit_release_cmd() will not cause any memory leak then we can move it. Thanks Varun -- 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/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c index 5041a9c8bdcb..8a022b5b2317 100644 --- a/drivers/target/iscsi/iscsi_target_util.c +++ b/drivers/target/iscsi/iscsi_target_util.c @@ -691,11 +691,17 @@ void iscsit_release_cmd(struct iscsi_cmd *cmd) { struct iscsi_session *sess; struct se_cmd *se_cmd = &cmd->se_cmd; + struct iscsi_conn *conn = cmd->conn; + void (*release)(struct iscsi_conn *, struct iscsi_cmd *); - if (cmd->conn) - sess = cmd->conn->sess; - else + if (conn) { + sess = conn->sess; + release = conn->conn_transport->iscsit_release_cmd; + if (release) + release(conn, cmd); + } else { sess = cmd->sess; + } BUG_ON(!sess || !sess->se_sess); @@ -728,9 +734,6 @@ void __iscsit_free_cmd(struct iscsi_cmd *cmd, bool scsi_cmd, iscsit_remove_cmd_from_immediate_queue(cmd, conn); iscsit_remove_cmd_from_response_queue(cmd, conn); } - - if (conn && conn->conn_transport->iscsit_release_cmd) - conn->conn_transport->iscsit_release_cmd(conn, cmd); } void iscsit_free_cmd(struct iscsi_cmd *cmd, bool shutdown)
While releasing a command __iscsit_free_cmd() can be called multiple times but .iscsit_release_cmd() must be called only once. Hence move the .iscsit_release_cmd() call into iscsit_release_cmd(). The latter function is only called once per command. The only driver that defines the .iscsit_release_cmd() callback is the cxgbit driver so this change only affects the cxgbit driver. Fixes: 7ec811a8e9c3 ("iscsi-target: add void (*iscsit_release_cmd)()") Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> Cc: Varun Prakash <varun@chelsio.com> Cc: Nicholas Bellinger <nab@linux-iscsi.org> Cc: <stable@vger.kernel.org> --- drivers/target/iscsi/iscsi_target_util.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)