Re: [PATCH 1/2] zram: free meta out of init_lock

[Jerome Marchand]

On 01/23/2015 03:24 PM, Sergey Senozhatsky wrote:
> On (01/23/15 14:58), Minchan Kim wrote:
>> We don't need to call zram_meta_free, zcomp_destroy and zs_free
>> under init_lock. What we need to prevent race with init_lock
>> in reset is setting NULL into zram->meta (ie, init_done).
>> This patch does it.
>>
>> Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
>> ---
>>  drivers/block/zram/zram_drv.c | 28 ++++++++++++++++------------
>>  1 file changed, 16 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
>> index 9250b3f54a8f..0299d82275e7 100644
>> --- a/drivers/block/zram/zram_drv.c
>> +++ b/drivers/block/zram/zram_drv.c
>> @@ -708,6 +708,7 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
>>  {
>>  	size_t index;
>>  	struct zram_meta *meta;
>> +	struct zcomp *comp;
>>  
>>  	down_write(&zram->init_lock);
>>  
>> @@ -719,20 +720,10 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
>>  	}
>>  
>>  	meta = zram->meta;
>> -	/* Free all pages that are still in this zram device */
>> -	for (index = 0; index < zram->disksize >> PAGE_SHIFT; index++) {
>> -		unsigned long handle = meta->table[index].handle;
>> -		if (!handle)
>> -			continue;
>> -
>> -		zs_free(meta->mem_pool, handle);
>> -	}
>> -
>> -	zcomp_destroy(zram->comp);
> 
> I'm not so sure about moving zcomp destruction. if we would have detached it
> from zram, then yes. otherwise, think of zram ->destoy vs ->init race.
> 
> suppose,
> CPU1 waits for down_write() init lock in disksize_store() with new comp already allocated;
> CPU0 detaches ->meta and releases write init lock;
> CPU1 grabs the lock and does zram->comp = comp;
> CPU0 reaches the point of zcomp_destroy(zram->comp);

I don't see your point: this patch does not call
zcomp_destroy(zram->comp) anymore, but zram_destroy(comp), where comp is
the old zram->comp.

> 
> 
> I'd probably prefer to keep zcomp destruction on its current place. I
> see a little real value in introducing zcomp detaching and moving
> destruction out of init_lock.
> 
> 	-ss
> 
>> +	comp = zram->comp;
>> +	zram->meta = NULL;
>>  	zram->max_comp_streams = 1;
>>  
>> -	zram_meta_free(zram->meta);
>> -	zram->meta = NULL;
>>  	/* Reset stats */
>>  	memset(&zram->stats, 0, sizeof(zram->stats));
>>  
>> @@ -742,6 +733,19 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
>>  
>>  	up_write(&zram->init_lock);
>>  
>> +	/* Free all pages that are still in this zram device */
>> +	for (index = 0; index < zram->disksize >> PAGE_SHIFT; index++) {
>> +		unsigned long handle = meta->table[index].handle;
>> +
>> +		if (!handle)
>> +			continue;
>> +
>> +		zs_free(meta->mem_pool, handle);
>> +	}
>> +
>> +	zcomp_destroy(comp);
>> +	zram_meta_free(meta);
>> +
>>  	/*
>>  	 * Revalidate disk out of the init_lock to avoid lockdep splat.
>>  	 * It's okay because disk's capacity is protected by init_lock
>> -- 
>> 1.9.1
>>

Attachment: signature.asc
Description: OpenPGP digital signature