| Message ID | 20200727112344.GH389488@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSv4.2: Fix an error code in nfs4_xattr_cache_init() | expand |
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
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?
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
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?
On Tue, Jul 28, 2020 at 05:10:34PM +0000, Trond Myklebust wrote: > 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? In the case of inode data, so you should probably invalidate it immediately, or accept that you're serving up known-stale data. So offloading it doesn't seem like a good idea, and you'll just have to accept the extra cycles you're using to do it. For this particular case, you're just reaping a cache that is no longer being used. There is no correctness gain in doing it in the I/O path - the cache has already been orphaned and new getxattr/listxattr calls will not see it. So there doesn't seem to be a reason to do it in the I/O path at all. The caches shouldn't become very large, no. In the normal case, there shouldn't be much of a performance difference. Then again, what do you gain by doing the reaping of the cache in the I/O path, instead of using a work queue? I concluded that there wasn't an upside, only a downside, so that's why I implemented it that way. If you think it's better to do it inline, I'm happy to change it, of course. It would just mean getting rid of the work queue and the reap_cache function, and calling discard_cache directly, instead of reap_cache. - Frank
On Tue, 2020-07-28 at 18:00 +0000, Frank van der Linden wrote: > On Tue, Jul 28, 2020 at 05:10:34PM +0000, Trond Myklebust wrote: > > 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? > > In the case of inode data, so you should probably invalidate it > immediately, or accept that you're serving up known-stale data. So > offloading it doesn't seem like a good idea, and you'll just have to > accept > the extra cycles you're using to do it. > > For this particular case, you're just reaping a cache that is no > longer > being used. There is no correctness gain in doing it in the I/O path > - > the cache has already been orphaned and new getxattr/listxattr calls > will not see it. So there doesn't seem to be a reason to do it in the > I/O path at all. > > The caches shouldn't become very large, no. In the normal case, there > shouldn't be much of a performance difference. > > Then again, what do you gain by doing the reaping of the cache in the > I/O path, > instead of using a work queue? I concluded that there wasn't an > upside, only > a downside, so that's why I implemented it that way. > > If you think it's better to do it inline, I'm happy to change it, of > course. > It would just mean getting rid of the work queue and the reap_cache > function, > and calling discard_cache directly, instead of reap_cache. > > - Frank I think we should start with doing the freeing of the old cache inline. If it turns out to be a real performance problem, then we can later revisit using a work queue, however in that case, I'd prefer to use nfsiod rather than adding a special workqueue that is reserved for xattrs.
On Tue, Jul 28, 2020 at 06:04:21PM +0000, Trond Myklebust wrote: > On Tue, 2020-07-28 at 18:00 +0000, Frank van der Linden wrote: > > On Tue, Jul 28, 2020 at 05:10:34PM +0000, Trond Myklebust wrote: > > > 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? > > > > In the case of inode data, so you should probably invalidate it > > immediately, or accept that you're serving up known-stale data. So > > offloading it doesn't seem like a good idea, and you'll just have to > > accept > > the extra cycles you're using to do it. > > > > For this particular case, you're just reaping a cache that is no > > longer > > being used. There is no correctness gain in doing it in the I/O path > > - > > the cache has already been orphaned and new getxattr/listxattr calls > > will not see it. So there doesn't seem to be a reason to do it in the > > I/O path at all. > > > > The caches shouldn't become very large, no. In the normal case, there > > shouldn't be much of a performance difference. > > > > Then again, what do you gain by doing the reaping of the cache in the > > I/O path, > > instead of using a work queue? I concluded that there wasn't an > > upside, only > > a downside, so that's why I implemented it that way. > > > > If you think it's better to do it inline, I'm happy to change it, of > > course. > > It would just mean getting rid of the work queue and the reap_cache > > function, > > and calling discard_cache directly, instead of reap_cache. > > > > - Frank > > I think we should start with doing the freeing of the old cache inline. > If it turns out to be a real performance problem, then we can later > revisit using a work queue, however in that case, I'd prefer to use > nfsiod rather than adding a special workqueue that is reserved for > xattrs. Sure, I can do that. Do you want me to send a new version of the patch series, or an incremental patch? - Frank
On Tue, 2020-07-28 at 18:13 +0000, Frank van der Linden wrote: > On Tue, Jul 28, 2020 at 06:04:21PM +0000, Trond Myklebust wrote: > > On Tue, 2020-07-28 at 18:00 +0000, Frank van der Linden wrote: > > > On Tue, Jul 28, 2020 at 05:10:34PM +0000, Trond Myklebust wrote: > > > > 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? > > > > > > In the case of inode data, so you should probably invalidate it > > > immediately, or accept that you're serving up known-stale data. > > > So > > > offloading it doesn't seem like a good idea, and you'll just have > > > to > > > accept > > > the extra cycles you're using to do it. > > > > > > For this particular case, you're just reaping a cache that is no > > > longer > > > being used. There is no correctness gain in doing it in the I/O > > > path > > > - > > > the cache has already been orphaned and new getxattr/listxattr > > > calls > > > will not see it. So there doesn't seem to be a reason to do it in > > > the > > > I/O path at all. > > > > > > The caches shouldn't become very large, no. In the normal case, > > > there > > > shouldn't be much of a performance difference. > > > > > > Then again, what do you gain by doing the reaping of the cache in > > > the > > > I/O path, > > > instead of using a work queue? I concluded that there wasn't an > > > upside, only > > > a downside, so that's why I implemented it that way. > > > > > > If you think it's better to do it inline, I'm happy to change it, > > > of > > > course. > > > It would just mean getting rid of the work queue and the > > > reap_cache > > > function, > > > and calling discard_cache directly, instead of reap_cache. > > > > > > - Frank > > > > I think we should start with doing the freeing of the old cache > > inline. > > If it turns out to be a real performance problem, then we can later > > revisit using a work queue, however in that case, I'd prefer to use > > nfsiod rather than adding a special workqueue that is reserved for > > xattrs. > > Sure, I can do that. > > Do you want me to send a new version of the patch series, or an > incremental patch? Please just send as an incremental patch. Thanks!
On Tue, Jul 28, 2020 at 06:21:19PM +0000, Trond Myklebust wrote: > > On Tue, 2020-07-28 at 18:13 +0000, Frank van der Linden wrote: > > On Tue, Jul 28, 2020 at 06:04:21PM +0000, Trond Myklebust wrote: > > > On Tue, 2020-07-28 at 18:00 +0000, Frank van der Linden wrote: > > > > On Tue, Jul 28, 2020 at 05:10:34PM +0000, Trond Myklebust wrote: > > > > > 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? > > > > > > > > In the case of inode data, so you should probably invalidate it > > > > immediately, or accept that you're serving up known-stale data. > > > > So > > > > offloading it doesn't seem like a good idea, and you'll just have > > > > to > > > > accept > > > > the extra cycles you're using to do it. > > > > > > > > For this particular case, you're just reaping a cache that is no > > > > longer > > > > being used. There is no correctness gain in doing it in the I/O > > > > path > > > > - > > > > the cache has already been orphaned and new getxattr/listxattr > > > > calls > > > > will not see it. So there doesn't seem to be a reason to do it in > > > > the > > > > I/O path at all. > > > > > > > > The caches shouldn't become very large, no. In the normal case, > > > > there > > > > shouldn't be much of a performance difference. > > > > > > > > Then again, what do you gain by doing the reaping of the cache in > > > > the > > > > I/O path, > > > > instead of using a work queue? I concluded that there wasn't an > > > > upside, only > > > > a downside, so that's why I implemented it that way. > > > > > > > > If you think it's better to do it inline, I'm happy to change it, > > > > of > > > > course. > > > > It would just mean getting rid of the work queue and the > > > > reap_cache > > > > function, > > > > and calling discard_cache directly, instead of reap_cache. > > > > > > > > - Frank > > > > > > I think we should start with doing the freeing of the old cache > > > inline. > > > If it turns out to be a real performance problem, then we can later > > > revisit using a work queue, however in that case, I'd prefer to use > > > nfsiod rather than adding a special workqueue that is reserved for > > > xattrs. > > > > Sure, I can do that. > > > > Do you want me to send a new version of the patch series, or an > > incremental patch? > > Please just send as an incremental patch. > > Thanks! Made the change, re-tested it. Patch sent - thanks! - Frank
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)
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(-)