[PATCH v3] btrfs: Use bd_dev to generate index when dev_state_hashtable add items.

[PATCH v3] btrfs: Use bd_dev to generate index when dev_state_hashtable add items.

[Gu Jinxiang]

From: Gu JinXiang <gujx@cn.fujitsu.com>

Fix missing modify of
commit f8f84b2dfda5 ("btrfs: index check-integrity state hash by a dev_t").
Function btrfsic_dev_state_hashtable_lookup use dev_t to generate hashval
when lookup a btrfsic_dev_state in hash table. So when add a
btrfsic_dev_state into hash table, it should also use dev_t.

Reproduce of this bug:
Use MOUNT_OPTIONS="-o check_int" when run xfstest, device can not be
mount successfully. So xfstest can not run. 

changelog:
v1->v2: Add how to reproduce this bug.
v2->v3: Add description of this bug.

Signed-off-by: Gu JinXiang <gujx@cn.fujitsu.com>
---
 fs/btrfs/check-integrity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
index 9d3854839038..86d79bc4cfb3 100644
--- a/fs/btrfs/check-integrity.c
+++ b/fs/btrfs/check-integrity.c
@@ -613,7 +613,7 @@ static void btrfsic_dev_state_hashtable_add(
 		struct btrfsic_dev_state_hashtable *h)
 {
 	const unsigned int hashval =
-	    (((unsigned int)((uintptr_t)ds->bdev)) &
+	    (((unsigned int)((uintptr_t)ds->bdev->bd_dev)) &
 	     (BTRFSIC_DEV2STATE_HASHTABLE_SIZE - 1));
 
 	list_add(&ds->collision_resolving_node, h->table + hashval);
-- 
2.13.5

Re: [PATCH v3] btrfs: Use bd_dev to generate index when dev_state_hashtable add items.

[Nikolay Borisov]

On 11.10.2017 11:44, Gu Jinxiang wrote:
> From: Gu JinXiang <gujx@cn.fujitsu.com>
> 
> Fix missing modify of
> commit f8f84b2dfda5 ("btrfs: index check-integrity state hash by a dev_t").
> Function btrfsic_dev_state_hashtable_lookup use dev_t to generate hashval
> when lookup a btrfsic_dev_state in hash table. So when add a
> btrfsic_dev_state into hash table, it should also use dev_t.

Your description contradicts the actual code as of a957fd420ca8.
btrfsic_block_hashtable_lookup creates the hash like so:

const unsigned int hashval =
            (((unsigned int)(dev_bytenr >> 16)) ^

             ((unsigned int)((uintptr_t)bdev))) &

             (BTRFSIC_BLOCK_HASHTABLE_SIZE - 1)

and bdev is being passed as an argument. And hashtable_add also uses the
pointer of the bdev, which seems correct. So what's the deal here?

> 
> Reproduce of this bug:
> Use MOUNT_OPTIONS="-o check_int" when run xfstest, device can not be
> mount successfully. So xfstest can not run. 
> 
> changelog:
> v1->v2: Add how to reproduce this bug.
> v2->v3: Add description of this bug.
> 
> Signed-off-by: Gu JinXiang <gujx@cn.fujitsu.com>
> ---
>  fs/btrfs/check-integrity.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
> index 9d3854839038..86d79bc4cfb3 100644
> --- a/fs/btrfs/check-integrity.c
> +++ b/fs/btrfs/check-integrity.c
> @@ -613,7 +613,7 @@ static void btrfsic_dev_state_hashtable_add(
>  		struct btrfsic_dev_state_hashtable *h)
>  {
>  	const unsigned int hashval =
> -	    (((unsigned int)((uintptr_t)ds->bdev)) &
> +	    (((unsigned int)((uintptr_t)ds->bdev->bd_dev)) &
>  	     (BTRFSIC_DEV2STATE_HASHTABLE_SIZE - 1));
>  
>  	list_add(&ds->collision_resolving_node, h->table + hashval);
> 

Re: [PATCH v3] btrfs: Use bd_dev to generate index when dev_state_hashtable add items.

[Nikolay Borisov]

On 11.10.2017 12:16, Nikolay Borisov wrote:
> 
> 
> On 11.10.2017 11:44, Gu Jinxiang wrote:
>> From: Gu JinXiang <gujx@cn.fujitsu.com>
>>
>> Fix missing modify of
>> commit f8f84b2dfda5 ("btrfs: index check-integrity state hash by a dev_t").
>> Function btrfsic_dev_state_hashtable_lookup use dev_t to generate hashval
>> when lookup a btrfsic_dev_state in hash table. So when add a
>> btrfsic_dev_state into hash table, it should also use dev_t.
> 
> Your description contradicts the actual code as of a957fd420ca8.
> btrfsic_block_hashtable_lookup creates the hash like so:
> 
> const unsigned int hashval =
>             (((unsigned int)(dev_bytenr >> 16)) ^
> 
>              ((unsigned int)((uintptr_t)bdev))) &
> 
>              (BTRFSIC_BLOCK_HASHTABLE_SIZE - 1)
> 
> and bdev is being passed as an argument. And hashtable_add also uses the
> pointer of the bdev, which seems correct. So what's the deal here?

Disregard, there seem to be multiple _lookup functions and I was looking
at the wrong one. btrfsic_dev_state_hashtable_lookup definitely uses the
passed in dev_t as the key. So this fix looks correct.


On a slightly different note the check-integrity.c file could really
make use of the hashtable.h API but that's a bit more work. In any case
I think what could be done to reduce the likelihood of such errors is to
introduce functions/macros which calculate the correct key when it's
necessary rather than open-coding it.

Any way:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> 
>>
>> Reproduce of this bug:
>> Use MOUNT_OPTIONS="-o check_int" when run xfstest, device can not be
>> mount successfully. So xfstest can not run. 
>>
>> changelog:
>> v1->v2: Add how to reproduce this bug.
>> v2->v3: Add description of this bug.
>>
>> Signed-off-by: Gu JinXiang <gujx@cn.fujitsu.com>
>> ---
>>  fs/btrfs/check-integrity.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
>> index 9d3854839038..86d79bc4cfb3 100644
>> --- a/fs/btrfs/check-integrity.c
>> +++ b/fs/btrfs/check-integrity.c
>> @@ -613,7 +613,7 @@ static void btrfsic_dev_state_hashtable_add(
>>  		struct btrfsic_dev_state_hashtable *h)
>>  {
>>  	const unsigned int hashval =
>> -	    (((unsigned int)((uintptr_t)ds->bdev)) &
>> +	    (((unsigned int)((uintptr_t)ds->bdev->bd_dev)) &
>>  	     (BTRFSIC_DEV2STATE_HASHTABLE_SIZE - 1));
>>  
>>  	list_add(&ds->collision_resolving_node, h->table + hashval);
>>
> --
> 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
> 

Re: [PATCH v3] btrfs: Use bd_dev to generate index when dev_state_hashtable add items.

[David Sterba]

On Wed, Oct 11, 2017 at 12:22:35PM +0300, Nikolay Borisov wrote:

Patch added to queue, thanks.

> On a slightly different note the check-integrity.c file could really
> make use of the hashtable.h API but that's a bit more work. In any case
> I think what could be done to reduce the likelihood of such errors is to
> introduce functions/macros which calculate the correct key when it's
> necessary rather than open-coding it.

The integrity checker is a debugging feature and sepate from the rest.
Though the code could use a lot of cleanups, it's not something I'm
concerned about, as long as it works.