On 11.10.2017 12:16, Nikolay Borisov wrote:
>
>
> On 11.10.2017 11:44, Gu Jinxiang wrote:
>> From: Gu JinXiang <gujx@xxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxx>
>
>>
>> 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@xxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
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
