On Mon, Jul 23, 2012 at 5:17 PM, Alex Lyakas
<alex.bolshoy.bt...@gmail.com> wrote:
> Hi Alexander,
> I did some testing of the case where same inode, but with a different
> generation, exists both in send_root and in parent_root.
> I know that this can happen primarily when "inode_cache" option is
> enabled. So first I just tested some differential sends, where parent
> and root are unrelated subvolumes. Here are some issues:
>
> 1) The top subvolume inode (ino=BTRFS_FIRST_FREE_OBJECTID) is treated
> also as deleted + recreated. So the code goes into process_all_refs()
> path and does several strange things, such as trying to orphanize the
> top inode. Also get_cur_path() always returns "" for the top subvolume
> (without checking whether it is an orphan). Another complication for
> the top inode is that its parent dir is itself.
> I made the following fix:
> @@ -3782,7 +3972,13 @@ static int changed_inode(struct send_ctx *sctx,
>
> right_gen = btrfs_inode_generation(sctx->right_path->nodes[0],
> right_ii);
> - if (left_gen != right_gen)
> + if (left_gen != right_gen && sctx->cur_ino !=
> BTRFS_FIRST_FREE_OBJECTID)
> sctx->cur_inode_new_gen = 1;
>
> So basically, don't try to delete and re-create it, but treat it like
> a change. Since the top subvolume inode is S_IFDIR, and dir can have
> only one hardlink (and hopefully it is always ".."), we will never
> need to change anything for this INODE_REF. I also added:
>
> @@ -2526,6 +2615,14 @@ static int process_recorded_refs(struct send_ctx *sctx)
> int did_overwrite = 0;
> int is_orphan = 0;
>
> + BUG_ON(sctx->cur_ino <= BTRFS_FIRST_FREE_OBJECTID);
I applied both fixes to for-chris now.
>
> 2) After I fixed this, I hit another issue, where inodes under the top
> subvolume dir, attempt to rmdir() the top dir, while iterating over
> check_dirs in process_recorded_refs(), because (top_dir_ino,
> top_dir_gen) indicate that it was deleted. So I added:
>
> @@ -2714,10 +2857,19 @@ verbose_printk("btrfs: process_recorded_refs
> %llu\n", sctx->cur_ino);
> */
> ULIST_ITER_INIT(&uit);
> while ((un = ulist_next(check_dirs, &uit))) {
> + /* Do not attempt to rmdir it the top subvolume dir */
> + if (un->val == BTRFS_FIRST_FREE_OBJECTID)
> + continue;
> +
> if (un->val > sctx->cur_ino)
> continue;
I applied a similar fix by adding a check to can_rmdir. The way you
suggested would also skip utime updates for the top dir.
>
> 3) process_recorded_refs() always increments the send_progress:
> /*
> * Current inode is now at it's new position, so we must increase
> * send_progress
> */
> sctx->send_progress = sctx->cur_ino + 1;
>
> However, in the changed_inode() path I am testing, process_all_refs()
> is called twice with the same inode (once for deleted inode, once for
> the recreated inode), so after the first call, send_progress is
> incremented and doesn't match the inode anymore. I don't think I hit
> any issues because of this, just that it's confusing.
I fixed this issue some days ago.
>
> 4)
>
>> +/*
>> + * Record and process all refs at once. Needed when an inode changes the
>> + * generation number, which means that it was deleted and recreated.
>> + */
>> +static int process_all_refs(struct send_ctx *sctx,
>> + enum btrfs_compare_tree_result cmd)
>> +{
>> + int ret;
>> + struct btrfs_root *root;
>> + struct btrfs_path *path;
>> + struct btrfs_key key;
>> + struct btrfs_key found_key;
>> + struct extent_buffer *eb;
>> + int slot;
>> + iterate_inode_ref_t cb;
>> +
>> + path = alloc_path_for_send();
>> + if (!path)
>> + return -ENOMEM;
>> +
>> + if (cmd == BTRFS_COMPARE_TREE_NEW) {
>> + root = sctx->send_root;
>> + cb = __record_new_ref;
>> + } else if (cmd == BTRFS_COMPARE_TREE_DELETED) {
>> + root = sctx->parent_root;
>> + cb = __record_deleted_ref;
>> + } else {
>> + BUG();
>> + }
>> +
>> + key.objectid = sctx->cmp_key->objectid;
>> + key.type = BTRFS_INODE_REF_KEY;
>> + key.offset = 0;
>> + while (1) {
>> + ret = btrfs_search_slot_for_read(root, &key, path, 1, 0);
>> + if (ret < 0) {
>> + btrfs_release_path(path);
>> + goto out;
>> + }
>> + if (ret) {
>> + btrfs_release_path(path);
>> + break;
>> + }
>> +
>> + eb = path->nodes[0];
>> + slot = path->slots[0];
>> + btrfs_item_key_to_cpu(eb, &found_key, slot);
>> +
>> + if (found_key.objectid != key.objectid ||
>> + found_key.type != key.type) {
>> + btrfs_release_path(path);
>> + break;
>> + }
>> +
>> + ret = iterate_inode_ref(sctx, sctx->parent_root, path,
>> + &found_key, 0, cb, sctx);
>
> Shouldn't it be the root that you calculated eariler and not
> sctx->parent_root? I guess in this case it doesn't matter, because
> "resolve" is 0, and the passed root is only used for resolve. But
> still confusing.
You're right, atm it does not matter which root we use here. It is
more correct to pass 'root' instead of parent_root.
>
> 5) When I started testing with "inode_cache" enabled, I hit another
> issue. When this mount option is enabled, then FREE_INO and FREE_SPACE
> items now appear in the file tree. As a result, the code tries to
> create the FREE_INO item with an orphan name, then tries to find its
> INODE_REF, but fails because it has no INODE_REFs. So
>
> @@ -3923,6 +4127,13 @@ static int changed_cb(struct btrfs_root *left_root,
> int ret = 0;
> struct send_ctx *sctx = ctx;
>
> + /* Ignore non-FS objects */
> + if (key->objectid == BTRFS_FREE_INO_OBJECTID ||
> + key->objectid == BTRFS_FREE_SPACE_OBJECTID)
> + return 0;
>
> makes sense?
Yepp. I however added it after the finish_inode_if_needed call. The
call is still required to finish the previous inode.
>
> Thanks,
> Alex.
Thanks again. Pushed to for-chris.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
