Re: [PATCH v13 17/28] ovl: During rename lock both source and target ovl_inode

[Vivek Goyal <vgoyal@redhat.com>]

On Fri, Mar 30, 2018 at 09:50:53AM +0300, Amir Goldstein wrote:
> On Thu, Mar 29, 2018 at 10:38 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > In some cases we need to take both source (old) and target(new) dentry
> > ovl_inode->lock. This patch adds support for that. Locks are taken in
> > order of increasing inode address to avoid deadlock. This code has been
> > taken from lock_two_nondirectories().
> >
> > As of now, metacopy needs this lock if we are planning to update redirect
> > information on source/target ovl_inode. nlink related accounting takes this
> > lock on target for the case of overwrite.
> >
> 
> This is very over complicated.
> 
> Here is how this could be simplified.
> 
> Setting redirects does NOT need to happen under all the complicated rename
> locks. Settings redirects can happen completely before everything else
> because it is harmless to set redirect and not rename afterwards.

hmm..., So basically set redirects before doing lock_rename(new_upperdir,
old_upperdir). I guess even before ovl_set_nlink_start() takes inode
lock. That way we don't have to hold two inode locks together and
we should be able to take one inode lock at a time, set redirect and
release lock.

I spent some time thinking if I can spot anything obviously wrong
but could not find anything. So I will try to implement this. If
this works, I like the idea of simplifying the locking a bit. Right
now trying to take two inode locks makes it complicated.

Vivek
> 
> I would split out a helper ovl_rename_prep() it can also take care of
> copy ups and check ovl_can_rename() and then set redirect if needed
> on old and/or new locking them one at a time.
> You can take the ovl_inode->lock inside ovl_set_redirect() and you can also
> override credentials or whatever is needed due to moving set redirect
> earlier.

> 
> You probably don't need all the nlink_start re-factoring with this change.
> 
> Thanks,
> Amir.