Re: [RFC 02/22] configfs: Add struct configfs_item_operations->check_link() in configfs_unlink()

["Nicholas A. Bellinger" <nab@linux-iscsi.org>]

On Mon, 2010-09-20 at 15:06 -0700, Joel Becker wrote:
> [Sorry on the delay, I was out of town]
> 

Hi Joel,

Many, thanks for your followup on this item, my comments are below.

> On Fri, Sep 10, 2010 at 12:52:03PM -0700, Nicholas A. Bellinger wrote:
> > On Fri, 2010-09-10 at 12:44 -0700, Joel Becker wrote:
> > > 	You can refcount without check_link().
> > 
> > So what do you recommend here..?
> 
> 	That your ACL object, or whatever it is that considers itself to
> be refcounted by the number of links, keep track of that and only free
> itself when all are gone rather than freeing itself when the first goes
> away.
> 

Ok, I see what you mean by internal refcounting within the configfs
consumer to handle this case..

> > The problem is that the 'unlink sub_child/group1/src_0/src_link' can't
> > signal to the other struct config_group to also call an internal 'unlink
> > sub_child/group2/dst_0/dst_link' to drop the child link outside of it's
> > struct config_group.
> 
> 	Nor should it.  I'm asking what is so wrong about a world where
> sub_child/group1/src_0/src_link is gone but
> sub_child/group2/dst_0/dst_link remains?  Maybe that target object can't
> work anymore, but the user broke it by breaking the link.
> 

Yes, so trying to avoid the unlink alltogether here was my main
intention thus far.  

Actually leaving the sub_child/group2/dst_0/dst_link in the example here
would be acceptable for the TCM MappedLUN case, because really we never
expect this case to this unless someone is poking at configfs directly,
and our userspace code will never do this intentionally.

> > > 	You're still fighting allowing the links to go away.  You
> > > haven't explained why that is necessary.  You had a problem with a crash
> > > because you expected one reference to your ACLs and actually have two,
> > > but you can fix that without modifying configfs.
> > 
> > If this is the case then I must be mis-understanding what you mean by
> > configfs consumer refcounting from allow_link() and drop_link().  Can
> > you give me a bit more detail where I should be looking..?
> 
> 	Here's how I sort of understood things.  First, you create the
> src_link pointing to $object.  This somehow allocates some sort of ACL
> structure hanging off of $object.  Then you create dst_link pointing to
> src_link, which really ends up pointing to the $object.  So now you have
> src_link and dst_link pointing to $object.
> 	Finally, someone unlinks src_link.  This triggers $object to
> free the ACL structure.  When the caller later removes dst_link, it
> crashes because it was expecting ACL to still be there.  Do I have it
> right?

Correct.

> 	I'm saying that $object should count how many people are
> pointing to it, so that when you remove src_link, ACL is *not* freed.
> It will only be freed when both src_link and dst_link are removed.  This
> way you do not crash.  Perhaps I'm not understanding the ACL object.
> Perhaps I'm missing the mechanism entirely.  But I don't see why the ACL
> object must necessarily be freed when one symlink is removed but not the
> other.
> 

No, I think your points here make perfect sense.  I will look into a
patch that leaves the TCM fabric MappedLUNs symlinks in place when the
underlying TPG fabric LUN symlink is removed without breaking anything,
but still does the necessary accounting to ensure that shutdown with
active I/Os still works as expected.  I will plan to drop the
->check_link() patch from the forthcoming RFC v2 series.

In the end I think his is the best approach for .37, eg: no configfs
change required.  I am still open to the discussion for resolving this
within fs/configfs proper, but at this point I don't have a strong
preference and will follow your direction here.

Many thanks for your invaluable input Joel!

--nab