Re: [PATCH] libdiskfs: Eliminate malloc from node-cache.c iteration.

[Samuel Thibault]

Milos Nikic, le lun. 30 mars 2026 14:32:47 -0700, a ecrit:
> diff --git a/libdiskfs/node-cache.c b/libdiskfs/node-cache.c
> index 5f6c3e36..891a8893 100644
> --- a/libdiskfs/node-cache.c
> +++ b/libdiskfs/node-cache.c
> @@ -19,7 +19,6 @@
>  
>  #include <hurd/ihash.h>
>  
> -#include "priv.h"
>  #include "diskfs.h"
>  
>  /* The node cache is implemented using a hash table.  Access to the

This seems unrelated?

> @@ -60,6 +59,43 @@ static struct hurd_ihash nodecache =
>    HURD_IHASH_INITIALIZER_GKI (offsetof (struct node, slot), NULL, NULL,
>                                hash, compare);
>  static pthread_rwlock_t nodecache_lock = PTHREAD_RWLOCK_INITIALIZER;
> +static struct node *active_nodes_head = NULL;
> +static struct node *active_nodes_tail = NULL;

I'd rather call it nodecache_list_head/tail? so it's clear it's the same
content as the nodecache hashtable.

> @@ -197,54 +236,46 @@ error_t __attribute__ ((weak))
>  diskfs_node_iterate (error_t (*fun)(struct node *))
>  {
>    error_t err = 0;
> -  size_t num_nodes;
> -  struct node *node, **node_list, **p;
> +  struct node *current, *next_node;
>  
>    pthread_rwlock_rdlock (&nodecache_lock);
> +  current = active_nodes_tail;
>  
> -  /* We must copy everything from the hash table into another data structure
> -     to avoid running into any problems with the hash-table being modified
> -     during processing (normally we delegate access to hash-table with
> -     nodecache_lock, but we can't hold this while locking the
> -     individual node locks).  */
> -  /* XXX: Can we?  */
> -  num_nodes = nodecache.nr_items;
> -
> -  /* TODO This method doesn't scale beyond a few dozen nodes and should be
> -     replaced.  */
> -  node_list = malloc (num_nodes * sizeof (struct node *));
> -  if (node_list == NULL)
> -    {
> -      pthread_rwlock_unlock (&nodecache_lock);
> -      return ENOMEM;
> -    }
> +  /* Bootstrap the loop by grabbing a ref to the very first node */

Actually, the very last. Please also explain in a comment why we
traverse the list from the end.

> +  if (current)
> +    refcounts_ref (&current->refcounts, NULL);
>  
> -  p = node_list;
> -  HURD_IHASH_ITERATE (&nodecache, i)
> +  while (current != NULL)
>      {

Samuel