Re: [PATCH 1/2] zram: Less checks in zram_bvec_write() after error detection

[SF Markus Elfring]

>> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
>> index 47915d7..69d7fcd 100644
>> --- a/drivers/block/zram/zram_drv.c
>> +++ b/drivers/block/zram/zram_drv.c
>> @@ -652,9 +652,9 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>>  	size_t clen;
>>  	unsigned long handle;
>>  	struct page *page;
>> -	unsigned char *user_mem, *cmem, *src, *uncmem = NULL;
>> +	unsigned char *user_mem, *cmem, *src, *uncmem;
>>  	struct zram_meta *meta = zram->meta;
>> -	struct zcomp_strm *zstrm = NULL;
>> +	struct zcomp_strm *zstrm;
>>  	unsigned long alloced_pages;
>>  
>>  	page = bvec->bv_page;
>> @@ -664,13 +664,11 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>>  		 * before to write the changes.
>>  		 */
>>  		uncmem = kmalloc(PAGE_SIZE, GFP_NOIO);
>> -		if (!uncmem) {
>> -			ret = -ENOMEM;
>> -			goto out;
>> -		}
>> +		if (!uncmem)
>> +			return -ENOMEM;
> 
> ok.

Thanks for your terse acknowledgement.


>>  		ret = zram_decompress_page(zram, uncmem, index);
>>  		if (ret)
>> -			goto out;
>> +			goto free_uncmem;
> 
> here and later, I don't want to split `out' label.

I guess that corresponding software design concerns can evolve a bit.


> you still need to do both 'if zstrm' and 'if is_partial_io' checks anyway, what's the gain?

How are the chances to reduce the number of dispensable sanity checks?


> the more labels we have the trickier it may get.

I hope that more unique jump labels can make the involved exception handling also clearer.


>> @@ -762,11 +760,13 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>>  	/* Update stats */
>>  	atomic64_add(clen, &zram->stats.compr_data_size);
>>  	atomic64_inc(&zram->stats.pages_stored);
>> -out:
>> +check_strm:
>>  	if (zstrm)
>>  		zcomp_strm_release(zram->comp, zstrm);
>> -	if (is_partial_io(bvec))
>> +	if (is_partial_io(bvec)) {
>> +free_uncmem:
>>  		kfree(uncmem);
>> +	}
> 
> a label inside of `if'?   no.

Do any more software developers find such an use case interesting?


> keep it the way it is please.

I suggest to make the affected exception handling a bit more efficient.
Such source code fine-tuning has got a few special consequences.

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/