[PATCH v2] ovl: replace dget/dput with d_drop in ovl_cleanup()

[PATCH v2] ovl: replace dget/dput with d_drop in ovl_cleanup()

[Miklos Szeredi]

The reason for the dget/dput pair was to force the upperdentry to be
dropped from the cache instead of turning it negative and keeping it
cached.

Simpler and cleaner way to achieve the same effect is to just drop the
dentry after unlink/rmdir if it was turned negative.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
v2:
 - use d_drop()

 fs/overlayfs/dir.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index ab65e98a1def..c7548c2bbc12 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -28,12 +28,14 @@ int ovl_cleanup(struct ovl_fs *ofs, struct inode *wdir, struct dentry *wdentry)
 {
 	int err;
 
-	dget(wdentry);
 	if (d_is_dir(wdentry))
 		err = ovl_do_rmdir(ofs, wdir, wdentry);
 	else
 		err = ovl_do_unlink(ofs, wdir, wdentry);
-	dput(wdentry);
+
+	/* A cached negative upper dentry is generally not useful, so drop it. */
+	if (d_is_negative(wdentry))
+		d_drop(wdentry);
 
 	if (err) {
 		pr_err("cleanup of '%pd2' failed (%i)\n",
-- 
2.47.0

Re: [PATCH v2] ovl: replace dget/dput with d_drop in ovl_cleanup()

[Amir Goldstein]

On Fri, Oct 25, 2024 at 5:02 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> The reason for the dget/dput pair was to force the upperdentry to be
> dropped from the cache instead of turning it negative and keeping it
> cached.
>
> Simpler and cleaner way to achieve the same effect is to just drop the
> dentry after unlink/rmdir if it was turned negative.
>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>

Looks sane.
Applied to overlayfs-next for testing.

Thanks,
Amir.

> ---
> v2:
>  - use d_drop()
>
>  fs/overlayfs/dir.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index ab65e98a1def..c7548c2bbc12 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -28,12 +28,14 @@ int ovl_cleanup(struct ovl_fs *ofs, struct inode *wdir, struct dentry *wdentry)
>  {
>         int err;
>
> -       dget(wdentry);
>         if (d_is_dir(wdentry))
>                 err = ovl_do_rmdir(ofs, wdir, wdentry);
>         else
>                 err = ovl_do_unlink(ofs, wdir, wdentry);
> -       dput(wdentry);
> +
> +       /* A cached negative upper dentry is generally not useful, so drop it. */
> +       if (d_is_negative(wdentry))
> +               d_drop(wdentry);
>
>         if (err) {
>                 pr_err("cleanup of '%pd2' failed (%i)\n",
> --
> 2.47.0
>

Re: [PATCH v2] ovl: replace dget/dput with d_drop in ovl_cleanup()

[Al Viro]

On Sat, Oct 26, 2024 at 08:30:54AM +0200, Amir Goldstein wrote:
> On Fri, Oct 25, 2024 at 5:02 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
> >
> > The reason for the dget/dput pair was to force the upperdentry to be
> > dropped from the cache instead of turning it negative and keeping it
> > cached.
> >
> > Simpler and cleaner way to achieve the same effect is to just drop the
> > dentry after unlink/rmdir if it was turned negative.
> >
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> 
> Looks sane.
> Applied to overlayfs-next for testing.

I thought it was about preventing an overlayfs objects with negative ->__upperdentry;
why would a negative dentry in upper layer be a problem otherwise?

Re: [PATCH v2] ovl: replace dget/dput with d_drop in ovl_cleanup()

[Miklos Szeredi]

On Sat, 26 Oct 2024 at 08:56, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Sat, Oct 26, 2024 at 08:30:54AM +0200, Amir Goldstein wrote:
> > On Fri, Oct 25, 2024 at 5:02 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
> > >
> > > The reason for the dget/dput pair was to force the upperdentry to be
> > > dropped from the cache instead of turning it negative and keeping it
> > > cached.
> > >
> > > Simpler and cleaner way to achieve the same effect is to just drop the
> > > dentry after unlink/rmdir if it was turned negative.
> > >
> > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> >
> > Looks sane.
> > Applied to overlayfs-next for testing.
>
> I thought it was about preventing an overlayfs objects with negative ->__upperdentry;

Yeah, I overlooked that aspect.   Amir, please drop this patch.

> why would a negative dentry in upper layer be a problem otherwise?

Double caching, see this commit:

commit 1434a65ea625c51317ccdf06dabf4bd27d20fa10
Author: Chengguang Xu <cgxu519@mykernel.net>
Date:   Tue May 26 09:35:57 2020 +0800

    ovl: drop negative dentry in upper layer

    Negative dentries of upper layer are 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 drop unnecessary
    negative dentry of upper layer to effectively reclaim memory.

The reason lower dentries are different is that lower layers could be
(and often are) shared, while the upper layer is always private.

Thanks,
Miklos

Re: [PATCH v2] ovl: replace dget/dput with d_drop in ovl_cleanup()

[Amir Goldstein]

On Tue, Nov 5, 2024 at 12:34 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sat, 26 Oct 2024 at 08:56, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Sat, Oct 26, 2024 at 08:30:54AM +0200, Amir Goldstein wrote:
> > > On Fri, Oct 25, 2024 at 5:02 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
> > > >
> > > > The reason for the dget/dput pair was to force the upperdentry to be
> > > > dropped from the cache instead of turning it negative and keeping it
> > > > cached.
> > > >
> > > > Simpler and cleaner way to achieve the same effect is to just drop the
> > > > dentry after unlink/rmdir if it was turned negative.
> > > >
> > > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > >
> > > Looks sane.
> > > Applied to overlayfs-next for testing.
> >
> > I thought it was about preventing an overlayfs objects with negative ->__upperdentry;
>
> Yeah, I overlooked that aspect.   Amir, please drop this patch.
>

Dropped.

Thanks,
Amir.