On Wed, Mar 14, 2018 at 03:15:33PM -0400, Vivek Goyal wrote: > 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? > > I think you found a good race situation. > > > > > 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. > > This is a good point. So we need to protect OVL_I(inode)->redirect with > oi->lock mutex as well (atleast for non-dirs). So ovl_rename() will nest > 3 locks (which it already does for index case). > > parent dir i_mutex. > oi->lock > dentry->d_lock(). > > I will try to write a patch for this and see what issues do I face Hi Amir, I am trying to understand better how you are taking oi->lock w.r.t nlink stuff and I am having a hard time. - Why do you keep oi->locked for the duration of operation (link, unlink etc) using ovl_nlink_start() and ovl_nlink_end(). - What exactly is oi->lock protecting. I understand its protecting copy up. What else. Comment just says "synchronize copy up and more" and its not clear what is that "more". I need to understand that better to be able to use this lock for protecting "oi->redirect" for the case of hard links. 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
