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 > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
