| Message ID | 1452458668-11034-2-git-send-email-nab@daterainc.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 01/10/2016 12:44 PM, Nicholas A. Bellinger wrote: > From: Nicholas Bellinger <nab@linux-iscsi.org> > > Based on HCH's original patch, this adds a full version to > support percpu-ida tag pre-allocation and callback function > pointer into fabric driver code to complete session setup. > > Reported-by: Christoph Hellwig <hch@lst.de> > Cc: Sagi Grimberg <sagig@mellanox.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Hannes Reinecke <hare@suse.de> > Cc: Andy Grover <agrover@redhat.com> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> > --- > drivers/target/target_core_transport.c | 55 ++++++++++++++++++++++++++++++++++ > include/target/target_core_fabric.h | 6 ++++ > 2 files changed, 61 insertions(+) > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index c5035b9..fd4404f6 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -374,6 +374,61 @@ void transport_register_session( > } > EXPORT_SYMBOL(transport_register_session); > > +struct se_session * > +target_alloc_session(struct se_portal_group *tpg, > + unsigned int tag_num, unsigned int tag_size, > + enum target_prot_op prot_op, > + const char *initiatorname, void *private, > + int (*callback)(struct se_portal_group *, > + struct se_session *, void *)) Please use a more descriptive name instead of "callback", e.g. "init_session". > +{ > + struct se_session *sess; > + > + if (tag_num != 0 && !tag_size) { > + pr_err("target_alloc_session called with percpu-ida tag_num:" > + " %u, but zero tag_size\n", tag_num); > + return ERR_PTR(-EINVAL); > + } > + if (!tag_num && tag_size) { > + pr_err("target_alloc_session called with percpu-ida tag_size:" > + " %u, but zero tag_num\n", tag_size); > + return ERR_PTR(-EINVAL); > + } Please combine the above two tests into a single test, e.g. !!tag_num != !!tag_size or (bool)tag_num != (bool)tag_size. Thanks, Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> +struct se_session * > +target_alloc_session(struct se_portal_group *tpg, > + unsigned int tag_num, unsigned int tag_size, > + enum target_prot_op prot_op, > + const char *initiatorname, void *private, > + int (*callback)(struct se_portal_group *, > + struct se_session *, void *)) > +{ I'd much rather have the fabrics drivers call transport_alloc_session_tags directly from the callback than growing even more arguments here. > + struct se_session *sess; > + > + if (tag_num != 0 && !tag_size) { > + pr_err("target_alloc_session called with percpu-ida tag_num:" > + " %u, but zero tag_size\n", tag_num); > + return ERR_PTR(-EINVAL); > + } > + if (!tag_num && tag_size) { > + pr_err("target_alloc_session called with percpu-ida tag_size:" > + " %u, but zero tag_num\n", tag_size); > + return ERR_PTR(-EINVAL); > + } These checks should be in transport_alloc_session_tags. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2016-01-12 at 16:11 +0100, Christoph Hellwig wrote: > > +struct se_session * > > +target_alloc_session(struct se_portal_group *tpg, > > + unsigned int tag_num, unsigned int tag_size, > > + enum target_prot_op prot_op, > > + const char *initiatorname, void *private, > > + int (*callback)(struct se_portal_group *, > > + struct se_session *, void *)) > > +{ > > I'd much rather have the fabrics drivers call transport_alloc_session_tags > directly from the callback than growing even more arguments here. > What's the point..? Only vhost-scsi currently needs to allocate extra resources for se_session percpu-ida tag pre-allocation. The rest of the consumers can just use this common code. > > + struct se_session *sess; > > + > > + if (tag_num != 0 && !tag_size) { > > + pr_err("target_alloc_session called with percpu-ida tag_num:" > > + " %u, but zero tag_size\n", tag_num); > > + return ERR_PTR(-EINVAL); > > + } > > + if (!tag_num && tag_size) { > > + pr_err("target_alloc_session called with percpu-ida tag_size:" > > + " %u, but zero tag_num\n", tag_size); > > + return ERR_PTR(-EINVAL); > > + } > > These checks should be in transport_alloc_session_tags. > Done. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 13, 2016 at 12:09:26AM -0800, Nicholas A. Bellinger wrote: > > I'd much rather have the fabrics drivers call transport_alloc_session_tags > > directly from the callback than growing even more arguments here. > > > > What's the point..? > > Only vhost-scsi currently needs to allocate extra resources for > se_session percpu-ida tag pre-allocation. The point is that allocating "tags" (or better command structures) isn't fundamentally related to allocating a session, and in fact not even used by many drivers. A flow where drivers that need these "tags" explicitly allocate them is a lot more obvious than forcing more arguments to a function that already has a lot and isn't related to these "tags" otherwise. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2016-01-13 at 09:17 +0100, Christoph Hellwig wrote: > On Wed, Jan 13, 2016 at 12:09:26AM -0800, Nicholas A. Bellinger wrote: > > > I'd much rather have the fabrics drivers call transport_alloc_session_tags > > > directly from the callback than growing even more arguments here. > > > > > > > What's the point..? > > > > Only vhost-scsi currently needs to allocate extra resources for > > se_session percpu-ida tag pre-allocation. > > The point is that allocating "tags" (or better command structures) isn't > fundamentally related to allocating a session, and in fact not even > used by many drivers. Huh..? It's used by iscsi-target, ib_isert, tcm_qla2xxx, vhost-scsi and tcm_fc. That's the majority of the drivers people care about. > A flow where drivers that need these "tags" > explicitly allocate them is a lot more obvious than forcing more > arguments to a function that already has a lot and isn't related to > these "tags" otherwise. The goal is to get to the point where all fabric drivers use tag pre-allocation, and not vice-versa. That said, I think it makes sense to leave it as is instead of adding new callbacks for iscsi-target, ib_isert, and tcm_fc to just handle se_session tag pre-allocation. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 13, 2016 at 01:22:49AM -0800, Nicholas A. Bellinger wrote: > Huh..? It's used by iscsi-target, ib_isert, tcm_qla2xxx, vhost-scsi and > tcm_fc. > > That's the majority of the drivers people care about. ib_isert never calls transport_init_session / transport_init_session_tags directly. We've got 3 callers to transport_init_session_tags vs 6 to transport_init_session. iSCSI already works roughly the way I suggest everything should work in the future - it uses transport_init_session but manually allocates the tags. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2016-01-13 at 10:33 +0100, Christoph Hellwig wrote: > On Wed, Jan 13, 2016 at 01:22:49AM -0800, Nicholas A. Bellinger wrote: > > Huh..? It's used by iscsi-target, ib_isert, tcm_qla2xxx, vhost-scsi and > > tcm_fc. > > > > That's the majority of the drivers people care about. > > ib_isert never calls transport_init_session / transport_init_session_tags > directly. > Yes. Not to mention, there is a new struct iscsit_transport based driver on the way that assumes percpu-ida tag preallocation using this same method. > We've got 3 callers to transport_init_session_tags vs 6 to > transport_init_session. > In that case, I'll look into converting sbp-target, usb-gadget and xen-scsiback to use percpu-ida like the other consumers here for v4.6 code. > iSCSI already works roughly the way I suggest everything should > work in the future - it uses transport_init_session but manually > allocates the tags. iscsi-target needs it open-coded for discovery purposes, because se_node_acl lookup does not occur. However for the normal session login se_node_acl lookup back, I don't see a reason why iscsi-target can't use target_alloc_session() here too. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" 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 c5035b9..fd4404f6 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -374,6 +374,61 @@ void transport_register_session( } EXPORT_SYMBOL(transport_register_session); +struct se_session * +target_alloc_session(struct se_portal_group *tpg, + unsigned int tag_num, unsigned int tag_size, + enum target_prot_op prot_op, + const char *initiatorname, void *private, + int (*callback)(struct se_portal_group *, + struct se_session *, void *)) +{ + struct se_session *sess; + + if (tag_num != 0 && !tag_size) { + pr_err("target_alloc_session called with percpu-ida tag_num:" + " %u, but zero tag_size\n", tag_num); + return ERR_PTR(-EINVAL); + } + if (!tag_num && tag_size) { + pr_err("target_alloc_session called with percpu-ida tag_size:" + " %u, but zero tag_num\n", tag_size); + return ERR_PTR(-EINVAL); + } + /* + * If the fabric driver is using percpu-ida based pre allocation + * of I/O descriptor tags, go ahead and perform that setup now.. + */ + if (tag_num != 0) + sess = transport_init_session_tags(tag_num, tag_size, prot_op); + else + sess = transport_init_session(prot_op); + + if (IS_ERR(sess)) + return sess; + + sess->se_node_acl = core_tpg_check_initiator_node_acl(tpg, + (unsigned char *)initiatorname); + if (!sess->se_node_acl) { + transport_free_session(sess); + return ERR_PTR(-EACCES); + } + /* + * Go ahead and perform any remaining fabric setup that is + * required before transport_register_session(). + */ + if (callback != NULL) { + int rc = callback(tpg, sess, private); + if (rc) { + transport_free_session(sess); + return ERR_PTR(rc); + } + } + + transport_register_session(tpg, sess->se_node_acl, sess, private); + return sess; +} +EXPORT_SYMBOL(target_alloc_session); + static void target_release_session(struct kref *kref) { struct se_session *se_sess = container_of(kref, diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h index dc6b09e..7d59273 100644 --- a/include/target/target_core_fabric.h +++ b/include/target/target_core_fabric.h @@ -108,6 +108,12 @@ void target_unregister_template(const struct target_core_fabric_ops *fo); int target_depend_item(struct config_item *item); void target_undepend_item(struct config_item *item); +struct se_session *target_alloc_session(struct se_portal_group *, + unsigned int, unsigned int, enum target_prot_op prot_op, + const char *, void *, + int (*callback)(struct se_portal_group *, + struct se_session *, void *)); + struct se_session *transport_init_session(enum target_prot_op); int transport_alloc_session_tags(struct se_session *, unsigned int, unsigned int);