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

[Sergey Senozhatsky]

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?


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


... 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()
>


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

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