| Message ID | 20241025150154.879541-1-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ovl: replace dget/dput with d_drop in ovl_cleanup() | expand |
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 >
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?
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
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.
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",
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(-)