[RFC PATCH] ovl: suppress negative dentry in lookup

[RFC PATCH] ovl: suppress negative dentry in lookup

[Chengguang Xu]

When a file is only in a lower layer, after lookup a negative
dentry will be generated in the upper layer or even worse many
negetive dentries will be generated in upper/lower layers. These
negative dentries will be useless after construction of overlayfs'
own dentry and may keep in the memory long time even after unmount
of overlayfs instance. This patch tries to kill unnecessary negative
dentry during lookup.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/overlayfs/namei.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 723d17744758..cf0ec4d7bcec 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -200,7 +200,7 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
 	int err;
 	bool last_element = !post[0];
 
-	this = lookup_positive_unlocked(name, base, namelen);
+	this = lookup_one_len_unlocked(name, base, namelen);
 	if (IS_ERR(this)) {
 		err = PTR_ERR(this);
 		this = NULL;
@@ -209,6 +209,15 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
 		goto out_err;
 	}
 
+	/* Borrow the check from lookup_positive_unlocked */
+	if (d_flags_negative(smp_load_acquire(&this->d_flags))) {
+		d_drop(this);
+		dput(this);
+		this = NULL;
+		err = -ENOENT;
+		goto out;
+	}
+
 	if (ovl_dentry_weird(this)) {
 		/* Don't support traversing automounts and other weirdness */
 		err = -EREMOTE;
-- 
2.20.1

Re: [RFC PATCH] ovl: suppress negative dentry in lookup

[Amir Goldstein]

On Thu, May 7, 2020 at 4:21 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
> When a file is only in a lower layer, after lookup a negative

Or in no layer at all...

> dentry will be generated in the upper layer or even worse many
> negetive dentries will be generated in upper/lower layers. These
> negative dentries will be useless after construction of overlayfs'
> own dentry and may keep in the memory long time even after unmount
> of overlayfs instance. This patch tries to kill unnecessary negative
> dentry during lookup.
>
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> ---
>  fs/overlayfs/namei.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 723d17744758..cf0ec4d7bcec 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -200,7 +200,7 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
>         int err;
>         bool last_element = !post[0];
>
> -       this = lookup_positive_unlocked(name, base, namelen);
> +       this = lookup_one_len_unlocked(name, base, namelen);
>         if (IS_ERR(this)) {
>                 err = PTR_ERR(this);
>                 this = NULL;
> @@ -209,6 +209,15 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
>                 goto out_err;
>         }
>
> +       /* Borrow the check from lookup_positive_unlocked */
> +       if (d_flags_negative(smp_load_acquire(&this->d_flags))) {
> +               d_drop(this);
> +               dput(this);
> +               this = NULL;
> +               err = -ENOENT;
> +               goto out;
> +       }
> +

This is a nice improvement, but my feeling is that this low level code
belongs in a vfs helper with well documented semantics.

Thanks,
Amir.

Re: [RFC PATCH] ovl: suppress negative dentry in lookup

[Miklos Szeredi]

On Fri, May 8, 2020 at 8:39 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, May 7, 2020 at 4:21 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
> >
> > When a file is only in a lower layer, after lookup a negative
>
> Or in no layer at all...
>
> > dentry will be generated in the upper layer or even worse many
> > negetive dentries will be generated in upper/lower layers. These
> > negative dentries will be useless after construction of overlayfs'
> > own dentry and may keep in the memory long time even after unmount
> > of overlayfs instance. This patch tries to kill unnecessary negative
> > dentry during lookup.
> >
> > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> > ---
> >  fs/overlayfs/namei.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index 723d17744758..cf0ec4d7bcec 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -200,7 +200,7 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
> >         int err;
> >         bool last_element = !post[0];
> >
> > -       this = lookup_positive_unlocked(name, base, namelen);
> > +       this = lookup_one_len_unlocked(name, base, namelen);
> >         if (IS_ERR(this)) {
> >                 err = PTR_ERR(this);
> >                 this = NULL;
> > @@ -209,6 +209,15 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
> >                 goto out_err;
> >         }
> >
> > +       /* Borrow the check from lookup_positive_unlocked */
> > +       if (d_flags_negative(smp_load_acquire(&this->d_flags))) {
> > +               d_drop(this);
> > +               dput(this);
> > +               this = NULL;
> > +               err = -ENOENT;
> > +               goto out;
> > +       }
> > +
>
> This is a nice improvement, but my feeling is that this low level code
> belongs in a vfs helper with well documented semantics.

I agree.  Using d_drop() with the parent dir unlocked is not a good idea.

We need a new helper that does all of lookup_positive_unlocked() but
with a conditional d_drop() after calling __lookup_slow().  In fact
not dropping already cached negatives is probably a good idea, so
doing it only in the slowpath should be the right thing.

Thanks,
Miklos