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

[Justus Winter]

Quoting Samuel Thibault (2014-06-02 03:41:55)
> 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.

Actually, the whole point of this excercise is to get rid of
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.

Good point...

Justus

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