Re: [PATCH] NFSv4.2: Fix an error code in nfs4_xattr_cache_init()

[Trond Myklebust <trondmy@hammerspace.com>]

On Tue, 2020-07-28 at 16:09 +0000, Frank van der Linden wrote:
> Hi Trond,
> 
> On Tue, Jul 28, 2020 at 03:17:12PM +0000, Trond Myklebust wrote:
> > On Mon, 2020-07-27 at 16:34 +0000, Frank van der Linden wrote:
> > > Hi Dan,
> > > 
> > > On Mon, Jul 27, 2020 at 02:23:44PM +0300, Dan Carpenter wrote:
> > > > This should return -ENOMEM on failure instead of success.
> > > > 
> > > > Fixes: 95ad37f90c33 ("NFSv4.2: add client side xattr caching.")
> > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > ---
> > > > ---
> > > >  fs/nfs/nfs42xattr.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/nfs/nfs42xattr.c b/fs/nfs/nfs42xattr.c
> > > > index 23fdab977a2a..e75c4bb70266 100644
> > > > --- a/fs/nfs/nfs42xattr.c
> > > > +++ b/fs/nfs/nfs42xattr.c
> > > > @@ -1040,8 +1040,10 @@ int __init nfs4_xattr_cache_init(void)
> > > >                 goto out2;
> > > > 
> > > >         nfs4_xattr_cache_wq = alloc_workqueue("nfs4_xattr",
> > > > WQ_MEM_RECLAIM, 0);
> > > > -       if (nfs4_xattr_cache_wq == NULL)
> > > > +       if (nfs4_xattr_cache_wq == NULL) {
> > > > +               ret = -ENOMEM;
> > > >                 goto out1;
> > > > +       }
> > > > 
> > > >         ret = register_shrinker(&nfs4_xattr_cache_shrinker);
> > > >         if (ret)
> > > > --
> > > > 2.27.0
> > > > 
> > > 
> > > Thanks for catching that one. Since this is against linux-next
> > > via
> > > Trond,
> > > I assume Trond will add it to his tree (right?)
> > > 
> > > In any case:
> > > 
> > > 
> > > Reviewed-by: Frank van der Linden <fllinden@amazon.com>
> > > 
> > > 
> > > - Frank
> > 
> > Frank, why do we need a workqueue here at all?
> 
> The xattr caches are per-inode, and get created on demand.
> Invalidating
> a cache is done by setting the invalidate flag (as it is for other
> cached attribues and data).
> 
> When nfs4_xattr_get_cache() sees an invalidated cache, it will just
> unlink it
> from the inode, and create a new one if needed.
> 
> The old cache then still needs to be freed. Theoretically, there can
> be
> quite a few entries in it, and nfs4_xattr_get_cache() will be called
> in
> the get/setxattr systemcall path. So my reasoning here was that it's
> better
> to use a workqueue to free the old invalidated cache instead of
> wasting
> cycles in the I/O path.
> 
> - Frank

I think we might want to explore the reasons for this argument. We do
not offload any other cache invalidations, and that includes the case
when we have to invalidate the entire inode data cache before reading.

So what is special about xattrs that causes invalidation to be a
problem in the I/O path? Why do we expect them to grow so large that
they are more unwieldy than the inode data cache?

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com