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