Re: [PATCH 2/2] Btrfs: fix the snapshot that should not exist
From: Hidetoshi Seto <hidden>
Date: 2012-07-27 07:52:58
(2012/07/26 15:57), Miao Xie wrote:
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. It introduces some problem and makes the users feel strange:
# 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 Ju1 24 12:11 1
^^^
# ll /mnt/1/snap0/
total 0
drwxr-xr-x 1 root root 10 Ju1 24 12:11 1
^^^
It is also 10, but...
# ll /mnt/1/snap0/1
total 0
[None]
# cd /mnt/1/snap0/1/snap0
[Enter a unexisted directory successfully...]I confirmed that "mkdir snap0" failed with "File exists" and that rmdir can remove the directory snap0. So it is a kind of "invisible" rather than "unexisted".
There is nothing in the directory 1 in snap0, but btrfs told the length of this directory is 10. Beside that, we can enter an unexisted directory, it is very strange to the users. # btrfs subvolume snapshot /mnt/1/snap0 /mnt/snap1 # ll /mnt/1/snap0/1/ total 0 [None] # ll /mnt/snap1/1/ total 0 drwxr-xr-x 1 root root 0 Ju1 24 12:14 snap0 And the source of snap1 did have any directory in Directory 1, but snap1 have a snap0, it is different between the source and the snapshot. So I think we should insert directory item and directory name index and update the parent inode as the last step of snapshot creation, and do not leave the useless metadata in the tree. Signed-off-by: Miao Xie <redacted> ---
(snip)
quoted hunk ↗ jump to hunk
@@ -1062,17 +1068,33 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, ret = btrfs_reloc_post_snapshot(trans, pending); if (ret) goto abort_trans; + + ret = btrfs_insert_dir_item(trans, parent_root, + dentry->d_name.name, dentry->d_name.len, + parent_inode, &key, + BTRFS_FT_DIR, index); + /* We have check the name at the beginning, so it is impossible. */ + BUG_ON(ret == -EEXIST); + if (ret) + goto abort_trans; + + btrfs_i_size_write(parent_inode, parent_inode->i_size + + dentry->d_name.len * 2); + ret = btrfs_update_inode(trans, parent_root, parent_inode); + if (ret) + goto abort_trans; fail: dput(parent); trans->block_rsv = rsv; no_free_objectid: kfree(new_root_item); root_item_alloc_fail: + btrfs_free_path(path); +path_alloc_fail: btrfs_block_rsv_release(root, &pending->block_rsv, (u64)-1); return ret; abort_trans: - dput(parent);
I think you can remove this line in your 1/2 patch.
quoted hunk ↗ jump to hunk
btrfs_abort_transaction(trans, root, ret); goto fail; }@@ -1386,13 +1408,13 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans, */ mutex_lock(&root->fs_info->reloc_mutex); - ret = btrfs_run_delayed_items(trans, root); + ret = create_pending_snapshots(trans, root->fs_info); if (ret) { mutex_unlock(&root->fs_info->reloc_mutex); goto cleanup_transaction; } - ret = create_pending_snapshots(trans, root->fs_info); + ret = btrfs_run_delayed_items(trans, root); if (ret) { mutex_unlock(&root->fs_info->reloc_mutex); goto cleanup_transaction;
It would be nice to have a patch description to tell why you have to change the order here. Thanks, H.Seto