On thu, 9 Aug 2012 14:04:05 -0400, Chris Mason wrote:
> On Wed, Aug 08, 2012 at 09:10:17PM -0600, Miao Xie wrote:
>> If we create several snapshots at the same time, the following BUG_ON() will be
>> triggered.
>>
>> kernel BUG at fs/btrfs/extent-tree.c:6047!
>>
>> Steps to reproduce:
>> # mkfs.btrfs <partition>
>> # mount <partition> <mnt>
>> # cd <mnt>
>> # for ((i=0;i<2400;i++)); do touch long_name_to_make_tree_more_deep$i; done
>> # for ((i=0; i<4; i++))
>> > do
>> > mkdir $i
>> > for ((j=0; j<200; j++))
>> > do
>> > btrfs sub snap . $i/$j
>> > done &
>> > done
>
> snapshot creation has a critical section. Once we copy a given root to
> its snapshot, we're not allowed to change it until the transaction
> is fully committed.
I knew this critical section. But I think we can kick it away by forcing the
snapshoted tree to do COW.
BTW, I will take a vacation next week, so I can not reply it until the week after next.
If it is not urgent, I will continue looking into this problem after I come back.
Thanks
Miao
>
> This means that if you're taking a snapshot of root A and storing the
> directory item of the snapshot in root A, you can only do it once per
> transaction with getting into trouble.
>
> Looks like the code doesn't enforce this though. Dave, could you please
> give this a try:
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index fcc8c21..9e7c621 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -1324,6 +1324,8 @@ noinline int btrfs_cow_block(struct btrfs_trans_handle *trans,
> u64 search_start;
> int ret;
>
> + WARN_ON(root->danger_transid == trans->transid);
> +
> if (trans->transaction != root->fs_info->running_transaction) {
> printk(KERN_CRIT "trans %llu running %llu\n",
> (unsigned long long)trans->transid,
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 0d195b5..35b5603 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1490,6 +1490,12 @@ struct btrfs_root {
> u64 objectid;
> u64 last_trans;
>
> + /*
> + * the last transaction that took a snapshot of this
> + * root. We're only allowed one snapshot per root per transaction
> + */
> + u64 snapshot_trans;
> +
> /* data allocations are done in sectorsize units */
> u32 sectorsize;
>
> @@ -1550,6 +1556,13 @@ struct btrfs_root {
>
> int force_cow;
>
> + /*
> + * this marks the critical section of snapshot creation. If we
> + * make any changes to a root after this critical section starts,
> + * we corrupt the FS. It is checked by btrfs_cow_block
> + */
> + u64 danger_transid;
> +
> spinlock_t root_times_lock;
> };
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 9df50fa..b20d835 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -524,13 +524,28 @@ static int create_snapshot(struct btrfs_root *root, struct dentry *dentry,
> pending_snapshot->inherit = *inherit;
> *inherit = NULL; /* take responsibility to free it */
> }
> -
> +again:
> trans = btrfs_start_transaction(root->fs_info->extent_root, 5);
> if (IS_ERR(trans)) {
> ret = PTR_ERR(trans);
> goto fail;
> }
>
> + /* we're only allowed to snapshot a given root once per transaction */
> + spin_lock(&root->fs_info->trans_lock);
> + if (root->snapshot_trans == trans->transid) {
> + spin_unlock(&root->fs_info->trans_lock);
> + ret = btrfs_commit_transaction(trans, root->fs_info->extent_root);
> + if (ret)
> + goto fail;
> + goto again;
> + }
> +
> + root->snapshot_trans = trans->transid;
> +
> + spin_unlock(&root->fs_info->trans_lock);
> +
> +
> ret = btrfs_snap_reserve_metadata(trans, pending_snapshot);
> BUG_ON(ret);
>
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 27c2600..4507421 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1093,6 +1093,7 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
>
> /* see comments in should_cow_block() */
> root->force_cow = 1;
> + root->danger_transid = trans->transid;
> smp_wmb();
>
> btrfs_set_root_node(new_root_item, tmp);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
