Re: [PATCH v13 27/28] ovl: Verify a data dentry has been found for metacopy inode

[Vivek Goyal <vgoyal@redhat.com>]

On Thu, Apr 05, 2018 at 10:37:57AM -0400, Vivek Goyal wrote:
> On Wed, Apr 04, 2018 at 06:51:57PM +0300, Amir Goldstein wrote:
> > On Wed, Apr 4, 2018 at 4:21 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > > On Wed, Apr 04, 2018 at 03:51:37PM +0300, Amir Goldstein wrote:
> > >> On Wed, Apr 4, 2018 at 3:29 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > >> > On Fri, Mar 30, 2018 at 01:53:24PM +0300, Amir Goldstein wrote:
> > >> >> On Thu, Mar 29, 2018 at 10:38 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > >> >> > If we find a upper metacopy inode, make sure we also found associated data
> > >> >> > dentry in lower. Otherwise copy up operation later will fail.
> > >> >> >
> > >> >> > There are two cases where this can happen. First case is that somehow
> > >> >> > data file was removed from lower layer. Other case is that REDIRECT
> > >> >> > xattr was removed due to copy up of file on another cpu (when inode is
> > >> >> > shared between two dentries) and hence ovl_lookup() could not find the
> > >> >> > correct dentry.
> > >> >> >
> > >> >>
> > >> >> Remind me again why we remove REDIRECT xattr?
> > >> >> Is it a must for functionality or just for being boy scouts?
> > >> >> I would prefer to avoid having to deal with races of this sort.
> > >> >> You can cleanup REDIRECT for non-dir that is not metacopy
> > >> >> on lookup when finding a I_NEW inode.
> > >> >
> > >> > Ok, thinking more about it. If we were to clean REDIRECT on lookup when
> > >> > finding I_NEW inode, that means we will have to always do
> > >> > vfs_removexattr(OVL_XATTR_REDIRECT) on all non-metacopy non-dir files.
> > >> > That does not sound like a very good idea. Its unnecessary overhead in
> > >> > lookup path.
> > >> >
> > >> > IOW, I think removing REDIRECT and doing appropriate locking around
> > >> > ovl_inode->redirect is probably better.
> > >> >
> > >>
> > >> Here is what I propose.
> > >> During lookup, you anyway check REDIRECT and check METACOPY
> > >> on upper and then call ovl_get_inode() with upper redirect and upper
> > >> metacopy information.
> > >
> > > We check for upperredirect only if metacopy xattr is found. Otherwise
> > > we skip checking for redirect.
> > >
> > > https://github.com/rhvgoyal/linux/blob/metacopy-v13/fs/overlayfs/namei.c#L270
> > >
> > >>
> > >> IF this is a new inode AND both REDIRECT and METACOPY were
> > >> found on upper THEN it is safe to remove REDIRECT xattr.
> > >
> > > If both METACOPY and REDIRECT were found, then we should not remove
> > > REDIRECT. That REDIRECT is still useful. REDIRECT should be removed
> > > only if METACOPY is not found and REDIRECT is found (on a non-dir file).
> > >
> > >>
> > >> Maybe I am missing something, but I don't see where the extra overhead
> > >> is, beyond the overhead already there for metacopy enabled lookup.
> > >
> > > Given we don't check for REDIRECT if upper is not METACOPY, that means
> > > we will have to always check for REDIRECT in ovl_get_inode() and add
> > > the unnecessary overhead (To all non-dir files).
> > >
> > 
> > I see.
> > 
> > >>
> > >> OTOH, I don't see what cleaning REDIRECT gets us in the first place.
> > >> During lookup, REDIRECT does not affect non metacopy non-dir,
> > >> because we skip ovl_check_redirect().
> > >> REDIRECT could actually be useful for reconstructing ORIGIN xattr
> > >> and index after copying layers, so not sure its a good thing to remove it
> > >> at all. After all, redirects are pretty rare as it is.
> > >
> > > I see it as unnecessary xattr present and feel that cleaning it is a
> > > good idea. Given we are thinking of packing REDIRECT xattr in tar file
> > > for layer backup and restore case, it makes even more sense to clean
> > > it up otherwise it shows up every where unnecessarily. I personally
> > > think it is always a good idea to cleanup information you don't need
> > > anymore, instead of letting it sit around.
> > >
> > 
> > Look. I have no objection to cleaning REDIRECT, but I am saying it
> > is tricky, so I think it is going to cost you a few more patches and maybe
> > a few more review cycles, so I advised against it.
> 
> Hi Amir,
> 
> Anyway, I doubt these patches are going to be merged for 4.17. So 
> I am fine if it takes few more revisions to properly review it. Doing
> it properly is more important. (Despite the fact that I am little
> exhausted now. :-))
> 
> > 
> > But here is another idea: store the redirect string in the METACOPY
> > xattr, this way, removal of METACOPY xattr atomically cleans up
> > REDIRECT and in lookup, only need to check METACOPY xattr
> > (exists but empty means no redirect).
> 
> I don't like this much. I had thought about it but did not pursue it.
> 
> - First of all, I don't like that REDIRECT for dir and non-dir will be
>   stored differently. 
> 
> - Secondly, xattr is just one pience. We also need to protet
>   ov_inode->redirect field and this will not solve that issue. That issue
>   can be solved only if we provide proper locking so that readers and
>   writers of ovl_inode->redirect don't race and run into unexpected
>   surprises.
> 
> Given VFS locking does not protect against copy up path races with
> rename(), I think that right solution for this problem is to protect
> against this race by taking ovl_inode->lock. I think this is something
> future readers can understand and build more functionality on top.
> 
> If we are primarily worried about races against copy up for redirect, then
> we probably don't have to double lock both ovl_inodes. As you suggested,
> I should be able to move out set_redirect() earlier in rename
> path and take one lock at a time. That should simplify the locking
> logic a bit. How about this instead?

If you don't like this locking ovl_inode->lock model, I guess for now
I could live with not removing REDIRECT after copy up. Once that gets
merged, I could do one patch series just to clean up REDIRECT after copy
up and do proper locking.

Vivek