Re: [PATCH] 9pfs: use GHashMap for fid table

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Montag, 5. September 2022 10:51:10 CEST Linus Heckemann wrote:
> Hi all, thanks for your reviews.
> 
> > @@ -4226,7 +4232,7 @@ int v9fs_device_realize_common(V9fsState *s, const
> > V9fsTransport *t,> 
> >      s->ctx.fmode = fse->fmode;
> >      s->ctx.dmode = fse->dmode;
> > 
> > -    QSIMPLEQ_INIT(&s->fid_list);
> > +    s->fids = g_hash_table_new(NULL, NULL);
> > 
> >      qemu_co_rwlock_init(&s->rename_lock);
> >      
> >      if (s->ops->init(&s->ctx, errp) < 0) {
> 
> I noticed that the hash table may be leaked as is. I'll address this in
> the next submission.
> 
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> > [Style nitpicking]
> 
> Applied these changes and will include them in the next version of the
> patch.
> Christian Schoenebeck <qemu_oss@crudebyte.com> writes:
> > > @@ -317,12 +315,9 @@ static V9fsFidState *alloc_fid(V9fsState *s,
> > > int32_t
> > > fid) {
> > > 
> > >      V9fsFidState *f;
> > > 
> > > -    QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
> > > +    if (g_hash_table_contains(s->fids, GINT_TO_POINTER(fid))) {
> > > 
> > >          /* If fid is already there return NULL */
> > > 
> > > -        BUG_ON(f->clunked);
> > > -        if (f->fid == fid) {
> > > -            return NULL;
> > > -        }
> > > +        return NULL;
> > 
> > Probably retaining BUG_ON(f->clunked) here?
> 
> I decided not to since this was a sanity check that was happening for
> _each_ fid, but only up to the one we were looking for. This seemed
> inconsistent and awkward to me, so I dropped it completely (and the
> invariant that no clunked fids remain in the table still seems to hold
> -- it's fairly trivial to check, in that the clunked flag is only set
> in two places, both of which also remove the map entry). My preference
> would be to leave it out, but I'd also be fine with restoring it for
> just the one we're looking for, or maybe moving the check to when we're
> iterating over the whole table, e.g. in v9fs_reclaim_fd. Thoughts?

Yeah, I think you are right, it would feel odd. Just drop BUG_ON() for now.

> > > @@ -424,12 +419,11 @@ static V9fsFidState *clunk_fid(V9fsState *s,
> > > int32_t
> > > fid) {
> > > 
> > >      V9fsFidState *fidp;
> > > 
> > > -    QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) {
> > > -        if (fidp->fid == fid) {
> > > -            QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next);
> > > -            fidp->clunked = true;
> > > -            return fidp;
> > > -        }
> > > +    fidp = g_hash_table_lookup(s->fids, GINT_TO_POINTER(fid));
> > > +    if (fidp) {
> > > +        g_hash_table_remove(s->fids, GINT_TO_POINTER(fid));
> > > +        fidp->clunked = true;
> > > +        return fidp;
> > 
> > We can't get rid of the double lookup here, can we? Surprisingly I don't
> > find a lookup function on the iterator based API.
> 
> It seems you're not the only one who had that idea:
> https://gitlab.gnome.org/GNOME/glib/-/issues/613
> 
> In this case, I think an extended remove function which returns the
> values that were present would be even nicer. But neither exists at this
> time (and that issue is pretty old), I guess we're stuck with this for
> now.

Well, all we would need was such a proposed 
g_hash_table_lookup_iter(table,key,iter) function. I just had a quick look at 
ghash.c and it looks like that would actually be straightforward to add as the 
iterator structure takes the same direct array index as the already existing 
g_hash_table_lookup() function. But anyway, that's the current situation, so 
be it.

> Daniel P. Berrangé writes:
> > In $SUBJECT it is called GHashTable, not GHashMap
> 
> Indeed, good catch. Will fix in the next version.
> 
> Linus