(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 <miaox@xxxxxxxxxxxxxx>
> ---
(snip)
> @@ -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.
> 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
--
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
