dm-writecache: convert wait queue to wake_up_process

This is second version of this patch - it also removes the label continue_locked, because it is no longer needed. If forgot to refresh the patch before sending it, so I sent an olded version.

From: Mikulas Patocka <[email protected]>
Subject: [patch 2/3 v2] dm-writecache: convert wait queue to wake_up_process

If there's just one process that can wait on a queue, we can use wake_up_process. According to Linus, it is safe to call wake_up_process on a process even if the process may be doing something else.

Signed-off-by Mikulas Patocka <[email protected]>

---
 drivers/md/dm-writecache.c |   34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

Index: linux-2.6/drivers/md/dm-writecache.c
===================================================================

drivers/md/dm-writecache.c +0 -0

@@ -1273,10 +1272,11 @@ static void writecache_writeback_endio(s
 	struct dm_writecache *wc = wb->wc;
 	unsigned long flags;

Quoted 25 lines · Mikulas Patocka

This is second version of this patch - it also removes the label continue_locked, because it is no longer needed. If forgot to refresh the patch before sending it, so I sent an olded version.

From: Mikulas Patocka <[email protected]>
Subject: [patch 2/3 v2] dm-writecache: convert wait queue to wake_up_process

If there's just one process that can wait on a queue, we can use wake_up_process. According to Linus, it is safe to call wake_up_process on a process even if the process may be doing something else.

Signed-off-by Mikulas Patocka <[email protected]>

---
 drivers/md/dm-writecache.c |   34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

Index: linux-2.6/drivers/md/dm-writecache.c
===================================================================

drivers/md/dm-writecache.c +0 -0

@@ -1273,10 +1272,11 @@ static void writecache_writeback_endio(s
 	struct dm_writecache *wc = wb->wc;
 	unsigned long flags;

drivers/md/dm-writecache.c static void writecache_writeback_endio(s · line 1280

Quoted 10 lines

drivers/md/dm-writecache.c +4 -3

 
-	raw_spin_lock_irqsave(&wc->endio_thread_wait.lock, flags);
+	raw_spin_lock_irqsave(&wc->endio_list_lock, flags);
+	if (unlikely(list_empty(&wc->endio_list)))
+		wake_up_process(wc->endio_thread);
 	list_add_tail(&wb->endio_entry, &wc->endio_list);
-	swake_up_locked(&wc->endio_thread_wait);
-	raw_spin_unlock_irqrestore(&wc->endio_thread_wait.lock, flags);
+	raw_spin_unlock_irqrestore(&wc->endio_list_lock, flags);
 }

I'm not following the logic you're using for the above pattern of using wake_up_process if the list is empty.. seems unintuitive.

Given you add to the list (be it endio here, or flush elsewhere), why not just add to the list and then always wake_up_process()?

On Fri, Jun 08 2018 at 11:13P -0400,
Mike Snitzer <[email protected]> wrote:

I'd prefer the following, so please help me understand why you aren't doing it this way. Thanks.

drivers/md/dm-writecache.c +3 -4

index 5961c7794ef3..17cd81ce6ec3 100644
@@ -1103,9 +1103,9 @@ static int writecache_flush_thread(void *data)
 
 static void writecache_offload_bio(struct dm_writecache *wc, struct bio *bio)
 {
-	if (bio_list_empty(&wc->flush_list))
-		wake_up_process(wc->flush_thread);
+	lockdep_assert_held(&wc->lock);
 	bio_list_add(&wc->flush_list, bio);
+	wake_up_process(wc->flush_thread);
 }
 
 static int writecache_map(struct dm_target *ti, struct bio *bio)

@@ -1295,10 +1295,9 @@ static void writecache_writeback_endio(struct bio *bio)
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&wc->endio_list_lock, flags);
-	if (unlikely(list_empty(&wc->endio_list)))
-		wake_up_process(wc->endio_thread);
 	list_add_tail(&wb->endio_entry, &wc->endio_list);
 	raw_spin_unlock_irqrestore(&wc->endio_list_lock, flags);
+	wake_up_process(wc->endio_thread);
 }
 
 static void writecache_copy_endio(int read_err, unsigned long write_err, void *ptr)

@@ -1309,10 +1308,9 @@ static void writecache_copy_endio(int read_err, unsigned long write_err, void *p
 	c->error = likely(!(read_err | write_err)) ? 0 : -EIO;

Quoted 36 lines · Mike Snitzer

On Fri, Jun 08 2018 at 11:13P -0400,
Mike Snitzer <[email protected]> wrote:

I'd prefer the following, so please help me understand why you aren't doing it this way. Thanks.

drivers/md/dm-writecache.c +3 -4

index 5961c7794ef3..17cd81ce6ec3 100644
@@ -1103,9 +1103,9 @@ static int writecache_flush_thread(void *data)
 
 static void writecache_offload_bio(struct dm_writecache *wc, struct bio *bio)
 {
-	if (bio_list_empty(&wc->flush_list))
-		wake_up_process(wc->flush_thread);
+	lockdep_assert_held(&wc->lock);
 	bio_list_add(&wc->flush_list, bio);
+	wake_up_process(wc->flush_thread);
 }
 
 static int writecache_map(struct dm_target *ti, struct bio *bio)

@@ -1295,10 +1295,9 @@ static void writecache_writeback_endio(struct bio *bio)
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&wc->endio_list_lock, flags);
-	if (unlikely(list_empty(&wc->endio_list)))
-		wake_up_process(wc->endio_thread);
 	list_add_tail(&wb->endio_entry, &wc->endio_list);
 	raw_spin_unlock_irqrestore(&wc->endio_list_lock, flags);
+	wake_up_process(wc->endio_thread);
 }
 
 static void writecache_copy_endio(int read_err, unsigned long write_err, void *ptr)

@@ -1309,10 +1308,9 @@ static void writecache_copy_endio(int read_err, unsigned long write_err, void *p
 	c->error = likely(!(read_err | write_err)) ? 0 : -EIO;

drivers/md/dm-writecache.c static void writecache_copy_endio(int read_err, unsigned long write_err, void *p · line 1316

Quoted 10 lines

drivers/md/dm-writecache.c +1 -2

 
 	raw_spin_lock_irq(&wc->endio_list_lock);
-	if (unlikely(list_empty(&wc->endio_list)))
-		wake_up_process(wc->endio_thread);
 	list_add_tail(&c->endio_entry, &wc->endio_list);
 	raw_spin_unlock_irq(&wc->endio_list_lock);
+	wake_up_process(wc->endio_thread);
 }
 
 static void __writecache_endio_pmem(struct dm_writecache *wc, struct list_head *list)

This is incorrect.

When you drop the spinlock, the endio thread may already take the item (it may take it even before wake_up_process is called). When the endio thread consumes all the items, the user may unload the device. When the user unloads the device, wc is freed and wc->endio_thread points to a non-existing process - and now you dereference freed "wc" structure and call wake_up_process on non-existing process and cause a crash.

wake_up_process must be inside the spinlock to avoid this race condition.

Quoted 25 lines · Mikulas Patocka

This is second version of this patch - it also removes the label continue_locked, because it is no longer needed. If forgot to refresh the patch before sending it, so I sent an olded version.

From: Mikulas Patocka <[email protected]>
Subject: [patch 2/3 v2] dm-writecache: convert wait queue to wake_up_process

If there's just one process that can wait on a queue, we can use wake_up_process. According to Linus, it is safe to call wake_up_process on a process even if the process may be doing something else.

Signed-off-by Mikulas Patocka <[email protected]>

---
 drivers/md/dm-writecache.c |   34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

Index: linux-2.6/drivers/md/dm-writecache.c
===================================================================

drivers/md/dm-writecache.c +0 -0

@@ -1273,10 +1272,11 @@ static void writecache_writeback_endio(s
 	struct dm_writecache *wc = wb->wc;
 	unsigned long flags;

drivers/md/dm-writecache.c static void writecache_writeback_endio(s · line 1280

Quoted 10 lines

drivers/md/dm-writecache.c +4 -3

 
-	raw_spin_lock_irqsave(&wc->endio_thread_wait.lock, flags);
+	raw_spin_lock_irqsave(&wc->endio_list_lock, flags);
+	if (unlikely(list_empty(&wc->endio_list)))
+		wake_up_process(wc->endio_thread);
 	list_add_tail(&wb->endio_entry, &wc->endio_list);
-	swake_up_locked(&wc->endio_thread_wait);
-	raw_spin_unlock_irqrestore(&wc->endio_thread_wait.lock, flags);
+	raw_spin_unlock_irqrestore(&wc->endio_list_lock, flags);
 }

I'm not following the logic you're using for the above pattern of using wake_up_process if the list is empty.. seems unintuitive.

Given you add to the list (be it endio here, or flush elsewhere), why not just add to the list and then always wake_up_process()?

Mike

Quoted 42 lines · Mike Snitzer

Quoted 25 lines · Mikulas Patocka

This is second version of this patch - it also removes the label continue_locked, because it is no longer needed. If forgot to refresh the patch before sending it, so I sent an olded version.

From: Mikulas Patocka <[email protected]>
Subject: [patch 2/3 v2] dm-writecache: convert wait queue to wake_up_process

If there's just one process that can wait on a queue, we can use wake_up_process. According to Linus, it is safe to call wake_up_process on a process even if the process may be doing something else.

Signed-off-by Mikulas Patocka <[email protected]>

---
 drivers/md/dm-writecache.c |   34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

Index: linux-2.6/drivers/md/dm-writecache.c
===================================================================

drivers/md/dm-writecache.c +0 -0

@@ -1273,10 +1272,11 @@ static void writecache_writeback_endio(s
 	struct dm_writecache *wc = wb->wc;
 	unsigned long flags;

drivers/md/dm-writecache.c static void writecache_writeback_endio(s · line 1280

Quoted 10 lines

drivers/md/dm-writecache.c +4 -3

 
-	raw_spin_lock_irqsave(&wc->endio_thread_wait.lock, flags);
+	raw_spin_lock_irqsave(&wc->endio_list_lock, flags);
+	if (unlikely(list_empty(&wc->endio_list)))
+		wake_up_process(wc->endio_thread);
 	list_add_tail(&wb->endio_entry, &wc->endio_list);
-	swake_up_locked(&wc->endio_thread_wait);
-	raw_spin_unlock_irqrestore(&wc->endio_thread_wait.lock, flags);
+	raw_spin_unlock_irqrestore(&wc->endio_list_lock, flags);
 }

I'm not following the logic you're using for the above pattern of using wake_up_process if the list is empty.. seems unintuitive.

Quoted 4 lines

Given you add to the list (be it endio here, or flush elsewhere), why not just add to the list and then always wake_up_process()?

Mike

Because wake_up_process is costly (it takes a spinlock on the process). If multiple CPUs are simultaneously hammering on a spinlock, it degrades performance.

The process checks if the list is empty before going to sleep (and doesn't sleep if it is non-empty) - consequently, if the process goes to sleep, the list must have been empty.

So, we can wake the process up only once - when the list transitions from empty to non-empty - we don't have to wake it up with every item added to the list.

Originally, the code was like this:

raw_spin_lock_irqsave(&wc->endio_list_lock, flags); need_wake = list_empty(&wc->endio_list); list_add_tail(&wb->endio_entry, &wc->endio_list); if (need_wake)

	wake_up_process(wc->endio_thread);

raw_spin_unlock_irqrestore(&wc->endio_thread_wait.lock, flags);

However, because the target process takes the spinlock too, we can wake it up before we add the entry to the list - it doesn't matter here if we wake it before or after adding the entry to the list, because the target process will take the same spinlock when it is woken up.

Calling wake_up_process before list_add_tail results in slightly smaller code.