On Wed, Mar 07, 2018 at 10:21:30AM +0200, Amir Goldstein wrote: > On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: > > When a metacopy file is no longer a metacopy and data has been copied up, > > remove REDIRECT xattr. Its not needed anymore. > > > > Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx> > > --- > > fs/overlayfs/copy_up.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c > > index 0c8d2755bd25..704febd2e2fa 100644 > > --- a/fs/overlayfs/copy_up.c > > +++ b/fs/overlayfs/copy_up.c > > @@ -775,6 +775,15 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c) > > if (err) > > return err; > > > > + /* > > + * A metacopy files does not need redirect xattr once data has > > + * been copied up. > > + */ > > + err = vfs_removexattr(upperpath.dentry, OVL_XATTR_REDIRECT); > > + if (err && err != -ENODATA && err != -EOPNOTSUPP) > > + return err; > > + > > + err = 0; > > ovl_set_upperdata(d_inode(c->dentry)); > > return err; > > By intuition, I would say that removing redirect should be done after setting > upperdata flag. Not sure if it really matters in real life. > Maybe when racing a lookup of a metacopy hardlink and copy up data of > an upper alias? > > Also, it would make sense to also ovl_dentry_set_redirect(c->dentry, NULL) > probably use a helper ovl_clear_redirect() for the locking. > > But that highlights a serious problem with current patches - > Access to OVL_I(inode)->redirect is protected with parent mutex in ovl_lookup() > and additionally with dentry->d_lock in ovl_rename() > That is sufficient for directories which can only have a single dentry > alias to an > inode but not at all sufficient for non-directories. Quick question on this. For non-dir files, will ovl_inode->redirect be protected by VFS locking. Atleast for our use case. We seem to be touching ovl_inode->redirect in rename, link and lookup path. VFS rename locks both source and destination inodes and link path will lock source and destination as well. So I think a rename and link can not make progress in parallel if any or the source or target is files are common. If that's the case two redirect writers can't stomp over each other. Lookup path will only set ovl_inode->redirect only if inode state is I_NEW and that means other vfs operations on same inode could not be making any progress. Now comes question of copy up path when we remove redirect xattr and modify ovl_inode->redirect. I don't know if we can assume that VFS is holding inode lock when copy up happens. If this assumption is valid, then it means rename and link can't make progress and it is safe to modify ovl_inode->redirect. I guess this assumption might not be valid, and that's why we need ovl_inode->lock. Not sure though. Can you throw some light at it. Thanks 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
