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

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

On Wed, 2010-09-22 at 13:18 +0200, Boaz Harrosh wrote:
> On 09/22/2010 09:16 AM, Nicholas A. Bellinger wrote:
> > 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.  
> 
> Perhaps a strengthen chmod here. And if then, done by root, a big fat
> "shoot self in the foot" message in dmsg for the poking where you don't
> need to. type.
> 

Hmm, I don't believe configfs currently supports a distinction between
permissions for something along these lines.

> (BTW: could you re establish the link after it's deleted the way you
>  do at setup?)
> 

Yes, the "dangling" MappedLUNs symlinks for this special case would need
to be explictly removed via unlink(2) and then re-created using a new
TPG LUN struct se_lun->lun_group containing a valid symlink back to
a TCM Core backstore a valid configfs symlink source.

Thanks!

--nab