Re: subvolumes missing from "btrfs subvolume list" output

[Goffredo Baroncelli <kreijack@libero.it>]

Hi Hugo

On 06/29/2011 06:47 PM, Hugo Mills wrote:
> On Wed, Jun 29, 2011 at 12:16:06PM -0400, Josef Bacik wrote:
>> On 06/29/2011 11:00 AM, Stephane Chazelas wrote:
>>> 2011-06-29 15:37:47 +0100, Stephane Chazelas:
>>> [...]
>>>> I found
>>>> http://thread.gmane.org/gmane.comp.file-systems.btrfs/8123/focus=8208
>>>>
>>>> which looks like the same issue, with Li Zefan saying he had a
>>>> fix, but I couldn't find any mention that it was actually fixed.
>>>>
>>>> Has anybody got any update on that?
>>> [...]
>>>
>>> I've found
>>> http://thread.gmane.org/gmane.comp.file-systems.btrfs/8232
>>>
>>> but no corresponding fix or ioctl.c
>>> http://git.kernel.org/?p=linux/kernel/git/mason/btrfs-unstable.git;a=history;f=fs/btrfs/ioctl.c
>>>
>>> I'm under the impression that the issue has been forgotten
>>> about.
>>>
>>> From what I managed to gather though, it seems that what's on
>>> disk is correct, it's just the ioctl and/or "btrfs sub list"
>>> that's wrong. Am I right?
>>
>> Yeah, did you apply the patch from that thread and verify that it fixes
>> your problem?  Thanks,
> 
>    Note that changing this API will probably break btrfs-gui's listing
> of subvolumes...
> 
>    The issue with that patch is that there are two distinct behaviours
> that people want or expect with the tree-search ioctl:
> 
> (A) Return all items with keys which collate linearly between
>     (min_objectid, min_type, min_offset) and 
>     (max_objectid, max_type, max_offset)
> 
>     i.e. treating keys as indivisible objects and sorting lexically,
>     as the trees do.
> 
> (B) Return all items with keys (i, t, o) which fulfil the criteria
>     (min_objectid <= i <= max_objectid,
>      min_type <= t <= max_type,
>      min_offset <= o <= max_offset)
> 
>     i.e. treating keys as 3-tuples, and selecting from a rectilinear
>     subsset of the tuple space, which is natural for some
>     applications.
> 
>    Clearly, we can't do both with the same call (except for some
> limited cases (*)). However, different users expect different
> behaviours. The current behaviour is (A), which is the "natural"
> behaviour for tree searches within the btrfs code, and is (IMO) the
> right thing to be doing for an API like this.

looking at the function copy_to_sk() it seems that the key advance is
made on the following logic:

        if (key->offset < (u64)-1 && key->offset < sk->max_offset)
                key->offset++;
        else if (key->type < (u8)-1 && key->type < sk->max_type) {
                key->offset = 0;
                key->type++;
        } else if (key->objectid < (u64)-1 &&
	   key->objectid < sk->max_objectid) {
                key->offset = 0;
                key->type = 0;
                key->objectid++;

which to me it seems a bit different from the case A. In fact (if I read
the code correctly) *both* the following condition are always true

     (min_objectid, min_type, min_offset) <= key   and
     key < (max_objectid, max_type, max_offset)    and
     (key_objectid <= max_objectid                 and
      key_type <= max_type                         and
      key_offset <= max_offset)

In conclusion the code is an hybrid between A and B.





> 
>    It sounds to me like the user of the API needs to be fixed, not the
> ioctl itself -- possibly the author of the subvol scanning code
> assumed (B) when they were getting (A). Note that there is at least
> one other user of the ioctl outside btrfs-progs: btrfs-gui, which uses
> the ioctl for several things, one of which is enumerating subvolumes
> as btrfs-progs does.



> 
>    It should be possible to write an additional ioctl for behaviour
> (B) which contains both min and max limits on each element of the key
> 3-tuple, *and* the current search state. That would reduce developer
> confusion (given appropriate comments or documentation to explain what
> the difference between the two is). However, I'm not sufficiently
> convinced that it's actually necessary right now. I may change my tune
> after I've started doing some of the more complex bits I'd thought of
> doing with btrfs-gui, but for now, it's perfectly possible to use the
> existing API without too much hassle.
> 
>    Hugo.
> 
> (*) The limited cases where both behaviours return the same set of
> keys are:
> 
> (i_0, 0, 0) to (i_1, -1UL, -1UL)
> (i, t_0, 0) to (i, t_1, -1UL)
> (i, t, o_0) to (i, t, o_1)
>