Re: [PATCH 3/6] ext2fs: use a seperate lock to protect nodehash

[Samuel Thibault]

Justus Winter, le Sun 01 Jun 2014 22:03:01 +0200, a écrit :
> @@ -46,8 +46,18 @@
>  #define      INOHASH(ino)    (((unsigned)(ino))%INOHSZ)
>  #endif
>  
> +/* The nodehash is a cache of nodes.
> +
> +   Access to nodehash and nodehash_nr_items is protected by
> +   nodecache_lock.
> +
> +   Every node in the nodehash carries a light reference.  When we are
> +   asked to give up that light reference, we reacquire our lock
> +   momentarily to check whether someone else reacquired a reference
> +   through the nodehash.  */
>  static struct node *nodehash[INOHSZ];
>  static size_t nodehash_nr_items;
> +static pthread_rwlock_t nodecache_lock = PTHREAD_RWLOCK_INITIALIZER;

Please also document that this is to be taken before
diskfs_node_refcnt_lock.

> @@ -71,24 +81,22 @@ diskfs_cached_lookup (ino_t inum, struct node **npp)
>    struct node *np;
>    struct disknode *dn;
>  
> -  pthread_spin_lock (&diskfs_node_refcnt_lock);
> +  pthread_rwlock_rdlock (&nodecache_lock);
>    for (np = nodehash[INOHASH(inum)]; np; np = np->dn->hnext)
>      if (np->cache_id == inum)
>        {
> -     np->references++;
> -     pthread_spin_unlock (&diskfs_node_refcnt_lock);
> +     diskfs_nref (np);
> +     pthread_rwlock_unlock (&nodecache_lock);
>       pthread_mutex_lock (&np->lock);
>       *npp = np;
>       return 0;
>        }
> +  pthread_rwlock_unlock (&nodecache_lock);

But what if two threads call diskfs_cached_lookup for the same node at
the same time?  They'll both think it's not in the hash yet, and will
both create the node and add it to the hash. So...

>    /* Format specific data for the new node.  */
>    dn = malloc (sizeof (struct disknode));
>    if (! dn)
> -    {
> -      pthread_spin_unlock (&diskfs_node_refcnt_lock);
> -      return ENOMEM;
> -    }
> +    return ENOMEM;
>    dn->dirents = 0;
>    dn->dir_idx = 0;
>    dn->pager = 0;
> @@ -102,14 +110,15 @@ diskfs_cached_lookup (ino_t inum, struct node **npp)
>    pthread_mutex_lock (&np->lock);
>  
>    /* Put NP in NODEHASH.  */
> +  pthread_rwlock_wrlock (&nodecache_lock);

... after taking the pthread_rwlock_wrlock again in wr mode, you need to
check for the existence of the node again.

>    dn->hnext = nodehash[INOHASH(inum)];
>    if (dn->hnext)
>      dn->hnext->dn->hprevp = &dn->hnext;
>    dn->hprevp = &nodehash[INOHASH(inum)];
>    nodehash[INOHASH(inum)] = np;
> +  diskfs_nref_light (np);
>    nodehash_nr_items += 1;
> -
> -  pthread_spin_unlock (&diskfs_node_refcnt_lock);
> +  pthread_rwlock_unlock (&nodecache_lock);
>  
>    /* Get the contents of NP off disk.  */
>    err = read_node (np);

Samuel