[PATCH] md/bitmap: clear BITMAP_WRITE_ERROR bit after reading it from sb

[PATCH] md/bitmap: clear BITMAP_WRITE_ERROR bit after reading it from sb

[Hou Tao]

For a RAID1 device using a file-based bitmap, if a bitmap write error
occurs and the later writes succeed, it's possible both BITMAP_STALE
and BITMAP_WRITE_ERROR bit will be written to the bitmap super block,
and the BITMAP_WRITE_ERROR bit in sb->flags will make bitmap_create()
to fail.

So clear it to protect against the write failure-success case.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 drivers/md/bitmap.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
index d212163..97c423a 100644
--- a/drivers/md/bitmap.c
+++ b/drivers/md/bitmap.c
@@ -688,8 +688,13 @@ static int bitmap_read_sb(struct bitmap *bitmap)
 		}
 	}
 
-	/* assign fields using values from superblock */
-	bitmap->flags |= le32_to_cpu(sb->state);
+	/*
+	 * assign fields using values from superblock
+	 * clear BITMAP_WRITE_ERROR bit to protect against the case that
+	 * a bitmap write error had occurred and the later writes had
+	 * succeeded.
+	 */
+	bitmap->flags |= (le32_to_cpu(sb->state) & ~BIT(BITMAP_WRITE_ERROR));
 	if (le32_to_cpu(sb->version) == BITMAP_MAJOR_HOSTENDIAN)
 		set_bit(BITMAP_HOSTENDIAN, &bitmap->flags);
 	bitmap->events_cleared = le64_to_cpu(sb->events_cleared);
-- 
2.5.0

Re: [PATCH] md/bitmap: clear BITMAP_WRITE_ERROR bit after reading it from sb

[Shaohua Li]

On Fri, Oct 13, 2017 at 06:36:31PM +0800, Hou Tao wrote:
> For a RAID1 device using a file-based bitmap, if a bitmap write error
> occurs and the later writes succeed, it's possible both BITMAP_STALE
> and BITMAP_WRITE_ERROR bit will be written to the bitmap super block,
> and the BITMAP_WRITE_ERROR bit in sb->flags will make bitmap_create()
> to fail.

Why don't clear the bit in the 'latter succeeded write'?
 
> So clear it to protect against the write failure-success case.
> 
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>  drivers/md/bitmap.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
> index d212163..97c423a 100644
> --- a/drivers/md/bitmap.c
> +++ b/drivers/md/bitmap.c
> @@ -688,8 +688,13 @@ static int bitmap_read_sb(struct bitmap *bitmap)
>  		}
>  	}
>  
> -	/* assign fields using values from superblock */
> -	bitmap->flags |= le32_to_cpu(sb->state);
> +	/*
> +	 * assign fields using values from superblock
> +	 * clear BITMAP_WRITE_ERROR bit to protect against the case that
> +	 * a bitmap write error had occurred and the later writes had
> +	 * succeeded.
> +	 */
> +	bitmap->flags |= (le32_to_cpu(sb->state) & ~BIT(BITMAP_WRITE_ERROR));
>  	if (le32_to_cpu(sb->version) == BITMAP_MAJOR_HOSTENDIAN)
>  		set_bit(BITMAP_HOSTENDIAN, &bitmap->flags);
>  	bitmap->events_cleared = le64_to_cpu(sb->events_cleared);
> -- 
> 2.5.0
> 

Re: [PATCH] md/bitmap: clear BITMAP_WRITE_ERROR bit after reading it from sb

[Hou Tao]

Hi,

On 2017/10/14 3:21, Shaohua Li wrote:
> On Fri, Oct 13, 2017 at 06:36:31PM +0800, Hou Tao wrote:
>> For a RAID1 device using a file-based bitmap, if a bitmap write error
>> occurs and the later writes succeed, it's possible both BITMAP_STALE
>> and BITMAP_WRITE_ERROR bit will be written to the bitmap super block,
>> and the BITMAP_WRITE_ERROR bit in sb->flags will make bitmap_create()
>> to fail.
> 
> Why don't clear the bit in the 'latter succeeded write'?

It will work too. Cleaning after read can ensure that after upgrading the
kernel we can activate the RAID1 successfully event if the write error
bit had been written to the bitmap.

>> So clear it to protect against the write failure-success case.
>>
>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>> ---
>>  drivers/md/bitmap.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
>> index d212163..97c423a 100644
>> --- a/drivers/md/bitmap.c
>> +++ b/drivers/md/bitmap.c
>> @@ -688,8 +688,13 @@ static int bitmap_read_sb(struct bitmap *bitmap)
>>  		}
>>  	}
>>  
>> -	/* assign fields using values from superblock */
>> -	bitmap->flags |= le32_to_cpu(sb->state);
>> +	/*
>> +	 * assign fields using values from superblock
>> +	 * clear BITMAP_WRITE_ERROR bit to protect against the case that
>> +	 * a bitmap write error had occurred and the later writes had
>> +	 * succeeded.
>> +	 */
>> +	bitmap->flags |= (le32_to_cpu(sb->state) & ~BIT(BITMAP_WRITE_ERROR));
>>  	if (le32_to_cpu(sb->version) == BITMAP_MAJOR_HOSTENDIAN)
>>  		set_bit(BITMAP_HOSTENDIAN, &bitmap->flags);
>>  	bitmap->events_cleared = le64_to_cpu(sb->events_cleared);
>> -- 
>> 2.5.0
>>
> 
> .
> 

Re: [PATCH] md/bitmap: clear BITMAP_WRITE_ERROR bit after reading it from sb

[Hou Tao]

Hi Shaohua,

On 2017/10/16 11:22, Hou Tao wrote:
> Hi,
> 
> On 2017/10/14 3:21, Shaohua Li wrote:
>> On Fri, Oct 13, 2017 at 06:36:31PM +0800, Hou Tao wrote:
>>> For a RAID1 device using a file-based bitmap, if a bitmap write error
>>> occurs and the later writes succeed, it's possible both BITMAP_STALE
>>> and BITMAP_WRITE_ERROR bit will be written to the bitmap super block,
>>> and the BITMAP_WRITE_ERROR bit in sb->flags will make bitmap_create()
>>> to fail.
>>
>> Why don't clear the bit in the 'latter succeeded write'?
> 
> It will work too. Cleaning after read can ensure that after upgrading the
> kernel we can activate the RAID1 successfully event if the write error
> bit had been written to the bitmap.

If you are OK with the fix which clears BITMAP_WRITE_ERROR before write, I
will send a v2 patch.

Regards,

Tao
>>> So clear it to protect against the write failure-success case.
>>>
>>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>>> ---
>>>  drivers/md/bitmap.c | 9 +++++++--
>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
>>> index d212163..97c423a 100644
>>> --- a/drivers/md/bitmap.c
>>> +++ b/drivers/md/bitmap.c
>>> @@ -688,8 +688,13 @@ static int bitmap_read_sb(struct bitmap *bitmap)
>>>  		}
>>>  	}
>>>  
>>> -	/* assign fields using values from superblock */
>>> -	bitmap->flags |= le32_to_cpu(sb->state);
>>> +	/*
>>> +	 * assign fields using values from superblock
>>> +	 * clear BITMAP_WRITE_ERROR bit to protect against the case that
>>> +	 * a bitmap write error had occurred and the later writes had
>>> +	 * succeeded.
>>> +	 */
>>> +	bitmap->flags |= (le32_to_cpu(sb->state) & ~BIT(BITMAP_WRITE_ERROR));
>>>  	if (le32_to_cpu(sb->version) == BITMAP_MAJOR_HOSTENDIAN)
>>>  		set_bit(BITMAP_HOSTENDIAN, &bitmap->flags);
>>>  	bitmap->events_cleared = le64_to_cpu(sb->events_cleared);
>>> -- 
>>> 2.5.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
> 
> .
>