[dm-devel] dm cache metadata: Fix loading discard bitset

[Nikos Tsironis]

On 4/17/19 5:35 PM, Mike Snitzer wrote:
> On Wed, Apr 17 2019 at 10:19am -0400,
> Nikos Tsironis <ntsironis at arrikto.com> wrote:
> 
>> Add missing dm_bitset_cursor_next() to properly advance the bitset
>> cursor.
>>
>> Otherwise, the discarded state of all blocks is set according to the
>> discarded state of the first block.
>>
>> Fixes: ae4a46a1f6 ("dm cache metadata: use bitset cursor api to load discard bitset")
>> Signed-off-by: Nikos Tsironis <ntsironis at arrikto.com>
> 
> Nice catch, I'll obviously mark this for stable@ too.
> 
> One comment below.
> 
>> ---
>>  drivers/md/dm-cache-metadata.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/dm-cache-metadata.c b/drivers/md/dm-cache-metadata.c
>> index 6fc93834da44..151aa95775be 100644
>> --- a/drivers/md/dm-cache-metadata.c
>> +++ b/drivers/md/dm-cache-metadata.c
>> @@ -1167,11 +1167,18 @@ static int __load_discards(struct dm_cache_metadata *cmd,
>>  		if (r)
>>  			return r;
>>  
>> -		for (b = 0; b < from_dblock(cmd->discard_nr_blocks); b++) {
>> +		for (b = 0; ; b++) {
>>  			r = fn(context, cmd->discard_block_size, to_dblock(b),
>>  			       dm_bitset_cursor_get_value(&c));
>>  			if (r)
>>  				break;
>> +
>> +			if (b >= (from_dblock(cmd->discard_nr_blocks) - 1))
>> +				break;
> 
> Not understanding why you moved the conditional inside the loop, I'd
> rather keep it in the for().  Or did you have some reason for this?
> 

After processing the last block, i.e., b == (from_dblock(cmd->discard_nr_blocks) - 1),
dm_bitset_cursor_next() will return -ENODATA. It seemed to me simpler to move the
conditional inside the loop, rather than check explicitly for -ENODATA and handle
it differently, e.g., set r = 0, if r == -ENODATA.

Nikos.

>> +
>> +			r = dm_bitset_cursor_next(&c);
>> +			if (r)
>> +				break;
>>  		}
>>  
>>  		dm_bitset_cursor_end(&c);
>> -- 
>> 2.11.0
>>