[PATCH] Btrfs: fix comp_oper to get right order

[PATCH] Btrfs: fix comp_oper to get right order

[Liu Bo]

Case (oper1->seq > oper2->seq) should differ with case (oper1->seq < oper2->seq).

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
This typo was hiden in a big patch, so we need to work harder on review.

 fs/btrfs/qgroup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 97159a8..058c79e 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1259,7 +1259,7 @@ static int comp_oper(struct btrfs_qgroup_operation *oper1,
 	if (oper1->seq < oper2->seq)
 		return -1;
 	if (oper1->seq > oper2->seq)
-		return -1;
+		return 1;
 	if (oper1->ref_root < oper2->ref_root)
 		return -1;
 	if (oper1->ref_root > oper2->ref_root)
-- 
1.8.1.4

Re: [PATCH] Btrfs: fix comp_oper to get right order

[David Sterba]

On Fri, Mar 13, 2015 at 02:24:38PM +0800, Liu Bo wrote:
> Case (oper1->seq > oper2->seq) should differ with case (oper1->seq < oper2->seq).
> 
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>

Reviewed-by: David Sterba <dsterba@suse.cz>

and should go to stable@ if not to 4.1-rc as it's a bug in the core of
qgroups. Waiting for the next merge window would not buy us anything.

> ---
> This typo was hiden in a big patch, so we need to work harder on review.

Of course, lack of reviews that come from non-maintainers is a perpetual
problem. There are never too many and each is highly appreciated.

Re: [PATCH] Btrfs: fix comp_oper to get right order

[Filipe David Manana]

On Fri, Mar 13, 2015 at 12:37 PM, David Sterba <dsterba@suse.cz> wrote:
> On Fri, Mar 13, 2015 at 02:24:38PM +0800, Liu Bo wrote:
>> Case (oper1->seq > oper2->seq) should differ with case (oper1->seq < oper2->seq).
>>
>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
>
> Reviewed-by: David Sterba <dsterba@suse.cz>
Reviewed-by: Filipe Manana <fdmanana@suse.com>

>
> and should go to stable@ if not to 4.1-rc as it's a bug in the core of
> qgroups. Waiting for the next merge window would not buy us anything.

Well, unless I missed something, it doesn't seem to cause any real
harm as the only other search operation on the rbtree uses
comp_oper_exist(), which totally ignores the seq field.

Of course it should be fixed anyway as it's logic error likely due to
copy-paste, but unlikely to fix any real user visible problem.

>
>> ---
>> This typo was hiden in a big patch, so we need to work harder on review.
>
> Of course, lack of reviews that come from non-maintainers is a perpetual
> problem. There are never too many and each is highly appreciated.
> --
> 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



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH] Btrfs: fix comp_oper to get right order

[Filipe David Manana]

On Fri, Mar 13, 2015 at 6:27 PM, Filipe David Manana <fdmanana@gmail.com> wrote:
> On Fri, Mar 13, 2015 at 12:37 PM, David Sterba <dsterba@suse.cz> wrote:
>> On Fri, Mar 13, 2015 at 02:24:38PM +0800, Liu Bo wrote:
>>> Case (oper1->seq > oper2->seq) should differ with case (oper1->seq < oper2->seq).
>>>
>>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
>>
>> Reviewed-by: David Sterba <dsterba@suse.cz>
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
>
>>
>> and should go to stable@ if not to 4.1-rc as it's a bug in the core of
>> qgroups. Waiting for the next merge window would not buy us anything.
>
> Well, unless I missed something, it doesn't seem to cause any real
> harm as the only other search operation on the rbtree uses
> comp_oper_exist(), which totally ignores the seq field.

So that actually makes qgroup_oper_exists() wrong (unrelated to this
patch), which ignores the seq field. In other words it can miss
elements of the tree with the desired bytenr and ref root, since the
second criteria for insertion is the seq value and comp_oper_exist()
only uses bytenr and ref_root for comparing nodes / navigate the tree,
so if the tree has multiple nodes with the same bytenr but different
seqs, it can miss the fact that an operation for the given bytenr and
ref root exists.

>
> Of course it should be fixed anyway as it's logic error likely due to
> copy-paste, but unlikely to fix any real user visible problem.
>
>>
>>> ---
>>> This typo was hiden in a big patch, so we need to work harder on review.
>>
>> Of course, lack of reviews that come from non-maintainers is a perpetual
>> problem. There are never too many and each is highly appreciated.
>> --
>> 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
>
>
>
> --
> Filipe David Manana,
>
> "Reasonable men adapt themselves to the world.
>  Unreasonable men adapt the world to themselves.
>  That's why all progress depends on unreasonable men."



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH] Btrfs: fix comp_oper to get right order

[David Sterba]

On Fri, Mar 13, 2015 at 06:27:31PM +0000, Filipe David Manana wrote:
> > and should go to stable@ if not to 4.1-rc as it's a bug in the core of
> > qgroups. Waiting for the next merge window would not buy us anything.
> 
> Well, unless I missed something, it doesn't seem to cause any real
> harm as the only other search operation on the rbtree uses
> comp_oper_exist(), which totally ignores the seq field.
> 
> Of course it should be fixed anyway as it's logic error likely due to
> copy-paste, but unlikely to fix any real user visible problem.

Thanks for checking.