Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1305689 > unrolled thread

[PATCH-for-4.6 1/6] target: Add target_alloc_session() helper function

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH-for-4.6 1/6] target: Add target_alloc_session() helper function "Nicholas A. Bellinger" <nab@daterainc.com> - 2016-01-10 21:50 +0100
    Re: [PATCH-for-4.6 1/6] target: Add target_alloc_session() helper  function Christoph Hellwig <hch@lst.de> - 2016-01-12 16:20 +0100
      Re: [PATCH-for-4.6 1/6] target: Add target_alloc_session() helper  function "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2016-01-13 09:10 +0100
        Re: [PATCH-for-4.6 1/6] target: Add target_alloc_session() helper  function Christoph Hellwig <hch@lst.de> - 2016-01-13 09:20 +0100
          Re: [PATCH-for-4.6 1/6] target: Add target_alloc_session() helper  function "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2016-01-13 10:30 +0100
            Re: [PATCH-for-4.6 1/6] target: Add target_alloc_session() helper  function Christoph Hellwig <hch@lst.de> - 2016-01-13 10:40 +0100
              Re: [PATCH-for-4.6 1/6] target: Add target_alloc_session() helper  function "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2016-01-13 11:20 +0100

#1305689 — [PATCH-for-4.6 1/6] target: Add target_alloc_session() helper function

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 *))
+{
+	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);
-- 
1.9.1

[toc] | [next] | [standalone]

#1307531 — Re: [PATCH-for-4.6 1/6] target: Add target_alloc_session() helper function

> +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.

[toc] | [prev] | [next] | [standalone]

#1308146 — Re: [PATCH-for-4.6 1/6] target: Add target_alloc_session() helper function

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.

[toc] | [prev] | [next] | [standalone]

#1308154 — Re: [PATCH-for-4.6 1/6] target: Add target_alloc_session() helper function

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.

[toc] | [prev] | [next] | [standalone]

#1308203 — Re: [PATCH-for-4.6 1/6] target: Add target_alloc_session() helper function

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.

[toc] | [prev] | [next] | [standalone]

#1308211 — Re: [PATCH-for-4.6 1/6] target: Add target_alloc_session() helper function

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.

[toc] | [prev] | [next] | [standalone]

#1308249 — Re: [PATCH-for-4.6 1/6] target: Add target_alloc_session() helper function

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web