Re: [PATCH] md/bitmap: don't read page from device with Bitmap_sync

[NeilBrown <neilb@suse.com>]

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

On Fri, Jun 16 2017, Shaohua Li wrote:

> On Fri, Jun 16, 2017 at 05:19:27PM +0800, Guoqing Jiang wrote:
>> The device owns Bitmap_sync flag needs recovery
>> to become in sync, and read page from this type
>> device could get stale status.
>> 
>> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
>> ---
>> When develop for clustered raid10 feature, if add a
>> disk under grow mode in master node, I could get
>> the "bitmap superblock UUID mismatch" warning due
>> to the page is read from Bitmap_sync device.
>> 
>>  drivers/md/bitmap.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
>> index bf7419a..bf34cd8 100644
>> --- a/drivers/md/bitmap.c
>> +++ b/drivers/md/bitmap.c
>> @@ -156,7 +156,8 @@ static int read_sb_page(struct mddev *mddev, loff_t offset,
>>  
>>  	rdev_for_each(rdev, mddev) {
>>  		if (! test_bit(In_sync, &rdev->flags)
>
> Why In_sync is set for the rdev? I think it shoudn't.

There are several states a device can be in.
If ->raid_disk is < 0, then the device is a spare, and doesn't contain
    and valid data.
Otherwise ->raid_disk >=0 and:
   In_sync is clear, which means that blocks before ->recovery_offset
       contain valid data, and blocks after there don't
   In_sync is set, which means ->recovery_offset is irrelevant and
       should be treated as MaxSector
   Bitmap_sync is set, which could apply in either of the above cases,
       and means blocks corresponding to bits that are set in the bitmap
       may not be up to date.

Bitmap_sync is only relevant before a device has been handed to the
personality.  After it has been added, ->recovery_cp ensures that the
blocks that might be wrong are not read until until the bitmap-based
recovery has fixed them up.

Bitmap_sync is only use to stop the device from being given to the
personality if a recovery won't be started because the array is
read-only.

So it is perfectly valid for both In_sync and Bitmap_sync to be set.
If they are, it makes sense to avoid reading bitmap information from the
Bitmap_sync device, as that will be out-of-date.

I'm not quite sure why Guoqing is getting a UUID mismatch ... it
suggests that the new device wasn't initialized properly.  So there
might be another bug.  But I think this is definitely a bug.
     

>
>> -		    || test_bit(Faulty, &rdev->flags))
>> +		    || test_bit(Faulty, &rdev->flags)
>> +		    || test_bit(Bitmap_sync, &rdev->flags))
>
> I didn't see code clears the Bitmap_sync bit after disk is synced, so likely
> there is bug in this side.

There is no need to clear Bitmap_sync.  It stays set until it becomes
irrelevant.

Thanks,
NeilBrown


>
>>  			continue;
>>  
>>  		target = offset + index * (PAGE_SIZE/512);
>> -- 
>> 2.10.0
>> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]