On Thu, 26 Sep 2024, Al Viro wrote: > On Thu, Sep 26, 2024 at 08:42:06AM +1000, NeilBrown wrote: > > > I don't think so. > > The old delegated_inode_new will be carried in to vfs_rename() and > > passed to try_break_deleg() which will notice that it is not-NULL and > > will "do the right thing". > > > > Both _old and _new are initialised to zero at the start of > > do_renameat2(), Both are passed to break_deleg_wait() on the last time > > through the retry_deleg loop which will drop the references - or will > > preserve the reference if it isn't the last time - and both are only set > > by try_break_deleg() which is careful to check if a prior value exists. > > So I think there are no leaks. > > Yecchhhh... What happens if break_deleg() in there returns e.g. -ENOMEM > when try_break_deleg() finds a matching inode? I don't see that that changes the calculus at all. > > I'm not even saying it won't work, but it's way too brittle for my taste ;-/ > I accept that the interactions are not immediately obvious and that is a problem. Maybe that could be fixed with documentation/code-comments. I'm certainly open to suggestions for restructuring the code to make it more obvious but as yet I cannot see a good alternative. The separation of responsibility between do_renameat2 and vfs_rename seems about right as between do_linkat and vfs_link etc. We want to keep the delegation blocked all around the loop to the next retry. Doing that through delegated_inode (or _new and _old versions) seems to work.... What particularly seems brittle to you? Maybe if I have one detail to focus on. Thanks, NeilBrown
