On 18.10.2017 06:43, Gu, Jinxiang wrote:
> Hi，
>
>> -----Original Message-----
>> From: Nikolay Borisov [mailto:nbori...@suse.com]
>> Sent: Tuesday, October 17, 2017 9:36 PM
>> To: Gu, Jinxiang/顾 金香 <g...@cn.fujitsu.com>; linux-btrfs@vger.kernel.org;
>> h...@lst.de
>> Subject: Re: [PATCH] btrfs: Fix bug for misused dev_t when lookup in dev
>> state hash table.
>>
>>
>>
>> On 17.10.2017 14:34, Gu Jinxiang wrote:
>>> From: Gu JinXiang <g...@cn.fujitsu.com>
>>>
>>> Fix bug of commit 74d46992e0d9
>>> ("block: replace bi_bdev with a gendisk pointer and partitions index").
>>>
>>> In this modify, use bio_dev(bio) to find dev state in function
>>> __btrfsic_submit_bio. But when dev_state added to hashtable, it is
>>> using dev_t of block_device.
>>
>> This is rather incomprehensible. So bio_dev(bio) actually returns the dev_t
>> of the device to which this bio is submitted
>> and the same dev_t should be used when btrfsic_dev_state_hashtable_add is
>> called? What am I missing in here?
>>
>
> bio_dev(bio) returns a dev_t of part0 which is different from dev_t in
> block_device(bd_dev).
> bd_dev in block_device represents the exact partition.
> block_device.bd_dev = bio->bi_partno (same as block_device.bd_partno) +
> bio_dev(bio).
>
> When add a dev_state into hashtable it is using the exact partition's dev_t.
> So when lookup it, it should also use the exact partition's dev_t.
Right, ok. Can you please put this explanation into the changelog of the
patch and resend
>
>>
>>>
>>> Reproduce of this bug:
>>> Use MOUNT_OPTIONS="-o check_int" when run btrfs/001 in xfstest.
>>> Then there will be WARNING like below.
>>> WARNING:
>>> btrfs: attempt to write superblock which references block M @29523968 (sda7
>>> /1111654400/2) which is never written!
>>>
>>> Signed-off-by: Gu JinXiang <g...@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 fb07e3c22b9a..02f9eb83173f 100644
>>> --- a/fs/btrfs/check-integrity.c
>>> +++ b/fs/btrfs/check-integrity.c
>>> @@ -2803,7 +2803,7 @@ static void __btrfsic_submit_bio(struct bio *bio)
>>> mutex_lock(&btrfsic_mutex);
>>> /* since btrfsic_submit_bio() is also called before
>>> * btrfsic_mount(), this might return NULL */
>>> - dev_state = btrfsic_dev_state_lookup(bio_dev(bio));
>>> + dev_state = btrfsic_dev_state_lookup(bio_dev(bio) + bio->bi_partno);
>>
>> So this function looks up in btrfsic_dev_state_hashtable. And stuff in this
>> hashtable ias added via
>> btrfsic_dev_state_hashtable_add function which seems to be only using the
>> dev_t (after your other patch is applied):
>>
>> static void btrfsic_dev_state_hashtable_add(
>>
>> struct btrfsic_dev_state *ds,
>>
>> struct btrfsic_dev_state_hashtable *h)
>>
>> {
>>
>> const unsigned int hashval =
>>
>> (((unsigned int)((uintptr_t)ds->bdev->bd_dev)) &
>>
>> (BTRFSIC_DEV2STATE_HASHTABLE_SIZE - 1));
>>
>>
>>
>> list_add(&ds->collision_resolving_node, h->table + hashval);
>>
>> }
>>
>>
>> So how come your change is correct since you are passing the dev_t +
>> partition number?
>>
>>> if (NULL != dev_state &&
>>> (bio_op(bio) == REQ_OP_WRITE) && bio_has_data(bio)) {
>>> unsigned int i = 0;
>>>
>>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
