[PATCH 1/4] ovl: use refcount_t in readdir

[PATCH 1/4] ovl: use refcount_t in readdir

[Miklos Szeredi]

At this point this is just a cleanup, since the refcount is also protected
by overlayfs inode lock.

This will change in a following patch.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/readdir.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 0ca8af060b0c..b894a97f8ef8 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -13,6 +13,7 @@
 #include <linux/security.h>
 #include <linux/cred.h>
 #include <linux/ratelimit.h>
+#include <linux/refcount.h>
 #include "overlayfs.h"
 
 struct ovl_cache_entry {
@@ -30,7 +31,7 @@ struct ovl_cache_entry {
 };
 
 struct ovl_dir_cache {
-	long refcount;
+	refcount_t refcount;
 	u64 version;
 	struct list_head entries;
 	struct rb_root root;
@@ -243,9 +244,7 @@ static void ovl_cache_put(struct ovl_dir_file *od, struct inode *inode)
 {
 	struct ovl_dir_cache *cache = od->cache;
 
-	WARN_ON(cache->refcount <= 0);
-	cache->refcount--;
-	if (!cache->refcount) {
+	if (refcount_dec_and_test(&cache->refcount)) {
 		if (ovl_dir_cache(inode) == cache)
 			ovl_set_dir_cache(inode, NULL);
 
@@ -405,8 +404,7 @@ static struct ovl_dir_cache *ovl_cache_get(struct dentry *dentry)
 
 	cache = ovl_dir_cache(inode);
 	if (cache && ovl_inode_version_get(inode) == cache->version) {
-		WARN_ON(!cache->refcount);
-		cache->refcount++;
+		refcount_inc(&cache->refcount);
 		return cache;
 	}
 	ovl_set_dir_cache(d_inode(dentry), NULL);
@@ -415,7 +413,7 @@ static struct ovl_dir_cache *ovl_cache_get(struct dentry *dentry)
 	if (!cache)
 		return ERR_PTR(-ENOMEM);
 
-	cache->refcount = 1;
+	refcount_set(&cache->refcount, 1);
 	INIT_LIST_HEAD(&cache->entries);
 	cache->root = RB_ROOT;
 
-- 
2.44.0

[PATCH 2/4] ovl: get rid of iterate wrapper

[Miklos Szeredi]

Commit 3e3271549670 ("vfs: get rid of old '->iterate' directory operation")
added a wrapper around ovl_iterate() to lock the inode exclusive.

Use the overlayfs private inode lock instead to provide exclusive locking.

Add ovl_inode_lock()/_unlock() to ovl_iterate() and replace
inode_lock/_unlock() with the ovl_ variant in ovl_dir_llseek() and
ovl_dir_release().

This replacement is valid, because the inode lock was taken only to provide
exclusion between these functions (for other files referring to the same
inode).

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/readdir.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index b894a97f8ef8..edee9f86f469 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -758,6 +758,7 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
 	if (!ctx->pos)
 		ovl_dir_reset(file);
 
+	ovl_inode_lock(file_inode(file));
 	if (od->is_real) {
 		/*
 		 * If parent is merge, then need to adjust d_ino for '..', if
@@ -806,6 +807,7 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
 	}
 	err = 0;
 out:
+	ovl_inode_unlock(file_inode(file));
 	revert_creds(old_cred);
 	return err;
 }
@@ -815,7 +817,7 @@ static loff_t ovl_dir_llseek(struct file *file, loff_t offset, int origin)
 	loff_t res;
 	struct ovl_dir_file *od = file->private_data;
 
-	inode_lock(file_inode(file));
+	ovl_inode_lock(file_inode(file));
 	if (!file->f_pos)
 		ovl_dir_reset(file);
 
@@ -845,7 +847,7 @@ static loff_t ovl_dir_llseek(struct file *file, loff_t offset, int origin)
 		res = offset;
 	}
 out_unlock:
-	inode_unlock(file_inode(file));
+	ovl_inode_unlock(file_inode(file));
 
 	return res;
 }
@@ -929,9 +931,9 @@ static int ovl_dir_release(struct inode *inode, struct file *file)
 	struct ovl_dir_file *od = file->private_data;
 
 	if (od->cache) {
-		inode_lock(inode);
+		ovl_inode_lock(inode);
 		ovl_cache_put(od, inode);
-		inode_unlock(inode);
+		ovl_inode_unlock(inode);
 	}
 	fput(od->realfile);
 	if (od->upperfile)
@@ -966,11 +968,10 @@ static int ovl_dir_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
-WRAP_DIR_ITER(ovl_iterate) // FIXME!
 const struct file_operations ovl_dir_operations = {
 	.read		= generic_read_dir,
 	.open		= ovl_dir_open,
-	.iterate_shared	= shared_ovl_iterate,
+	.iterate_shared	= ovl_iterate,
 	.llseek		= ovl_dir_llseek,
 	.fsync		= ovl_dir_fsync,
 	.release	= ovl_dir_release,
-- 
2.44.0

[PATCH 3/4] ovl: only lock readdir for accessing the cache

[Miklos Szeredi]

The only reason parallel readdirs cannot run on the same inode is shared
access to the readdir cache.

Move lock/unlock to only protect the cache.  Exception is the refcount
which now uses atomic ops.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/readdir.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index edee9f86f469..b98e0d17f40e 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -245,8 +245,10 @@ static void ovl_cache_put(struct ovl_dir_file *od, struct inode *inode)
 	struct ovl_dir_cache *cache = od->cache;
 
 	if (refcount_dec_and_test(&cache->refcount)) {
+		ovl_inode_lock(inode);
 		if (ovl_dir_cache(inode) == cache)
 			ovl_set_dir_cache(inode, NULL);
+		ovl_inode_unlock(inode);
 
 		ovl_cache_free(&cache->entries);
 		kfree(cache);
@@ -733,12 +735,18 @@ static int ovl_iterate_real(struct file *file, struct dir_context *ctx)
 	}
 
 	if (ovl_is_impure_dir(file)) {
+		ovl_inode_lock(file_inode(file));
 		rdt.cache = ovl_cache_get_impure(&file->f_path);
-		if (IS_ERR(rdt.cache))
+		if (IS_ERR(rdt.cache)) {
+			ovl_inode_unlock(file_inode(file));
 			return PTR_ERR(rdt.cache);
+		}
 	}
 
 	err = iterate_dir(od->realfile, &rdt.ctx);
+
+	if (rdt.cache)
+		ovl_inode_unlock(file_inode(file));
 	ctx->pos = rdt.ctx.pos;
 
 	return err;
@@ -758,7 +766,6 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
 	if (!ctx->pos)
 		ovl_dir_reset(file);
 
-	ovl_inode_lock(file_inode(file));
 	if (od->is_real) {
 		/*
 		 * If parent is merge, then need to adjust d_ino for '..', if
@@ -773,9 +780,10 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
 		} else {
 			err = iterate_dir(od->realfile, ctx);
 		}
-		goto out;
+		goto out_revert;
 	}
 
+	ovl_inode_lock(file_inode(file));
 	if (!od->cache) {
 		struct ovl_dir_cache *cache;
 
@@ -808,6 +816,7 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
 	err = 0;
 out:
 	ovl_inode_unlock(file_inode(file));
+out_revert:
 	revert_creds(old_cred);
 	return err;
 }
@@ -817,7 +826,6 @@ static loff_t ovl_dir_llseek(struct file *file, loff_t offset, int origin)
 	loff_t res;
 	struct ovl_dir_file *od = file->private_data;
 
-	ovl_inode_lock(file_inode(file));
 	if (!file->f_pos)
 		ovl_dir_reset(file);
 
@@ -834,21 +842,22 @@ static loff_t ovl_dir_llseek(struct file *file, loff_t offset, int origin)
 		case SEEK_SET:
 			break;
 		default:
-			goto out_unlock;
+			goto out;
 		}
 		if (offset < 0)
-			goto out_unlock;
+			goto out;
 
 		if (offset != file->f_pos) {
 			file->f_pos = offset;
-			if (od->cache)
+			if (od->cache) {
+				ovl_inode_lock(file_inode(file));
 				ovl_seek_cursor(od, offset);
+				ovl_inode_unlock(file_inode(file));
+			}
 		}
 		res = offset;
 	}
-out_unlock:
-	ovl_inode_unlock(file_inode(file));
-
+out:
 	return res;
 }
 
@@ -930,11 +939,8 @@ static int ovl_dir_release(struct inode *inode, struct file *file)
 {
 	struct ovl_dir_file *od = file->private_data;
 
-	if (od->cache) {
-		ovl_inode_lock(inode);
+	if (od->cache)
 		ovl_cache_put(od, inode);
-		ovl_inode_unlock(inode);
-	}
 	fput(od->realfile);
 	if (od->upperfile)
 		fput(od->upperfile);
-- 
2.44.0

Re: [PATCH 3/4] ovl: only lock readdir for accessing the cache

[Amir Goldstein]

On Thu, Mar 7, 2024 at 1:02 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> The only reason parallel readdirs cannot run on the same inode is shared
> access to the readdir cache.

I did not see a cover letter, so I am assuming that the reason for this change
is to improve concurrent readdir.

If I am reading this correctly users can only iterate pure real dirs in parallel
but not merged and impure dirs. Right?

Is there a reason why a specific cached readdir version cannot be iterated
in parallel?

>
> Move lock/unlock to only protect the cache.  Exception is the refcount
> which now uses atomic ops.
>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/overlayfs/readdir.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index edee9f86f469..b98e0d17f40e 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -245,8 +245,10 @@ static void ovl_cache_put(struct ovl_dir_file *od, struct inode *inode)
>         struct ovl_dir_cache *cache = od->cache;
>
>         if (refcount_dec_and_test(&cache->refcount)) {

What is stopping ovl_cache_get() to be called now, find a valid cache
and increment its refcount and use it while it is being freed?

Do we need refcount_inc_not_zero() in ovl_cache_get()?

> +               ovl_inode_lock(inode);
>                 if (ovl_dir_cache(inode) == cache)
>                         ovl_set_dir_cache(inode, NULL);
> +               ovl_inode_unlock(inode);
>
>                 ovl_cache_free(&cache->entries);
>                 kfree(cache);

P.S. A guard for ovl_inode_lock() would have been useful in this patch set,
but it's up to you if you want to define one and use it.

Thanks,
Amir.

Re: [PATCH 3/4] ovl: only lock readdir for accessing the cache

[Miklos Szeredi]

On Thu, 7 Mar 2024 at 14:11, Amir Goldstein <amir73il@gmail.com> wrote:

> I did not see a cover letter, so I am assuming that the reason for this change
> is to improve concurrent readdir.

That's a nice to have, but the real reason was just to get rid of the FIXME.

> If I am reading this correctly users can only iterate pure real dirs in parallel
> but not merged and impure dirs. Right?

Right.

> Is there a reason why a specific cached readdir version cannot be iterated
> in parallel?

It could, but it would take more thought (ovl _cache_update() may
modify a cache entry).

>
> >
> > Move lock/unlock to only protect the cache.  Exception is the refcount
> > which now uses atomic ops.
> >
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
> >  fs/overlayfs/readdir.c | 34 ++++++++++++++++++++--------------
> >  1 file changed, 20 insertions(+), 14 deletions(-)
> >
> > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> > index edee9f86f469..b98e0d17f40e 100644
> > --- a/fs/overlayfs/readdir.c
> > +++ b/fs/overlayfs/readdir.c
> > @@ -245,8 +245,10 @@ static void ovl_cache_put(struct ovl_dir_file *od, struct inode *inode)
> >         struct ovl_dir_cache *cache = od->cache;
> >
> >         if (refcount_dec_and_test(&cache->refcount)) {
>
> What is stopping ovl_cache_get() to be called now, find a valid cache
> and increment its refcount and use it while it is being freed?
>
> Do we need refcount_inc_not_zero() in ovl_cache_get()?

Yes.  But it would still be racy (winning ovl_cache_get() would set
oi->cache, then losing ovl_cache_put() would reset it).  It would be a
harmless race, but I find it ugly, so I'll just move the locking
outside of the refcount_dec_and_test().  It's not a performance
sensitive path.


>
> > +               ovl_inode_lock(inode);
> >                 if (ovl_dir_cache(inode) == cache)
> >                         ovl_set_dir_cache(inode, NULL);
> > +               ovl_inode_unlock(inode);
> >
> >                 ovl_cache_free(&cache->entries);
> >                 kfree(cache);
>
> P.S. A guard for ovl_inode_lock() would have been useful in this patch set,
> but it's up to you if you want to define one and use it.

Will look into it.

Thanks for the review.

Miklos

Re: [PATCH 3/4] ovl: only lock readdir for accessing the cache

[Miklos Szeredi]

On Thu, 7 Mar 2024 at 15:09, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, 7 Mar 2024 at 14:11, Amir Goldstein <amir73il@gmail.com> wrote:

> > P.S. A guard for ovl_inode_lock() would have been useful in this patch set,
> > but it's up to you if you want to define one and use it.

I like the concept of guards, though documentation and examples are
lacking and the API is not trivial to understand at first sight.

For overlayfs I'd start with ovl_override_creds(), since that is used
much more extensively than ovl_inode_lock().

Thanks,
Miklos

Re: [PATCH 3/4] ovl: only lock readdir for accessing the cache

[Amir Goldstein]

On Thu, Mar 7, 2024 at 6:13 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, 7 Mar 2024 at 15:09, Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Thu, 7 Mar 2024 at 14:11, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > > P.S. A guard for ovl_inode_lock() would have been useful in this patch set,
> > > but it's up to you if you want to define one and use it.
>
> I like the concept of guards, though documentation and examples are
> lacking and the API is not trivial to understand at first sight.
>
> For overlayfs I'd start with ovl_override_creds(), since that is used
> much more extensively than ovl_inode_lock().
>

OK. let's wait for this to land first:
https://lore.kernel.org/linux-unionfs/20240216051640.197378-1-vinicius.gomes@intel.com/

As I wrote in the review of v2,
I'd rather that Christian will review and pick up the non-overlayfs bits,
which head suggested and only after that will I review the overlayfs
patch.

Thanks,
Amir.

Re: [PATCH 3/4] ovl: only lock readdir for accessing the cache

[Christian Brauner]

On Thu, Mar 07, 2024 at 07:31:35PM +0200, Amir Goldstein wrote:
> On Thu, Mar 7, 2024 at 6:13 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Thu, 7 Mar 2024 at 15:09, Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Thu, 7 Mar 2024 at 14:11, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > > > P.S. A guard for ovl_inode_lock() would have been useful in this patch set,
> > > > but it's up to you if you want to define one and use it.
> >
> > I like the concept of guards, though documentation and examples are
> > lacking and the API is not trivial to understand at first sight.
> >
> > For overlayfs I'd start with ovl_override_creds(), since that is used
> > much more extensively than ovl_inode_lock().
> >
> 
> OK. let's wait for this to land first:
> https://lore.kernel.org/linux-unionfs/20240216051640.197378-1-vinicius.gomes@intel.com/
> 
> As I wrote in the review of v2,
> I'd rather that Christian will review and pick up the non-overlayfs bits,
> which head suggested and only after that will I review the overlayfs
> patch.

On it. Had been on my queue but didn't get around to it. I wanted to
play with this a bit.

[PATCH 4/4] ovl: clean up struct ovl_dir_cache use outside readdir.c

[Miklos Szeredi]

Remove unnecessary forward declaration in super.c and move helper functions
that are only used inside readdir.c

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/overlayfs.h |  2 --
 fs/overlayfs/readdir.c   | 10 ++++++++++
 fs/overlayfs/super.c     |  2 --
 fs/overlayfs/util.c      | 10 ----------
 4 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index ee949f3e7c77..167dc37f804c 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -470,8 +470,6 @@ struct inode *ovl_inode_lowerdata(struct inode *inode);
 struct inode *ovl_inode_real(struct inode *inode);
 struct inode *ovl_inode_realdata(struct inode *inode);
 const char *ovl_lowerdata_redirect(struct inode *inode);
-struct ovl_dir_cache *ovl_dir_cache(struct inode *inode);
-void ovl_set_dir_cache(struct inode *inode, struct ovl_dir_cache *cache);
 void ovl_dentry_set_flag(unsigned long flag, struct dentry *dentry);
 void ovl_dentry_clear_flag(unsigned long flag, struct dentry *dentry);
 bool ovl_dentry_test_flag(unsigned long flag, struct dentry *dentry);
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index b98e0d17f40e..4a20a44b34f2 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -61,6 +61,16 @@ struct ovl_dir_file {
 	struct file *upperfile;
 };
 
+static struct ovl_dir_cache *ovl_dir_cache(struct inode *inode)
+{
+	return inode && S_ISDIR(inode->i_mode) ? OVL_I(inode)->cache : NULL;
+}
+
+static void ovl_set_dir_cache(struct inode *inode, struct ovl_dir_cache *cache)
+{
+	OVL_I(inode)->cache = cache;
+}
+
 static struct ovl_cache_entry *ovl_cache_entry_from_node(struct rb_node *n)
 {
 	return rb_entry(n, struct ovl_cache_entry, node);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 2eef6c70b2ae..2413d3107335 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -26,8 +26,6 @@ MODULE_DESCRIPTION("Overlay filesystem");
 MODULE_LICENSE("GPL");
 
 
-struct ovl_dir_cache;
-
 static struct dentry *ovl_d_real(struct dentry *dentry,
 				 const struct inode *inode)
 {
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index a8e17f14d7a2..cfe625717c47 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -421,16 +421,6 @@ const char *ovl_lowerdata_redirect(struct inode *inode)
 		OVL_I(inode)->lowerdata_redirect : NULL;
 }
 
-struct ovl_dir_cache *ovl_dir_cache(struct inode *inode)
-{
-	return inode && S_ISDIR(inode->i_mode) ? OVL_I(inode)->cache : NULL;
-}
-
-void ovl_set_dir_cache(struct inode *inode, struct ovl_dir_cache *cache)
-{
-	OVL_I(inode)->cache = cache;
-}
-
 void ovl_dentry_set_flag(unsigned long flag, struct dentry *dentry)
 {
 	set_bit(flag, OVL_E_FLAGS(dentry));
-- 
2.44.0