* [PATCH 0/3] 9pfs: Improve unreclaim logic @ 2021-01-18 14:22 Greg Kurz 2021-01-18 14:22 ` [PATCH 1/3] 9pfs: Convert V9fsFidState::clunked to bool Greg Kurz ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Greg Kurz @ 2021-01-18 14:22 UTC (permalink / raw) To: qemu-devel; +Cc: Christian Schoenebeck, Greg Kurz clone of "master" Greg Kurz (3): 9pfs: Convert V9fsFidState::clunked to bool 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ 9pfs: Improve unreclaim loop hw/9pfs/9p.c | 83 +++++++++++++++++++++++++++++----------------------- hw/9pfs/9p.h | 6 ++-- 2 files changed, 50 insertions(+), 39 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] 9pfs: Convert V9fsFidState::clunked to bool 2021-01-18 14:22 [PATCH 0/3] 9pfs: Improve unreclaim logic Greg Kurz @ 2021-01-18 14:22 ` Greg Kurz 2021-01-18 14:42 ` Christian Schoenebeck 2021-01-18 14:22 ` [PATCH 2/3] 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ Greg Kurz ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Greg Kurz @ 2021-01-18 14:22 UTC (permalink / raw) To: qemu-devel; +Cc: Christian Schoenebeck, Greg Kurz This can only be 0 or 1. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/9pfs/9p.c | 4 ++-- hw/9pfs/9p.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 6026b51a1c04..37c3379b7462 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -413,7 +413,7 @@ static V9fsFidState *clunk_fid(V9fsState *s, int32_t fid) } fidp = *fidpp; *fidpp = fidp->next; - fidp->clunked = 1; + fidp->clunked = true; return fidp; } @@ -544,7 +544,7 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu) /* Clunk fid */ s->fid_list = fidp->next; - fidp->clunked = 1; + fidp->clunked = true; put_fid(pdu, fidp); } diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h index 32df81f360ea..93656323d1d7 100644 --- a/hw/9pfs/9p.h +++ b/hw/9pfs/9p.h @@ -279,7 +279,7 @@ struct V9fsFidState { int open_flags; uid_t uid; int ref; - int clunked; + bool clunked; V9fsFidState *next; V9fsFidState *rclm_lst; }; -- 2.26.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] 9pfs: Convert V9fsFidState::clunked to bool 2021-01-18 14:22 ` [PATCH 1/3] 9pfs: Convert V9fsFidState::clunked to bool Greg Kurz @ 2021-01-18 14:42 ` Christian Schoenebeck 0 siblings, 0 replies; 13+ messages in thread From: Christian Schoenebeck @ 2021-01-18 14:42 UTC (permalink / raw) To: qemu-devel; +Cc: Greg Kurz On Montag, 18. Januar 2021 15:22:58 CET Greg Kurz wrote: > This can only be 0 or 1. > > Signed-off-by: Greg Kurz <groug@kaod.org> Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com> I still need to go through the other 2 patches. I should be done tomorrow. > --- > hw/9pfs/9p.c | 4 ++-- > hw/9pfs/9p.h | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index 6026b51a1c04..37c3379b7462 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -413,7 +413,7 @@ static V9fsFidState *clunk_fid(V9fsState *s, int32_t > fid) } > fidp = *fidpp; > *fidpp = fidp->next; > - fidp->clunked = 1; > + fidp->clunked = true; > return fidp; > } > > @@ -544,7 +544,7 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu) > > /* Clunk fid */ > s->fid_list = fidp->next; > - fidp->clunked = 1; > + fidp->clunked = true; > > put_fid(pdu, fidp); > } > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h > index 32df81f360ea..93656323d1d7 100644 > --- a/hw/9pfs/9p.h > +++ b/hw/9pfs/9p.h > @@ -279,7 +279,7 @@ struct V9fsFidState { > int open_flags; > uid_t uid; > int ref; > - int clunked; > + bool clunked; > V9fsFidState *next; > V9fsFidState *rclm_lst; > }; ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ 2021-01-18 14:22 [PATCH 0/3] 9pfs: Improve unreclaim logic Greg Kurz 2021-01-18 14:22 ` [PATCH 1/3] 9pfs: Convert V9fsFidState::clunked to bool Greg Kurz @ 2021-01-18 14:22 ` Greg Kurz 2021-01-19 13:31 ` Christian Schoenebeck 2021-01-18 14:23 ` [PATCH 3/3] 9pfs: Improve unreclaim loop Greg Kurz 2021-01-21 17:05 ` [PATCH 0/3] 9pfs: Improve unreclaim logic Greg Kurz 3 siblings, 1 reply; 13+ messages in thread From: Greg Kurz @ 2021-01-18 14:22 UTC (permalink / raw) To: qemu-devel; +Cc: Christian Schoenebeck, Greg Kurz The fid_list is currently open-coded. This doesn't seem to serve any purpose that cannot be met with QEMU's generic lists. Let's go for a QSIMPLEQ : this will allow to add new fids at the end of the list and to improve the logic in v9fs_mark_fids_unreclaim(). Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/9pfs/9p.c | 41 ++++++++++++++++++----------------------- hw/9pfs/9p.h | 4 ++-- 2 files changed, 20 insertions(+), 25 deletions(-) diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 37c3379b7462..b65f320e6518 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -260,7 +260,7 @@ static V9fsFidState *coroutine_fn get_fid(V9fsPDU *pdu, int32_t fid) V9fsFidState *f; V9fsState *s = pdu->s; - for (f = s->fid_list; f; f = f->next) { + QSIMPLEQ_FOREACH(f, &s->fid_list, next) { BUG_ON(f->clunked); if (f->fid == fid) { /* @@ -295,7 +295,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid) { V9fsFidState *f; - for (f = s->fid_list; f; f = f->next) { + QSIMPLEQ_FOREACH(f, &s->fid_list, next) { /* If fid is already there return NULL */ BUG_ON(f->clunked); if (f->fid == fid) { @@ -311,8 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid) * reclaim won't close the file descriptor */ f->flags |= FID_REFERENCED; - f->next = s->fid_list; - s->fid_list = f; + QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next); v9fs_readdir_init(s->proto_version, &f->fs.dir); v9fs_readdir_init(s->proto_version, &f->fs_reclaim.dir); @@ -401,20 +400,16 @@ static int coroutine_fn put_fid(V9fsPDU *pdu, V9fsFidState *fidp) static V9fsFidState *clunk_fid(V9fsState *s, int32_t fid) { - V9fsFidState **fidpp, *fidp; + V9fsFidState *fidp; - for (fidpp = &s->fid_list; *fidpp; fidpp = &(*fidpp)->next) { - if ((*fidpp)->fid == fid) { - break; + QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) { + if (fidp->fid == fid) { + QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next); + fidp->clunked = true; + return fidp; } } - if (*fidpp == NULL) { - return NULL; - } - fidp = *fidpp; - *fidpp = fidp->next; - fidp->clunked = true; - return fidp; + return NULL; } void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu) @@ -423,7 +418,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu) V9fsState *s = pdu->s; V9fsFidState *f, *reclaim_list = NULL; - for (f = s->fid_list; f; f = f->next) { + QSIMPLEQ_FOREACH(f, &s->fid_list, next) { /* * Unlink fids cannot be reclaimed. Check * for them and skip them. Also skip fids @@ -505,7 +500,7 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU *pdu, V9fsPath *path) V9fsFidState *fidp; again: - for (fidp = s->fid_list; fidp; fidp = fidp->next) { + QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) { if (fidp->path.size != path->size) { continue; } @@ -537,13 +532,13 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu) V9fsFidState *fidp; /* Free all fids */ - while (s->fid_list) { + while (!QSIMPLEQ_EMPTY(&s->fid_list)) { /* Get fid */ - fidp = s->fid_list; + fidp = QSIMPLEQ_FIRST(&s->fid_list); fidp->ref++; /* Clunk fid */ - s->fid_list = fidp->next; + QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next); fidp->clunked = true; put_fid(pdu, fidp); @@ -3121,7 +3116,7 @@ static int coroutine_fn v9fs_complete_rename(V9fsPDU *pdu, V9fsFidState *fidp, * Fixup fid's pointing to the old name to * start pointing to the new name */ - for (tfidp = s->fid_list; tfidp; tfidp = tfidp->next) { + QSIMPLEQ_FOREACH(tfidp, &s->fid_list, next) { if (v9fs_path_is_ancestor(&fidp->path, &tfidp->path)) { /* replace the name */ v9fs_fix_path(&tfidp->path, &new_path, strlen(fidp->path.data)); @@ -3215,7 +3210,7 @@ static int coroutine_fn v9fs_fix_fid_paths(V9fsPDU *pdu, V9fsPath *olddir, * Fixup fid's pointing to the old name to * start pointing to the new name */ - for (tfidp = s->fid_list; tfidp; tfidp = tfidp->next) { + QSIMPLEQ_FOREACH(tfidp, &s->fid_list, next) { if (v9fs_path_is_ancestor(&oldpath, &tfidp->path)) { /* replace the name */ v9fs_fix_path(&tfidp->path, &newpath, strlen(oldpath.data)); @@ -4081,7 +4076,7 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t, s->ctx.fmode = fse->fmode; s->ctx.dmode = fse->dmode; - s->fid_list = NULL; + QSIMPLEQ_INIT(&s->fid_list); qemu_co_rwlock_init(&s->rename_lock); if (s->ops->init(&s->ctx, errp) < 0) { diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h index 93656323d1d7..85fb6930b0ca 100644 --- a/hw/9pfs/9p.h +++ b/hw/9pfs/9p.h @@ -280,7 +280,7 @@ struct V9fsFidState { uid_t uid; int ref; bool clunked; - V9fsFidState *next; + QSIMPLEQ_ENTRY(V9fsFidState) next; V9fsFidState *rclm_lst; }; @@ -339,7 +339,7 @@ typedef struct { struct V9fsState { QLIST_HEAD(, V9fsPDU) free_list; QLIST_HEAD(, V9fsPDU) active_list; - V9fsFidState *fid_list; + QSIMPLEQ_HEAD(, V9fsFidState) fid_list; FileOperations *ops; FsContext ctx; char *tag; -- 2.26.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ 2021-01-18 14:22 ` [PATCH 2/3] 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ Greg Kurz @ 2021-01-19 13:31 ` Christian Schoenebeck 2021-01-19 14:34 ` Greg Kurz 0 siblings, 1 reply; 13+ messages in thread From: Christian Schoenebeck @ 2021-01-19 13:31 UTC (permalink / raw) To: qemu-devel; +Cc: Greg Kurz On Montag, 18. Januar 2021 15:22:59 CET Greg Kurz wrote: > The fid_list is currently open-coded. This doesn't seem to serve any > purpose that cannot be met with QEMU's generic lists. Let's go for a > QSIMPLEQ : this will allow to add new fids at the end of the list and > to improve the logic in v9fs_mark_fids_unreclaim(). > > Signed-off-by: Greg Kurz <groug@kaod.org> In general LGTM hence: Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com> Some comments below ... > --- > hw/9pfs/9p.c | 41 ++++++++++++++++++----------------------- > hw/9pfs/9p.h | 4 ++-- > 2 files changed, 20 insertions(+), 25 deletions(-) > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index 37c3379b7462..b65f320e6518 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -260,7 +260,7 @@ static V9fsFidState *coroutine_fn get_fid(V9fsPDU *pdu, > int32_t fid) V9fsFidState *f; > V9fsState *s = pdu->s; > > - for (f = s->fid_list; f; f = f->next) { > + QSIMPLEQ_FOREACH(f, &s->fid_list, next) { > BUG_ON(f->clunked); > if (f->fid == fid) { > /* > @@ -295,7 +295,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t > fid) { > V9fsFidState *f; > > - for (f = s->fid_list; f; f = f->next) { > + QSIMPLEQ_FOREACH(f, &s->fid_list, next) { > /* If fid is already there return NULL */ > BUG_ON(f->clunked); > if (f->fid == fid) { > @@ -311,8 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t > fid) * reclaim won't close the file descriptor > */ > f->flags |= FID_REFERENCED; > - f->next = s->fid_list; > - s->fid_list = f; > + QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next); Not related to this series, but I wonder why queue.h wraps everything into: do { ... } while (0); No comment about it in git history. Looks like a leftover of its initial import from 2009. > v9fs_readdir_init(s->proto_version, &f->fs.dir); > v9fs_readdir_init(s->proto_version, &f->fs_reclaim.dir); > @@ -401,20 +400,16 @@ static int coroutine_fn put_fid(V9fsPDU *pdu, > V9fsFidState *fidp) > > static V9fsFidState *clunk_fid(V9fsState *s, int32_t fid) > { > - V9fsFidState **fidpp, *fidp; > + V9fsFidState *fidp; > > - for (fidpp = &s->fid_list; *fidpp; fidpp = &(*fidpp)->next) { > - if ((*fidpp)->fid == fid) { > - break; > + QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) { > + if (fidp->fid == fid) { > + QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next); > + fidp->clunked = true; > + return fidp; > } > } > - if (*fidpp == NULL) { > - return NULL; > - } > - fidp = *fidpp; > - *fidpp = fidp->next; > - fidp->clunked = true; > - return fidp; > + return NULL; > } Good cleanup there! > void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu) > @@ -423,7 +418,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu) > V9fsState *s = pdu->s; > V9fsFidState *f, *reclaim_list = NULL; > > - for (f = s->fid_list; f; f = f->next) { > + QSIMPLEQ_FOREACH(f, &s->fid_list, next) { > /* > * Unlink fids cannot be reclaimed. Check > * for them and skip them. Also skip fids > @@ -505,7 +500,7 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU > *pdu, V9fsPath *path) V9fsFidState *fidp; > > again: > - for (fidp = s->fid_list; fidp; fidp = fidp->next) { > + QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) { > if (fidp->path.size != path->size) { > continue; > } > @@ -537,13 +532,13 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu) > V9fsFidState *fidp; > > /* Free all fids */ > - while (s->fid_list) { > + while (!QSIMPLEQ_EMPTY(&s->fid_list)) { > /* Get fid */ > - fidp = s->fid_list; > + fidp = QSIMPLEQ_FIRST(&s->fid_list); > fidp->ref++; > > /* Clunk fid */ > - s->fid_list = fidp->next; > + QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next); > fidp->clunked = true; > > put_fid(pdu, fidp); > @@ -3121,7 +3116,7 @@ static int coroutine_fn v9fs_complete_rename(V9fsPDU > *pdu, V9fsFidState *fidp, * Fixup fid's pointing to the old name to > * start pointing to the new name > */ > - for (tfidp = s->fid_list; tfidp; tfidp = tfidp->next) { > + QSIMPLEQ_FOREACH(tfidp, &s->fid_list, next) { > if (v9fs_path_is_ancestor(&fidp->path, &tfidp->path)) { > /* replace the name */ > v9fs_fix_path(&tfidp->path, &new_path, > strlen(fidp->path.data)); @@ -3215,7 +3210,7 @@ static int coroutine_fn > v9fs_fix_fid_paths(V9fsPDU *pdu, V9fsPath *olddir, * Fixup fid's pointing > to the old name to > * start pointing to the new name > */ > - for (tfidp = s->fid_list; tfidp; tfidp = tfidp->next) { > + QSIMPLEQ_FOREACH(tfidp, &s->fid_list, next) { > if (v9fs_path_is_ancestor(&oldpath, &tfidp->path)) { > /* replace the name */ > v9fs_fix_path(&tfidp->path, &newpath, strlen(oldpath.data)); > @@ -4081,7 +4076,7 @@ int v9fs_device_realize_common(V9fsState *s, const > V9fsTransport *t, s->ctx.fmode = fse->fmode; > s->ctx.dmode = fse->dmode; > > - s->fid_list = NULL; > + QSIMPLEQ_INIT(&s->fid_list); > qemu_co_rwlock_init(&s->rename_lock); > > if (s->ops->init(&s->ctx, errp) < 0) { > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h > index 93656323d1d7..85fb6930b0ca 100644 > --- a/hw/9pfs/9p.h > +++ b/hw/9pfs/9p.h > @@ -280,7 +280,7 @@ struct V9fsFidState { > uid_t uid; > int ref; > bool clunked; > - V9fsFidState *next; > + QSIMPLEQ_ENTRY(V9fsFidState) next; > V9fsFidState *rclm_lst; > }; Is there a reason for not migrating 'rclm_lst' as well? Anyway, if you decide to do that as well, then it would IMO be fine with a separate patch later on, not worth making it more complicated with a v2 just for that. > @@ -339,7 +339,7 @@ typedef struct { > struct V9fsState { > QLIST_HEAD(, V9fsPDU) free_list; > QLIST_HEAD(, V9fsPDU) active_list; > - V9fsFidState *fid_list; > + QSIMPLEQ_HEAD(, V9fsFidState) fid_list; Not an issue for this series, but I wonder whether that macro's first argument shouldn't simply be dropped in queue.h, because there are only 2 occurences in the entire QEMU code base using it. It looks like nobody really needs it. > FileOperations *ops; > FsContext ctx; > char *tag; Best regards, Christian Schoenebeck ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ 2021-01-19 13:31 ` Christian Schoenebeck @ 2021-01-19 14:34 ` Greg Kurz 2021-01-19 15:28 ` Christian Schoenebeck 0 siblings, 1 reply; 13+ messages in thread From: Greg Kurz @ 2021-01-19 14:34 UTC (permalink / raw) To: Christian Schoenebeck; +Cc: qemu-devel On Tue, 19 Jan 2021 14:31:26 +0100 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Montag, 18. Januar 2021 15:22:59 CET Greg Kurz wrote: > > The fid_list is currently open-coded. This doesn't seem to serve any > > purpose that cannot be met with QEMU's generic lists. Let's go for a > > QSIMPLEQ : this will allow to add new fids at the end of the list and > > to improve the logic in v9fs_mark_fids_unreclaim(). > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > In general LGTM hence: > > Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > Some comments below ... > > > --- > > hw/9pfs/9p.c | 41 ++++++++++++++++++----------------------- > > hw/9pfs/9p.h | 4 ++-- > > 2 files changed, 20 insertions(+), 25 deletions(-) > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > index 37c3379b7462..b65f320e6518 100644 > > --- a/hw/9pfs/9p.c > > +++ b/hw/9pfs/9p.c > > @@ -260,7 +260,7 @@ static V9fsFidState *coroutine_fn get_fid(V9fsPDU *pdu, > > int32_t fid) V9fsFidState *f; > > V9fsState *s = pdu->s; > > > > - for (f = s->fid_list; f; f = f->next) { > > + QSIMPLEQ_FOREACH(f, &s->fid_list, next) { > > BUG_ON(f->clunked); > > if (f->fid == fid) { > > /* > > @@ -295,7 +295,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t > > fid) { > > V9fsFidState *f; > > > > - for (f = s->fid_list; f; f = f->next) { > > + QSIMPLEQ_FOREACH(f, &s->fid_list, next) { > > /* If fid is already there return NULL */ > > BUG_ON(f->clunked); > > if (f->fid == fid) { > > @@ -311,8 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t > > fid) * reclaim won't close the file descriptor > > */ > > f->flags |= FID_REFERENCED; > > - f->next = s->fid_list; > > - s->fid_list = f; > > + QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next); > > Not related to this series, but I wonder why queue.h wraps everything into: > > do { > ... > } while (0); > Note, this is do { ... } while (0) *without* the trailing semi-colon, which is the corner stone of this trick. > No comment about it in git history. Looks like a leftover of its initial > import from 2009. > This is the usual way to define multi-statement macros. More details here: http://c-faq.com/cpp/multistmt.html > > v9fs_readdir_init(s->proto_version, &f->fs.dir); > > v9fs_readdir_init(s->proto_version, &f->fs_reclaim.dir); > > @@ -401,20 +400,16 @@ static int coroutine_fn put_fid(V9fsPDU *pdu, > > V9fsFidState *fidp) > > > > static V9fsFidState *clunk_fid(V9fsState *s, int32_t fid) > > { > > - V9fsFidState **fidpp, *fidp; > > + V9fsFidState *fidp; > > > > - for (fidpp = &s->fid_list; *fidpp; fidpp = &(*fidpp)->next) { > > - if ((*fidpp)->fid == fid) { > > - break; > > + QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) { > > + if (fidp->fid == fid) { > > + QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next); > > + fidp->clunked = true; > > + return fidp; > > } > > } > > - if (*fidpp == NULL) { > > - return NULL; > > - } > > - fidp = *fidpp; > > - *fidpp = fidp->next; > > - fidp->clunked = true; > > - return fidp; > > + return NULL; > > } > > Good cleanup there! > Thanks ! > > void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu) > > @@ -423,7 +418,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu) > > V9fsState *s = pdu->s; > > V9fsFidState *f, *reclaim_list = NULL; > > > > - for (f = s->fid_list; f; f = f->next) { > > + QSIMPLEQ_FOREACH(f, &s->fid_list, next) { > > /* > > * Unlink fids cannot be reclaimed. Check > > * for them and skip them. Also skip fids > > @@ -505,7 +500,7 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU > > *pdu, V9fsPath *path) V9fsFidState *fidp; > > > > again: > > - for (fidp = s->fid_list; fidp; fidp = fidp->next) { > > + QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) { > > if (fidp->path.size != path->size) { > > continue; > > } > > @@ -537,13 +532,13 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu) > > V9fsFidState *fidp; > > > > /* Free all fids */ > > - while (s->fid_list) { > > + while (!QSIMPLEQ_EMPTY(&s->fid_list)) { > > /* Get fid */ > > - fidp = s->fid_list; > > + fidp = QSIMPLEQ_FIRST(&s->fid_list); > > fidp->ref++; > > > > /* Clunk fid */ > > - s->fid_list = fidp->next; > > + QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next); > > fidp->clunked = true; > > > > put_fid(pdu, fidp); > > @@ -3121,7 +3116,7 @@ static int coroutine_fn v9fs_complete_rename(V9fsPDU > > *pdu, V9fsFidState *fidp, * Fixup fid's pointing to the old name to > > * start pointing to the new name > > */ > > - for (tfidp = s->fid_list; tfidp; tfidp = tfidp->next) { > > + QSIMPLEQ_FOREACH(tfidp, &s->fid_list, next) { > > if (v9fs_path_is_ancestor(&fidp->path, &tfidp->path)) { > > /* replace the name */ > > v9fs_fix_path(&tfidp->path, &new_path, > > strlen(fidp->path.data)); @@ -3215,7 +3210,7 @@ static int coroutine_fn > > v9fs_fix_fid_paths(V9fsPDU *pdu, V9fsPath *olddir, * Fixup fid's pointing > > to the old name to > > * start pointing to the new name > > */ > > - for (tfidp = s->fid_list; tfidp; tfidp = tfidp->next) { > > + QSIMPLEQ_FOREACH(tfidp, &s->fid_list, next) { > > if (v9fs_path_is_ancestor(&oldpath, &tfidp->path)) { > > /* replace the name */ > > v9fs_fix_path(&tfidp->path, &newpath, strlen(oldpath.data)); > > @@ -4081,7 +4076,7 @@ int v9fs_device_realize_common(V9fsState *s, const > > V9fsTransport *t, s->ctx.fmode = fse->fmode; > > s->ctx.dmode = fse->dmode; > > > > - s->fid_list = NULL; > > + QSIMPLEQ_INIT(&s->fid_list); > > qemu_co_rwlock_init(&s->rename_lock); > > > > if (s->ops->init(&s->ctx, errp) < 0) { > > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h > > index 93656323d1d7..85fb6930b0ca 100644 > > --- a/hw/9pfs/9p.h > > +++ b/hw/9pfs/9p.h > > @@ -280,7 +280,7 @@ struct V9fsFidState { > > uid_t uid; > > int ref; > > bool clunked; > > - V9fsFidState *next; > > + QSIMPLEQ_ENTRY(V9fsFidState) next; > > V9fsFidState *rclm_lst; > > }; > > Is there a reason for not migrating 'rclm_lst' as well? > Yeah. I did consider doing so at first but these were too many changes. Also, the fid list and the reclaim list have quite different needs, that each deserve its own patch. > Anyway, if you decide to do that as well, then it would IMO be fine with a > separate patch later on, not worth making it more complicated with a v2 just > for that. Yes, I already have a patch to convert the reclaim list to QSLIST. I'll post it later as it is merely cosmetic, while this series changes the behavior. > > @@ -339,7 +339,7 @@ typedef struct { > > struct V9fsState { > > QLIST_HEAD(, V9fsPDU) free_list; > > QLIST_HEAD(, V9fsPDU) active_list; > > - V9fsFidState *fid_list; > > + QSIMPLEQ_HEAD(, V9fsFidState) fid_list; > > Not an issue for this series, but I wonder whether that macro's first argument > shouldn't simply be dropped in queue.h, because there are only 2 occurences in > the entire QEMU code base using it. It looks like nobody really needs it. > A similar macro exist for the other list types as well, with some more users. If we go for such a change, I think this should stay consistent accros list types. All users should hence be audited first. > > FileOperations *ops; > > FsContext ctx; > > char *tag; > > Best regards, > Christian Schoenebeck > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ 2021-01-19 14:34 ` Greg Kurz @ 2021-01-19 15:28 ` Christian Schoenebeck 2021-01-19 17:23 ` Greg Kurz 0 siblings, 1 reply; 13+ messages in thread From: Christian Schoenebeck @ 2021-01-19 15:28 UTC (permalink / raw) To: qemu-devel; +Cc: Greg Kurz On Dienstag, 19. Januar 2021 15:34:07 CET Greg Kurz wrote: > On Tue, 19 Jan 2021 14:31:26 +0100 > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > On Montag, 18. Januar 2021 15:22:59 CET Greg Kurz wrote: > > > The fid_list is currently open-coded. This doesn't seem to serve any > > > purpose that cannot be met with QEMU's generic lists. Let's go for a > > > QSIMPLEQ : this will allow to add new fids at the end of the list and > > > to improve the logic in v9fs_mark_fids_unreclaim(). > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > In general LGTM hence: > > > > Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > > > Some comments below ... > > > > > --- > > > > > > hw/9pfs/9p.c | 41 ++++++++++++++++++----------------------- > > > hw/9pfs/9p.h | 4 ++-- > > > 2 files changed, 20 insertions(+), 25 deletions(-) > > > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > > index 37c3379b7462..b65f320e6518 100644 > > > --- a/hw/9pfs/9p.c > > > +++ b/hw/9pfs/9p.c > > > @@ -260,7 +260,7 @@ static V9fsFidState *coroutine_fn get_fid(V9fsPDU > > > *pdu, > > > int32_t fid) V9fsFidState *f; > > > > > > V9fsState *s = pdu->s; > > > > > > - for (f = s->fid_list; f; f = f->next) { > > > + QSIMPLEQ_FOREACH(f, &s->fid_list, next) { > > > > > > BUG_ON(f->clunked); > > > if (f->fid == fid) { > > > > > > /* > > > > > > @@ -295,7 +295,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t > > > fid) { > > > > > > V9fsFidState *f; > > > > > > - for (f = s->fid_list; f; f = f->next) { > > > + QSIMPLEQ_FOREACH(f, &s->fid_list, next) { > > > > > > /* If fid is already there return NULL */ > > > BUG_ON(f->clunked); > > > if (f->fid == fid) { > > > > > > @@ -311,8 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t > > > fid) * reclaim won't close the file descriptor > > > > > > */ > > > > > > f->flags |= FID_REFERENCED; > > > > > > - f->next = s->fid_list; > > > - s->fid_list = f; > > > + QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next); > > > > Not related to this series, but I wonder why queue.h wraps everything > > into: > > > > do { > > > > ... > > > > } while (0); > > Note, this is do { ... } while (0) *without* the trailing semi-colon, which > is the corner stone of this trick. Yes, I got that. What I meant was, unless I am missing something, a simple compound statement (without trailing semi-colon) a.k.a. a 'block' is the more common way to handle this, like: #define QSIMPLEQ_INIT(head) { \ (head)->sqh_first = NULL; \ (head)->sqh_last = &(head)->sqh_first; \ } Third patch tomorrow ... thanks Greg! Best regards, Christian Schoenebeck ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ 2021-01-19 15:28 ` Christian Schoenebeck @ 2021-01-19 17:23 ` Greg Kurz 0 siblings, 0 replies; 13+ messages in thread From: Greg Kurz @ 2021-01-19 17:23 UTC (permalink / raw) To: Christian Schoenebeck; +Cc: qemu-devel On Tue, 19 Jan 2021 16:28:01 +0100 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Dienstag, 19. Januar 2021 15:34:07 CET Greg Kurz wrote: > > On Tue, 19 Jan 2021 14:31:26 +0100 > > > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > > On Montag, 18. Januar 2021 15:22:59 CET Greg Kurz wrote: > > > > The fid_list is currently open-coded. This doesn't seem to serve any > > > > purpose that cannot be met with QEMU's generic lists. Let's go for a > > > > QSIMPLEQ : this will allow to add new fids at the end of the list and > > > > to improve the logic in v9fs_mark_fids_unreclaim(). > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > > > In general LGTM hence: > > > > > > Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > > > > > Some comments below ... > > > > > > > --- > > > > > > > > hw/9pfs/9p.c | 41 ++++++++++++++++++----------------------- > > > > hw/9pfs/9p.h | 4 ++-- > > > > 2 files changed, 20 insertions(+), 25 deletions(-) > > > > > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > > > index 37c3379b7462..b65f320e6518 100644 > > > > --- a/hw/9pfs/9p.c > > > > +++ b/hw/9pfs/9p.c > > > > @@ -260,7 +260,7 @@ static V9fsFidState *coroutine_fn get_fid(V9fsPDU > > > > *pdu, > > > > int32_t fid) V9fsFidState *f; > > > > > > > > V9fsState *s = pdu->s; > > > > > > > > - for (f = s->fid_list; f; f = f->next) { > > > > + QSIMPLEQ_FOREACH(f, &s->fid_list, next) { > > > > > > > > BUG_ON(f->clunked); > > > > if (f->fid == fid) { > > > > > > > > /* > > > > > > > > @@ -295,7 +295,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t > > > > fid) { > > > > > > > > V9fsFidState *f; > > > > > > > > - for (f = s->fid_list; f; f = f->next) { > > > > + QSIMPLEQ_FOREACH(f, &s->fid_list, next) { > > > > > > > > /* If fid is already there return NULL */ > > > > BUG_ON(f->clunked); > > > > if (f->fid == fid) { > > > > > > > > @@ -311,8 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t > > > > fid) * reclaim won't close the file descriptor > > > > > > > > */ > > > > > > > > f->flags |= FID_REFERENCED; > > > > > > > > - f->next = s->fid_list; > > > > - s->fid_list = f; > > > > + QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next); > > > > > > Not related to this series, but I wonder why queue.h wraps everything > > > into: > > > > > > do { > > > > > > ... > > > > > > } while (0); > > > > Note, this is do { ... } while (0) *without* the trailing semi-colon, which > > is the corner stone of this trick. > > Yes, I got that. What I meant was, unless I am missing something, a simple > compound statement (without trailing semi-colon) a.k.a. a 'block' is the more > common way to handle this, like: > > #define QSIMPLEQ_INIT(head) { \ > (head)->sqh_first = NULL; \ > (head)->sqh_last = &(head)->sqh_first; \ > } > No this isn't the more common way because it doesn't work with if...else statements, i.e. if (some_condition) QSIMPLEQ_INIT(&head); else do_some_other_stuff() has illagal syntax because of the semi-colon. Of course, since the QEMU coding styles mandates { } in if...else statements, your suggestion should just work, but do { } while(0) doesn't do harm (compilers optimize that flawlessly) and it is so commonly used elsewhere that I don't think its worth changing. > Third patch tomorrow ... thanks Greg! > > Best regards, > Christian Schoenebeck > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] 9pfs: Improve unreclaim loop 2021-01-18 14:22 [PATCH 0/3] 9pfs: Improve unreclaim logic Greg Kurz 2021-01-18 14:22 ` [PATCH 1/3] 9pfs: Convert V9fsFidState::clunked to bool Greg Kurz 2021-01-18 14:22 ` [PATCH 2/3] 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ Greg Kurz @ 2021-01-18 14:23 ` Greg Kurz 2021-01-21 12:50 ` Christian Schoenebeck 2021-01-21 17:05 ` [PATCH 0/3] 9pfs: Improve unreclaim logic Greg Kurz 3 siblings, 1 reply; 13+ messages in thread From: Greg Kurz @ 2021-01-18 14:23 UTC (permalink / raw) To: qemu-devel; +Cc: Christian Schoenebeck, Greg Kurz If a fid was actually re-open by v9fs_reopen_fid(), we re-traverse the fid list from the head in case some other request created a fid that needs to be marked unreclaimable as well (ie. the client open a new handle on the path that is being unlinked). This is a suboptimal since most if not all fids that require it have likely been taken care of already. This is mostly the result of new fids being added to the head of the list. Since the list is now a QSIMPLEQ, add new fids at the end instead. Take a reference on the fid to ensure it doesn't go away during v9fs_reopen_fid() and that it can be safely passed to QSIMPLEQ_NEXT() afterwards. Since the associated put_fid() can also yield, same is done with the next fid. So the logic here is to get a reference on a fid and only put it back during the next iteration after we could get a reference on the next fid. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/9pfs/9p.c | 44 ++++++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index b65f320e6518..b0ab5cf61c1f 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -311,7 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid) * reclaim won't close the file descriptor */ f->flags |= FID_REFERENCED; - QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next); + QSIMPLEQ_INSERT_TAIL(&s->fid_list, f, next); v9fs_readdir_init(s->proto_version, &f->fs.dir); v9fs_readdir_init(s->proto_version, &f->fs_reclaim.dir); @@ -497,32 +497,48 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU *pdu, V9fsPath *path) { int err; V9fsState *s = pdu->s; - V9fsFidState *fidp; + V9fsFidState *fidp, *fidp_next; -again: - QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) { - if (fidp->path.size != path->size) { - continue; - } - if (!memcmp(fidp->path.data, path->data, path->size)) { + fidp = QSIMPLEQ_FIRST(&s->fid_list); + assert(fidp); + + /* + * v9fs_reopen_fid() can yield : a reference on the fid must be held + * to ensure its pointer remains valid and we can safely pass it to + * QSIMPLEQ_NEXT(). The corresponding put_fid() can also yield so + * we must keep a reference on the next fid as well. So the logic here + * is to get a reference on a fid and only put it back during the next + * iteration after we could get a reference on the next fid. Start with + * the first one. + */ + for (fidp->ref++; fidp; fidp = fidp_next) { + if (fidp->path.size == path->size && + !memcmp(fidp->path.data, path->data, path->size)) { /* Mark the fid non reclaimable. */ fidp->flags |= FID_NON_RECLAIMABLE; /* reopen the file/dir if already closed */ err = v9fs_reopen_fid(pdu, fidp); if (err < 0) { + put_fid(pdu, fidp); return err; } + } + + fidp_next = QSIMPLEQ_NEXT(fidp, next); + + if (fidp_next) { /* - * Go back to head of fid list because - * the list could have got updated when - * switched to the worker thread + * Ensure the next fid survives a potential clunk request during + * put_fid() below and v9fs_reopen_fid() in the next iteration. */ - if (err == 0) { - goto again; - } + fidp_next->ref++; } + + /* We're done with this fid */ + put_fid(pdu, fidp); } + return 0; } -- 2.26.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] 9pfs: Improve unreclaim loop 2021-01-18 14:23 ` [PATCH 3/3] 9pfs: Improve unreclaim loop Greg Kurz @ 2021-01-21 12:50 ` Christian Schoenebeck 2021-01-21 16:34 ` Greg Kurz 0 siblings, 1 reply; 13+ messages in thread From: Christian Schoenebeck @ 2021-01-21 12:50 UTC (permalink / raw) To: qemu-devel; +Cc: Greg Kurz On Montag, 18. Januar 2021 15:23:00 CET Greg Kurz wrote: > If a fid was actually re-open by v9fs_reopen_fid(), we re-traverse the "re-opened" > fid list from the head in case some other request created a fid that > needs to be marked unreclaimable as well (ie. the client open a new "i.e." and either "opens" or "opened" > handle on the path that is being unlinked). This is a suboptimal since No "a" here: "This is suboptimal since" > most if not all fids that require it have likely been taken care of > already. > > This is mostly the result of new fids being added to the head of the > list. Since the list is now a QSIMPLEQ, add new fids at the end instead. > Take a reference on the fid to ensure it doesn't go away during > v9fs_reopen_fid() and that it can be safely passed to QSIMPLEQ_NEXT() > afterwards. Since the associated put_fid() can also yield, same is done > with the next fid. So the logic here is to get a reference on a fid and > only put it back during the next iteration after we could get a reference > on the next fid. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/9pfs/9p.c | 44 ++++++++++++++++++++++++++++++-------------- > 1 file changed, 30 insertions(+), 14 deletions(-) > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index b65f320e6518..b0ab5cf61c1f 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -311,7 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t > fid) * reclaim won't close the file descriptor > */ > f->flags |= FID_REFERENCED; > - QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next); > + QSIMPLEQ_INSERT_TAIL(&s->fid_list, f, next); I wondered whether that behaviour change could have negative side effects, but I think the reason why they added it to the head of the list was simply because they only had a head pointer (i.e. they would have needed a loop to insert to tail). So yes, I think that change makes sense now with QSIMPLEQ. > > v9fs_readdir_init(s->proto_version, &f->fs.dir); > v9fs_readdir_init(s->proto_version, &f->fs_reclaim.dir); > @@ -497,32 +497,48 @@ static int coroutine_fn > v9fs_mark_fids_unreclaim(V9fsPDU *pdu, V9fsPath *path) { > int err; > V9fsState *s = pdu->s; > - V9fsFidState *fidp; > + V9fsFidState *fidp, *fidp_next; > > -again: > - QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) { > - if (fidp->path.size != path->size) { > - continue; > - } > - if (!memcmp(fidp->path.data, path->data, path->size)) { > + fidp = QSIMPLEQ_FIRST(&s->fid_list); > + assert(fidp); And fidp is under regular circumstances always non-null here? The assumption is that there is at least the root fid in the list, which the user should not have permission to unlink, right? > + > + /* > + * v9fs_reopen_fid() can yield : a reference on the fid must be held > + * to ensure its pointer remains valid and we can safely pass it to > + * QSIMPLEQ_NEXT(). The corresponding put_fid() can also yield so > + * we must keep a reference on the next fid as well. So the logic here > + * is to get a reference on a fid and only put it back during the next > + * iteration after we could get a reference on the next fid. Start with > + * the first one. > + */ > + for (fidp->ref++; fidp; fidp = fidp_next) { > + if (fidp->path.size == path->size && > + !memcmp(fidp->path.data, path->data, path->size)) { > /* Mark the fid non reclaimable. */ > fidp->flags |= FID_NON_RECLAIMABLE; > > /* reopen the file/dir if already closed */ > err = v9fs_reopen_fid(pdu, fidp); > if (err < 0) { > + put_fid(pdu, fidp); > return err; > } > + } > + > + fidp_next = QSIMPLEQ_NEXT(fidp, next); > + > + if (fidp_next) { > /* > - * Go back to head of fid list because > - * the list could have got updated when > - * switched to the worker thread > + * Ensure the next fid survives a potential clunk request > during + * put_fid() below and v9fs_reopen_fid() in the next > iteration. */ > - if (err == 0) { > - goto again; > - } > + fidp_next->ref++; Mmm, that works as intended if fidp_next matches the requested path. However if it is not (like it would in the majority of cases) then the loop breaks next and the bumped reference count would never be reverted. Or am I missing something here? > } > + > + /* We're done with this fid */ > + put_fid(pdu, fidp); > } > + > return 0; > } Best regards, Christian Schoenebeck ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] 9pfs: Improve unreclaim loop 2021-01-21 12:50 ` Christian Schoenebeck @ 2021-01-21 16:34 ` Greg Kurz 2021-01-21 17:02 ` Christian Schoenebeck 0 siblings, 1 reply; 13+ messages in thread From: Greg Kurz @ 2021-01-21 16:34 UTC (permalink / raw) To: Christian Schoenebeck; +Cc: qemu-devel On Thu, 21 Jan 2021 13:50:37 +0100 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Montag, 18. Januar 2021 15:23:00 CET Greg Kurz wrote: > > If a fid was actually re-open by v9fs_reopen_fid(), we re-traverse the > > "re-opened" > > > fid list from the head in case some other request created a fid that > > needs to be marked unreclaimable as well (ie. the client open a new > > "i.e." and either "opens" or "opened" > > > handle on the path that is being unlinked). This is a suboptimal since > > No "a" here: "This is suboptimal since" > Thanks for the careful reading. I'll fix those :) > > most if not all fids that require it have likely been taken care of > > already. > > > > This is mostly the result of new fids being added to the head of the > > list. Since the list is now a QSIMPLEQ, add new fids at the end instead. > > Take a reference on the fid to ensure it doesn't go away during > > v9fs_reopen_fid() and that it can be safely passed to QSIMPLEQ_NEXT() > > afterwards. Since the associated put_fid() can also yield, same is done > > with the next fid. So the logic here is to get a reference on a fid and > > only put it back during the next iteration after we could get a reference > > on the next fid. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/9pfs/9p.c | 44 ++++++++++++++++++++++++++++++-------------- > > 1 file changed, 30 insertions(+), 14 deletions(-) > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > index b65f320e6518..b0ab5cf61c1f 100644 > > --- a/hw/9pfs/9p.c > > +++ b/hw/9pfs/9p.c > > @@ -311,7 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t > > fid) * reclaim won't close the file descriptor > > */ > > f->flags |= FID_REFERENCED; > > - QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next); > > + QSIMPLEQ_INSERT_TAIL(&s->fid_list, f, next); > > I wondered whether that behaviour change could have negative side effects, but > I think the reason why they added it to the head of the list was simply > because they only had a head pointer (i.e. they would have needed a loop to > insert to tail). > That's my thinking as well. And the open-code fid list was there from the start, while reclaim was only added ~1 year later. Before reclaim, adding to the head was an obvious choice. > So yes, I think that change makes sense now with QSIMPLEQ. > > > > > v9fs_readdir_init(s->proto_version, &f->fs.dir); > > v9fs_readdir_init(s->proto_version, &f->fs_reclaim.dir); > > @@ -497,32 +497,48 @@ static int coroutine_fn > > v9fs_mark_fids_unreclaim(V9fsPDU *pdu, V9fsPath *path) { > > int err; > > V9fsState *s = pdu->s; > > - V9fsFidState *fidp; > > + V9fsFidState *fidp, *fidp_next; > > > > -again: > > - QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) { > > - if (fidp->path.size != path->size) { > > - continue; > > - } > > - if (!memcmp(fidp->path.data, path->data, path->size)) { > > + fidp = QSIMPLEQ_FIRST(&s->fid_list); > > + assert(fidp); > > And fidp is under regular circumstances always non-null here? The assumption > is that there is at least the root fid in the list, which the user should not > have permission to unlink, right? > Oops this is a left-over... The assumption was that the client didn't clunk all fids at the time v9fs_mark_fids_unreclaim() is called. This is true with v9fs_remove() which gets a fid from the list and directly calls v9fs_mark_fids_unreclaim(). This isn't true though for v9fs_unlinkat() which calls v9fs_co_name_to_path() in between, and thus could potentially yield and let the client clunk all fids. Good catch ! > > + > > + /* > > + * v9fs_reopen_fid() can yield : a reference on the fid must be held > > + * to ensure its pointer remains valid and we can safely pass it to > > + * QSIMPLEQ_NEXT(). The corresponding put_fid() can also yield so > > + * we must keep a reference on the next fid as well. So the logic here > > + * is to get a reference on a fid and only put it back during the next > > + * iteration after we could get a reference on the next fid. Start with > > + * the first one. > > + */ > > + for (fidp->ref++; fidp; fidp = fidp_next) { > > + if (fidp->path.size == path->size && > > + !memcmp(fidp->path.data, path->data, path->size)) { > > /* Mark the fid non reclaimable. */ > > fidp->flags |= FID_NON_RECLAIMABLE; > > > > /* reopen the file/dir if already closed */ > > err = v9fs_reopen_fid(pdu, fidp); > > if (err < 0) { > > + put_fid(pdu, fidp); > > return err; > > } > > + } > > + > > + fidp_next = QSIMPLEQ_NEXT(fidp, next); > > + > > + if (fidp_next) { > > /* > > - * Go back to head of fid list because > > - * the list could have got updated when > > - * switched to the worker thread > > + * Ensure the next fid survives a potential clunk request > > during + * put_fid() below and v9fs_reopen_fid() in the next > > iteration. */ > > - if (err == 0) { > > - goto again; > > - } > > + fidp_next->ref++; > > Mmm, that works as intended if fidp_next matches the requested path. However > if it is not (like it would in the majority of cases) then the loop breaks > next and the bumped reference count would never be reverted. Or am I missing > something here? > Each iteration of the loop starts with an already referenced fidp. The loop can only break if: - v9fs_reopen_fid() fails, in which case put_fid(pdu, fidp) is called on the error path above - end of list is reached, in which case the reference on fidp is dropped just like in all previous iterations... > > } > > + > > + /* We're done with this fid */ > > + put_fid(pdu, fidp); ... here. > > } > > + > > return 0; > > } > > Best regards, > Christian Schoenebeck > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] 9pfs: Improve unreclaim loop 2021-01-21 16:34 ` Greg Kurz @ 2021-01-21 17:02 ` Christian Schoenebeck 0 siblings, 0 replies; 13+ messages in thread From: Christian Schoenebeck @ 2021-01-21 17:02 UTC (permalink / raw) To: qemu-devel; +Cc: Greg Kurz On Donnerstag, 21. Januar 2021 17:34:55 CET Greg Kurz wrote: > > > + > > > + /* > > > + * v9fs_reopen_fid() can yield : a reference on the fid must be > > > held > > > + * to ensure its pointer remains valid and we can safely pass it to > > > + * QSIMPLEQ_NEXT(). The corresponding put_fid() can also yield so > > > + * we must keep a reference on the next fid as well. So the logic > > > here > > > + * is to get a reference on a fid and only put it back during the > > > next > > > + * iteration after we could get a reference on the next fid. Start > > > with + * the first one. > > > + */ > > > + for (fidp->ref++; fidp; fidp = fidp_next) { > > > + if (fidp->path.size == path->size && > > > + !memcmp(fidp->path.data, path->data, path->size)) { > > > > > > /* Mark the fid non reclaimable. */ > > > fidp->flags |= FID_NON_RECLAIMABLE; > > > > > > /* reopen the file/dir if already closed */ > > > err = v9fs_reopen_fid(pdu, fidp); > > > if (err < 0) { > > > > > > + put_fid(pdu, fidp); > > > > > > return err; > > > > > > } > > > > > > + } > > > + > > > + fidp_next = QSIMPLEQ_NEXT(fidp, next); > > > + > > > + if (fidp_next) { > > > > > > /* > > > > > > - * Go back to head of fid list because > > > - * the list could have got updated when > > > - * switched to the worker thread > > > + * Ensure the next fid survives a potential clunk request > > > during + * put_fid() below and v9fs_reopen_fid() in the next > > > iteration. */ > > > - if (err == 0) { > > > - goto again; > > > - } > > > + fidp_next->ref++; > > > > Mmm, that works as intended if fidp_next matches the requested path. > > However if it is not (like it would in the majority of cases) then the > > loop breaks next and the bumped reference count would never be reverted. > > Or am I missing something here? > > Each iteration of the loop starts with an already referenced fidp. > > The loop can only break if: > > - v9fs_reopen_fid() fails, in which case put_fid(pdu, fidp) is called > on the error path above > - end of list is reached, in which case the reference on fidp is > dropped just like in all previous iterations... Ah yes, you're right. It's fine! So just the assert(fidp); change then, and the log comment fixes would be nice to have. ;-) That's it. > > > } > > > > > > + > > > + /* We're done with this fid */ > > > + put_fid(pdu, fidp); > > ... here. > > > > } > > > > > > + > > > > > > return 0; > > > > > > } > > > > Best regards, > > Christian Schoenebeck Best regards, Christian Schoenebeck ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] 9pfs: Improve unreclaim logic 2021-01-18 14:22 [PATCH 0/3] 9pfs: Improve unreclaim logic Greg Kurz ` (2 preceding siblings ...) 2021-01-18 14:23 ` [PATCH 3/3] 9pfs: Improve unreclaim loop Greg Kurz @ 2021-01-21 17:05 ` Greg Kurz 3 siblings, 0 replies; 13+ messages in thread From: Greg Kurz @ 2021-01-21 17:05 UTC (permalink / raw) To: qemu-devel; +Cc: Christian Schoenebeck On Mon, 18 Jan 2021 15:22:57 +0100 Greg Kurz <groug@kaod.org> wrote: > clone of "master" > Drat... this text seems to have leaked from stgit and replaced the one I had written in git publish: --- We currently restart the unreclaim loop all over when we detect that the current iteration yielded execution. This is because a new fid might have been added to the head of the list. This is sub-optimal : add new fids to the end of the list instead to avoid that. --- I'll have to sort this out... > Greg Kurz (3): > 9pfs: Convert V9fsFidState::clunked to bool > 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ > 9pfs: Improve unreclaim loop > > hw/9pfs/9p.c | 83 +++++++++++++++++++++++++++++----------------------- > hw/9pfs/9p.h | 6 ++-- > 2 files changed, 50 insertions(+), 39 deletions(-) > I've applied patches 1 and 2 to: https://gitlab.com/gkurz/qemu/-/tree/9p-next ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-01-21 17:15 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-18 14:22 [PATCH 0/3] 9pfs: Improve unreclaim logic Greg Kurz 2021-01-18 14:22 ` [PATCH 1/3] 9pfs: Convert V9fsFidState::clunked to bool Greg Kurz 2021-01-18 14:42 ` Christian Schoenebeck 2021-01-18 14:22 ` [PATCH 2/3] 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ Greg Kurz 2021-01-19 13:31 ` Christian Schoenebeck 2021-01-19 14:34 ` Greg Kurz 2021-01-19 15:28 ` Christian Schoenebeck 2021-01-19 17:23 ` Greg Kurz 2021-01-18 14:23 ` [PATCH 3/3] 9pfs: Improve unreclaim loop Greg Kurz 2021-01-21 12:50 ` Christian Schoenebeck 2021-01-21 16:34 ` Greg Kurz 2021-01-21 17:02 ` Christian Schoenebeck 2021-01-21 17:05 ` [PATCH 0/3] 9pfs: Improve unreclaim logic Greg Kurz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.