On Thu, May 3, 2018 at 11:07 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > On Thu, May 3, 2018 at 6:12 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: >> On Tue, May 01, 2018 at 05:37:09PM +0300, Amir Goldstein wrote: [...] >>> >>> w.r.t METACOPY things will get complicated if we remove >>> METACOPY xattr and the upper does not have ORIGIN. >> >> I am not able to understand the problem you are referring to. >> >>> In that case we may have an inode that is already hashed by lower, >>> but for a new lookup that does not see METACOPY xattr it >>> looks up the inode in cache by upper and this is not good. >> >> Here is my understanding. Following happens without metacopy feature. >> >> - As of now, only time we do not create ORIGIN on upper is for the >> case of broken hardlink. (for a non-dir file). >> >> - In case of broken hard link (index=off), if file is on lower only, we >> do not put new inode in inode cache at all. Once file gets copied up, >> we hash it by upper inode. And reported inode number changes over >> copy up. >> >> Now above behavior should remain same even with metacopy=on. For the >> case of broken hard link. >> >> - If file is on lower only, we will not put inode in cache. And once >> file gets copied up metadata only, we will hash it with upper inode. >> >> The only difference here is that without metacopy non-dir file will >> have lowerdentry=NULL while with metacopy on, lowerdentry will be >> non-null. But bylower should still be false because of following >> check in ovl_hash_bylower(). >> >> /* No, if lower hardlink is or will be broken on copy up */ >> if ((upper || !ovl_indexdir(sb)) && >> !d_is_dir(lower) && d_inode(lower)->i_nlink > 1) >> return false; >> >> And if bylower is false, we hash inode using upper inode as key. >> >> struct inode *key = d_inode(bylower ? lowerdentry : >> upperdentry); >> >> IOW, despite the fact lowerdentry is there (without ORIGIN on upper), >> we don't use that lowerdentry for inode hashing. And hence behavior >> should not change. >> >> What am I missing? >> > > Just maybe missing the case of nlink=1 and ORIGIN cannot be > followed because lower fs doesn't support file handles. > I just wanted to make sure this case is handled correctly. > OK. re-read the code and ready to explain the situation. The meaning of new OVL_CONST_INO flag is: "inode preserves st_ino across copy up". If lower fs support file handles it also means: "...and across eviction from inode cache". but if lower fs does not support file handles it means: "...while non-dir inode remains in cache" How does METACOPY change the situation? very slightly - with METACOPY, instead of st_ino change after copy_up+cache_evict, st_ino will not change after meta_copy_up+cache_evict, but will change after data_copy_up+cache_evict, which is not a problem as far as I can tell. Specifically, removing METACOPY xattr will not change st_ino nor hash_by_lower until after cache eviction and as long as we have one-to-one mapping of upper inode to lower inode that's good enough. Things will only break with filesystem inconsistencies like in xfstest overlay/049, but AFAICT, METACOPY does not introduce any real regressions, the only thing it does is add another way of creating an inconsistency with redirect on non-dir, which probably calls for a new xfstest. I hope I got it all right. Thanks, Amir. -- 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
