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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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? 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
