Message-Id: <1306452490.23461.60.camel@haakon2.linux-iscsi.org>
Date:	Thu, 26 May 2011 16:28:10 -0700
From:	"Nicholas A. Bellinger" <nab@...ux-iscsi.org>
To:	James Bottomley <James.Bottomley@...senPartnership.com>
Cc:	FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>,
	linux-kernel@...r.kernel.org, linux-scsi@...r.kernel.org,
	hch@....de, hare@...e.de, agrover@...hat.com, michaelc@...wisc.edu,
	bharrosh@...asas.com, akpm@...ux-foundation.org,
	martin.svec@...er.cz, Jerome Martin <jxm@...ingtidesystems.com>
Subject: Re: [PATCH-v5 07/13] iscsi-target: Add iSCSI Login Negotiation +
	Parameter logic

On Thu, 2011-05-26 at 18:04 -0500, James Bottomley wrote:
> On Thu, 2011-05-26 at 14:33 -0700, Nicholas A. Bellinger wrote:
> > On Thu, 2011-05-26 at 15:14 -0500, James Bottomley wrote:
> > > On Thu, 2011-05-26 at 12:49 -0700, Nicholas A. Bellinger wrote:
> > > > On Fri, 2011-05-27 at 04:29 +0900, FUJITA Tomonori wrote:
> > > > > On Thu, 26 May 2011 12:07:12 -0700
> > > > > "Nicholas A. Bellinger" <nab@...ux-iscsi.org> wrote:
> > > > > 
> > > > > > On Thu, 2011-05-26 at 11:46 -0500, James Bottomley wrote:
> > > > > > > On Thu, 2011-05-19 at 20:37 -0700, Nicholas A. Bellinger wrote:
> > > > > > > > From: Nicholas Bellinger <nab@...ux-iscsi.org>
> > > > > > > > 
> > > > > > > > This patch adds the princple RFC-3720 compatiable iSCSI Login
> > > > > > > > phase negotiation for iscsi_target_mod.  This also includes the
> > > > > > > > target RX/TX thread queue logic which is called directly from iSCSI
> > > > > > > > login associated code.
> > > > > > > > 
> > > > > > > > Signed-off-by: Nicholas A. Bellinger <nab@...ux-iscsi.org> 
> > > > > > > 
> > > > > > > I thought the upshot of the thread with Tomo was that we wouldn't be
> > > > > > > doing all of this in-kernel.  Where's the userspace upcall for this?
> > > > > > > 
> > > > > > 
> > > > > > The technical reasons why I want to avoid this have not changed for the
> > > > > > 1) authentication disabled and 2) 'required-to-implement' CHAP
> > > > > > authentication cases.  These where discussed at the bottom of the thread
> > > > > > from March with Tomo-san here:
> > > > > > 
> > > > > > http://marc.info/?l=linux-scsi&m=130108812405710&w=2
> > > > > > 
> > > > > > As mentioned, I am open to adding a userspace upcall for authentication
> > > > > > payloads post merge in order to support the 'optional-to-implement'
> > > > > > authentication cases.  However, pushing the above two cases out to
> > > > > 
> > > > > We don't need such, passing payloads from kernel to user space. You do the pre
> > > > > SCSI nexus operations in user space then kernel takes care of established
> > > > > nexuses.
> > > > > 
> > > > 
> > > > I understand what you have in mind, but I still think this the wrong
> > > > approach for the default cases.  For an in-kernel iscsi-target capable
> > > > of changing any aspect of the control plane on the fly, this type of
> > > > split is problematic to support and maintain and does not actually buy
> > > > us anything for the default cases.
> > > 
> > > Handling the default case separately from the less usual ones is
> > > actually the bigger recipe for disaster because the less used path
> > > becomes less well tested.
> > > 
> > > I don't really see how a kernel/user split is problematic.  After all,
> > > it's what we use for a lot of things (udev being the most notable).
> > > 
> > 
> > The fact is that none of the mainstream initiators support anything
> > beyond CHAP at this point, so the default cases are realistically what
> > everyone will be using for 2.8.0 and need to be rock solid for all
> > cases.
> 
> This doesn't impact the argument that we need to use the same path for
> common and special auth cases.  We're not putting all the exotic RFC
> auth cases in the kernel, so the common path needs to be out as well.
> 

It impacts what actually needs to be pushed into userspace, eg: the
'optional-to-implement' authentication payloads.

Pushing the control plane for NodeACLs into userspace for real-time
management of active login attempts for a kernel-level iscsi-target has
always been, and always will be a bad idea when having to sychronize the
aspects across 10,000 /sys/kernel/config/target/iscsi/$TARGET_IQN
endpoints.

As we have discussed at length over the years, the split needs to be all
userspace or all kernelspace, and when implementations start doing
things in-between they quickly get painful to debug, maintain and
extend.  I have no interest in trying to evolve this further when LIO
users are happy that things 'just-work' for the default case.

This is why I want to limit the complexity of the userspace passthrough
of authentication payloads for individual iSCSI target endpoints, and
not the complete iSCSI login state machine for all possible iSCSI target
endpoints.

> > > > > > userspace really does add unnecessary complexity and limitiations that I
> > > > > > want to avoid for the default iSCSI login cases.
> > > > > > 
> > > > > > It also would break existing rtslib/rtsadmin-v2 userspace code, and
> > > > > 
> > > > > I don't think breaking the existing code matters.
> > > > 
> > > > Sure it does.  It means the difference between if the
> > > > 'required-to-implement' cases can be exposed via configfs to a native
> > > > python object library and shell, or if we need to have an external
> > > > daemon + configuration that has to be kept in sync between the two,
> > > > parse external configuration files, et al.
> > > 
> > >      1. We can always handle this in an ABI shim in userspace
> > >      2. The current *kernel* code doesn't exist, therefore it has no
> > >         users.  Maintaining compatibility with the out of tree code is
> > >         fine, but not at the expense of less optimal design decisions.
> > > 
> > > > With the current design, the NodeACLs + authentication are available
> > > > directly as part of the rtslib python object library, and python code
> > > > including rtslib can reference all aspects of the initiator
> > > > configuration directly.  Breaking this up to an external daemon and
> > > > configuration is a step backwards for the default cases from the
> > > > perspective of rtslib, and making it work with an external
> > > > configuration / daemon for the NodeACLs + default authentication case is
> > > > an hack compared to how iscsi-target functionality is exposed to
> > > > application level progammers via rtslib today.
> > > 
> > > I don't buy this.  It doesn't have to be a daemon (udev isn't).  And
> > > udev certainly isn't a hack compared to what went before (which was
> > > fully in-kernel).
> > > 
> > 
> > To elaborate a bit more with an rtslib example (Jerome CC'ed).
> > 
> > Consider the following python code that uses rtslib to create a iSCSI
> > target+tpgt, iSCSI Initiator NodeACL, and set+read authentication
> > information for said NodeACL using native library objects:
> > 
> > #!/usr/bin/python
> > 
> > import rtslib
> > 
> > fabric = rtslib.FabricModule('iscsi')
> > 
> > print("Creating a new iqn.2003-01.org.linux-iscsi.rtslinux.foobar iscsi target")
> > target = rtslib.Target(fabric, "iqn.2003-01.org.linux-iscsi.rtslinux.foobar")
> > 
> > print
> > print("Creating a new iscsi TPG, TPG5 in that target")
> > tpg = rtslib.TPG(target, 5)
> > 
> > print
> > print("Creating a node ACL for initiator iqn.2003-01.org.linux-iscsi.rtslinux.initiator")
> > node_acl = tpg.node_acl("iqn.2003-01.org.linux-iscsi.rtslinux.initiator")
> > 
> > print
> > print("Setting the CHAP password and username to foo/bar for that initiator")
> > node_acl.chap_username = "foo"
> > node_acl.chap_password = "bar"
> > 
> > print
> > print("Reading CHAP password and username for that initiator")
> > print("Username: %s\n Password: %s" % (node_acl.chap_username, node_acl.chap_password))
> > 
> > print
> > print("For fun, let's list all of our existing node ACLs paths, accross all fabric/targets/etc.")
> > acls = [acl.path for acl in rtslib.RTSRoot().node_acls]
> > print "\n".join(acls)
> > 
> > -----------------------------------------------------------------------
> > 
> > Being able to have access to the complete set of underlying iscsi-target
> > features in native python library code is very important for target mode
> > userspace, and the current kernel+user code is solving this elegantly
> > for the two default iSCSI login cases.
> 
> This seems a bit orthogonal to the argument.  Whether the driver uses
> userspace upcalls or not, the scripts still have access.
> 

scripts != library

The python library drives the configuration of the kernel control plane
for target core and $FABRIC_MOD, and currently we use configfs for this
with the default two cases.  What rtsadmin does with rtslib is one thing
to provide the complete set of iscsi-target functionality to an
high-level end-user, but what application developers, test script
writers and others do with rtslib on their own code is quite another
question.  :)

> > We are open to looking at how to extend the kernel+user code to support
> > other 'optional-to-implement' iscsi authentication cases, but not at the
> > expense making drastic changes to kernel code this late, or removing /
> > breaking python library object functionality for the default
> > 'required-to-implement' cases.
> 
> That's fine ... I can easily give you another release cycle to do this.
> 

Delaying the merging of this for the same reasons as .39 after all of
this work would really an huge waste.  The code is ready to go for the
real-time management cases that 99% of current users care about H/A, and
slipping (again) for a case that we don't even have initiators to test
with at this point and is IMHO an unnacceptable answer to the people who
have put in the time to really make this all go.

Again, I am open to addressing this as a next-merge-window item once we
have access to a functioning initiator that supports something aside
from the default two cases provided in their series, but lets please,
please avoid slipping yet another window here when we still have >= 15K
LOC in three other HW target mode fabric drivers in flight for the next
round

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists