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

SF Markus Elfring
2015-12-14T06:58:51Z

>> 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