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

[Pavel Tikhomirov]

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?
> 
> 
> 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,
> 

-- 
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.