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