On thu, 30 Jun 2011 16:03:21 +0800, Miao Xie wrote:
> Hi, Chris
>
> I think the snapshot should be the image of the fs tree before it was created,
> so the metadata of the snapshot should not exist in the its tree. But now, we
> found the directory item and directory name index is in both the snapshot tree
> and the fs tree.
>
> Besides that, it also makes the users feel strange. For example:
> # mkfs.btrfs /dev/sda1
> # mount /dev/sda1 /mnt
> # mkdir /mnt/1
> # cd /mnt/1
> # btrfs subvolume snapshot /mnt snap0
> # ll /mnt/1
> total 0
> drwxr-xr-x 1 root root 10 Jun 30 15:01 1
> ^^^
> # ll /mnt/1/snap0/
> total 0
> drwxr-xr-x 1 root root 10 Jun 30 15:01 1
> ^^^
> It is also 10, but...
> # ll /mnt/1/snap0/1
> total 0
> [None]
If we do
# touch /mnt/1/snap0/1/snap0/a
WARN_ON_ONCE(in d_set_d_op(), fs/dcache.c:1345) will be triggered.
If we insert directory item and directory name index and update the parent inode
as the last step, this warning will not happen.
Thanks
Miao
> There is nothing in the directory 1 in snap0, but btrfs told the length of
> this directory is 10, it is strange.
>
> So I think we should insert directory item and directory name index and update
> the parent inode as the last step of snapshot creation.
>
> Thanks
> Miao
>
> On Fri, 17 Jun 2011 17:12:28 -0400, Chris Mason wrote:
>> commit e999376f094162aa425ae749aa1df95ab928d010
>> Author: Chris Mason <chris.mason@xxxxxxxxxx>
>> Date: Fri Jun 17 16:14:09 2011 -0400
>>
>> Btrfs: avoid delayed metadata items during commits
>>
>> Snapshot creation has two phases. One is the initial snapshot setup,
>> and the second is done during commit, while nobody is allowed to modify
>> the root we are snapshotting.
>>
>> The delayed metadata insertion code can break that rule, it does a
>> delayed inode update on the inode of the parent of the snapshot,
>> and delayed directory item insertion.
>>
>> This makes sure to run the pending delayed operations before we
>> record the snapshot root, which avoids corruptions.
>>
>> Signed-off-by: Chris Mason <chris.mason@xxxxxxxxxx>
>>
>> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
>> index fc515b7..f1cbd02 100644
>> --- a/fs/btrfs/delayed-inode.c
>> +++ b/fs/btrfs/delayed-inode.c
>> @@ -1237,6 +1237,13 @@ again:
>> return 0;
>> }
>>
>> +void btrfs_assert_delayed_root_empty(struct btrfs_root *root)
>> +{
>> + struct btrfs_delayed_root *delayed_root;
>> + delayed_root = btrfs_get_delayed_root(root);
>> + WARN_ON(btrfs_first_delayed_node(delayed_root));
>> +}
>> +
>> void btrfs_balance_delayed_items(struct btrfs_root *root)
>> {
>> struct btrfs_delayed_root *delayed_root;
>> diff --git a/fs/btrfs/delayed-inode.h b/fs/btrfs/delayed-inode.h
>> index cb79b67..d1a6a29 100644
>> --- a/fs/btrfs/delayed-inode.h
>> +++ b/fs/btrfs/delayed-inode.h
>> @@ -137,4 +137,8 @@ int btrfs_readdir_delayed_dir_index(struct file *filp, void *dirent,
>> /* for init */
>> int __init btrfs_delayed_inode_init(void);
>> void btrfs_delayed_inode_exit(void);
>> +
>> +/* for debugging */
>> +void btrfs_assert_delayed_root_empty(struct btrfs_root *root);
>> +
>> #endif
>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
>> index c073d85..51dcec8 100644
>> --- a/fs/btrfs/transaction.c
>> +++ b/fs/btrfs/transaction.c
>> @@ -957,6 +957,15 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
>> ret = btrfs_update_inode(trans, parent_root, parent_inode);
>> BUG_ON(ret);
>>
>> + /*
>> + * pull in the delayed directory update
>> + * and the delayed inode item
>> + * otherwise we corrupt the FS during
>> + * snapshot
>> + */
>> + ret = btrfs_run_delayed_items(trans, root);
>> + BUG_ON(ret);
>> +
>> record_root_in_trans(trans, root);
>> btrfs_set_root_last_snapshot(&root->root_item, trans->transid);
>> memcpy(new_root_item, &root->root_item, sizeof(*new_root_item));
>> @@ -1018,14 +1027,6 @@ static noinline int create_pending_snapshots(struct btrfs_trans_handle *trans,
>> int ret;
>>
>> list_for_each_entry(pending, head, list) {
>> - /*
>> - * We must deal with the delayed items before creating
>> - * snapshots, or we will create a snapthot with inconsistent
>> - * information.
>> - */
>> - ret = btrfs_run_delayed_items(trans, fs_info->fs_root);
>> - BUG_ON(ret);
>> -
>> ret = create_pending_snapshot(trans, fs_info, pending);
>> BUG_ON(ret);
>> }
>> @@ -1319,15 +1320,21 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
>> */
>> mutex_lock(&root->fs_info->reloc_mutex);
>>
>> - ret = create_pending_snapshots(trans, root->fs_info);
>> + ret = btrfs_run_delayed_items(trans, root);
>> BUG_ON(ret);
>>
>> - ret = btrfs_run_delayed_items(trans, root);
>> + ret = create_pending_snapshots(trans, root->fs_info);
>> BUG_ON(ret);
>>
>> ret = btrfs_run_delayed_refs(trans, root, (unsigned long)-1);
>> BUG_ON(ret);
>>
>> + /*
>> + * make sure none of the code above managed to slip in a
>> + * delayed item
>> + */
>> + btrfs_assert_delayed_root_empty(root);
>> +
>> WARN_ON(cur_trans != trans->transaction);
>>
>> btrfs_scrub_pause(root);
>>
>
> --
> 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
