On Thu, Apr 5, 2018 at 9:22 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: > On Thu, Apr 05, 2018 at 10:37:57AM -0400, Vivek Goyal wrote: >> 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? > > If you don't like this locking ovl_inode->lock model, I guess for now > I could live with not removing REDIRECT after copy up. Once that gets > merged, I could do one patch series just to clean up REDIRECT after copy > up and do proper locking. > I think we are confusing two different things in this discussion. Locking ovl_inode->lock for changing redirect is something that should stay in the patch set IMO and should be simplified by moving set redirect before taking rename locks on two inodes. I was asking about removal of REDIRECT because this patch (ovl_verify_metacopy_data) is a bit tricky for me to review and I still don't feel confident about it. My intuition says we could go other ways as well: - unite METACOPY/REDIRECT xattr (we can call the unite xattr REDIRECT and not METACOPY) - memory barriers between setting/clearing/checking METACOPY/REDIRECT (there is already a barrier for setting upperdata flag, so that's half the work already. We can either have this discussion now or, as you suggested leave it to a following patch set. Rule of thumb - if this is v13 with 28 patches, might not be a bad idea to deffer 2 patches and reduce complexity... 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
