| Message ID | CAOcd+r2VUEjti7uNht-vaa603ZHE5XDXUdW1zODhrfO4iPMxUA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 17 Oct 2012 18:06:52 +0200, Alex Lyakas wrote: > -static int __list_subvol_fill_paths(int fd, struct root_lookup *root_lookup) > +static int __list_subvol_fill_paths(int fd, struct root_lookup *root_lookup, > + struct root_lookup *root_lookup_final) > { > struct rb_node *n; > > + root_lookup_init(root_lookup_final); > + > n = rb_first(&root_lookup->root); > while (n) { > struct root_info *entry; > int ret; > entry = rb_entry(n, struct root_info, rb_node); > ret = lookup_ino_path(fd, entry); > - if(ret < 0) > + if(ret < 0 && ret != -ENOENT) > return ret; > - n = rb_next(n); > + rb_erase(&entry->rb_node, &root_lookup->root); > + /* > + * If lookup_ino_path() returned ENOENT, some of the path > + * components are missing. Let's consider this subvolume > + * as deleted then. > + */ > + if (ret == -ENOENT) > + __free_root_info(entry); > + else > + root_tree_insert(root_lookup_final, entry); > + n = rb_first(&root_lookup->root); I don't think we need a final rb-tree since we have removed the deleted root entries from the tree and freed them. Beside that, this patch didn't fix the problem completely. Consider the following case: Subv0 |-> Subv1 |-> Subv2 When we fill the path, we may deal with Subv2 firstly, and then deal with Subv1. But if someone deletes Subv2 and Subv1 after we fill the path of Subv2 and before we fill the path of Subv1, we may deal with Subv2 successfully, then we will find Subv1 has been deleted, so we will clean up Subv2 entry. If so, we will fail to resolve the full path of Subv2 because we can not find Subv1. My partner made a patch which can fix the above problem, I will send it out later. Thanks Miao > } > > return 0; > @@ -1446,6 +1472,7 @@ int btrfs_list_subvols(int fd, struct > btrfs_list_filter_set *filter_set, > int is_tab_result) > { > struct root_lookup root_lookup; > + struct root_lookup root_lookup_final; > struct root_lookup root_sort; > int ret; > > @@ -1460,15 +1487,15 @@ int btrfs_list_subvols(int fd, struct > btrfs_list_filter_set *filter_set, > * now we have an rbtree full of root_info objects, but we need to fill > * in their path names within the subvol that is referencing each one. > */ > - ret = __list_subvol_fill_paths(fd, &root_lookup); > + ret = __list_subvol_fill_paths(fd, &root_lookup, &root_lookup_final); > if (ret < 0) > return ret; > > - __filter_and_sort_subvol(&root_lookup, &root_sort, filter_set, > + __filter_and_sort_subvol(&root_lookup_final, &root_sort, filter_set, > comp_set, fd); > > print_all_volume_info(&root_sort, is_tab_result); > - __free_all_subvolumn(&root_lookup); > + __free_all_subvolumn(&root_lookup_final); > return ret; > } > > @@ -1655,6 +1682,7 @@ int btrfs_list_find_updated_files(int fd, u64 > root_id, u64 oldest_gen) > char *btrfs_list_path_for_root(int fd, u64 root) > { > struct root_lookup root_lookup; > + struct root_lookup root_lookup_final; > struct rb_node *n; > char *ret_path = NULL; > int ret; > @@ -1664,16 +1692,16 @@ char *btrfs_list_path_for_root(int fd, u64 root) > if (ret < 0) > return ERR_PTR(ret); > > - ret = __list_subvol_fill_paths(fd, &root_lookup); > + ret = __list_subvol_fill_paths(fd, &root_lookup, &root_lookup_final); > if (ret < 0) > return ERR_PTR(ret); > > - n = rb_last(&root_lookup.root); > + n = rb_last(&root_lookup_final.root); > while (n) { > struct root_info *entry; > > entry = rb_entry(n, struct root_info, rb_node); > - resolve_root(&root_lookup, entry, top_id); > + resolve_root(&root_lookup_final, entry, top_id); > if (entry->root_id == root) { > ret_path = entry->full_path; > entry->full_path = NULL; > @@ -1681,7 +1709,7 @@ char *btrfs_list_path_for_root(int fd, u64 root) > > n = rb_prev(n); > } > - __free_all_subvolumn(&root_lookup); > + __free_all_subvolumn(&root_lookup_final); > > return ret_path; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Miao, On Thu, Oct 18, 2012 at 4:00 AM, Miao Xie <miaox@cn.fujitsu.com> wrote: > On Wed, 17 Oct 2012 18:06:52 +0200, Alex Lyakas wrote: >> -static int __list_subvol_fill_paths(int fd, struct root_lookup *root_lookup) >> +static int __list_subvol_fill_paths(int fd, struct root_lookup *root_lookup, >> + struct root_lookup *root_lookup_final) >> { >> struct rb_node *n; >> >> + root_lookup_init(root_lookup_final); >> + >> n = rb_first(&root_lookup->root); >> while (n) { >> struct root_info *entry; >> int ret; >> entry = rb_entry(n, struct root_info, rb_node); >> ret = lookup_ino_path(fd, entry); >> - if(ret < 0) >> + if(ret < 0 && ret != -ENOENT) >> return ret; >> - n = rb_next(n); >> + rb_erase(&entry->rb_node, &root_lookup->root); >> + /* >> + * If lookup_ino_path() returned ENOENT, some of the path >> + * components are missing. Let's consider this subvolume >> + * as deleted then. >> + */ >> + if (ret == -ENOENT) >> + __free_root_info(entry); >> + else >> + root_tree_insert(root_lookup_final, entry); >> + n = rb_first(&root_lookup->root); > > I don't think we need a final rb-tree since we have removed the deleted root entries from > the tree and freed them. I just did not want to start the search from the beginning of the same tree (although it will be cheaper, because lookup_ino_path has a shortcut). > > Beside that, this patch didn't fix the problem completely. Consider the following case: > Subv0 > |-> Subv1 > |-> Subv2 > > When we fill the path, we may deal with Subv2 firstly, and then deal with Subv1. But > if someone deletes Subv2 and Subv1 after we fill the path of Subv2 and before we fill > the path of Subv1, we may deal with Subv2 successfully, then we will find Subv1 has been > deleted, so we will clean up Subv2 entry. I don't see where we will clean the Subv2 entry? Once we have filled Subv2's path successfully in lookup_ino_path(), we consider it as existing and won't check ever again, I think. If so, we will fail to resolve the full path of > Subv2 because we can not find Subv1. Note that in your particular example, I think we will never fail to fill the paths (even if all three subvolumes are deleted), because of the following code in btrfs_search_path_in_tree(): if (dirid == BTRFS_FIRST_FREE_OBJECTID) { name[0]='\0'; return 0; } You need to have a regular subdirectory in-between the two roots for btrfs_search_path_in_tree() to really check the path. I noted that in the commit message. > > My partner made a patch which can fix the above problem, I will send it out later. Perfect! Thanks, Alex. > > Thanks > Miao > >> } >> >> return 0; >> @@ -1446,6 +1472,7 @@ int btrfs_list_subvols(int fd, struct >> btrfs_list_filter_set *filter_set, >> int is_tab_result) >> { >> struct root_lookup root_lookup; >> + struct root_lookup root_lookup_final; >> struct root_lookup root_sort; >> int ret; >> >> @@ -1460,15 +1487,15 @@ int btrfs_list_subvols(int fd, struct >> btrfs_list_filter_set *filter_set, >> * now we have an rbtree full of root_info objects, but we need to fill >> * in their path names within the subvol that is referencing each one. >> */ >> - ret = __list_subvol_fill_paths(fd, &root_lookup); >> + ret = __list_subvol_fill_paths(fd, &root_lookup, &root_lookup_final); >> if (ret < 0) >> return ret; >> >> - __filter_and_sort_subvol(&root_lookup, &root_sort, filter_set, >> + __filter_and_sort_subvol(&root_lookup_final, &root_sort, filter_set, >> comp_set, fd); >> >> print_all_volume_info(&root_sort, is_tab_result); >> - __free_all_subvolumn(&root_lookup); >> + __free_all_subvolumn(&root_lookup_final); >> return ret; >> } >> >> @@ -1655,6 +1682,7 @@ int btrfs_list_find_updated_files(int fd, u64 >> root_id, u64 oldest_gen) >> char *btrfs_list_path_for_root(int fd, u64 root) >> { >> struct root_lookup root_lookup; >> + struct root_lookup root_lookup_final; >> struct rb_node *n; >> char *ret_path = NULL; >> int ret; >> @@ -1664,16 +1692,16 @@ char *btrfs_list_path_for_root(int fd, u64 root) >> if (ret < 0) >> return ERR_PTR(ret); >> >> - ret = __list_subvol_fill_paths(fd, &root_lookup); >> + ret = __list_subvol_fill_paths(fd, &root_lookup, &root_lookup_final); >> if (ret < 0) >> return ERR_PTR(ret); >> >> - n = rb_last(&root_lookup.root); >> + n = rb_last(&root_lookup_final.root); >> while (n) { >> struct root_info *entry; >> >> entry = rb_entry(n, struct root_info, rb_node); >> - resolve_root(&root_lookup, entry, top_id); >> + resolve_root(&root_lookup_final, entry, top_id); >> if (entry->root_id == root) { >> ret_path = entry->full_path; >> entry->full_path = NULL; >> @@ -1681,7 +1709,7 @@ char *btrfs_list_path_for_root(int fd, u64 root) >> >> n = rb_prev(n); >> } >> - __free_all_subvolumn(&root_lookup); >> + __free_all_subvolumn(&root_lookup_final); >> >> return ret_path; >> } >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/btrfs-list.c b/btrfs-list.c index e5f0f96..faf5b1a 100644 --- a/btrfs-list.c +++ b/btrfs-list.c @@ -672,6 +672,13 @@ static int lookup_ino_path(int fd, struct root_info *ri) if (ri->path) return 0; + /* + * If the subvolume's ROOT_BACKREF does not exist anymore, consider it + * as deleted. + */ + if (ri->ref_tree == 0) + return -ENOENT; + memset(&args, 0, sizeof(args)); args.treeid = ri->ref_tree; args.objectid = ri->dir_id; @@ -679,9 +686,15 @@ static int lookup_ino_path(int fd, struct root_info *ri) ret = ioctl(fd, BTRFS_IOC_INO_LOOKUP, &args); e = errno; if (ret) { - fprintf(stderr, "ERROR: Failed to lookup path for root %llu - %s\n", - (unsigned long long)ri->ref_tree, - strerror(e)); + ret = -e; + /* + * If one of the path components is missing, we will conside + * the subvolume as deleted. + */ + if (ret != -ENOENT) + fprintf(stderr, "ERROR: Failed to lookup path for root %llu - %s\n", + (unsigned long long)ri->ref_tree, + strerror(e)); return ret; } @@ -1290,19 +1303,32 @@ static void __filter_and_sort_subvol(struct root_lookup *all_subvols, } } -static int __list_subvol_fill_paths(int fd, struct root_lookup *root_lookup) +static int __list_subvol_fill_paths(int fd, struct root_lookup *root_lookup, + struct root_lookup *root_lookup_final) { struct rb_node *n; + root_lookup_init(root_lookup_final); + n = rb_first(&root_lookup->root); while (n) { struct root_info *entry; int ret; entry = rb_entry(n, struct root_info, rb_node); ret = lookup_ino_path(fd, entry); - if(ret < 0) + if(ret < 0 && ret != -ENOENT) return ret; - n = rb_next(n); + rb_erase(&entry->rb_node, &root_lookup->root); + /* + * If lookup_ino_path() returned ENOENT, some of the path + * components are missing. Let's consider this subvolume + * as deleted then. + */ + if (ret == -ENOENT) + __free_root_info(entry); + else + root_tree_insert(root_lookup_final, entry); + n = rb_first(&root_lookup->root); }
If a subvolume is deleted between __list_subvol_search() and __list_subvol_fill_paths(), the tool would have stopped with an error. So drop those subvolumes that did not have ROOT_BACKREF, and those for whom a path component to the parent root was missing. (Note that BTRFS_IOC_INO_LOOKUP does not do a search if dirid is BTRFS_FIRST_FREE_OBJECTID, so such a subvolume will be considered as still existing, but subvolume listing will not fail at least). Reported-by: Stefan Priebe <s.priebe@profihost.ag> Signed-off-by: Alex Lyakas <alex.btrfs@zadarastorage.com> return 0; @@ -1446,6 +1472,7 @@ int btrfs_list_subvols(int fd, struct btrfs_list_filter_set *filter_set, int is_tab_result) { struct root_lookup root_lookup; + struct root_lookup root_lookup_final; struct root_lookup root_sort; int ret; @@ -1460,15 +1487,15 @@ int btrfs_list_subvols(int fd, struct btrfs_list_filter_set *filter_set, * now we have an rbtree full of root_info objects, but we need to fill * in their path names within the subvol that is referencing each one. */ - ret = __list_subvol_fill_paths(fd, &root_lookup); + ret = __list_subvol_fill_paths(fd, &root_lookup, &root_lookup_final); if (ret < 0) return ret; - __filter_and_sort_subvol(&root_lookup, &root_sort, filter_set, + __filter_and_sort_subvol(&root_lookup_final, &root_sort, filter_set, comp_set, fd); print_all_volume_info(&root_sort, is_tab_result); - __free_all_subvolumn(&root_lookup); + __free_all_subvolumn(&root_lookup_final); return ret; } @@ -1655,6 +1682,7 @@ int btrfs_list_find_updated_files(int fd, u64 root_id, u64 oldest_gen) char *btrfs_list_path_for_root(int fd, u64 root) { struct root_lookup root_lookup; + struct root_lookup root_lookup_final; struct rb_node *n; char *ret_path = NULL; int ret; @@ -1664,16 +1692,16 @@ char *btrfs_list_path_for_root(int fd, u64 root) if (ret < 0) return ERR_PTR(ret); - ret = __list_subvol_fill_paths(fd, &root_lookup); + ret = __list_subvol_fill_paths(fd, &root_lookup, &root_lookup_final); if (ret < 0) return ERR_PTR(ret); - n = rb_last(&root_lookup.root); + n = rb_last(&root_lookup_final.root); while (n) { struct root_info *entry; entry = rb_entry(n, struct root_info, rb_node); - resolve_root(&root_lookup, entry, top_id); + resolve_root(&root_lookup_final, entry, top_id); if (entry->root_id == root) { ret_path = entry->full_path; entry->full_path = NULL; @@ -1681,7 +1709,7 @@ char *btrfs_list_path_for_root(int fd, u64 root) n = rb_prev(n); } - __free_all_subvolumn(&root_lookup); + __free_all_subvolumn(&root_lookup_final); return ret_path; } -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html