[Devel] [RFC PATCH vz9 v6 05/62] dm-ploop: remove unnecessary lock

[Alexander Atanasov]

On 9.01.25 5:24, Pavel Tikhomirov wrote:
> 
> 
> On 1/9/25 11:17, Pavel Tikhomirov wrote:
>> Why is lock unnecessary?
>>
>> For instance what will happen if ploop_md_make_dirty and 
>> ploop_make_md_wb are executed concurrently?


You are right, locks are reimplemented in later patches - both
md->lock and bat_lock are used. The idea to use only bitops didn't work.


>>
>>
>> Thread 1:                                  Thread 2:
>> ploop_make_md_wb() {
>>                                             ploop_md_make_dirty() {
>>
>> WARN_ON_ONCE(test_bit(MD_WRITEBACK, &md->status));
>>    set_bit(MD_WRITEBACK, &md->status);
>>                                               if (! 
>> test_and_set_bit(MD_DIRTY, &md->status)) ...
>>                                             }
>> }
> 
> Sorry for wrap destroying alignment... fixed version:
> 
> Thread 1:               Thread 2:
> ploop_make_md_wb() {
>                          ploop_md_make_dirty() {
>                            WARN_ON_ONCE(test_bit(MD_WRITEBACK,
>                                                  &md->status));
>    set_bit(MD_WRITEBACK,
>            &md->status);
>                            if (!test_and_set_bit(MD_DIRTY,
>                                                  &md->status)) ...
>                          }
> }
> 
>>
>> The warning will be false-negative.
>>
>> In case we have concurrent ploop_advance_local_after_bat_wb and 
>> ploop_make_md_wb, there can be a false-positive warning.
>>
>> By the above I want to emphasize that in previous code with locks 
>> everywhere, several operations with md->status in one block were 
>> "together" atomic, and after switching to bit ops and removing lock we 
>> only have atomicity for single operations, not for blocks of 
>> operations, that can lead to any unexpected consequences.
>>
>> I think we need semi-formal proof why all those changes with locks are 
>> valid. And not just saying switching lock to atomic is ok without any 
>> proof.
>>
>> On 12/6/24 05:55, Alexander Atanasov wrote:
>>> now proper bitops are used for status we do not need to lock.
>>>
>>> https://virtuozzo.atlassian.net/browse/VSTOR-91820
>>> Signed-off-by: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
>>> ---
>>>   drivers/md/dm-ploop-cmd.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/md/dm-ploop-cmd.c b/drivers/md/dm-ploop-cmd.c
>>> index 8bbb1680c579..61daaf8415c9 100644
>>> --- a/drivers/md/dm-ploop-cmd.c
>>> +++ b/drivers/md/dm-ploop-cmd.c
>>> @@ -273,9 +273,7 @@ static int ploop_write_zero_cluster_sync(struct 
>>> ploop *ploop,
>>>   static void ploop_make_md_wb(struct ploop *ploop, struct md_page *md)
>>>   {
>>> -    write_lock_irq(&ploop->bat_rwlock);
>>>       set_bit(MD_WRITEBACK, &md->status);
>>> -    write_unlock_irq(&ploop->bat_rwlock);
>>>   }
>>>   static int ploop_grow_relocate_cluster(struct ploop *ploop,
>>
> 

-- 
Regards,
Alexander Atanasov