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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Filesystems Devel]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux