On Sat, May 30, 2020 at 12:30 AM Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>
> Currently we use a variable "metacopy" which signifies that dentry
> could be either uppermetacopy or lowermetacopy. Amir suggested that
> we can move code around and use d.metacopy in such a way that we
> don't need lowermetacopy and just can do away with uppermetacopy.
>
> So this patch replaces "metacopy" with "uppermetacopy".
>
> It also moves some code little higher to keep reading little simpler.
>
> Suggested-by: Amir Goldstein <amir73il@xxxxxxxxx>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> ---
> fs/overlayfs/namei.c | 57 ++++++++++++++++++++++----------------------
> 1 file changed, 28 insertions(+), 29 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 5d80d8cc0063..a1889a160708 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -823,7 +823,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> struct dentry *this;
> unsigned int i;
> int err;
> - bool metacopy = false;
> + bool uppermetacopy = false;
> struct ovl_lookup_data d = {
> .sb = dentry->d_sb,
> .name = dentry->d_name,
> @@ -869,7 +869,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> goto out_put_upper;
>
> if (d.metacopy)
> - metacopy = true;
> + uppermetacopy = true;
> }
>
> if (d.redirect) {
> @@ -906,6 +906,22 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> if (!this)
> continue;
>
> + if ((uppermetacopy || d.metacopy) && !ofs->config.metacopy) {
> + err = -EPERM;
> + pr_warn_ratelimited("refusing to follow metacopy origin"
> + " for (%pd2)\n", dentry);
> + goto out_put;
> + }
> +
> + /*
> + * Do not store intermediate metacopy dentries in chain,
> + * except top most lower metacopy dentry
> + */
> + if (d.metacopy && ctr) {
> + dput(this);
> + continue;
> + }
> +
> /*
> * If no origin fh is stored in upper of a merge dir, store fh
> * of lower dir and set upper parent "impure".
> @@ -940,17 +956,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> origin = this;
> }
>
> - if (d.metacopy)
> - metacopy = true;
> - /*
> - * Do not store intermediate metacopy dentries in chain,
> - * except top most lower metacopy dentry
> - */
> - if (d.metacopy && ctr) {
> - dput(this);
> - continue;
> - }
> -
> stack[ctr].dentry = this;
> stack[ctr].layer = lower.layer;
> ctr++;
> @@ -982,23 +987,17 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> }
> }
>
> - if (metacopy) {
> - /*
> - * Found a metacopy dentry but did not find corresponding
> - * data dentry
> - */
> - if (d.metacopy) {
> - err = -EIO;
> - goto out_put;
> - }
> + /* Found a metacopy dentry but did not find corresponding data dentry */
> + if (d.metacopy) {
I suggested this change and I think it is correct, but it is correct for a bit
of a subtle reason.
It is correct because ovl_lookup_layer() (currently) cannot return NULL
and set d.metacopy to false.
So I suggest to be a bit more defensive and write this condition as:
if (d.metacopy || (uppermetacopy && !ctr)) {
> + err = -EIO;
> + goto out_put;
> + }
>
> - err = -EPERM;
> - if (!ofs->config.metacopy) {
> - pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n",
> - dentry);
> - goto out_put;
> - }
> - } else if (!d.is_dir && upperdentry && !ctr && origin_path) {
> + /* For regular non-metacopy upper dentries, there is no lower
> + * path based lookup, hence ctr will be zero. dentry found using
> + * ORIGIN xattr on upper, install it in stack.
> + */
> + if (!d.is_dir && upperdentry && !ctr && origin_path) {
I don't like this comment style for multi line comment and I don't
like that you detached this if statement from else if.
I think it made more sense with the else because this is (as you write)
the non-metacopy case.
Thanks,
Amir.
