Re: [RFC PATCH 7/7] Btrfs: introduce BTRFS_IOC_SEND for btrfs send/receive (part 2)

[Alex Lyakas <alex.bolshoy.btrfs@gmail.com>]

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);

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;

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.

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.

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?

Thanks,
Alex.