Message-id: <000001cdd6e1$1ac24f60$5046ee20$%p@samsung.com>
Date:	Mon, 10 Dec 2012 15:17:34 +0100
From:	Andrzej Pietrasiewicz <andrzej.p@...sung.com>
To:	Andrzej Pietrasiewicz <andrzej.p@...sung.com>,
	'Joel Becker' <jlbec@...lplan.org>,
	'Sebastian Andrzej Siewior' <bigeasy@...utronix.de>
Cc:	'Michal Nazarewicz' <mina86@...a86.com>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	'Kyungmin Park' <kyungmin.park@...sung.com>,
	'Felipe Balbi' <balbi@...com>,
	'Greg Kroah-Hartman' <gregkh@...uxfoundation.org>,
	Marek Szyprowski <m.szyprowski@...sung.com>
Subject: RE: [RFC][PATCH] fs: configfs: programmatically create config groups

@Joel in particular: please see my comment in the bottom.

On Monday, December 10, 2012 12:57 PM I wrote:
> Subject: RE: [RFC][PATCH] fs: configfs: programmatically create config
> groups
> 
> Hello Joel,
> 
> So you are alive, I'm glad to hear from you ;) Thank you for your
response.
> 
> On Saturday, December 08, 2012 12:18 AM Joel Becker wrote:
> > Subject: Re: [RFC][PATCH] fs: configfs: programmatically create config
> > groups
> >
> > Hey Guys,
> > 	Sorry I missed this for a while.  I'll make a couple of inline
> > comments, and then I'll summarize my (incomplete) thoughts at the
bottom.
> >
> > On Wed, Nov 28, 2012 at 02:50:13PM +0100, Sebastian Andrzej Siewior
> wrote:
> > > On 11/28/2012 02:05 PM, Michal Nazarewicz wrote:
> > > >>On 11/27/2012 05:23 PM, Michal Nazarewicz wrote:
> > > >>>How should a generic tool know what kind of actions are needed for
> > > >>>given function to be removed?  If you ask me, there should be a way
> > > >>>to unbind gadget and unload all modules without any specific
> > > >>>knowledge of the functions.  If there is no such mechanism, then
> > > >>>it's a bad user interface.
> >
> > 	Please remember that configfs is not a "user" interface, it's a
> > userspace<->kernelspace interface.  Like sysfs, it's not required to be
> > convenient for someone at a bash prompt.  My goal is that it is *usable*
> > from a bash prompt.  So it must be that you can create/destroy/configure
> > objects via mkdir/rmkdir/cat/echo, but you might have a lot of those
> > mkdir/echo combos to configure something.
> > When it comes to the "user" interface, a wrapper script or library
> should
> > be converting a user intention into all that boilerplate.
> >
> 
> If you say so there is little we can do, is there? :O
> 
> <snip>
> 
> >
> > 	Yeah, user tools are expected (and should be).
> >
> 
> ditto
> 
> > > Anyway, for this to work we have to go through Joel.
> > >
> > > >	rmdir udcs/*		# unload all UDCs
> > >
> > > No, for this you still have to rmmod :)
> > >
> > >
> > > >>>I think the question is of information flow direction.  If user
> > > >>>gives some information to the kernel, she should be the one
> > > >>>creating any necessary directories.  But if the information comes
> > > >>>from kernel to the user, the kernel should create the structure.
> >
> > 	This last paragraph actually describes the distinction between
> > configfs and sysfs.  More specifically, if userspace wants to create an
> > object in the kernel, it should use configfs.  If the kernel has created
> > an object on its own, it exposes it via sysfs.  It is a deliberate non-
> > goal for configfs to replicate sysfs behavior.
> 
> So if the lifetime of some object is controlled by the user, it belongs
> to configfs. If, on the other hand, the lifetime is controlled by the
> kernel, it belongs to sysfs. I think that the trouble with e.g. luns
> is that they might want to behave like both (at least in the "more
> automated"
> approach where they are programmatically created): they are created
> by the kernel (=> sysfs) but their lifetime is controlled by the user
> (=>configfs).
> 
> >
> > [General Thoughts]
> >
> > 	First let me restate your problem to see if I understand it.
> > 	You want to expose e.g. five LUNs.  They should eventually appear in
> > configfs as five items to configure (.../{lun0,lun1,...lun4}/.  The
> > current configfs way to do this is to have your setup script do:
> >
> > 	cd /cfg/.../mass_storage
> > 	mkdir lun0
> > 	echo blah >lun0/attr1
> > 	echo blahh >lun0/attr2
> > 	mkdir lun1
> > 	echo blag >lun1/attr1
> > 	echo blagg >lun1/attr1
> > 	...
> >
> > I think the primary concern expressed by Andrzej is that a random user
> > could come along and say "mkdir lun8", even though the gadget cannot
> > support it.  A secondary concern from Michal is that userspace has to
> run
> > all of those mkdirs.  The thread has described varying solutions.
> > 	If these original directories were default_groups, you could
> > disallow any child creation and not require the setup script to create
> > directories.  Here I will point out that you *can* create default_groups
> > programmatically.  The default_groups pointer on each configfs_group can
> > be different.  ->make_group() can attach whatever groups it wants.
> > If this would work, it would fit both of your concerns, but you do not
> > have a priori knowledge of the LUN count.
> > 	Your original email proposed that the max lun be set like so:
> >
> > 	cd /cfg/.../mass_storage
> > 	echo 5 >luns_allowed
> >
> > There are then a couple of proposed ways to enforce the limit.  Your
> > ->create_group() idea wants luns_allowed to be able to create subdirs in
> > the ->store() function.  No ->mkdir() is provided, so a lunN cannot be
> > created by hand.
> > 	The ->create_group() idea has three challenges.  First, it creates
> > "default" groups *after* the object is created.  This makes no sense if
> > they are attributes of the toplevel object.  Second, it volates the
> > configfs mantra that user-generated objects are explicitly created.
> > The kicker, however, is the technical problem.  configfs explicitly
> > forbids tree changes inside attribute operations for deadlock reasons.
> > Trying to create new tree parts in luns_allowed->store() is exactly
that.
> > 	Another suggestion (I think I read this in the thread) would be to
> > have ->mkdir() function parse the name and check that it is lunN, where
> N
> > is less than the limit.  I think that this was shot down because of
> > parsing the LUN name.  I don't think that's too terrible, but there's
> > certainly room for argument.
> >
> > [Possible Solutions]
> >
> > 	Before coming back to the proposed patch, let's look at what I might
> > do if I were fitting this into configfs.
> > 	I would actually do what I described at first:
> >
> > 	cd /cfg/.../mass_storage
> > 	mkdir lun0
> > 	echo blah >lun0/attr1
> > 	...
> >
> > This is idiomatic configfs usage.  Sebastian noted this earlier in the
> > thread.  Michal thought the repeated mkdirs were a problem ("I think
> > that's suboptimal, and we can do better").  This is Michal's objection
> to
> > making the user do work.  This is where I point out we are not designing
> > configfs interfaces for ease of end-user use.  We're defining them for
> > transparent and discoverable interfaces for tools to create things in
> the
> > kernel.  So the repeated mkdir()s are actually a good thing from my
> > perspective; userspace is telling the kernel to create LUN objects.
> > 	But this doesn't alleviate Andrzej's primary concern, nor does it
> > answer Michal's concerns about corner cases.  Let's see if we can solve
> > those.
> > 	First, what about preventing extraneous LUNs?  There are two
> > approaches: the prevent approach, and the ignore approach.  In the
> Target
> > module, they take the ignore approach.  If you create a LUN that is
> > outside the scope, they just ignore it.  So a user can make 10 LUNs, but
> > if only five are allowed, the other five are ignored.  In the prevent
> > approach, we try to fail the mkdir() when we go over the limit.
> 
> I think I like the prevent approach better, because I don't know what to
> do with the unused luns in the ignore approach. I also think that it would
> be nice if configfs described the actual state of the system instead of
> presenting all the possible user-created garbage (i.e. something which is
> ignored).
> 
> > 	So you've set a limit with "echo 5 >luns_allowed".  How do you
> > restrict?  If you don't care about contiguous LUN numbering, you could
> > literally do this in your make_group():
> >
> > 	if (count_luns(parent) >= parent->luns_allowed)
> > 		return -ENOSPC;
> >
> > Note that we don't check the LUN number; SCSI does not require
> contiguous
> > numbering.  It does require LUN 0, which should probably be a default
> > group.  So your lun count starts at 1.
> > 	What if you decide you really want contiguous LUN numbers, which is
> > considered nice to have?  Then we get to Michal's concern about parsing
> > the name.  If you "mkdir lun1", the code has to confirm that it
> > a) starts with "lun", b) finishes with a parsable number, c) that number
> > is < luns_allowed.
> > 	But you need to do the parsing anyway!  If the LUN is created via
> > mkdir(), somehow the gadget system needs to know what LUN number to
> > advertise.  I would actually decouple it from the name.  The name should
> > be free-form; if it happens to be comprehensible, that's good.  Imagine
> > this:
> > 	cd /cfg/.../mass_storage
> > 	echo 4 >max_lun_id
> > 	mkdir lun100
> > 	echo 1 >lun100/lun_id
> >
> > The LUN actually has the ID of 1.  The fact that the directory name is
> > wrong is irrelevant to the gadget processing; non-hackers use the
> tooling,
> > and the tooling should  make sane names.  Then your
> > ->make_group() function can choose either approach to avoiding LUN IDs
> > that are too large.  It can prevent:
> >
> > 	lun_id->store() {
> > 		if (lun_id_exists(parent, value))
> > 			return ERR_PTR(-EINVAL);
> > 		if (value > parent->max_lun_id)
> > 			return ERR_PTR(-ENOSPC) /* or EINVAL */
> > 		setup_lun();
> > 		make_live();
> > 	}
> >
> > Or it can ignore:
> >
> > 	lun_id->store() {
> > 		if (lun_id_exists(parent, value))
> > 			return ERR_PTR(-EINVAL);
> > 		if (value <= parent->max_lun_id) {
> > 			setup_lun();
> > 			make_live();
> > 		}
> > 		/* Do nothing for the ignored sucker */
> > 	}
> >
> > 	I think that I, personally, would go with this <name>/lun_id scheme.
> > Given a requirement for contiguous LUNs and the max_lun_id restriction,
> > I'd go with the "prevent" option myself.
> 
> Taking into account what you say about the expected userspace tooling
> using a free-form name plus the lun_id attribute could be attractive,
> because the lun_id attribute's value can be analyzed at ->store() and,
> if it doesn't fit, an appropriate error can be easily returned using
> just what we already have in configfs. Prevent seems better to me, too.
> 
> > 	But is max LUNs a requirement?  Why not just have the LUN count be
> > the number of LUNs created?  In that case, you can allow or disallow
> > discontiguous LUN IDs without worrying about a max ID.
> >
> 
> I am not sure, but I think that Michal would say that he wouldn't like
> things to change once the gadget is up and running, so the number of
> luns should be fixed up front. On the other hand, after the gadget is
> set up, we can fail in ->make_group() to prevent the user from creating
> additional luns with which we don't know what to do. So what you suggest
> seems fine, but I would ask Michal to be on a safe side.
> 
> > [Really Hate Mkdir]
> >
> > 	If you considered it appropriate for the LUN objects to be created
> > by the kernel, the standard way would to have "echo 5 >luns_allowed"
> > create LUN objects in sysfs.  Yes, sysfs.
> > There's no reason you can't have things in configfs *and* sysfs.  My
> > understanding of your code says to me that the mkdir approach is the way
> > to go, but if you hate it, this works.
> >
> 
> I guess you are right, but see my point about luns' lifetime being
> controlled by the user and their creation being performed by the kernel.
> 
> > [What About the Patch?]
> >
> > 	In general, attribute setting that turns into created configfs
> > objects is against the theory of configfs.
> 
> This looks like an authoritative answer.
> 
> > In practice, it's a nightmare
> > locking change.  Coming into this discussion, I though you were doing
> > dymanic things at ->make_group() time, but that is already supported.
> > 	But I want to hear your thoughts.  I've dumped a bunch of alternate
> > approaches here.  Do any seem to fit?  What other challenges do you see?
> >
> > Joel
> >
> 
> Once upon a time, Felipe expressed interest in having the information
> about endpoints and interfaces (mostly read-only stuff, but not all of it)
> accessible in the gadget subsystem in configfs. This kind of information
> is fully available only after the gadget has been bound, that is, after
> the user created all the groups/items and filled all the attributes
> and told the gadget to start (with e.g. filling some attribute or
> creating a symlink). So this looks like a programmatic creation of
> configfs directories. On the other hand, Felipe wanted it for debugging
> purposes, so the user could just as well create the required directories
> by hand when they need to, as Sebastian once suggested. Again, this
> requires
> ->make_group() to fail if all the information is not available yet.
> But isn't it kind of similar to the luns' case? It is a bunch of objects
> whose lifetime is controlled by the user (bind the gadget, unbind the
> gadget),
> but which are created by the kernel.
> Another problem with this is that the user must at some point _guess_
> what name to use in mkdir. Example:
> 
> For each interface there should be a folder. So the user must know how
> many
> interfaces' folders to create and how to name them, like:
> 
> $ mkdir ...../some_usb_function/interface00
> $ mkdir ...../some_usb_function/interface01
> ...
> ...
> ...
> 
> In each interface folder, there should be some endpoint folders and the
> user
> must know how many endpoints there are and how to name them:
> 
> $ mkdir ...../some_usb_function/interface00/endpoint02
> $ mkdir ...../some_usb_function/interface00/endpoint81
> 
> So probably there should be some attribute in some_usb_function,
> which contains a list of available interface names, and some attribute in
> interface#, which contains a list of available endpoint names.
> Workable, but not so convenient. And there could be a problem, too,
> if the gadget is unbound: there is no way to remove the user-created
> directories other than by the user themselves, so there will be times
> (i.e. between gadget unbind and manual rmdir) when these
> interface/endpoint
> directories will not correspond to anything present in the system.
> 
> So maybe this kind of information should live in sysfs? But I don't
> know now if it would be any easier. Joel? Felipe?
> 

I forgot to mention, representing udcs (USB Device Controllers) in
configfs is similar to interfaces/endpoints: the user needs to guess
what name to use in mkdir, e.g. in:

$ mkdir ....../udcs/s3c-hsotg

the poor user needs to know that the udc is actually called s3c-hsotg.
And after the udc goes away from the system, the s3c-hsotg directory
remains in the filesystem until the user deletes it (as nobody else
but the user can do it). Again, an attribute like available_udcs in
the udcs directory can help. But still, there will be times when
its contents will not be consistent with the directories actually
present.

@Joel: Any thoughts about it? Commitable items? E.g. the user creates
whatever they want, so the "uncommitted" directory contains "garbage",
that is directories which do not have to correspond to anything
actually present in the system, but on commiting the gadget's item
(group in fact) everything is verified and only "correct" gadgets
are actually bound? Then, in the "uncommitted" directory the user
can still do whatever they want, but this is not effective until
uncommitting the old gadget and committing it again? There is just
a tiny issue with committable items: Are they implemented? :O

AP



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