Re: [Devel] [RFC PATCH vz9 v6 12/62] dm-ploop: WIP move from wq to kthread

[Pavel Tikhomirov]

On 1/10/25 18:36, Alexander Atanasov wrote:
On 10.01.25 12:28, Pavel Tikhomirov wrote:


On 12/6/24 05:55, Alexander Atanasov wrote:
This is a base for moving to multithreaded model.

https://virtuozzo.atlassian.net/browse/VSTOR-91821
Signed-off-by: Alexander Atanasov <alexander.atana...@virtuozzo.com>
---
  drivers/md/dm-ploop-map.c    | 65 ++++++++++++++++++++++++++++++++----
  drivers/md/dm-ploop-target.c | 60 +++++++++++++++++++++++++++++++++
  drivers/md/dm-ploop.h        | 10 ++++++
  3 files changed, 128 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index 0240d6fc376c..28bced7684e5 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -338,6 +338,16 @@ static int ploop_split_pio_to_list(struct ploop 
*ploop, struct pio *pio,
  }
  ALLOW_ERROR_INJECTION(ploop_split_pio_to_list, ERRNO);
+#define USE_KTHREAD
+static void ploop_schedule_work(struct ploop *ploop)
+{
+#ifndef USE_KTHREAD
+    queue_work(ploop->wq, &ploop->worker);
+#else
+    wake_up_process(ploop->kt_worker->task);
+#endif
+}
+
  static void ploop_dispatch_pio(struct ploop *ploop, struct pio *pio)
  {
      struct llist_head *list = (struct llist_head *)&ploop- 
>pios[pio->queue_list_id];
@@ -361,7 +371,7 @@ void ploop_dispatch_pios(struct ploop *ploop, 
struct pio *pio,
              ploop_dispatch_pio(ploop, pio);
      }
-    queue_work(ploop->wq, &ploop->worker);
+    ploop_schedule_work(ploop);
  }
  static bool ploop_delay_if_md_busy(struct ploop *ploop, struct 
md_page *md,
@@ -693,7 +703,7 @@ static void ploop_complete_cow(struct ploop_cow 
*cow, blk_status_t bi_status)
      ploop_queue_or_fail(ploop, blk_status_to_errno(bi_status), 
cow_pio);
-    queue_work(ploop->wq, &ploop->worker);
+    ploop_schedule_work(ploop);
      ploop_free_pio_with_pages(ploop, cow->aux_pio);
      kmem_cache_free(cow_cache, cow);
  }
@@ -1142,7 +1152,7 @@ static void ploop_queue_resubmit(struct pio *pio)
      llist_add((struct llist_node *)(&pio->list), &ploop- 
>llresubmit_pios);
-    queue_work(ploop->wq, &ploop->worker);
+    ploop_schedule_work(ploop);
  }
  static void ploop_check_standby_mode(struct ploop *ploop, long res)
@@ -1815,9 +1825,8 @@ static void process_ploop_fsync_work(struct 
ploop *ploop)
      }
  }
-void do_ploop_work(struct work_struct *ws)
+void do_ploop_run_work(struct ploop *ploop)
  {
-    struct ploop *ploop = container_of(ws, struct ploop, worker);
      LIST_HEAD(deferred_pios);
      struct llist_node *llembedded_pios;
      struct llist_node *lldeferred_pios;
@@ -1829,12 +1838,14 @@ void do_ploop_work(struct work_struct *ws)
      current->flags |= PF_IO_THREAD|PF_LOCAL_THROTTLE|PF_MEMALLOC_NOIO;
      llembedded_pios = llist_del_all(&ploop->pios[PLOOP_LIST_PREPARE]);
+    smp_wmb(); /* */
+
      lldeferred_pios = llist_del_all(&ploop- 
>pios[PLOOP_LIST_DEFERRED]);
      lldiscard_pios = llist_del_all(&ploop->pios[PLOOP_LIST_DISCARD]);
      llcow_pios = llist_del_all(&ploop->pios[PLOOP_LIST_COW]);
      llresubmit = llist_del_all(&ploop->llresubmit_pios);
-    /* add old deferred to the list */
+    /* add old deferred back to the list */
      if (lldeferred_pios) {
          struct llist_node *pos, *t;
          struct pio *pio;
@@ -1866,6 +1877,46 @@ void do_ploop_work(struct work_struct *ws)
      current->flags = old_flags;
  }
+void do_ploop_work(struct work_struct *ws)
+{
+    struct ploop *ploop = container_of(ws, struct ploop, worker);
+
+    do_ploop_run_work(ploop);
+}
+
+int ploop_worker(void *data)
+{
+    struct ploop_worker *worker = data;
+    struct ploop *ploop = worker->ploop;
+
+    for (;;) {
+        set_current_state(TASK_INTERRUPTIBLE);
+
+        if (kthread_should_stop()) {
+            __set_current_state(TASK_RUNNING);
+            break;
+        }
+#ifdef USE_KTHREAD
+        smp_rmb(); /* */

1. This requires explanation both in comments and in commit message. 
What reads and what writes do you try to separate by memory barriers?

2. It is very hard to guess. It can't be to divide task interruptible 
state setting and llist_empty checks, because when you should've used 
smp_mb (as you have both write and read). It can be to divide 
kthread_should_stop read and llist_empty check reads. But, if it was 
so, then why do we have smp_rmb (and not wmb) in ploop_destroy where 
we have kthread_stop which is effectively write.

This we alrady discussed with andrey - barriers are leftovers which i 
missed to remove.

3. Side note, here we probably need atomic flag setting else flugs can 
get corrupted in case concurrent ploop_destroy->kthread_stop.

do_ploop_run_work() {
   current->flags |= PF_IO_THREAD|PF_LOCAL_THROTTLE|PF_MEMALLOC_NOIO;
}


That is interesting ... as we play a lot with those flags.
Even if we implement some atomicity inside ploop code - how we will 
protect them if changed from other kernel code.
i will think about it.

4. Also the use of smp_rmb in ploop_destroy looks like separating 
llist_empty reads between themselves on while loop iterations. But 
it's unclear to me why we might need it...

5. Also note that wake_up_process already has implicit write/read 
memory bariers on succesful wakeup.

barriers are removed - it was an early experiment


Please write some explanations, else reviewing it is too complex.

These patches are step by step as the work progressed and are still an 
RFC.  So to make things easier it is good to check the final result
- some of the issues are resolved there and some patches are to be 
merged but i give priority to bugs/design issues first.


Yes I understand, I just got lost between those barriers =) Please try 
to write followups to major things which were already resolved, just to 
keep everybody updated, I seem to miss at least partially messages about 
barriers.



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

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