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