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: >> On Mon, Apr 30, 2018 at 9:08 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: >> > On Mon, Apr 30, 2018 at 5:02 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: >> [...] >> >> >> >> I think we need to check metacopy here otherwise it becomes racy. For >> >> example, what if there is a hard link (say, l1 and l2) with metacopy xattr. >> >> ovl_lookup(l1) will think metacopy is on while another thread on another >> >> cpu might have trigged copy up, remove metacopy xattr. And it is possible >> >> that inode got flushed out of cache. So by the time ovl_lookup(l1), calls >> >> iget5_locked(), it will get a new inode and it will initialize inode >> >> with wrong information. >> >> >> >> I had done similar thing for REDIRECT, but once we removed logic to >> >> remove REDIRECT on copy up, I felt I did not have to check redirect >> >> again here. >> >> >> >> In general, I feel that once we have the inode lock, we should check >> >> metacopy and redirect both and then initialize inode. And not rely >> >> on information which was checked outside the lock and might have >> >> been stale by now. >> >> >> > >> > Hmmm... so as you once already said, we have a race with INDEX as well. >> > In general, I feel it would be better to do a minimal sanity check inside >> > inode lock for INDEX and METACOPY (maybe REDIRECT) and make >> > sure they are consistent with the info in ovl_inode_info. >> > If they are not, return -ESTALE and that may result in re-calling >> > lookup. >> > >> >> After thinking about it some more, I believe the way we should do it >> is implement ovl_iget_locked() and call it from ovl_lookup() *before* >> we lookup index. >> >> Then, *after* looking up index, >> If this turns out to be a broken hardlink, we throw away the >> inode that we got by lower and get a new inode hashed by upper. >> >> Then we can initialize the inode with another helper >> or directly in ovl_lookup() and unlock_new_inode(). >> >> 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. 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
