[V5] Btrfs: snapshot-aware defrag

Commit Message

Mitch Harder Jan. 28, 2013, 5:20 a.m. UTC

On Sun, Jan 27, 2013 at 6:41 AM, Liu Bo <[e-mail]> wrote:
>
> Hi Mitch,
>
> Many thanks for testing it!
>
> Well, after some debugging, I finally figure out the whys:
>
> (1) btrfs_ioctl_snap_destroy() will free the inode of snapshot and set
> root's refs to zero(btrfs_set_root_refs()), if this inode happens to
> be the only one in the rbtree of the snapshot's root at this moment,
> we add this root to the dead_root list.
>
> (2) Unfortunately, after (1), our snapshot-aware defrag work may read
> another inode in this snapshot into memory during 'relink' stage, and
> later after we finish relink work and iput() will force us to add the
> snapshot's root to the dead_root list again.
>
> So that's why we get double list_add and list_del corruption.
>
> And IMO, it can also take place without snapshot-aware defrag, but it's a
> rare case.

I'm seeing a smattering of reports that resemble list corruption on
the M/L, so that is possible.

>
> So could you please try this?
>
> thanks,
> liubo
>
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index f154946..d4ee66b 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -885,7 +885,15 @@ static noinline int commit_cowonly_roots(struct btrfs_trans_handle *trans,
>  int btrfs_add_dead_root(struct btrfs_root *root)
>  {
>         spin_lock(&root->fs_info->trans_lock);
> +       if (!list_empty(&root->root_list)) {
> +               struct btrfs_root *tmp;
> +               list_for_each_entry(tmp, &root->fs_info->dead_roots, root_list)
> +                       if (tmp == root)
> +                               goto unlock;
> +       }
> +
>         list_add(&root->root_list, &root->fs_info->dead_roots);
> +unlock:
>         spin_unlock(&root->fs_info->trans_lock);
>         return 0;
>  }
>

It feels like we're correcting the problem after-the-fact with this
method, instead of addressing the root problem.  But I was able to
successfully run with this patch.

I slightly modified your patch as follows by introducing a WARN_ON in
order to get a back trace, and also to give me a positive confirmation
that I was triggering the problem.

 }

Patch

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index d6b17fa..0c1066e 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -885,7 +885,18 @@  static noinline int commit_cowonly_roots(struct
btrfs_trans_handle *trans,
 int btrfs_add_dead_root(struct btrfs_root *root)
 {
 	spin_lock(&root->fs_info->trans_lock);
+	if (!list_empty(&root->root_list)) {
+		struct btrfs_root *tmp;
+		list_for_each_entry(tmp, &root->fs_info->dead_roots, root_list)
+			if (tmp == root) {
+				printk(KERN_ERR "btrfs: Duplicate dead root entry.\n");
+				WARN_ON(1);
+				goto unlock;
+			}
+	}
+
 	list_add(&root->root_list, &root->fs_info->dead_roots);
+unlock:
 	spin_unlock(&root->fs_info->trans_lock);
 	return 0;