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

[Greg Kurz <groug@kaod.org>]

Hi Linus,

Thanks for this promising change !

On Mon, 05 Sep 2022 10:51:10 +0200
Linus Heckemann <git@sphalerite.org> 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.
> 

Pay attention that this rollback should be added to
v9fs_device_unrealize_common() which is assumed to be
idempotent and is already called on the error path of
v9fs_device_realize_common().

> 
> 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?
> 

Well... finding at least one clunked fid state in this table is
definitely a bug so I'll keep the BUG_ON() anyway.

> > > @@ -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.

g_hash_table_steal_extended() [1] actually allows to do just that. Since the
hash table is allocated with g_hash_table_new() and doesn't care for destroy
functions, the code change would be something like:

@@ -424,12 +419,10 @@ 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;
-        }
+    if (g_hash_table_steal_extended(s->fids, GINT_TO_POINTER(fid), NULL,
+                                    (gpointer *)&fidp)) {
+        fidp->clunked = true;
+        return fidp;
     }
     return NULL;
 }

[1] https://developer-old.gnome.org/glib/stable/glib-Hash-Tables.html#g-hash-table-steal-extended

Cheers,

--
Greg

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