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.atana...@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.

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Reply via email to