On Fri, May 8, 2020 at 8:39 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Thu, May 7, 2020 at 4:21 AM Chengguang Xu <cgxu519@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> > 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
