Re: Btrfs: fix locking in btrfs_destroy_delayed_refs

On Mon, Jun 18, 2012 at 04:12:39PM +0300, Dan Carpenter wrote:

Hello Josef Bacik,

The patch b939d1ab76b4: "Btrfs: fix locking in
btrfs_destroy_delayed_refs" from May 31, 2012, leads to the following
warning: Btrfs: fix locking in btrfs_destroy_delayed_refs

fs/btrfs/disk-io.c
  3412          while ((node = rb_first(&delayed_refs->root)) != NULL) {
  3413                  ref = rb_entry(node, struct btrfs_delayed_ref_node, rb_node);
  3414  
  3415                  atomic_set(&ref->refs, 1);
  3416                  if (btrfs_delayed_ref_is_head(ref)) {
  3417                          struct btrfs_delayed_ref_head *head;
  3418  
  3419                          head = btrfs_delayed_node_to_head(ref);
  3420                          if (!mutex_trylock(&head->mutex)) {
  3421                                  atomic_inc(&ref->refs);
  3422                                  spin_unlock(&delayed_refs->lock);
  3423  
  3424                                  /* Need to wait for the delayed ref to run */
  3425                                  mutex_lock(&head->mutex);
  3426                                  mutex_unlock(&head->mutex);
  3427                                  btrfs_put_delayed_ref(ref);
  3428  
  3429                                  continue;
                                        ^^^^^^^^^
We're not holding the &delayed_refs->lock here.

  3430                          }
  3431  
  3432                          kfree(head->extent_op);
  3433                          delayed_refs->num_heads--;
  3434                          if (list_empty(&head->cluster))
  3435                                  delayed_refs->num_heads_ready--;
  3436                          list_del_init(&head->cluster);
  3437                  }
  3438                  ref->in_tree = 0;
  3439                  rb_erase(&ref->rb_node, &delayed_refs->root);
  3440                  delayed_refs->num_entries--;
  3441  
  3442                  spin_unlock(&delayed_refs->lock);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

So this is a double unlock.

  3443                  btrfs_put_delayed_ref(ref);
  3444  
  3445                  cond_resched();
  3446                  spin_lock(&delayed_refs->lock);
  3447          }
  3448  
  3449          spin_unlock(&delayed_refs->lock);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Or if we exit, then this is a double unlock.

There is some complicated locking going on in that function so I don't
pretend to understand it.  Sorry, if I've misread something.

Ooops, nope you are right, I'll fix it.  Thanks,

Josef