[Devel] [PATCH hci-8.1 v3 5/6] drivers/md/push_backup: arm deadline timer under pb->lock

[Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>]

When push_backup_write acks last delayed request it sets
deadline_jiffies = S64_MAX and the next fired timer uses this
for mod_timer(): expiry is set to some very distant future.

Previously a later push_backup_write() would accidentally
repair the expiry; since it no longer touches the timer, nothing
does until stop.

Make the decision and the arming atomic: call mod_timer() under
pb->lock at both sites, and do not re-arm when nothing is delayed or
the backup is not alive. mod_timer() and queue_work() are fine under
a spinlock, and the timer base lock is dropped while the callback
runs, so there is no lock inversion.

The callback racing with push_backup_stop() was never handled either:
timer_delete_sync_try() is try_to_del_timer_sync(), it returns -1
without waiting when the callback is running, and we only WARN on that.
A callback blocked on pb->lock could thus outlive stop and re-arm the
timer or queue do_pb_work() after flush_workqueue(), expiring a later
session. Use timer_delete_sync() in push_backup_stop() and
timer_shutdown_sync() in pb_destroy() to wait for it; the work it may
have queued is flushed by flush_workqueue()/destroy_workqueue() which
follow.

As a bonus the rcu_read_lock()/synchronize_rcu() dance between
postpone_if_required_for_backup() and push_backup_stop() is no longer
needed: cleanup_backup() clears pb->alive under pb->lock, the timer is
only armed under pb->lock while pb->alive is set, and the callback
itself is waited for. Drop it.

https://virtuozzo.atlassian.net/browse/VSTOR-152912
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
v2: new patch
v3: bail out of pb_timer_func() when !pb->alive; wait for a running
    callback with timer_delete_sync() in push_backup_stop() and
    timer_shutdown_sync() in pb_destroy() instead of WARNing on
    timer_delete_sync_try() < 0; fixed the commit message claim about it

 drivers/md/dm-push-backup.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/md/dm-push-backup.c b/drivers/md/dm-push-backup.c
index b1861204f6e3e..6951e3fcdbfbf 100644
--- a/drivers/md/dm-push-backup.c
+++ b/drivers/md/dm-push-backup.c
@@ -241,23 +241,24 @@ static void pb_timer_func(struct timer_list *timer)
 
 	spin_lock_irqsave(&pb->lock, flags);
 	deadline = pb->deadline_jiffies;
-	spin_unlock_irqrestore(&pb->lock, flags);
-
+	if (!pb->alive || deadline == S64_MAX)
+		goto unlock;
 	if (unlikely(time_before64(now, deadline)))
 		mod_timer(timer, jiffies + (deadline - now) + 1);
 	else
 		queue_work(pb->wq, &pb->worker);
+unlock:
+	spin_unlock_irqrestore(&pb->lock, flags);
 }
 
 static int postpone_if_required_for_backup(struct push_backup *pb,
 					   struct request *rq)
 {
-	bool queue_timer = false;
 	struct pb_bio *pbio;
 	unsigned long flags;
+	bool wake = false;
 	int ret = 0;
 
-	rcu_read_lock(); /* See push_backup_stop() */
 	spin_lock_irqsave(&pb->lock, flags);
 	if (!pb->alive)
 		goto unlock;
@@ -272,16 +273,13 @@ static int postpone_if_required_for_backup(struct push_backup *pb,
 	pb->nr_delayed += 1;
 	if (pb->nr_delayed == 1) {
 		pb->deadline_jiffies = get_jiffies_64() + pb->timeout_in_jiffies;
-		queue_timer = true;
+		mod_timer(&pb->deadline_timer, jiffies + pb->timeout_in_jiffies + 1);
+		wake = true;
 	}
 unlock:
 	spin_unlock_irqrestore(&pb->lock, flags);
 
-	if (queue_timer)
-		mod_timer(&pb->deadline_timer, jiffies + pb->timeout_in_jiffies + 1);
-	rcu_read_unlock();
-
-	if (queue_timer)
+	if (wake)
 		wake_up_interruptible(&pb->waitq);
 
 	return ret;
@@ -400,9 +398,7 @@ static int push_backup_stop(struct push_backup *pb,
 		return -EBADF;
 	cleanup_backup(pb);
 
-	/* Wait postpone_if_required_for_backup() starts timer */
-	synchronize_rcu();
-	WARN_ON_ONCE(timer_delete_sync_try(&pb->deadline_timer) < 0);
+	timer_delete_sync(&pb->deadline_timer);
 	flush_workqueue(pb->wq);
 
 	spin_lock_irq(&pb->lock);
@@ -628,7 +624,7 @@ static void pb_destroy(struct push_backup *pb)
 {
 	WARN_ON_ONCE(pb->nr_delayed);
 
-	WARN_ON_ONCE(timer_delete_sync_try(&pb->deadline_timer) < 0);
+	timer_shutdown_sync(&pb->deadline_timer);
 	if (pb->wq)
 		destroy_workqueue(pb->wq);
 	kvfree(pb->map); /* Is's not zero if stop was not called */
-- 
2.43.5

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