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

[Pavel Tikhomirov]

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

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.