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

Boaz Harrosh
2010-09-22T11:18:49Z

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.

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

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