Re: [PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Vivek Goyal <vgoyal@redhat.com>]

On Fri, Mar 16, 2018 at 06:09:31PM +0200, Amir Goldstein wrote:
> On Fri, Mar 16, 2018 at 5:06 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Fri, Mar 16, 2018 at 03:17:47PM +0200, Amir Goldstein wrote:
> [...]
> >
> > I am just trying to understand this nlink stuff and associated locking
> > better. It has confused me many a times.
> >
> 
> There are only two rules to understand:
> 1. The delta between upper nlink and union nlink doesn't change on link()
>     unlink() rename()
> 2. The delta between lower nlink and union nlink doesn't change op copyup
> 
> So all we need to do to make union nlink crash consistent is make sure
> that we store NLINK xattr relative to lower before copyup and store it
> relative to upper nlink before link/unlink/rename.
> 
> If we allow copyup (of lower hardlink) and link (of upper hardlink) at the
> same time, we cannot guaranty crash consistency of union nlink.
> 
> > Can you give me an example where things will go wrong if we drop the
> > lock after setting ovl_set_nlink_upper(). I have spent enough time
> > thinking about it and can't think what will go wrong.
> >
> 
> lower nlink = 2
> upper nlink = 2 (1 copy up and 1 index)
> union nlink = 2
> NLINK xattr = "U+0"
> 
> start link():
> oi->lock
> store NLINK xattr = "U+0"
> oi->unlock
> ...
> ovl_do_link() (but not yet inc_nlink(inode))
> 
> start copyup():
> oi->lock
> store NLINK xattr = "L+0"
> copy up inode
> store NLINK xattr = "U-2" (because upper nlink is now 4, but
> inode->i_nlink is still 2)
> 
> CRASH
> 
> BOOT
> 
> ovl_get_nlink()
> 
> lower nlink = 2
> upper nlink = 4 (2 copy ups, 1 hardlink and 1 index)
> NLINK xattr = "U-2"
> union nlink = 2 (WRONG should be 3)
> 
> Now unlink the 2 copy ups and the new hardlinks and you hit
> WARN_ON(inode->i_nlink == 0) in drop_nlink()
> 
> Hope I got this right...

Aha... I get it now. This is a good example which shows why we need
to keep holding the ovl_inode->lock. Thanks.

> 
> 
> >>
> >> What is exactly the problem that you are trying to solve?
> >> It seems that you need to protect oi->redirect in copyup/rename/link.
> >> copyup/link already take the oi->lock and rename takes oi->lock
> >> on new inode in case of "overwrite".
> >> A simple solution would be to call ovl_nlink_start()/ovl_nlink_end()
> >> in rename for both old and new inodes, regardless of "overwrite".
> >> It may be unneeded, but in fact, ovl_nlink_start() doesn't do
> >> anything wrong, it just recomputes NLINK xattr and most of those
> >> recomputes will store the same value anyway, unless machine crashes
> >> during copyup between ovl_set_nlink_lower() and
> >> ovl_set_nlink_upper() and leaves the value of NLINK xattr relative to
> >> lower nlink.
> >
> > ovl_nlink_start() also assumes that file is indexed. metadata copy up
> > stuff does not have dependency on index.
> 
> That's probably ok because you can set independent redirects on
> different broken hardlinks of the same lower.
> 
> >
> > So I am instead passing "locked" state to ovl_set_redirect() and
> > ovl_get_redirect(), and if oi->lock is not already held, then
> > these functions will acquire it for non-dir.
> 
> Sounds ok.
> 
> >
> > I meant to ask you one more question. Without indexing it is possible
> > that two upper layer hardlinks (broken hardlinks), have redirects to
> > same lower. I know that for the case of directories, you don't want
> > two redirects to same lower. I am wondering what's the problem it
> > leads to and if same problem applies for non-dir as well?
> 
> Yes, see in the test:
> https://github.com/amir73il/xfstests/blob/overlayfs-devel/tests/overlay/049#L98
> 
> two redirects can have the same st_ino if lower nlink == 1 and
> they are not indexed.

Ok, thanks. I will need to spend some more time on this and see if
I should make index=on mandatory for metacopy=on.

Vivek