On Fri, Apr 06, 2018 at 07:21:07PM +0300, Amir Goldstein wrote: > 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. Actually, I think ovl_inode->lock is at same layer as inode->i_rwsem. So VFS first takes i_rwsem and then d_lock. And we probably should do the same thing. In fact we are already taking ovl_inode->lock in ovl_nlink_start() first and then calling ovl_set_redirect() which takes d_lock. > > So better not add ovl_inode->lock > Sorry for the noise. I will not add ovl_inode->lock if I can explain everything. I have a feeling that redirect upgrade path will have some funny interactions with ovl_lookup() path. Let me write the new patches and see if ovl_inode->lock is still required. > > I hope moving set redirect before lock_rename simplified your > patches, so this whole exercise served a point. It definitely does. Thanks for that idea. 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
