Hi Miao,
On Thu, Oct 18, 2012 at 4:00 AM, Miao Xie <miaox@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
