| Message ID | 20230118173139.71846-4-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: random cleanup and doc work | expand |
> On Jan 18, 2023, at 12:31 PM, Jeff Layton <jlayton@kernel.org> wrote: > > When queueing a dispose list to the appropriate "freeme" lists, it > pointlessly queues the objects one at a time to an intermediate list. > > Remove a few helpers and just open code a list_move to make it more > clear and efficient. Better document the resulting functions with > kerneldoc comments. I'd like to freeze the filecache code until we've sorted out the destroy_deleg_unhashed crashes. Shall I simply maintain 3/6 and 4/6 and any subsequent filecache changes (like my rhltable rewrite) on a topic branch? One good reason to do that is to enable an eventual fix to be backported to stable kernels without also needing to pull in intervening clean-up patches. I've already applied a couple small changes that I would rather wait on for this reason. I might move those over to the topic branch as well... I promise to keep it based on nfsd-next so it makes sense to continue developing filecache work on top of the topic branch. The other patches in this series are sensible clean-ups that I plan to apply for v6.3 if there are no other objections. > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/filecache.c | 63 +++++++++++++++------------------------------ > 1 file changed, 21 insertions(+), 42 deletions(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index 58ac93e7e680..a2bc4bd90b9a 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -513,49 +513,25 @@ nfsd_file_dispose_list(struct list_head *dispose) > } > } > > -static void > -nfsd_file_list_remove_disposal(struct list_head *dst, > - struct nfsd_fcache_disposal *l) > -{ > - spin_lock(&l->lock); > - list_splice_init(&l->freeme, dst); > - spin_unlock(&l->lock); > -} > - > -static void > -nfsd_file_list_add_disposal(struct list_head *files, struct net *net) > -{ > - struct nfsd_net *nn = net_generic(net, nfsd_net_id); > - struct nfsd_fcache_disposal *l = nn->fcache_disposal; > - > - spin_lock(&l->lock); > - list_splice_tail_init(files, &l->freeme); > - spin_unlock(&l->lock); > - queue_work(nfsd_filecache_wq, &l->work); > -} > - > -static void > -nfsd_file_list_add_pernet(struct list_head *dst, struct list_head *src, > - struct net *net) > -{ > - struct nfsd_file *nf, *tmp; > - > - list_for_each_entry_safe(nf, tmp, src, nf_lru) { > - if (nf->nf_net == net) > - list_move_tail(&nf->nf_lru, dst); > - } > -} > - > +/** > + * nfsd_file_dispose_list_delayed - move list of dead files to net's freeme list > + * @dispose: list of nfsd_files to be disposed > + * > + * Transfers each file to the "freeme" list for its nfsd_net, to eventually > + * be disposed of by the per-net garbage collector. > + */ > static void > nfsd_file_dispose_list_delayed(struct list_head *dispose) > { > - LIST_HEAD(list); > - struct nfsd_file *nf; > - > while(!list_empty(dispose)) { > - nf = list_first_entry(dispose, struct nfsd_file, nf_lru); > - nfsd_file_list_add_pernet(&list, dispose, nf->nf_net); > - nfsd_file_list_add_disposal(&list, nf->nf_net); > + struct nfsd_file *nf = list_first_entry(dispose, > + struct nfsd_file, nf_lru); > + struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id); > + struct nfsd_fcache_disposal *l = nn->fcache_disposal; > + > + spin_lock(&l->lock); > + list_move_tail(&nf->nf_lru, &l->freeme); > + spin_unlock(&l->lock); > } > } > > @@ -765,8 +741,8 @@ nfsd_file_close_inode_sync(struct inode *inode) > * nfsd_file_delayed_close - close unused nfsd_files > * @work: dummy > * > - * Walk the LRU list and destroy any entries that have not been used since > - * the last scan. > + * Scrape the freeme list for this nfsd_net, and then dispose of them > + * all. > */ > static void > nfsd_file_delayed_close(struct work_struct *work) > @@ -775,7 +751,10 @@ nfsd_file_delayed_close(struct work_struct *work) > struct nfsd_fcache_disposal *l = container_of(work, > struct nfsd_fcache_disposal, work); > > - nfsd_file_list_remove_disposal(&head, l); > + spin_lock(&l->lock); > + list_splice_init(&l->freeme, &head); > + spin_unlock(&l->lock); > + > nfsd_file_dispose_list(&head); > } > > -- > 2.39.0 > -- Chuck Lever
On Wed, 2023-01-18 at 19:08 +0000, Chuck Lever III wrote: > > > On Jan 18, 2023, at 12:31 PM, Jeff Layton <jlayton@kernel.org> wrote: > > > > When queueing a dispose list to the appropriate "freeme" lists, it > > pointlessly queues the objects one at a time to an intermediate list. > > > > Remove a few helpers and just open code a list_move to make it more > > clear and efficient. Better document the resulting functions with > > kerneldoc comments. > > I'd like to freeze the filecache code until we've sorted out the > destroy_deleg_unhashed crashes. Shall I simply maintain 3/6 and > 4/6 and any subsequent filecache changes (like my rhltable > rewrite) on a topic branch? > > One good reason to do that is to enable an eventual fix to be > backported to stable kernels without also needing to pull in > intervening clean-up patches. > > I've already applied a couple small changes that I would rather > wait on for this reason. I might move those over to the topic > branch as well... I promise to keep it based on nfsd-next so it > makes sense to continue developing filecache work on top of the > topic branch. > > The other patches in this series are sensible clean-ups that I > plan to apply for v6.3 if there are no other objections. > So that means you won't take patches 3 and 4, but the rest are ok?
> On Jan 18, 2023, at 2:42 PM, Jeff Layton <jlayton@kernel.org> wrote: > > On Wed, 2023-01-18 at 19:08 +0000, Chuck Lever III wrote: >> >>> On Jan 18, 2023, at 12:31 PM, Jeff Layton <jlayton@kernel.org> wrote: >>> >>> When queueing a dispose list to the appropriate "freeme" lists, it >>> pointlessly queues the objects one at a time to an intermediate list. >>> >>> Remove a few helpers and just open code a list_move to make it more >>> clear and efficient. Better document the resulting functions with >>> kerneldoc comments. >> >> I'd like to freeze the filecache code until we've sorted out the >> destroy_deleg_unhashed crashes. Shall I simply maintain 3/6 and >> 4/6 and any subsequent filecache changes (like my rhltable >> rewrite) on a topic branch? >> >> One good reason to do that is to enable an eventual fix to be >> backported to stable kernels without also needing to pull in >> intervening clean-up patches. >> >> I've already applied a couple small changes that I would rather >> wait on for this reason. I might move those over to the topic >> branch as well... I promise to keep it based on nfsd-next so it >> makes sense to continue developing filecache work on top of the >> topic branch. >> >> The other patches in this series are sensible clean-ups that I >> plan to apply for v6.3 if there are no other objections. >> > > So that means you won't take patches 3 and 4, but the rest are ok? They all look fine to me. I've applied 1, 2, 5, and 6 to nfsd-next, and 3 and 4 along with several others have been applied to a branch called topic-filecache-cleanups, which is published now so you can continue developing against that and so it will get pulled into the 0-day test harness. I will merge the stuff in that topic branch at some point, I'm just not committing yet to applying it specifically to v6.3. Yes, you can call me "too conservative." :-) But I am sensitive to addressing the destroy_unhashed_deleg crashers in v6.1, which is to become an LTS if I understand correctly. That's the main reason for adding this extra step for filecache-related clean-ups. Narrow bug fixes still go right into nfsd-next or nfsd-fixes, as usual. If this arrangement becomes onerous, I will mix those commits back into the general nfsd-next population and we will never speak of it again. -- Chuck Lever
> On Jan 18, 2023, at 12:31 PM, Jeff Layton <jlayton@kernel.org> wrote: > > When queueing a dispose list to the appropriate "freeme" lists, it > pointlessly queues the objects one at a time to an intermediate list. > > Remove a few helpers and just open code a list_move to make it more > clear and efficient. Better document the resulting functions with > kerneldoc comments. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/filecache.c | 63 +++++++++++++++------------------------------ > 1 file changed, 21 insertions(+), 42 deletions(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index 58ac93e7e680..a2bc4bd90b9a 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -513,49 +513,25 @@ nfsd_file_dispose_list(struct list_head *dispose) > } > } > > -static void > -nfsd_file_list_remove_disposal(struct list_head *dst, > - struct nfsd_fcache_disposal *l) > -{ > - spin_lock(&l->lock); > - list_splice_init(&l->freeme, dst); > - spin_unlock(&l->lock); > -} > - > -static void > -nfsd_file_list_add_disposal(struct list_head *files, struct net *net) > -{ > - struct nfsd_net *nn = net_generic(net, nfsd_net_id); > - struct nfsd_fcache_disposal *l = nn->fcache_disposal; > - > - spin_lock(&l->lock); > - list_splice_tail_init(files, &l->freeme); > - spin_unlock(&l->lock); > - queue_work(nfsd_filecache_wq, &l->work); > -} > - > -static void > -nfsd_file_list_add_pernet(struct list_head *dst, struct list_head *src, > - struct net *net) > -{ > - struct nfsd_file *nf, *tmp; > - > - list_for_each_entry_safe(nf, tmp, src, nf_lru) { > - if (nf->nf_net == net) > - list_move_tail(&nf->nf_lru, dst); > - } > -} > - > +/** > + * nfsd_file_dispose_list_delayed - move list of dead files to net's freeme list > + * @dispose: list of nfsd_files to be disposed > + * > + * Transfers each file to the "freeme" list for its nfsd_net, to eventually > + * be disposed of by the per-net garbage collector. > + */ > static void > nfsd_file_dispose_list_delayed(struct list_head *dispose) > { > - LIST_HEAD(list); > - struct nfsd_file *nf; > - > while(!list_empty(dispose)) { > - nf = list_first_entry(dispose, struct nfsd_file, nf_lru); > - nfsd_file_list_add_pernet(&list, dispose, nf->nf_net); > - nfsd_file_list_add_disposal(&list, nf->nf_net); > + struct nfsd_file *nf = list_first_entry(dispose, > + struct nfsd_file, nf_lru); > + struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id); > + struct nfsd_fcache_disposal *l = nn->fcache_disposal; > + > + spin_lock(&l->lock); > + list_move_tail(&nf->nf_lru, &l->freeme); > + spin_unlock(&l->lock); > } > } > > @@ -765,8 +741,8 @@ nfsd_file_close_inode_sync(struct inode *inode) > * nfsd_file_delayed_close - close unused nfsd_files > * @work: dummy > * > - * Walk the LRU list and destroy any entries that have not been used since > - * the last scan. > + * Scrape the freeme list for this nfsd_net, and then dispose of them > + * all. > */ > static void > nfsd_file_delayed_close(struct work_struct *work) > @@ -775,7 +751,10 @@ nfsd_file_delayed_close(struct work_struct *work) > struct nfsd_fcache_disposal *l = container_of(work, > struct nfsd_fcache_disposal, work); > > - nfsd_file_list_remove_disposal(&head, l); > + spin_lock(&l->lock); > + list_splice_init(&l->freeme, &head); > + spin_unlock(&l->lock); > + > nfsd_file_dispose_list(&head); > } Hey Jeff - After applying this one, tmpfs exports appear to leak space, even after all files and directories are deleted. Eventually the filesystem is "full" -- modifying operations return ENOSPC but removing files doesn't recover the used space. Can you have a look at this? -- Chuck Lever
On Fri, 2023-04-14 at 18:20 +0000, Chuck Lever III wrote: > > On Jan 18, 2023, at 12:31 PM, Jeff Layton <jlayton@kernel.org> > > wrote: > > > > When queueing a dispose list to the appropriate "freeme" lists, it > > pointlessly queues the objects one at a time to an intermediate > > list. > > > > Remove a few helpers and just open code a list_move to make it more > > clear and efficient. Better document the resulting functions with > > kerneldoc comments. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfsd/filecache.c | 63 +++++++++++++++---------------------------- > > -- > > 1 file changed, 21 insertions(+), 42 deletions(-) > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > index 58ac93e7e680..a2bc4bd90b9a 100644 > > --- a/fs/nfsd/filecache.c > > +++ b/fs/nfsd/filecache.c > > @@ -513,49 +513,25 @@ nfsd_file_dispose_list(struct list_head > > *dispose) > > } > > } > > > > -static void > > -nfsd_file_list_remove_disposal(struct list_head *dst, > > - struct nfsd_fcache_disposal *l) > > -{ > > - spin_lock(&l->lock); > > - list_splice_init(&l->freeme, dst); > > - spin_unlock(&l->lock); > > -} > > - > > -static void > > -nfsd_file_list_add_disposal(struct list_head *files, struct net > > *net) > > -{ > > - struct nfsd_net *nn = net_generic(net, nfsd_net_id); > > - struct nfsd_fcache_disposal *l = nn->fcache_disposal; > > - > > - spin_lock(&l->lock); > > - list_splice_tail_init(files, &l->freeme); > > - spin_unlock(&l->lock); > > - queue_work(nfsd_filecache_wq, &l->work); > > -} > > - > > -static void > > -nfsd_file_list_add_pernet(struct list_head *dst, struct list_head > > *src, > > - struct net *net) > > -{ > > - struct nfsd_file *nf, *tmp; > > - > > - list_for_each_entry_safe(nf, tmp, src, nf_lru) { > > - if (nf->nf_net == net) > > - list_move_tail(&nf->nf_lru, dst); > > - } > > -} > > - > > +/** > > + * nfsd_file_dispose_list_delayed - move list of dead files to > > net's freeme list > > + * @dispose: list of nfsd_files to be disposed > > + * > > + * Transfers each file to the "freeme" list for its nfsd_net, to > > eventually > > + * be disposed of by the per-net garbage collector. > > + */ > > static void > > nfsd_file_dispose_list_delayed(struct list_head *dispose) > > { > > - LIST_HEAD(list); > > - struct nfsd_file *nf; > > - > > while(!list_empty(dispose)) { > > - nf = list_first_entry(dispose, struct nfsd_file, nf_lru); > > - nfsd_file_list_add_pernet(&list, dispose, nf->nf_net); > > - nfsd_file_list_add_disposal(&list, nf->nf_net); > > + struct nfsd_file *nf = list_first_entry(dispose, > > + struct nfsd_file, nf_lru); > > + struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id); > > + struct nfsd_fcache_disposal *l = nn->fcache_disposal; > > + > > + spin_lock(&l->lock); > > + list_move_tail(&nf->nf_lru, &l->freeme); > > + spin_unlock(&l->lock); > > } > > } > > > > @@ -765,8 +741,8 @@ nfsd_file_close_inode_sync(struct inode *inode) > > * nfsd_file_delayed_close - close unused nfsd_files > > * @work: dummy > > * > > - * Walk the LRU list and destroy any entries that have not been > > used since > > - * the last scan. > > + * Scrape the freeme list for this nfsd_net, and then dispose of > > them > > + * all. > > */ > > static void > > nfsd_file_delayed_close(struct work_struct *work) > > @@ -775,7 +751,10 @@ nfsd_file_delayed_close(struct work_struct > > *work) > > struct nfsd_fcache_disposal *l = container_of(work, > > struct nfsd_fcache_disposal, work); > > > > - nfsd_file_list_remove_disposal(&head, l); > > + spin_lock(&l->lock); > > + list_splice_init(&l->freeme, &head); > > + spin_unlock(&l->lock); > > + > > nfsd_file_dispose_list(&head); > > } > > Hey Jeff - > > After applying this one, tmpfs exports appear to leak space, > even after all files and directories are deleted. Eventually > the filesystem is "full" -- modifying operations return ENOSPC > but removing files doesn't recover the used space. > > Can you have a look at this? > Hrm, ok. Do you have a reproducer? Thanks,
On Fri, 2023-04-14 at 18:20 +0000, Chuck Lever III wrote: > > On Jan 18, 2023, at 12:31 PM, Jeff Layton <jlayton@kernel.org> > > wrote: > > > > When queueing a dispose list to the appropriate "freeme" lists, it > > pointlessly queues the objects one at a time to an intermediate > > list. > > > > Remove a few helpers and just open code a list_move to make it more > > clear and efficient. Better document the resulting functions with > > kerneldoc comments. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfsd/filecache.c | 63 +++++++++++++++---------------------------- > > -- > > 1 file changed, 21 insertions(+), 42 deletions(-) > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > index 58ac93e7e680..a2bc4bd90b9a 100644 > > --- a/fs/nfsd/filecache.c > > +++ b/fs/nfsd/filecache.c > > @@ -513,49 +513,25 @@ nfsd_file_dispose_list(struct list_head > > *dispose) > > } > > } > > > > -static void > > -nfsd_file_list_remove_disposal(struct list_head *dst, > > - struct nfsd_fcache_disposal *l) > > -{ > > - spin_lock(&l->lock); > > - list_splice_init(&l->freeme, dst); > > - spin_unlock(&l->lock); > > -} > > - > > -static void > > -nfsd_file_list_add_disposal(struct list_head *files, struct net > > *net) > > -{ > > - struct nfsd_net *nn = net_generic(net, nfsd_net_id); > > - struct nfsd_fcache_disposal *l = nn->fcache_disposal; > > - > > - spin_lock(&l->lock); > > - list_splice_tail_init(files, &l->freeme); > > - spin_unlock(&l->lock); > > - queue_work(nfsd_filecache_wq, &l->work); > > -} > > - > > -static void > > -nfsd_file_list_add_pernet(struct list_head *dst, struct list_head > > *src, > > - struct net *net) > > -{ > > - struct nfsd_file *nf, *tmp; > > - > > - list_for_each_entry_safe(nf, tmp, src, nf_lru) { > > - if (nf->nf_net == net) > > - list_move_tail(&nf->nf_lru, dst); > > - } > > -} > > - > > +/** > > + * nfsd_file_dispose_list_delayed - move list of dead files to > > net's freeme list > > + * @dispose: list of nfsd_files to be disposed > > + * > > + * Transfers each file to the "freeme" list for its nfsd_net, to > > eventually > > + * be disposed of by the per-net garbage collector. > > + */ > > static void > > nfsd_file_dispose_list_delayed(struct list_head *dispose) > > { > > - LIST_HEAD(list); > > - struct nfsd_file *nf; > > - > > while(!list_empty(dispose)) { > > - nf = list_first_entry(dispose, struct nfsd_file, nf_lru); > > - nfsd_file_list_add_pernet(&list, dispose, nf->nf_net); > > - nfsd_file_list_add_disposal(&list, nf->nf_net); > > + struct nfsd_file *nf = list_first_entry(dispose, > > + struct nfsd_file, nf_lru); > > + struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id); > > + struct nfsd_fcache_disposal *l = nn->fcache_disposal; > > + > > + spin_lock(&l->lock); > > + list_move_tail(&nf->nf_lru, &l->freeme); > > + spin_unlock(&l->lock); > > } > > } > > > > @@ -765,8 +741,8 @@ nfsd_file_close_inode_sync(struct inode *inode) > > * nfsd_file_delayed_close - close unused nfsd_files > > * @work: dummy > > * > > - * Walk the LRU list and destroy any entries that have not been > > used since > > - * the last scan. > > + * Scrape the freeme list for this nfsd_net, and then dispose of > > them > > + * all. > > */ > > static void > > nfsd_file_delayed_close(struct work_struct *work) > > @@ -775,7 +751,10 @@ nfsd_file_delayed_close(struct work_struct > > *work) > > struct nfsd_fcache_disposal *l = container_of(work, > > struct nfsd_fcache_disposal, work); > > > > - nfsd_file_list_remove_disposal(&head, l); > > + spin_lock(&l->lock); > > + list_splice_init(&l->freeme, &head); > > + spin_unlock(&l->lock); > > + > > nfsd_file_dispose_list(&head); > > } > > Hey Jeff - > > After applying this one, tmpfs exports appear to leak space, > even after all files and directories are deleted. Eventually > the filesystem is "full" -- modifying operations return ENOSPC > but removing files doesn't recover the used space. > > Can you have a look at this? > Actually, I may see the bug. Does this fix it? diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index c173d460b17d..f40d8f3b35a4 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c @@ -421,6 +421,7 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose) spin_lock(&l->lock); list_move_tail(&nf->nf_lru, &l->freeme); spin_unlock(&l->lock); + queue_work(nfsd_filecache_wq, &l->work); } }
> On Apr 14, 2023, at 3:17 PM, Jeff Layton <jlayton@kernel.org> wrote: > > On Fri, 2023-04-14 at 18:20 +0000, Chuck Lever III wrote: >>> On Jan 18, 2023, at 12:31 PM, Jeff Layton <jlayton@kernel.org> >>> wrote: >>> >>> When queueing a dispose list to the appropriate "freeme" lists, it >>> pointlessly queues the objects one at a time to an intermediate >>> list. >>> >>> Remove a few helpers and just open code a list_move to make it more >>> clear and efficient. Better document the resulting functions with >>> kerneldoc comments. >>> >>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >>> --- >>> fs/nfsd/filecache.c | 63 +++++++++++++++---------------------------- >>> -- >>> 1 file changed, 21 insertions(+), 42 deletions(-) >>> >>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c >>> index 58ac93e7e680..a2bc4bd90b9a 100644 >>> --- a/fs/nfsd/filecache.c >>> +++ b/fs/nfsd/filecache.c >>> @@ -513,49 +513,25 @@ nfsd_file_dispose_list(struct list_head >>> *dispose) >>> } >>> } >>> >>> -static void >>> -nfsd_file_list_remove_disposal(struct list_head *dst, >>> - struct nfsd_fcache_disposal *l) >>> -{ >>> - spin_lock(&l->lock); >>> - list_splice_init(&l->freeme, dst); >>> - spin_unlock(&l->lock); >>> -} >>> - >>> -static void >>> -nfsd_file_list_add_disposal(struct list_head *files, struct net >>> *net) >>> -{ >>> - struct nfsd_net *nn = net_generic(net, nfsd_net_id); >>> - struct nfsd_fcache_disposal *l = nn->fcache_disposal; >>> - >>> - spin_lock(&l->lock); >>> - list_splice_tail_init(files, &l->freeme); >>> - spin_unlock(&l->lock); >>> - queue_work(nfsd_filecache_wq, &l->work); >>> -} >>> - >>> -static void >>> -nfsd_file_list_add_pernet(struct list_head *dst, struct list_head >>> *src, >>> - struct net *net) >>> -{ >>> - struct nfsd_file *nf, *tmp; >>> - >>> - list_for_each_entry_safe(nf, tmp, src, nf_lru) { >>> - if (nf->nf_net == net) >>> - list_move_tail(&nf->nf_lru, dst); >>> - } >>> -} >>> - >>> +/** >>> + * nfsd_file_dispose_list_delayed - move list of dead files to >>> net's freeme list >>> + * @dispose: list of nfsd_files to be disposed >>> + * >>> + * Transfers each file to the "freeme" list for its nfsd_net, to >>> eventually >>> + * be disposed of by the per-net garbage collector. >>> + */ >>> static void >>> nfsd_file_dispose_list_delayed(struct list_head *dispose) >>> { >>> - LIST_HEAD(list); >>> - struct nfsd_file *nf; >>> - >>> while(!list_empty(dispose)) { >>> - nf = list_first_entry(dispose, struct nfsd_file, nf_lru); >>> - nfsd_file_list_add_pernet(&list, dispose, nf->nf_net); >>> - nfsd_file_list_add_disposal(&list, nf->nf_net); >>> + struct nfsd_file *nf = list_first_entry(dispose, >>> + struct nfsd_file, nf_lru); >>> + struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id); >>> + struct nfsd_fcache_disposal *l = nn->fcache_disposal; >>> + >>> + spin_lock(&l->lock); >>> + list_move_tail(&nf->nf_lru, &l->freeme); >>> + spin_unlock(&l->lock); >>> } >>> } >>> >>> @@ -765,8 +741,8 @@ nfsd_file_close_inode_sync(struct inode *inode) >>> * nfsd_file_delayed_close - close unused nfsd_files >>> * @work: dummy >>> * >>> - * Walk the LRU list and destroy any entries that have not been >>> used since >>> - * the last scan. >>> + * Scrape the freeme list for this nfsd_net, and then dispose of >>> them >>> + * all. >>> */ >>> static void >>> nfsd_file_delayed_close(struct work_struct *work) >>> @@ -775,7 +751,10 @@ nfsd_file_delayed_close(struct work_struct >>> *work) >>> struct nfsd_fcache_disposal *l = container_of(work, >>> struct nfsd_fcache_disposal, work); >>> >>> - nfsd_file_list_remove_disposal(&head, l); >>> + spin_lock(&l->lock); >>> + list_splice_init(&l->freeme, &head); >>> + spin_unlock(&l->lock); >>> + >>> nfsd_file_dispose_list(&head); >>> } >> >> Hey Jeff - >> >> After applying this one, tmpfs exports appear to leak space, >> even after all files and directories are deleted. Eventually >> the filesystem is "full" -- modifying operations return ENOSPC >> but removing files doesn't recover the used space. >> >> Can you have a look at this? > Hrm, ok. Do you have a reproducer? Nothing special. Any workload that cleans up after itself should leave the "df -k" %Used column at zero percent when it finishes. What I'm seeing is that %Used never goes down. > Actually, I may see the bug. Does this fix it? > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index c173d460b17d..f40d8f3b35a4 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -421,6 +421,7 @@ nfsd_file_dispose_list_delayed(struct list_head > *dispose) > spin_lock(&l->lock); > list_move_tail(&nf->nf_lru, &l->freeme); > spin_unlock(&l->lock); > + queue_work(nfsd_filecache_wq, &l->work); > } > } Yes, that addresses the symptom. I'll drop this version of the patch, send me a refresh? -- Chuck Lever
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index 58ac93e7e680..a2bc4bd90b9a 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c @@ -513,49 +513,25 @@ nfsd_file_dispose_list(struct list_head *dispose) } } -static void -nfsd_file_list_remove_disposal(struct list_head *dst, - struct nfsd_fcache_disposal *l) -{ - spin_lock(&l->lock); - list_splice_init(&l->freeme, dst); - spin_unlock(&l->lock); -} - -static void -nfsd_file_list_add_disposal(struct list_head *files, struct net *net) -{ - struct nfsd_net *nn = net_generic(net, nfsd_net_id); - struct nfsd_fcache_disposal *l = nn->fcache_disposal; - - spin_lock(&l->lock); - list_splice_tail_init(files, &l->freeme); - spin_unlock(&l->lock); - queue_work(nfsd_filecache_wq, &l->work); -} - -static void -nfsd_file_list_add_pernet(struct list_head *dst, struct list_head *src, - struct net *net) -{ - struct nfsd_file *nf, *tmp; - - list_for_each_entry_safe(nf, tmp, src, nf_lru) { - if (nf->nf_net == net) - list_move_tail(&nf->nf_lru, dst); - } -} - +/** + * nfsd_file_dispose_list_delayed - move list of dead files to net's freeme list + * @dispose: list of nfsd_files to be disposed + * + * Transfers each file to the "freeme" list for its nfsd_net, to eventually + * be disposed of by the per-net garbage collector. + */ static void nfsd_file_dispose_list_delayed(struct list_head *dispose) { - LIST_HEAD(list); - struct nfsd_file *nf; - while(!list_empty(dispose)) { - nf = list_first_entry(dispose, struct nfsd_file, nf_lru); - nfsd_file_list_add_pernet(&list, dispose, nf->nf_net); - nfsd_file_list_add_disposal(&list, nf->nf_net); + struct nfsd_file *nf = list_first_entry(dispose, + struct nfsd_file, nf_lru); + struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id); + struct nfsd_fcache_disposal *l = nn->fcache_disposal; + + spin_lock(&l->lock); + list_move_tail(&nf->nf_lru, &l->freeme); + spin_unlock(&l->lock); } } @@ -765,8 +741,8 @@ nfsd_file_close_inode_sync(struct inode *inode) * nfsd_file_delayed_close - close unused nfsd_files * @work: dummy * - * Walk the LRU list and destroy any entries that have not been used since - * the last scan. + * Scrape the freeme list for this nfsd_net, and then dispose of them + * all. */ static void nfsd_file_delayed_close(struct work_struct *work) @@ -775,7 +751,10 @@ nfsd_file_delayed_close(struct work_struct *work) struct nfsd_fcache_disposal *l = container_of(work, struct nfsd_fcache_disposal, work); - nfsd_file_list_remove_disposal(&head, l); + spin_lock(&l->lock); + list_splice_init(&l->freeme, &head); + spin_unlock(&l->lock); + nfsd_file_dispose_list(&head); }
When queueing a dispose list to the appropriate "freeme" lists, it pointlessly queues the objects one at a time to an intermediate list. Remove a few helpers and just open code a list_move to make it more clear and efficient. Better document the resulting functions with kerneldoc comments. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/filecache.c | 63 +++++++++++++++------------------------------ 1 file changed, 21 insertions(+), 42 deletions(-)