On Fri, Apr 6, 2018 at 6:37 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: > On Fri, Apr 06, 2018 at 12:46:31PM +0300, Amir Goldstein wrote: >> On Thu, Apr 5, 2018 at 11:45 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: >> [...] >> >> > >> >> > 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... >> >> >> > >> > Hi Amir, >> > >> > Ok, let me drop removing REDIRECT from the patchset to reduce >> > complexity. Lets first try to get in a basic version in. >> > >> > Now coming to the question of locking ovl_inode->lock. If REDIRECTS >> > are never removed and only upgraded (from relative to absolute), my >> > understanding is that current VFS locking is sufficient to prevent >> > races. Only rename and link path set redirects and both the paths take >> > inode locks and that means two set redirects can't make progress in >> > parallel on an inode. >> > >> > When it comes to ovl_lookup(), we will set ovl_inode->redirect only for >> > the case of I_NEW. So no races should be there as well. >> > >> > So far we had to add locking due to copy up path and now copy up path >> > will not touch redirect xattr. I probably will not even clear >> > ovl_inode->inode redirect as we are not removing REDIRECT. >> > >> > Do you see any other path racing and still needed ovl_inode->lock? >> > >> >> Let's see. >> >> Current code is taking A->d_lock in ovl_set_redirect(A) to protect >> against racing with redirect path traversal in ovl_get_redirect(A/B/c). >> >> For the purpose of protecting ancestors redirect path, d_lock is >> sufficient and is needed anyway to access d_name. >> >> Also any rename in the filesystem is *also* serialized with >> ovl_sb->s_vfs_rename_mutex. That would make the change I suggested >> to move ovl_set_redirect() before lock_rename() a bit tricky. >> You may say that taking d_lock in ovl_set_redirect() is not strictly >> needed, but I guess it is better coding (or maybe something I am >> missing). >> >> Now when adding ovl_set_redirect() for non-dir and in ovl_link() the >> plot thickens, because: >> 1. link is not serialized with ovl_sb->s_vfs_rename_mutex lock, >> so path traversal in ovl_get_redirect() in unstable >> 2. 'old' dentry in ovl_link() can be disconnected, so there is >> no path for ovl_get_redirect() at all. >> >> The only way to get a disconnected dentry is with nfs export, so for >> now, it is enough to WARN_ON and return error in ovl_set_redirect() >> for (dentry->d_flags & DCACHE_DISCONNECTED) >> with a comment referring to nfs_export+metacopy support. > > Hi Amir, > > I can put this warning for DCACHE_DISCONNECTED dentries. > > Now, most interesting part for me here is that why do we need to > stop/synchronize other renames in the system while some set_redirect/get > _redirect operation is taking place. That's the part I don't understand. > When I am looking at current code, I feel d_lock, seems to be good enough > to make sure that ovl_get_redirect() works fine when parallel renames > progressing on other cpu. Right. > > So a simple example, is say I am creating a link bar/foo-link.txt to a file > foo/foo.txt and that triggers setting absolute redirect on foo.txt > and we will call ovl_get_direct() and traverse the tree up till root. > Now say a part of the tree is also being renamed. Say foo/ is being > renamed to alpha/. I am wondering is d_lock is not enough to make sure > this is not a problem. > > We always set redirect first and then do rename. That means d_parent > should be changed only after redirect has been set on a dentry. And > that should guarantee that if ovl_get_redirect() sees new parent, > then parent_dentry->redirect has been already set. If it sees dentry > before rename, then redirect might be there if not, dentry name would > be used and it will also see the old parent and continue traversal > up. > > Is there anything I am missing? No. I think you got it right. I was confused. > >> >> Maybe the simplest solution w.r.t stabilizing path traversal >> is to move ovl_set_redirect() above lock_rename() >> as I suggested and use a new lock ofs->ovl_rename_mutex >> inside ovl_get_redirect() to protect the !samedir traversal >> and also take that lock before lock_rename() in ovl_rename(). > > I already made changes to move ovl_set_redirect() above lock_rename() > in my copy. I still can't see the need of ofs->ovl_rename_mutex. Please > help me understand the need with an example. > No need. >> >> Back to the original question: how should ovl_inode->redirect >> be protected if at all it needs protection? > > At this point I am thinking at max we need to just take ovl_inode->lock > to protect ovl_inode->redirect. It just makes it little safer and > relatively easier to understand. > I agree. I don't think ovl_inode->lock is needed, but I would add a comment explaining why (VFS inode lock). The problem is that we cannot get rid of d_lock for path traversal and I don't see how we can easily take both ovl dentry d_lock with ovl_inode->lock, because the latter is a sleeping lock, but layering wise it is logically below the overlay dentry lock. So better not add ovl_inode->lock Sorry for the noise. I hope moving set redirect before lock_rename simplified your patches, so this whole exercise served a point. 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
