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.
--
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
