Re: [PATCH] Btrfs: copy the certain type of item if min_type equals to max_type

[Hugo Mills <hugo@carfax.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 2965 bytes --]

On Fri, Jan 15, 2016 at 12:52:28PM -0800, Liu Bo wrote:
> Some tools in btrfs-progs utilize ioctl 'BTRFS_IOC_TREE_SEARCH' and
> ioctl 'BTRFS_IOC_TREE_SEARCH_V2' to look up metadata btree for what
> they want, and several tools in fact only look for one certain type,
> where they set a certain value for both 'sk->min_type' and 'sk->max_type'.
> 
> For example,
> if we want to get the information of block groups, the current btrfs
> searches extent_tree and returns not only block groups's items, but also
> EXTENT_ITEM's items which could cost a large amount of user's buffer,
> and tools then needs to read the buffer and spends several loops to
> pick up what they want.
> 
> This lets the above two ioctl only return the certain type of items
> that tools wants.

   This changes the semantics of the ioctl in a subtle and
incompatible way.

   The keyspace used by btrfs trees can be viewed in two different and
semantically incompatible ways. A key is an (Ob, T, Of) tuple. The
first way of looking at this is as a one-dimensional keyspace, ordered
lexically, as Ob+T+Of. This is what btrfs uses internally, and it's
the way that the TREE_SEARCH ioctl works. A search simply returns a
linear subset of the keys between the minimum and the maximum.

   The other view of the keyspace, which is more useful in some
circumstances, is of a 3-dimensional keyspace, with the obvious
lattice-like ordering, where K1 <= K2 iff Ob1 <= Ob2 and T1 <= T2 and
Of1 <= Of2. This offers a very different interpretation of searching,
where you are carving out a rectangular block of the 3-dimensional
keyspace. This is the behaviour you're trying to impose on the search
ioctl for a specific special case of search.

   I would argue that if you want to have the second form of search
(and it's a useful one, certainly), you should implement an
alternative search ioctl, rather than trying to retrofit that
behaviour on something with very different, already well-defined
semantics.

   In other words, this change makes for an awkward and confusing
interface, and I think it shouldn't be done this way.

   Hugo.

> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  fs/btrfs/ioctl.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index da94138..f795423 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1911,6 +1911,10 @@ static noinline int key_in_sk(struct btrfs_key *key,
>  	struct btrfs_key test;
>  	int ret;
>  
> +	/* All we want is this type of key. */
> +	if (sk->min_type == sk->max_type && key->type != sk->min_type)
> +		return 0;
> +
>  	test.objectid = sk->min_objectid;
>  	test.type = sk->min_type;
>  	test.offset = sk->min_offset;

-- 
Hugo Mills             | "He's a nutcase, you know. There's no getting away
hugo@... carfax.org.uk | from it -- he'll end up with a knighthood"
http://carfax.org.uk/  |
PGP: E2AB1DE4          |                         Lexy, The League of Gentlemen

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]