On Thu, Apr 26, 2018 at 12:10 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> When we create a hardlink to a metacopy upper file, first the redirect
> on that inode. Path based lookup will not work with newly created link
> and redirect will solve that issue.
>
> Also use absolute redirect as two hardlinks could be in different directores
> and relative redirect will not work.
>
> I have not put any additional locking around setting redirects while
> introducing redirects for non-dir files. For now it feels like existing
> locking is sufficient. If that's not the case, we will have add more
> locking. Following is my rationale about why do I think current locking
> seems ok.
>
> Basic problem for non-dir files is that more than on dentry could be
> pointing to same inode and in theory only relying on dentry based locks
> (d->d_lock) did not seem sufficient.
>
> We set redirect upon rename and upon link creation. In both the paths
> for non-dir file, VFS locks both source and target inodes (->i_rwsem).
> That means vfs rename and link operations on same source and target
> can't he happening in parallel (Even if there are multiple dentries
> pointing to same inode). So that probably means that at a time on an inode,
> only one call of ovl_set_redirect() could be working and we don't need
> additional locking in ovl_set_redirect().
>
> ovl_inode->redirect is initialized only when inode is created new. That
> means it should not race with any other path and setting ovl_inode->redirect
> should be fine.
>
> Reading of ovl_inode->redirect happens in ovl_get_redirect() path. And this
> called only in ovl_set_redirect(). And ovl_set_redirect() already seemed
> to be protected using ->i_rwsem. That means ovl_set_redirect() and
> ovl_get_redirect() on source/target inode should not make progress in
> parallel and is mutually exclusive. Hence no additional locking required.
>
> Now, only case where ovl_set_redirect() and ovl_get_redirect() could race
> seems to be case of absolute redirects where ovl_get_redirect() has to
> travel up the tree. In that case we already take d->d_lock and that should
> be sufficient as directories will not have multiple dentries pointing to
> same inode.
>
> So given VFS locking and current usage of redirect, current locking around
> redirect seems to be ok for non-dir as well. Once we have the logic to
> remove redirect when metacopy file gets copied up, then we probably will
> need additional locking.
>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
> fs/overlayfs/dir.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 1acea6887b05..546ad7d34160 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -24,6 +24,8 @@ module_param_named(redirect_max, ovl_redirect_max, ushort, 0644);
> MODULE_PARM_DESC(ovl_redirect_max,
> "Maximum length of absolute redirect xattr value");
>
> +static int ovl_set_redirect(struct dentry *dentry, bool samedir);
> +
> int ovl_cleanup(struct inode *wdir, struct dentry *wdentry)
> {
> int err;
> @@ -468,6 +470,9 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> const struct cred *old_cred;
> struct cred *override_cred;
> struct dentry *parent = dentry->d_parent;
> + struct dentry *hardlink_upper;
> +
> + hardlink_upper = hardlink ? ovl_dentry_upper(hardlink) : NULL;
>
> err = ovl_copy_up(parent);
> if (err)
> @@ -502,12 +507,19 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> put_cred(override_creds(override_cred));
> put_cred(override_cred);
>
> + if (hardlink && ovl_is_metacopy_dentry(hardlink)) {
> + err = ovl_set_redirect(hardlink, false);
> + if (err)
> + goto out_revert_creds;
> + }
> +
> +
> if (!ovl_dentry_is_whiteout(dentry))
> err = ovl_create_upper(dentry, inode, attr,
> - hardlink);
> + hardlink_upper);
> else
> err = ovl_create_over_whiteout(dentry, inode, attr,
> - hardlink);
> + hardlink_upper);
> ovl_copytimes_with_parent(dentry);
> }
> out_revert_creds:
> @@ -603,8 +615,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
> inode = d_inode(old);
> ihold(inode);
>
> - err = ovl_create_or_link(new, inode, NULL, ovl_dentry_upper(old),
> - ovl_type_origin(old));
> + err = ovl_create_or_link(new, inode, NULL, old, ovl_type_origin(old));
> if (err)
> iput(inode);
>
> --
> 2.13.6
>
--
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
