Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request

[Ganesh Mahendran <opensource.ganesh@gmail.com>]

Hello, Sergey

2015-01-31 19:07 GMT+08:00 Sergey Senozhatsky <sergey.senozhatsky@gmail.com>:
> On (01/31/15 16:50), Ganesh Mahendran wrote:
>> >> > after umount we still have init device. so, *theoretically*, we
>> >> > can see something like
>> >> >
>> >> >         CPU0                            CPU1
>> >> > umount
>> >> > reset_store
>> >> > bdev->bd_holders == 0                   mount
>> >> > ...                                     zram_make_request()
>> >> > zram_reset_device()
> [..]
>
>
>>
>> Maybe I did not explain clearly. I send a patch about this issue:
>>
>> https://patchwork.kernel.org/patch/5754041/
>
>
> excuse me? explain to me clearly what? my finding and my analysis?

Sorry, I missed this mail
https://lkml.org/lkml/2015/1/27/1029

That's why I ask questions in this
https://lkml.org/lkml/2015/1/29/580
after Minchan's description.

>
>
> this is the second time in a week that you hijack someone's work
> and you don't even bother to give any credit to people.
>
>
> Minchan moved zram_meta_free(meta) out of init_lock here
> https://lkml.org/lkml/2015/1/21/29
>
> I proposed to also move zs_free() of meta->handles here
> https://lkml.org/lkml/2015/1/21/384

I thought you wanted move the code block after
       up_write(&zram->init_lock);

And I found the code block can be even encapsulated in
zram_meta_free().

That's why I sent:
https://lkml.org/lkml/2015/1/24/50

>
>
> ... so what happened then -- you jumped in and sent a patch.
> https://lkml.org/lkml/2015/1/24/50
>
>
> Minchan sent you a hint https://lkml.org/lkml/2015/1/26/471
>
>>   but it seems the patch is based on my recent work "zram: free meta out of init_lock".
>
>
>
>  "the patch is based on my work"!
>
>
>
> now, for the last few days we were discussing init_lock and I first
> expressed my concerns and spoke about 'free' vs. 'use' problem
> here (but still didn't have enough spare to submit, besides we are in
> the middle of reset/init/write rework)
>
> https://lkml.org/lkml/2015/1/27/1029
>
>>
>>bdev->bd_holders protects from resetting device which has read/write
>>operation ongoing on the onther CPU.
>>
>>I need to refresh on how ->bd_holders actually incremented/decremented.
>>can the following race condition take a place?
>>
>>        CPU0                                    CPU1
>>reset_store()
>>bdev->bd_holders == false
>>                                        zram_make_request
>>                                                -rm- down_read(&zram->init_lock);
>>                                        init_done(zram) == true
>>zram_reset_device()                     valid_io_request()
>>                                        __zram_make_request
>>down_write(&zram->init_lock);           zram_bvec_rw
>>[..]
>>set_capacity(zram->disk, 0);
>>zram->init_done = false;
>>kick_all_cpus_sync();                   zram_bvec_write or zram_bvec_read()
>>zram_meta_free(zram->meta);
>>zcomp_destroy(zram->comp);              zcomp_compress() or zcomp_decompress()

Sorry, I did not check this mail.

>>
>
>
> and later here https://lkml.org/lkml/2015/1/29/645
>
>>
>>after umount we still have init device. so, *theoretically*, we
>>can see something like
>>
>>
>>        CPU0                            CPU1
>>umount
>>reset_store
>>bdev->bd_holders == 0                   mount
>>...                                     zram_make_request()
>>zram_reset_device()
>>
>
>
>
> so what happened next? your patch happened next.
> with quite familiar problem description
>
>>
>>      CPU0                    CPU1
>> t1:  bdput
>> t2:                          mount /dev/zram0 /mnt
>> t3:  zram_reset_device
>>
>
> and now you say that I don't understant something in "your analysis"?
>
>
>
> stop doing this. this is not how it works.
>
>
>         -ss
>