On Thu, 7 Mar 2024 at 14:11, Amir Goldstein <amir73il@xxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
