Enable PM hibernation on guest VMs

From: Munehisa Kamata <[email protected]

Add freeze, thaw and restore callbacks for PM suspend and hibernation support. All frontend drivers that needs to use PM_HIBERNATION/PM_SUSPEND events, need to implement these xenbus_driver callbacks.
The freeze handler stops a block-layer queue and disconnect the frontend from the backend while freeing ring_info and associated resources. The restore handler re-allocates ring_info and re-connect to the backend, so the rest of the kernel can continue to use the block device transparently. Also, the handlers are used for both PM suspend and hibernation so that we can keep the existing suspend/resume callbacks for Xen suspend without modification. Before disconnecting from backend, we need to prevent any new IO from being queued and wait for existing IO to complete.

Freeze/unfreeze of the queues will guarantee that there are no requests in use on the shared ring.

Note:For older backends,if a backend doesn't have commit'12ea729645ace' xen/blkback: unmap all persistent grants when frontend gets disconnected, the frontend may see massive amount of grant table warning when freeing resources.

[   36.852659] deferring g.e. 0xf9 (pfn 0xffffffffffffffff)
[   36.855089] xen:grant_table: WARNING:e.g. 0x112 still in use!

In this case, persistent grants would need to be disabled.

[Anchal Changelog: Removed timeout/request during blkfront freeze. Fixed major part of the code to work with blk-mq]

Signed-off-by Anchal Agarwal <[email protected]>

Signed-off-by Munehisa Kamata <[email protected]>

---
 drivers/block/xen-blkfront.c | 119 ++++++++++++++++++++++++++++++++---
 1 file changed, 112 insertions(+), 7 deletions(-)

drivers/block/xen-blkfront.c +66 -7

index 478120233750..d715ed3cb69a 100644
@@ -47,6 +47,8 @@
 #include <linux/bitmap.h>
 #include <linux/list.h>
 #include <linux/workqueue.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
 
 #include <xen/xen.h>
 #include <xen/xenbus.h>

@@ -79,6 +81,8 @@ enum blkif_state {
 	BLKIF_STATE_DISCONNECTED,
 	BLKIF_STATE_CONNECTED,
 	BLKIF_STATE_SUSPENDED,
+	BLKIF_STATE_FREEZING,
+	BLKIF_STATE_FROZEN
 };
 
 struct grant {

@@ -220,6 +224,7 @@ struct blkfront_info
 	struct list_head requests;
 	struct bio_list bio_list;
 	struct list_head info_list;
+	struct completion wait_backend_disconnected;
 };
 
 static unsigned int nr_minors;

@@ -261,6 +266,7 @@ static DEFINE_SPINLOCK(minor_lock);
 static int blkfront_setup_indirect(struct blkfront_ring_info *rinfo);
 static void blkfront_gather_backend_features(struct blkfront_info *info);
 static int negotiate_mq(struct blkfront_info *info);
+static void __blkif_free(struct blkfront_info *info);
 
 static int get_id_from_freelist(struct blkfront_ring_info *rinfo)
 {

@@ -995,6 +1001,7 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size,
 	info->sector_size = sector_size;
 	info->physical_sector_size = physical_sector_size;
 	blkif_set_queue_limits(info);
+	init_completion(&info->wait_backend_disconnected);
 
 	return 0;
 }

@@ -1218,6 +1225,8 @@ static void xlvbd_release_gendisk(struct blkfront_info *info)
 /* Already hold rinfo->ring_lock. */
 static inline void kick_pending_request_queues_locked(struct blkfront_ring_info *rinfo)
 {
+	if (unlikely(rinfo->dev_info->connected == BLKIF_STATE_FREEZING))
+		return;
 	if (!RING_FULL(&rinfo->ring))
 		blk_mq_start_stopped_hw_queues(rinfo->dev_info->rq, true);
 }

@@ -1341,8 +1350,6 @@ static void blkif_free_ring(struct blkfront_ring_info *rinfo)
 
 static void blkif_free(struct blkfront_info *info, int suspend)
 {
-	unsigned int i;
-
 	/* Prevent new requests being issued until we fix things up. */
 	info->connected = suspend ?
 		BLKIF_STATE_SUSPENDED : BLKIF_STATE_DISCONNECTED;

@@ -1350,6 +1357,13 @@ static void blkif_free(struct blkfront_info *info, int suspend)
 	if (info->rq)
 		blk_mq_stop_hw_queues(info->rq);
 
+	__blkif_free(info);
+}
+
+static void __blkif_free(struct blkfront_info *info)
+{
+	unsigned int i;
+
 	for (i = 0; i < info->nr_rings; i++)
 		blkif_free_ring(&info->rinfo[i]);
 

@@ -1553,8 +1567,10 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 	struct blkfront_ring_info *rinfo = (struct blkfront_ring_info *)dev_id;
 	struct blkfront_info *info = rinfo->dev_info;
 
-	if (unlikely(info->connected != BLKIF_STATE_CONNECTED))
-		return IRQ_HANDLED;
+	if (unlikely(info->connected != BLKIF_STATE_CONNECTED)) {
+		if (info->connected != BLKIF_STATE_FREEZING)
+			return IRQ_HANDLED;
+	}
 
 	spin_lock_irqsave(&rinfo->ring_lock, flags);
  again:

@@ -2020,6 +2036,7 @@ static int blkif_recover(struct blkfront_info *info)
 	struct bio *bio;
 	unsigned int segs;
 
+	bool frozen = info->connected == BLKIF_STATE_FROZEN;
 	blkfront_gather_backend_features(info);
 	/* Reset limits changed by blk_mq_update_nr_hw_queues(). */
 	blkif_set_queue_limits(info);

@@ -2046,6 +2063,9 @@ static int blkif_recover(struct blkfront_info *info)
 		kick_pending_request_queues(rinfo);
 	}
 
+	if (frozen)
+		return 0;
+
 	list_for_each_entry_safe(req, n, &info->requests, queuelist) {
 		/* Requeue pending requests (flush or discard) */
 		list_del_init(&req->queuelist);

@@ -2359,6 +2379,7 @@ static void blkfront_connect(struct blkfront_info *info)
 
 		return;
 	case BLKIF_STATE_SUSPENDED:
+	case BLKIF_STATE_FROZEN:
 		/*
 		 * If we are recovering from suspension, we need to wait
 		 * for the backend to announce it's features before

@@ -2476,12 +2497,37 @@ static void blkback_changed(struct xenbus_device *dev,
 		break;
 
 	case XenbusStateClosed:
-		if (dev->state == XenbusStateClosed)
+		if (dev->state == XenbusStateClosed) {
+			if (info->connected == BLKIF_STATE_FREEZING) {
+				__blkif_free(info);
+				info->connected = BLKIF_STATE_FROZEN;
+				complete(&info->wait_backend_disconnected);
+				break;
+			}
+
 			break;
+		}
+
+		/*
+		 * We may somehow receive backend's Closed again while thawing
+		 * or restoring and it causes thawing or restoring to fail.
+		 * Ignore such unexpected state anyway.
+		 */
+		if (info->connected == BLKIF_STATE_FROZEN &&
+				dev->state == XenbusStateInitialised) {
+			dev_dbg(&dev->dev,
+					"ignore the backend's Closed state: %s",
+					dev->nodename);
+			break;
+		}
 		/* fall through */
 	case XenbusStateClosing:
-		if (info)
-			blkfront_closing(info);
+		if (info) {
+			if (info->connected == BLKIF_STATE_FREEZING)
+				xenbus_frontend_closed(dev);
+			else
+				blkfront_closing(info);
+		}
 		break;
 	}
 }

@@ -2625,6 +2671,62 @@ static void blkif_release(struct gendisk *disk, fmode_t mode)
 	mutex_unlock(&blkfront_mutex);
 }
 
+static int blkfront_freeze(struct xenbus_device *dev)
+{
+	unsigned int i;
+	struct blkfront_info *info = dev_get_drvdata(&dev->dev);
+	struct blkfront_ring_info *rinfo;
+	/* This would be reasonable timeout as used in xenbus_dev_shutdown() */
+	unsigned int timeout = 5 * HZ;
+	int err = 0;
+
+	info->connected = BLKIF_STATE_FREEZING;
+
+	blk_mq_freeze_queue(info->rq);
+	blk_mq_quiesce_queue(info->rq);

Quoted 14 lines · Anchal Agarwal

From: Munehisa Kamata <[email protected]

Add freeze, thaw and restore callbacks for PM suspend and hibernation support. All frontend drivers that needs to use PM_HIBERNATION/PM_SUSPEND events, need to implement these xenbus_driver callbacks.
The freeze handler stops a block-layer queue and disconnect the frontend from the backend while freeing ring_info and associated resources. The restore handler re-allocates ring_info and re-connect to the backend, so the rest of the kernel can continue to use the block device transparently. Also, the handlers are used for both PM suspend and hibernation so that we can keep the existing suspend/resume callbacks for Xen suspend without modification. Before disconnecting from backend, we need to prevent any new IO from being queued and wait for existing IO to complete.

This is different from Xen (xenstore) initiated suspension, as in that case Linux doesn't flush the rings or disconnects from the backend.

This is done so that in case suspensions fails the recovery doesn't need to reconnect the PV devices, and in order to speed up suspension time (ie: waiting for all queues to be flushed can take time as Linux supports multiqueue, multipage rings and indirect descriptors), and the backend could be contended if there's a lot of IO pressure from guests.

Linux already keeps a shadow of the ring contents, so in-flight requests can be re-issued after the frontend has reconnected during resume.

Quoted 199 lines

Freeze/unfreeze of the queues will guarantee that there are no requests in use on the shared ring.

Note:For older backends,if a backend doesn't have commit'12ea729645ace' xen/blkback: unmap all persistent grants when frontend gets disconnected, the frontend may see massive amount of grant table warning when freeing resources.

[   36.852659] deferring g.e. 0xf9 (pfn 0xffffffffffffffff)
[   36.855089] xen:grant_table: WARNING:e.g. 0x112 still in use!

In this case, persistent grants would need to be disabled.

[Anchal Changelog: Removed timeout/request during blkfront freeze. Fixed major part of the code to work with blk-mq]

Signed-off-by Anchal Agarwal <[email protected]>

Signed-off-by Munehisa Kamata <[email protected]>

---
 drivers/block/xen-blkfront.c | 119 ++++++++++++++++++++++++++++++++---
 1 file changed, 112 insertions(+), 7 deletions(-)

drivers/block/xen-blkfront.c +70 -7

index 478120233750..d715ed3cb69a 100644
@@ -47,6 +47,8 @@
 #include <linux/bitmap.h>
 #include <linux/list.h>
 #include <linux/workqueue.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
 
 #include <xen/xen.h>
 #include <xen/xenbus.h>

@@ -79,6 +81,8 @@ enum blkif_state {
 	BLKIF_STATE_DISCONNECTED,
 	BLKIF_STATE_CONNECTED,
 	BLKIF_STATE_SUSPENDED,
+	BLKIF_STATE_FREEZING,
+	BLKIF_STATE_FROZEN
 };
 
 struct grant {

@@ -220,6 +224,7 @@ struct blkfront_info
 	struct list_head requests;
 	struct bio_list bio_list;
 	struct list_head info_list;
+	struct completion wait_backend_disconnected;
 };
 
 static unsigned int nr_minors;

@@ -261,6 +266,7 @@ static DEFINE_SPINLOCK(minor_lock);
 static int blkfront_setup_indirect(struct blkfront_ring_info *rinfo);
 static void blkfront_gather_backend_features(struct blkfront_info *info);
 static int negotiate_mq(struct blkfront_info *info);
+static void __blkif_free(struct blkfront_info *info);
 
 static int get_id_from_freelist(struct blkfront_ring_info *rinfo)
 {

@@ -995,6 +1001,7 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size,
 	info->sector_size = sector_size;
 	info->physical_sector_size = physical_sector_size;
 	blkif_set_queue_limits(info);
+	init_completion(&info->wait_backend_disconnected);
 
 	return 0;
 }

@@ -1218,6 +1225,8 @@ static void xlvbd_release_gendisk(struct blkfront_info *info)
 /* Already hold rinfo->ring_lock. */
 static inline void kick_pending_request_queues_locked(struct blkfront_ring_info *rinfo)
 {
+	if (unlikely(rinfo->dev_info->connected == BLKIF_STATE_FREEZING))
+		return;
 	if (!RING_FULL(&rinfo->ring))
 		blk_mq_start_stopped_hw_queues(rinfo->dev_info->rq, true);
 }

@@ -1341,8 +1350,6 @@ static void blkif_free_ring(struct blkfront_ring_info *rinfo)
 
 static void blkif_free(struct blkfront_info *info, int suspend)
 {
-	unsigned int i;
-
 	/* Prevent new requests being issued until we fix things up. */
 	info->connected = suspend ?
 		BLKIF_STATE_SUSPENDED : BLKIF_STATE_DISCONNECTED;

@@ -1350,6 +1357,13 @@ static void blkif_free(struct blkfront_info *info, int suspend)
 	if (info->rq)
 		blk_mq_stop_hw_queues(info->rq);
 
+	__blkif_free(info);
+}
+
+static void __blkif_free(struct blkfront_info *info)
+{
+	unsigned int i;
+
 	for (i = 0; i < info->nr_rings; i++)
 		blkif_free_ring(&info->rinfo[i]);
 

@@ -1553,8 +1567,10 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 	struct blkfront_ring_info *rinfo = (struct blkfront_ring_info *)dev_id;
 	struct blkfront_info *info = rinfo->dev_info;
 
-	if (unlikely(info->connected != BLKIF_STATE_CONNECTED))
-		return IRQ_HANDLED;
+	if (unlikely(info->connected != BLKIF_STATE_CONNECTED)) {
+		if (info->connected != BLKIF_STATE_FREEZING)
+			return IRQ_HANDLED;
+	}
 
 	spin_lock_irqsave(&rinfo->ring_lock, flags);
  again:

@@ -2020,6 +2036,7 @@ static int blkif_recover(struct blkfront_info *info)
 	struct bio *bio;
 	unsigned int segs;
 
+	bool frozen = info->connected == BLKIF_STATE_FROZEN;
 	blkfront_gather_backend_features(info);
 	/* Reset limits changed by blk_mq_update_nr_hw_queues(). */
 	blkif_set_queue_limits(info);

@@ -2046,6 +2063,9 @@ static int blkif_recover(struct blkfront_info *info)
 		kick_pending_request_queues(rinfo);
 	}
 
+	if (frozen)
+		return 0;
+
 	list_for_each_entry_safe(req, n, &info->requests, queuelist) {
 		/* Requeue pending requests (flush or discard) */
 		list_del_init(&req->queuelist);

@@ -2359,6 +2379,7 @@ static void blkfront_connect(struct blkfront_info *info)
 
 		return;
 	case BLKIF_STATE_SUSPENDED:
+	case BLKIF_STATE_FROZEN:
 		/*
 		 * If we are recovering from suspension, we need to wait
 		 * for the backend to announce it's features before

@@ -2476,12 +2497,37 @@ static void blkback_changed(struct xenbus_device *dev,
 		break;
 
 	case XenbusStateClosed:
-		if (dev->state == XenbusStateClosed)
+		if (dev->state == XenbusStateClosed) {
+			if (info->connected == BLKIF_STATE_FREEZING) {
+				__blkif_free(info);
+				info->connected = BLKIF_STATE_FROZEN;
+				complete(&info->wait_backend_disconnected);
+				break;
+			}
+
 			break;
+		}
+
+		/*
+		 * We may somehow receive backend's Closed again while thawing
+		 * or restoring and it causes thawing or restoring to fail.
+		 * Ignore such unexpected state anyway.
+		 */
+		if (info->connected == BLKIF_STATE_FROZEN &&
+				dev->state == XenbusStateInitialised) {
+			dev_dbg(&dev->dev,
+					"ignore the backend's Closed state: %s",
+					dev->nodename);
+			break;
+		}
 		/* fall through */
 	case XenbusStateClosing:
-		if (info)
-			blkfront_closing(info);
+		if (info) {
+			if (info->connected == BLKIF_STATE_FREEZING)
+				xenbus_frontend_closed(dev);
+			else
+				blkfront_closing(info);
+		}
 		break;
 	}
 }

@@ -2625,6 +2671,62 @@ static void blkif_release(struct gendisk *disk, fmode_t mode)
 	mutex_unlock(&blkfront_mutex);
 }
 
+static int blkfront_freeze(struct xenbus_device *dev)
+{
+	unsigned int i;
+	struct blkfront_info *info = dev_get_drvdata(&dev->dev);
+	struct blkfront_ring_info *rinfo;
+	/* This would be reasonable timeout as used in xenbus_dev_shutdown() */
+	unsigned int timeout = 5 * HZ;
+	int err = 0;
+
+	info->connected = BLKIF_STATE_FREEZING;
+
+	blk_mq_freeze_queue(info->rq);
+	blk_mq_quiesce_queue(info->rq);
+
+	for (i = 0; i < info->nr_rings; i++) {
+		rinfo = &info->rinfo[i];
+

drivers/block/xen-blkfront.c static void blkif_release(struct gendisk *disk, fmode_t mode) · line 2696

Quoted 6 lines

drivers/block/xen-blkfront.c +6 -0

+		gnttab_cancel_free_callback(&rinfo->callback);
+		flush_work(&rinfo->work);
+	}
+
+	/* Kick the backend to disconnect */
+	xenbus_switch_state(dev, XenbusStateClosing);

Are you sure this is safe?

I don't think you wait for all requests pending on the ring to be finished by the backend, and hence you might loose requests as the ones on the ring would not be re-issued by blkfront_restore AFAICT.

Thanks, Roger.

Quoted 16 lines · Roger Pau Monné

Quoted 14 lines · Anchal Agarwal

From: Munehisa Kamata <[email protected]

Add freeze, thaw and restore callbacks for PM suspend and hibernation support. All frontend drivers that needs to use PM_HIBERNATION/PM_SUSPEND events, need to implement these xenbus_driver callbacks.
The freeze handler stops a block-layer queue and disconnect the frontend from the backend while freeing ring_info and associated resources. The restore handler re-allocates ring_info and re-connect to the backend, so the rest of the kernel can continue to use the block device transparently. Also, the handlers are used for both PM suspend and hibernation so that we can keep the existing suspend/resume callbacks for Xen suspend without modification. Before disconnecting from backend, we need to prevent any new IO from being queued and wait for existing IO to complete.

Quoted 2 lines

This is different from Xen (xenstore) initiated suspension, as in that case Linux doesn't flush the rings or disconnects from the backend.

Yes, AFAIK in xen initiated suspension backend takes care of it.

Quoted 17 lines

Quoted 17 lines

drivers/block/xen-blkfront.c +17 -0

+static int blkfront_freeze(struct xenbus_device *dev)
+{
+	unsigned int i;
+	struct blkfront_info *info = dev_get_drvdata(&dev->dev);
+	struct blkfront_ring_info *rinfo;
+	/* This would be reasonable timeout as used in xenbus_dev_shutdown() */
+	unsigned int timeout = 5 * HZ;
+	int err = 0;
+
+	info->connected = BLKIF_STATE_FREEZING;
+
+	blk_mq_freeze_queue(info->rq);
+	blk_mq_quiesce_queue(info->rq);
+
+	for (i = 0; i < info->nr_rings; i++) {
+		rinfo = &info->rinfo[i];
+

Quoted 9 lines

drivers/block/xen-blkfront.c static void blkif_release(struct gendisk *disk, fmode_t mode) · line 2696

Quoted 6 lines

drivers/block/xen-blkfront.c +6 -0

+		gnttab_cancel_free_callback(&rinfo->callback);
+		flush_work(&rinfo->work);
+	}
+
+	/* Kick the backend to disconnect */
+	xenbus_switch_state(dev, XenbusStateClosing);

Are you sure this is safe?

In my testing running multiple fio jobs, other test scenarios running a memory loader works fine. I did not came across a scenario that would have failed resume due to blkfront issues unless you can sugest some?

Quoted 4 lines

I don't think you wait for all requests pending on the ring to be finished by the backend, and hence you might loose requests as the ones on the ring would not be re-issued by blkfront_restore AFAICT.

AFAIU, blk_mq_freeze_queue/blk_mq_quiesce_queue should take care of no used request on the shared ring. Also, we I want to pause the queue and flush all the pending requests in the shared ring before disconnecting from backend.

Quiescing the queue seemed a better option here as we want to make sure ongoing requests dispatches are totally drained.
I should accept that some of these notion is borrowed from how nvme freeze/unfreeze is done although its not apple to apple comparison.

Quoted 17 lines · Anchal Agarwal

Quoted 16 lines · Roger Pau Monné

Quoted 14 lines · Anchal Agarwal

From: Munehisa Kamata <[email protected]

Add freeze, thaw and restore callbacks for PM suspend and hibernation support. All frontend drivers that needs to use PM_HIBERNATION/PM_SUSPEND events, need to implement these xenbus_driver callbacks.
The freeze handler stops a block-layer queue and disconnect the frontend from the backend while freeing ring_info and associated resources. The restore handler re-allocates ring_info and re-connect to the backend, so the rest of the kernel can continue to use the block device transparently. Also, the handlers are used for both PM suspend and hibernation so that we can keep the existing suspend/resume callbacks for Xen suspend without modification. Before disconnecting from backend, we need to prevent any new IO from being queued and wait for existing IO to complete.

Quoted 3 lines

Quoted 2 lines

This is different from Xen (xenstore) initiated suspension, as in that case Linux doesn't flush the rings or disconnects from the backend.

Yes, AFAIK in xen initiated suspension backend takes care of it.

No, in Xen initiated suspension backend doesn't take care of flushing the rings, the frontend has a shadow copy of the ring contents and it re-issues the requests on resume.

Quoted 26 lines

Quoted 17 lines

Quoted 17 lines

drivers/block/xen-blkfront.c +17 -0

+static int blkfront_freeze(struct xenbus_device *dev)
+{
+	unsigned int i;
+	struct blkfront_info *info = dev_get_drvdata(&dev->dev);
+	struct blkfront_ring_info *rinfo;
+	/* This would be reasonable timeout as used in xenbus_dev_shutdown() */
+	unsigned int timeout = 5 * HZ;
+	int err = 0;
+
+	info->connected = BLKIF_STATE_FREEZING;
+
+	blk_mq_freeze_queue(info->rq);
+	blk_mq_quiesce_queue(info->rq);
+
+	for (i = 0; i < info->nr_rings; i++) {
+		rinfo = &info->rinfo[i];
+

Quoted 9 lines

drivers/block/xen-blkfront.c static void blkif_release(struct gendisk *disk, fmode_t mode) · line 2696

Quoted 6 lines

drivers/block/xen-blkfront.c +6 -0

+		gnttab_cancel_free_callback(&rinfo->callback);
+		flush_work(&rinfo->work);
+	}
+
+	/* Kick the backend to disconnect */
+	xenbus_switch_state(dev, XenbusStateClosing);

Are you sure this is safe?

Quoted 3 lines

In my testing running multiple fio jobs, other test scenarios running a memory loader works fine. I did not came across a scenario that would have failed resume due to blkfront issues unless you can sugest some?

AFAICT you don't wait for the in-flight requests to be finished, and just rely on blkback to finish processing those. I'm not sure all blkback implementations out there can guarantee that.

The approach used by Xen initiated suspension is to re-issue the in-flight requests when resuming. I have to admit I don't think this is the best approach, but I would like to keep both the Xen and the PM initiated suspension using the same logic, and hence I would request that you try to re-use the existing resume logic (blkfront_resume).

Quoted 4 lines

Quoted 4 lines

I don't think you wait for all requests pending on the ring to be finished by the backend, and hence you might loose requests as the ones on the ring would not be re-issued by blkfront_restore AFAICT.

Quoted 3 lines

AFAIU, blk_mq_freeze_queue/blk_mq_quiesce_queue should take care of no used request on the shared ring. Also, we I want to pause the queue and flush all the pending requests in the shared ring before disconnecting from backend.

Oh, so blk_mq_freeze_queue does wait for in-flight requests to be finished. I guess it's fine then.

Quoted 4 lines

Quiescing the queue seemed a better option here as we want to make sure ongoing requests dispatches are totally drained.
I should accept that some of these notion is borrowed from how nvme freeze/unfreeze is done although its not apple to apple comparison.

That's fine, but I would still like to requests that you use the same logic (as much as possible) for both the Xen and the PM initiated suspension.

So you either apply this freeze/unfreeze to the Xen suspension (and drop the re-issuing of requests on resume) or adapt the same approach as the Xen initiated suspension. Keeping two completely different approaches to suspension / resume on blkfront is not suitable long term.

Thanks, Roger.

Quoted 20 lines · Roger Pau Monné

Quoted 19 lines · Anchal Agarwal

Quoted 18 lines · Roger Pau Monné

Quoted 14 lines · Anchal Agarwal

From: Munehisa Kamata <[email protected]

Add freeze, thaw and restore callbacks for PM suspend and hibernation support. All frontend drivers that needs to use PM_HIBERNATION/PM_SUSPEND events, need to implement these xenbus_driver callbacks.
The freeze handler stops a block-layer queue and disconnect the frontend from the backend while freeing ring_info and associated resources. The restore handler re-allocates ring_info and re-connect to the backend, so the rest of the kernel can continue to use the block device transparently. Also, the handlers are used for both PM suspend and hibernation so that we can keep the existing suspend/resume callbacks for Xen suspend without modification. Before disconnecting from backend, we need to prevent any new IO from being queued and wait for existing IO to complete.

This is different from Xen (xenstore) initiated suspension, as in that case Linux doesn't flush the rings or disconnects from the backend.

Quoted 6 lines

Quoted 1 line

Yes, AFAIK in xen initiated suspension backend takes care of it.

No, in Xen initiated suspension backend doesn't take care of flushing the rings, the frontend has a shadow copy of the ring contents and it re-issues the requests on resume.

Yes, I meant suspension in general where both xenstore and backend knows system is going under suspension and not flushing of rings.

That happens
in frontend when backend indicates that state is closing and so on. I may have written it in wrong context.

Quoted 44 lines

Quoted 26 lines

Quoted 17 lines

Quoted 17 lines

drivers/block/xen-blkfront.c +17 -0

+static int blkfront_freeze(struct xenbus_device *dev)
+{
+	unsigned int i;
+	struct blkfront_info *info = dev_get_drvdata(&dev->dev);
+	struct blkfront_ring_info *rinfo;
+	/* This would be reasonable timeout as used in xenbus_dev_shutdown() */
+	unsigned int timeout = 5 * HZ;
+	int err = 0;
+
+	info->connected = BLKIF_STATE_FREEZING;
+
+	blk_mq_freeze_queue(info->rq);
+	blk_mq_quiesce_queue(info->rq);
+
+	for (i = 0; i < info->nr_rings; i++) {
+		rinfo = &info->rinfo[i];
+

Quoted 9 lines

drivers/block/xen-blkfront.c static void blkif_release(struct gendisk *disk, fmode_t mode) · line 2696

Quoted 6 lines

drivers/block/xen-blkfront.c +6 -0

+		gnttab_cancel_free_callback(&rinfo->callback);
+		flush_work(&rinfo->work);
+	}
+
+	/* Kick the backend to disconnect */
+	xenbus_switch_state(dev, XenbusStateClosing);

Are you sure this is safe?

Quoted 3 lines

In my testing running multiple fio jobs, other test scenarios running a memory loader works fine. I did not came across a scenario that would have failed resume due to blkfront issues unless you can sugest some?

AFAICT you don't wait for the in-flight requests to be finished, and just rely on blkback to finish processing those. I'm not sure all blkback implementations out there can guarantee that.

The approach used by Xen initiated suspension is to re-issue the in-flight requests when resuming. I have to admit I don't think this is the best approach, but I would like to keep both the Xen and the PM initiated suspension using the same logic, and hence I would request that you try to re-use the existing resume logic (blkfront_resume).

Quoted 4 lines

Quoted 4 lines

I don't think you wait for all requests pending on the ring to be finished by the backend, and hence you might loose requests as the ones on the ring would not be re-issued by blkfront_restore AFAICT.

Quoted 7 lines

Quoted 3 lines

AFAIU, blk_mq_freeze_queue/blk_mq_quiesce_queue should take care of no used request on the shared ring. Also, we I want to pause the queue and flush all the pending requests in the shared ring before disconnecting from backend.

Oh, so blk_mq_freeze_queue does wait for in-flight requests to be finished. I guess it's fine then.

Ok.

Quoted 9 lines

Quoted 4 lines

Quiescing the queue seemed a better option here as we want to make sure ongoing requests dispatches are totally drained.
I should accept that some of these notion is borrowed from how nvme freeze/unfreeze is done although its not apple to apple comparison.

That's fine, but I would still like to requests that you use the same logic (as much as possible) for both the Xen and the PM initiated suspension.

Quoted 6 lines

So you either apply this freeze/unfreeze to the Xen suspension (and drop the re-issuing of requests on resume) or adapt the same approach as the Xen initiated suspension. Keeping two completely different approaches to suspension / resume on blkfront is not suitable long term.

I agree with you on overhaul of xen suspend/resume wrt blkfront is a good idea however, IMO that is a work for future and this patch series should not be blocked for it. What do you think?

-----Original Message-----
From: Xen-devel <[email protected]> On Behalf Of Roger Pau Monné
Sent: 20 February 2020 08:39
To: Agarwal, Anchal <[email protected]>
Cc: Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kamata, Munehisa <[email protected]>; [email protected]; [email protected]; Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation

Thanks for this work, please see below.

Quoted 7 lines · Anchal Agarwal

Quoted 6 lines · Roger Pau Monné

Quoted 5 lines · Anchal Agarwal

Quoted 4 lines · Roger Pau Monné

Quoted 3 lines · Anchal Agarwal

From: Munehisa Kamata <[email protected]

Add freeze, thaw and restore callbacks for PM suspend and

hibernation

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

support. All frontend drivers that needs to use

PM_HIBERNATION/PM_SUSPEND

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

events, need to implement these xenbus_driver callbacks. The freeze handler stops a block-layer queue and disconnect the frontend from the backend while freeing ring_info and associated

resources.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

The restore handler re-allocates ring_info and re-connect to the backend, so the rest of the kernel can continue to use the block

device

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

transparently. Also, the handlers are used for both PM suspend

and

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

hibernation so that we can keep the existing suspend/resume

callbacks for

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Xen suspend without modification. Before disconnecting from

backend,

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

we need to prevent any new IO from being queued and wait for

existing

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

IO to complete.

This is different from Xen (xenstore) initiated suspension, as in

that

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

case Linux doesn't flush the rings or disconnects from the

backend.

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

Yes, AFAIK in xen initiated suspension backend takes care of it.

No, in Xen initiated suspension backend doesn't take care of flushing the rings, the frontend has a shadow copy of the ring contents and it re-issues the requests on resume.

Yes, I meant suspension in general where both xenstore and backend knows system is going under suspension and not flushing of rings.

backend has no idea the guest is going to be suspended. Backend code is completely agnostic to suspension/resume.

Quoted 3 lines

That happens
in frontend when backend indicates that state is closing and so on. I may have written it in wrong context.

I'm afraid I'm not sure I fully understand this last sentence.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/block/xen-blkfront.c static void blkif_release(struct gendisk *disk, fmode_t mode)

Quoted 6 lines

drivers/block/xen-blkfront.c +6 -0

+static int blkfront_freeze(struct xenbus_device *dev)
+{
+	unsigned int i;
+	struct blkfront_info *info = dev_get_drvdata(&dev->dev);
+	struct blkfront_ring_info *rinfo;
+	/* This would be reasonable timeout as used in

xenbus_dev_shutdown() */

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 7 lines

drivers/block/xen-blkfront.c +7 -0

+	unsigned int timeout = 5 * HZ;
+	int err = 0;
+
+	info->connected = BLKIF_STATE_FREEZING;
+
+	blk_mq_freeze_queue(info->rq);
+	blk_mq_quiesce_queue(info->rq);

drivers/block/xen-blkfront.c static void blkif_release(struct gendisk *disk, fmode_t mode) · line 2696

Quoted 10 lines

drivers/block/xen-blkfront.c +10 -0

+
+	for (i = 0; i < info->nr_rings; i++) {
+		rinfo = &info->rinfo[i];
+
+		gnttab_cancel_free_callback(&rinfo->callback);
+		flush_work(&rinfo->work);
+	}
+
+	/* Kick the backend to disconnect */
+	xenbus_switch_state(dev, XenbusStateClosing);

Are you sure this is safe?

Quoted 1 line

Quoted 1 line

Quoted 1 line

In my testing running multiple fio jobs, other test scenarios

running

Quoted 1 line

Quoted 1 line

Quoted 1 line

a memory loader works fine. I did not came across a scenario that

would

Quoted 1 line

Quoted 1 line

Quoted 1 line

have failed resume due to blkfront issues unless you can sugest

some?

Quoted 11 lines

Quoted 11 lines

AFAICT you don't wait for the in-flight requests to be finished, and just rely on blkback to finish processing those. I'm not sure all blkback implementations out there can guarantee that.

The approach used by Xen initiated suspension is to re-issue the in-flight requests when resuming. I have to admit I don't think this is the best approach, but I would like to keep both the Xen and the PM initiated suspension using the same logic, and hence I would request that you try to re-use the existing resume logic (blkfront_resume).

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

I don't think you wait for all requests pending on the ring to be finished by the backend, and hence you might loose requests as the ones on the ring would not be re-issued by blkfront_restore

AFAICT.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

AFAIU, blk_mq_freeze_queue/blk_mq_quiesce_queue should take care of

no used

Quoted 1 line

Quoted 1 line

Quoted 1 line

request on the shared ring. Also, we I want to pause the queue and

flush all

Quoted 1 line

Quoted 1 line

Quoted 1 line

the pending requests in the shared ring before disconnecting from

backend.

Quoted 6 lines

Quoted 4 lines

Oh, so blk_mq_freeze_queue does wait for in-flight requests to be finished. I guess it's fine then.

Ok.

Quoted 1 line

Quoted 1 line

Quiescing the queue seemed a better option here as we want to make

sure ongoing

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

requests dispatches are totally drained.
I should accept that some of these notion is borrowed from how nvme

freeze/unfreeze

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

is done although its not apple to apple comparison.

That's fine, but I would still like to requests that you use the same logic (as much as possible) for both the Xen and the PM initiated suspension.

Quoted 7 lines

Quoted 6 lines

So you either apply this freeze/unfreeze to the Xen suspension (and drop the re-issuing of requests on resume) or adapt the same approach as the Xen initiated suspension. Keeping two completely different approaches to suspension / resume on blkfront is not suitable long term.

I agree with you on overhaul of xen suspend/resume wrt blkfront is a

good

Quoted 2 lines

idea however, IMO that is a work for future and this patch series should not be blocked for it. What do you think?

It's not so much that I think an overhaul of suspend/resume in blkfront is needed, it's just that I don't want to have two completely different suspend/resume paths inside blkfront.

So from my PoV I think the right solution is to either use the same code (as much as possible) as it's currently used by Xen initiated suspend/resume, or to also switch Xen initiated suspension to use the newly introduced code.

Having two different approaches to suspend/resume in the same driver is a recipe for disaster IMO: it adds complexity by forcing developers to take into account two different suspend/resume approaches when there's no need for it.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

Quoted 56 lines

-----Original Message-----
From: Xen-devel <[email protected]> On Behalf Of Roger Pau Monné
Sent: 20 February 2020 08:39
To: Agarwal, Anchal <[email protected]>
Cc: Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kamata, Munehisa <[email protected]>; [email protected]; [email protected]; Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation

Thanks for this work, please see below.

Quoted 5 lines · Anchal Agarwal

Quoted 4 lines · Roger Pau Monné

Quoted 3 lines · Anchal Agarwal

Quoted 1 line · Roger Pau Monné

On Fri, Feb 14, 2020 at 11:25:34PM +0000, Anchal Agarwal wrote:

Quiescing the queue seemed a better option here as we want to make

sure ongoing

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

requests dispatches are totally drained.
I should accept that some of these notion is borrowed from how nvme

freeze/unfreeze

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

is done although its not apple to apple comparison.

That's fine, but I would still like to requests that you use the same logic (as much as possible) for both the Xen and the PM initiated suspension.

Quoted 7 lines

Quoted 6 lines

So you either apply this freeze/unfreeze to the Xen suspension (and drop the re-issuing of requests on resume) or adapt the same approach as the Xen initiated suspension. Keeping two completely different approaches to suspension / resume on blkfront is not suitable long term.

I agree with you on overhaul of xen suspend/resume wrt blkfront is a

good

Quoted 2 lines

idea however, IMO that is a work for future and this patch series should not be blocked for it. What do you think?

It's not so much that I think an overhaul of suspend/resume in blkfront is needed, it's just that I don't want to have two completely different suspend/resume paths inside blkfront.

So from my PoV I think the right solution is to either use the same code (as much as possible) as it's currently used by Xen initiated suspend/resume, or to also switch Xen initiated suspension to use the newly introduced code.

Quoted 4 lines

Having two different approaches to suspend/resume in the same driver is a recipe for disaster IMO: it adds complexity by forcing developers to take into account two different suspend/resume approaches when there's no need for it.

I disagree. S3 or S4 suspend/resume (or perhaps we should call them power state transitions to avoid confusion) are quite different from Xen suspend/resume.
Power state transitions ought to be, and indeed are, visible to the software running inside the guest. Applications, as well as drivers, can receive notification and take whatever action they deem appropriate.
Xen suspend/resume OTOH is used when a guest is migrated and the code should go to all lengths possible to make any software running inside the guest (other than Xen specific enlightened code, such as PV drivers) completely unaware that anything has actually happened.

So, whilst it may be possible to use common routines to, for example, re-establish PV frontend/backend communication, PV frontend code should be acutely aware of the circumstances they are operating in. I can cite example code in the Windows PV driver, which have supported guest S3/S4 power state transitions since day 1.

-----Original Message-----
From: Roger Pau Monné <[email protected]>
Sent: 20 February 2020 15:45
To: Durrant, Paul <[email protected]>
Cc: Agarwal, Anchal <[email protected]>; Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kamata, Munehisa <[email protected]>; [email protected]; [email protected]; Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation

Quoted 10 lines · Paul

Quoted 10 lines

-----Original Message-----
From: Xen-devel <[email protected]> On Behalf Of Roger Pau Monné
Sent: 20 February 2020 08:39
To: Agarwal, Anchal <[email protected]>
Cc: Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kamata, Munehisa <[email protected]>; [email protected]; xen-

[email protected];

Quoted 6 lines

Quoted 6 lines

Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add

callbacks

Quoted 9 lines

Quoted 9 lines

for PM suspend and hibernation

Thanks for this work, please see below.

Quoted 4 lines · Anchal Agarwal

Quoted 3 lines · Roger Pau Monné

Quoted 2 lines · Anchal Agarwal

Quoted 1 line · Roger Pau Monné

On Fri, Feb 14, 2020 at 11:25:34PM +0000, Anchal Agarwal

Quoted 1 line · wrote:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quiescing the queue seemed a better option here as we want to

make

Quoted 3 lines

Quoted 3 lines

sure ongoing

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

requests dispatches are totally drained.
I should accept that some of these notion is borrowed from how

nvme

Quoted 4 lines

Quoted 4 lines

freeze/unfreeze

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

is done although its not apple to apple comparison.

That's fine, but I would still like to requests that you use the

same

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

logic (as much as possible) for both the Xen and the PM initiated suspension.

So you either apply this freeze/unfreeze to the Xen suspension

(and

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

drop the re-issuing of requests on resume) or adapt the same

approach

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

Quoted 4 lines

as the Xen initiated suspension. Keeping two completely different approaches to suspension / resume on blkfront is not suitable long term.

I agree with you on overhaul of xen suspend/resume wrt blkfront is a

good

Quoted 1 line

idea however, IMO that is a work for future and this patch series

should

Quoted 11 lines

Quoted 11 lines

Quoted 1 line

not be blocked for it. What do you think?

It's not so much that I think an overhaul of suspend/resume in blkfront is needed, it's just that I don't want to have two completely different suspend/resume paths inside blkfront.

So from my PoV I think the right solution is to either use the same code (as much as possible) as it's currently used by Xen initiated suspend/resume, or to also switch Xen initiated suspension to use the newly introduced code.

Quoted 6 lines

Quoted 4 lines

Having two different approaches to suspend/resume in the same driver is a recipe for disaster IMO: it adds complexity by forcing developers to take into account two different suspend/resume approaches when there's no need for it.

I disagree. S3 or S4 suspend/resume (or perhaps we should call them

power state transitions to avoid confusion) are quite different from Xen suspend/resume.

Quoted 1 line

Power state transitions ought to be, and indeed are, visible to the

software running inside the guest. Applications, as well as drivers, can receive notification and take whatever action they deem appropriate.

Quoted 1 line

Xen suspend/resume OTOH is used when a guest is migrated and the code

should go to all lengths possible to make any software running inside the guest (other than Xen specific enlightened code, such as PV drivers) completely unaware that anything has actually happened.

So from what you say above PM state transitions are notified to all drivers, and Xen suspend/resume is only notified to PV drivers, and here we are speaking about blkfront which is a PV driver, and should get notified in both cases. So I'm unsure why the same (or at least very similar) approach can't be used in both cases.

The suspend/resume approach proposed by this patch is completely different than the one used by a xenbus initiated suspend/resume, and I don't see a technical reason that warrants this difference.

I'm not saying that the approach used here is wrong, it's just that I don't see the point in having two different ways to do suspend/resume in the same driver, unless there's a technical reason for it, which I don't think has been provided.

I would be fine with switching xenbus initiated suspend/resume to also use the approach proposed here: freeze the queues and drain the shared rings before suspending.

Quoted 1 line

So, whilst it may be possible to use common routines to, for example,

re-establish PV frontend/backend communication, PV frontend code should be acutely aware of the circumstances they are operating in. I can cite example code in the Windows PV driver, which have supported guest S3/S4 power state transitions since day 1.

Hm, please bear with me, as I'm not sure I fully understand. Why isn't the current suspend/resume logic suitable for PM transitions?

As said above, I'm happy to switch xenbus initiated suspend/resume to use the logic in this patch, but unless there's a technical reason for it I don't see why blkfront should have two completely different approaches to suspend/resume depending on whether it's a PM or a xenbus state change.

Quoted 106 lines

-----Original Message-----
From: Roger Pau Monné <[email protected]>
Sent: 20 February 2020 15:45
To: Durrant, Paul <[email protected]>
Cc: Agarwal, Anchal <[email protected]>; Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kamata, Munehisa <[email protected]>; [email protected]; [email protected]; Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation

Quoted 10 lines · Paul

Quoted 10 lines

-----Original Message-----
From: Xen-devel <[email protected]> On Behalf Of Roger Pau Monné
Sent: 20 February 2020 08:39
To: Agarwal, Anchal <[email protected]>
Cc: Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kamata, Munehisa <[email protected]>; [email protected]; xen-

[email protected];

Quoted 6 lines

Quoted 6 lines

Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add

callbacks

Quoted 9 lines

Quoted 9 lines

for PM suspend and hibernation

Thanks for this work, please see below.

Quoted 4 lines · Anchal Agarwal

Quoted 3 lines · Roger Pau Monné

Quoted 2 lines · Anchal Agarwal

Quoted 1 line · Roger Pau Monné

On Fri, Feb 14, 2020 at 11:25:34PM +0000, Anchal Agarwal

Quoted 1 line · wrote:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quiescing the queue seemed a better option here as we want to

make

Quoted 3 lines

Quoted 3 lines

sure ongoing

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

requests dispatches are totally drained.
I should accept that some of these notion is borrowed from how

nvme

Quoted 4 lines

Quoted 4 lines

freeze/unfreeze

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

is done although its not apple to apple comparison.

That's fine, but I would still like to requests that you use the

same

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

logic (as much as possible) for both the Xen and the PM initiated suspension.

So you either apply this freeze/unfreeze to the Xen suspension

(and

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

drop the re-issuing of requests on resume) or adapt the same

approach

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

Quoted 4 lines

as the Xen initiated suspension. Keeping two completely different approaches to suspension / resume on blkfront is not suitable long term.

I agree with you on overhaul of xen suspend/resume wrt blkfront is a

good

Quoted 1 line

idea however, IMO that is a work for future and this patch series

should

Quoted 11 lines

Quoted 11 lines

Quoted 1 line

not be blocked for it. What do you think?

It's not so much that I think an overhaul of suspend/resume in blkfront is needed, it's just that I don't want to have two completely different suspend/resume paths inside blkfront.

So from my PoV I think the right solution is to either use the same code (as much as possible) as it's currently used by Xen initiated suspend/resume, or to also switch Xen initiated suspension to use the newly introduced code.

Quoted 6 lines

Quoted 4 lines

Having two different approaches to suspend/resume in the same driver is a recipe for disaster IMO: it adds complexity by forcing developers to take into account two different suspend/resume approaches when there's no need for it.

I disagree. S3 or S4 suspend/resume (or perhaps we should call them

power state transitions to avoid confusion) are quite different from Xen suspend/resume.

Quoted 1 line

Power state transitions ought to be, and indeed are, visible to the

software running inside the guest. Applications, as well as drivers, can receive notification and take whatever action they deem appropriate.

Quoted 1 line

Xen suspend/resume OTOH is used when a guest is migrated and the code

should go to all lengths possible to make any software running inside the guest (other than Xen specific enlightened code, such as PV drivers) completely unaware that anything has actually happened.

So from what you say above PM state transitions are notified to all drivers, and Xen suspend/resume is only notified to PV drivers, and here we are speaking about blkfront which is a PV driver, and should get notified in both cases. So I'm unsure why the same (or at least very similar) approach can't be used in both cases.

Quoted 4 lines

The suspend/resume approach proposed by this patch is completely different than the one used by a xenbus initiated suspend/resume, and I don't see a technical reason that warrants this difference.

Within an individual PV driver it may well be ok to use common mechanisms for connecting to the backend but issues will arise if any subsequent action is visible to the guest. E.g. a network frontend needs to issue gratuitous ARPs without anything else in the network stack (or monitoring the network stack) knowing that it has happened.

Quoted 4 lines

I'm not saying that the approach used here is wrong, it's just that I don't see the point in having two different ways to do suspend/resume in the same driver, unless there's a technical reason for it, which I don't think has been provided.

The technical justification is that the driver needs to know what kind of suspend or resume it is doing, so that it doesn't do the wrong thing. There may also be differences in the state of the system e.g. in Windows, at least some of the resume-from-xen-suspend code runs with interrupts disabled (which is necessary to make sure enough state is restored before things become visible to other kernel code).

Quoted 5 lines

I would be fine with switching xenbus initiated suspend/resume to also use the approach proposed here: freeze the queues and drain the shared rings before suspending.

I think abstracting away at the xenbus level to some degree is probably feasible, but some sort of flag should be passed to the individual drivers so they know what circumstances they are operating under.

Quoted 9 lines

Quoted 1 line

So, whilst it may be possible to use common routines to, for example,

re-establish PV frontend/backend communication, PV frontend code should be acutely aware of the circumstances they are operating in. I can cite example code in the Windows PV driver, which have supported guest S3/S4 power state transitions since day 1.

Hm, please bear with me, as I'm not sure I fully understand. Why isn't the current suspend/resume logic suitable for PM transitions?

I don’t know the details for Linux but it may well be to do with assumptions made about the system e.g. the ability to block waiting for something to happen on another CPU (which may have already been quiesced in a PM context).

Quoted 6 lines

As said above, I'm happy to switch xenbus initiated suspend/resume to use the logic in this patch, but unless there's a technical reason for it I don't see why blkfront should have two completely different approaches to suspend/resume depending on whether it's a PM or a xenbus state change.

Hopefully what I said above illustrates why it may not be 100% common.

-----Original Message-----
From: Roger Pau Monné <[email protected]>
Sent: 20 February 2020 16:49
To: Durrant, Paul <[email protected]>
Cc: Agarwal, Anchal <[email protected]>; Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kamata, Munehisa <[email protected]>; [email protected]; [email protected]; Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation

Quoted 8 lines · Paul

Quoted 8 lines

-----Original Message-----
From: Roger Pau Monné <[email protected]>
Sent: 20 February 2020 15:45
To: Durrant, Paul <[email protected]>
Cc: Agarwal, Anchal <[email protected]>; Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected];

[email protected];

Quoted 6 lines

Quoted 6 lines

[email protected]; Kamata, Munehisa <[email protected]>; [email protected]; [email protected]; Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected];

[email protected];

Quoted 2 lines

Quoted 2 lines

Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add

callbacks

Quoted 5 lines

Quoted 5 lines

for PM suspend and hibernation

Quoted 2 lines · Paul

Quoted 2 lines

-----Original Message-----
From: Xen-devel <[email protected]> On Behalf

Of

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Roger Pau Monné
Sent: 20 February 2020 08:39
To: Agarwal, Anchal <[email protected]>
Cc: Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected];

linux-

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

[email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kamata, Munehisa <[email protected]>; [email protected]; xen-

[email protected];

Quoted 4 lines

Quoted 4 lines

Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected];

[email protected];

Quoted 10 lines

Quoted 10 lines

Quoted 2 lines

Quoted 2 lines

[email protected]; Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add

callbacks

Quoted 7 lines

Quoted 7 lines

for PM suspend and hibernation

Thanks for this work, please see below.

Quoted 2 lines · Anchal Agarwal

Quoted 1 line · Roger Pau Monné

On Mon, Feb 17, 2020 at 11:05:53PM +0000, Anchal Agarwal

Quoted 1 line · wrote:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

On Mon, Feb 17, 2020 at 11:05:09AM +0100, Roger Pau Monné

Quoted 3 lines · wrote:

Quoted 3 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

On Fri, Feb 14, 2020 at 11:25:34PM +0000, Anchal Agarwal

Quoted 1 line · wrote:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quiescing the queue seemed a better option here as we want

to

Quoted 4 lines

Quoted 4 lines

make

Quoted 3 lines

Quoted 3 lines

sure ongoing

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

requests dispatches are totally drained.
I should accept that some of these notion is borrowed from

how

Quoted 5 lines

Quoted 5 lines

nvme

Quoted 4 lines

Quoted 4 lines

freeze/unfreeze

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

is done although its not apple to apple comparison.

That's fine, but I would still like to requests that you use

the

Quoted 2 lines

Quoted 2 lines

same

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

logic (as much as possible) for both the Xen and the PM

initiated

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

suspension.

So you either apply this freeze/unfreeze to the Xen suspension

(and

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

drop the re-issuing of requests on resume) or adapt the same

approach

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

as the Xen initiated suspension. Keeping two completely

different

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

approaches to suspension / resume on blkfront is not suitable

long

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

term.

I agree with you on overhaul of xen suspend/resume wrt blkfront

is a

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

good

Quoted 1 line

idea however, IMO that is a work for future and this patch

series

Quoted 5 lines

Quoted 5 lines

should

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

not be blocked for it. What do you think?

It's not so much that I think an overhaul of suspend/resume in blkfront is needed, it's just that I don't want to have two

completely

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

different suspend/resume paths inside blkfront.

So from my PoV I think the right solution is to either use the

same

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

code (as much as possible) as it's currently used by Xen initiated suspend/resume, or to also switch Xen initiated suspension to use

the

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

newly introduced code.

Having two different approaches to suspend/resume in the same

driver

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

is a recipe for disaster IMO: it adds complexity by forcing

developers

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

to take into account two different suspend/resume approaches when there's no need for it.

I disagree. S3 or S4 suspend/resume (or perhaps we should call them

power state transitions to avoid confusion) are quite different from

Xen

Quoted 3 lines

Quoted 3 lines

suspend/resume.

Quoted 1 line

Power state transitions ought to be, and indeed are, visible to the

software running inside the guest. Applications, as well as drivers,

can

Quoted 2 lines

Quoted 2 lines

receive notification and take whatever action they deem appropriate.

Quoted 1 line

Xen suspend/resume OTOH is used when a guest is migrated and the

code

Quoted 1 line

Quoted 1 line

should go to all lengths possible to make any software running inside

the

Quoted 9 lines

Quoted 9 lines

guest (other than Xen specific enlightened code, such as PV drivers) completely unaware that anything has actually happened.

So from what you say above PM state transitions are notified to all drivers, and Xen suspend/resume is only notified to PV drivers, and here we are speaking about blkfront which is a PV driver, and should get notified in both cases. So I'm unsure why the same (or at least very similar) approach can't be used in both cases.

Quoted 6 lines

Quoted 4 lines

The suspend/resume approach proposed by this patch is completely different than the one used by a xenbus initiated suspend/resume, and I don't see a technical reason that warrants this difference.

Within an individual PV driver it may well be ok to use common

mechanisms for connecting to the backend but issues will arise if any subsequent action is visible to the guest. E.g. a network frontend needs to issue gratuitous ARPs without anything else in the network stack (or monitoring the network stack) knowing that it has happened.

Quoted 7 lines

Quoted 5 lines

I'm not saying that the approach used here is wrong, it's just that I don't see the point in having two different ways to do suspend/resume in the same driver, unless there's a technical reason for it, which I don't think has been provided.

The technical justification is that the driver needs to know what kind

of suspend or resume it is doing, so that it doesn't do the wrong thing. There may also be differences in the state of the system e.g. in Windows, at least some of the resume-from-xen-suspend code runs with interrupts disabled (which is necessary to make sure enough state is restored before things become visible to other kernel code).

Quoted 8 lines

Quoted 6 lines

I would be fine with switching xenbus initiated suspend/resume to also use the approach proposed here: freeze the queues and drain the shared rings before suspending.

I think abstracting away at the xenbus level to some degree is probably

feasible, but some sort of flag should be passed to the individual drivers so they know what circumstances they are operating under.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

So, whilst it may be possible to use common routines to, for

example,

Quoted 1 line

Quoted 1 line

re-establish PV frontend/backend communication, PV frontend code

should be

Quoted 2 lines

Quoted 2 lines

acutely aware of the circumstances they are operating in. I can cite example code in the Windows PV driver, which have supported guest

S3/S4

Quoted 7 lines

Quoted 5 lines

power state transitions since day 1.

Hm, please bear with me, as I'm not sure I fully understand. Why isn't the current suspend/resume logic suitable for PM transitions?

I don’t know the details for Linux but it may well be to do with

assumptions made about the system e.g. the ability to block waiting for something to happen on another CPU (which may have already been quiesced in a PM context).

Quoted 9 lines

Quoted 7 lines

As said above, I'm happy to switch xenbus initiated suspend/resume to use the logic in this patch, but unless there's a technical reason for it I don't see why blkfront should have two completely different approaches to suspend/resume depending on whether it's a PM or a xenbus state change.

Hopefully what I said above illustrates why it may not be 100% common.

Yes, that's fine. I don't expect it to be 100% common (as I guess that the hooks will have different prototypes), but I expect that routines can be shared, and that the approach taken can be the same.

For example one necessary difference will be that xenbus initiated suspend won't close the PV connection, in case suspension fails. On PM suspend you seem to always close the connection beforehand, so you will always have to re-negotiate on resume even if suspension failed.

What I'm mostly worried about is the different approach to ring draining. Ie: either xenbus is changed to freeze the queues and drain the shared rings, or PM uses the already existing logic of not flushing the rings an re-issuing in-flight requests on resume.

Quoted 215 lines

-----Original Message-----
From: Roger Pau Monné <[email protected]>
Sent: 20 February 2020 16:49
To: Durrant, Paul <[email protected]>
Cc: Agarwal, Anchal <[email protected]>; Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kamata, Munehisa <[email protected]>; [email protected]; [email protected]; Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation

Quoted 8 lines · Paul

Quoted 8 lines

-----Original Message-----
From: Roger Pau Monné <[email protected]>
Sent: 20 February 2020 15:45
To: Durrant, Paul <[email protected]>
Cc: Agarwal, Anchal <[email protected]>; Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected];

[email protected];

Quoted 6 lines

Quoted 6 lines

[email protected]; Kamata, Munehisa <[email protected]>; [email protected]; [email protected]; Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected];

[email protected];

Quoted 2 lines

Quoted 2 lines

Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add

callbacks

Quoted 5 lines

Quoted 5 lines

for PM suspend and hibernation

Quoted 2 lines · Paul

Quoted 2 lines

-----Original Message-----
From: Xen-devel <[email protected]> On Behalf

Of

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Roger Pau Monné
Sent: 20 February 2020 08:39
To: Agarwal, Anchal <[email protected]>
Cc: Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected];

linux-

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

[email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kamata, Munehisa <[email protected]>; [email protected]; xen-

[email protected];

Quoted 4 lines

Quoted 4 lines

Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected];

[email protected];

Quoted 10 lines

Quoted 10 lines

Quoted 2 lines

Quoted 2 lines

[email protected]; Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add

callbacks

Quoted 7 lines

Quoted 7 lines

for PM suspend and hibernation

Thanks for this work, please see below.

Quoted 2 lines · Anchal Agarwal

Quoted 1 line · Roger Pau Monné

On Mon, Feb 17, 2020 at 11:05:53PM +0000, Anchal Agarwal

Quoted 1 line · wrote:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

On Mon, Feb 17, 2020 at 11:05:09AM +0100, Roger Pau Monné

Quoted 3 lines · wrote:

Quoted 3 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

On Fri, Feb 14, 2020 at 11:25:34PM +0000, Anchal Agarwal

Quoted 1 line · wrote:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quiescing the queue seemed a better option here as we want

to

Quoted 4 lines

Quoted 4 lines

make

Quoted 3 lines

Quoted 3 lines

sure ongoing

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

requests dispatches are totally drained.
I should accept that some of these notion is borrowed from

how

Quoted 5 lines

Quoted 5 lines

nvme

Quoted 4 lines

Quoted 4 lines

freeze/unfreeze

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

is done although its not apple to apple comparison.

That's fine, but I would still like to requests that you use

the

Quoted 2 lines

Quoted 2 lines

same

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

logic (as much as possible) for both the Xen and the PM

initiated

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

suspension.

So you either apply this freeze/unfreeze to the Xen suspension

(and

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

drop the re-issuing of requests on resume) or adapt the same

approach

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

as the Xen initiated suspension. Keeping two completely

different

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

approaches to suspension / resume on blkfront is not suitable

long

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

term.

I agree with you on overhaul of xen suspend/resume wrt blkfront

is a

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

good

Quoted 1 line

idea however, IMO that is a work for future and this patch

series

Quoted 5 lines

Quoted 5 lines

should

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

not be blocked for it. What do you think?

It's not so much that I think an overhaul of suspend/resume in blkfront is needed, it's just that I don't want to have two

completely

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

different suspend/resume paths inside blkfront.

So from my PoV I think the right solution is to either use the

same

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

code (as much as possible) as it's currently used by Xen initiated suspend/resume, or to also switch Xen initiated suspension to use

the

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

newly introduced code.

Having two different approaches to suspend/resume in the same

driver

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

is a recipe for disaster IMO: it adds complexity by forcing

developers

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

to take into account two different suspend/resume approaches when there's no need for it.

I disagree. S3 or S4 suspend/resume (or perhaps we should call them

power state transitions to avoid confusion) are quite different from

Xen

Quoted 3 lines

Quoted 3 lines

suspend/resume.

Quoted 1 line

Power state transitions ought to be, and indeed are, visible to the

software running inside the guest. Applications, as well as drivers,

can

Quoted 2 lines

Quoted 2 lines

receive notification and take whatever action they deem appropriate.

Quoted 1 line

Xen suspend/resume OTOH is used when a guest is migrated and the

code

Quoted 1 line

Quoted 1 line

should go to all lengths possible to make any software running inside

the

Quoted 9 lines

Quoted 9 lines

guest (other than Xen specific enlightened code, such as PV drivers) completely unaware that anything has actually happened.

So from what you say above PM state transitions are notified to all drivers, and Xen suspend/resume is only notified to PV drivers, and here we are speaking about blkfront which is a PV driver, and should get notified in both cases. So I'm unsure why the same (or at least very similar) approach can't be used in both cases.

Quoted 6 lines

Quoted 4 lines

The suspend/resume approach proposed by this patch is completely different than the one used by a xenbus initiated suspend/resume, and I don't see a technical reason that warrants this difference.

Within an individual PV driver it may well be ok to use common

mechanisms for connecting to the backend but issues will arise if any subsequent action is visible to the guest. E.g. a network frontend needs to issue gratuitous ARPs without anything else in the network stack (or monitoring the network stack) knowing that it has happened.

Quoted 7 lines

Quoted 5 lines

I'm not saying that the approach used here is wrong, it's just that I don't see the point in having two different ways to do suspend/resume in the same driver, unless there's a technical reason for it, which I don't think has been provided.

The technical justification is that the driver needs to know what kind

of suspend or resume it is doing, so that it doesn't do the wrong thing. There may also be differences in the state of the system e.g. in Windows, at least some of the resume-from-xen-suspend code runs with interrupts disabled (which is necessary to make sure enough state is restored before things become visible to other kernel code).

Quoted 8 lines

Quoted 6 lines

I would be fine with switching xenbus initiated suspend/resume to also use the approach proposed here: freeze the queues and drain the shared rings before suspending.

I think abstracting away at the xenbus level to some degree is probably

feasible, but some sort of flag should be passed to the individual drivers so they know what circumstances they are operating under.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

So, whilst it may be possible to use common routines to, for

example,

Quoted 1 line

Quoted 1 line

re-establish PV frontend/backend communication, PV frontend code

should be

Quoted 2 lines

Quoted 2 lines

acutely aware of the circumstances they are operating in. I can cite example code in the Windows PV driver, which have supported guest

S3/S4

Quoted 7 lines

Quoted 5 lines

power state transitions since day 1.

Hm, please bear with me, as I'm not sure I fully understand. Why isn't the current suspend/resume logic suitable for PM transitions?

I don’t know the details for Linux but it may well be to do with

assumptions made about the system e.g. the ability to block waiting for something to happen on another CPU (which may have already been quiesced in a PM context).

Quoted 9 lines

Quoted 7 lines

As said above, I'm happy to switch xenbus initiated suspend/resume to use the logic in this patch, but unless there's a technical reason for it I don't see why blkfront should have two completely different approaches to suspend/resume depending on whether it's a PM or a xenbus state change.

Hopefully what I said above illustrates why it may not be 100% common.

Yes, that's fine. I don't expect it to be 100% common (as I guess that the hooks will have different prototypes), but I expect that routines can be shared, and that the approach taken can be the same.

Quoted 5 lines

For example one necessary difference will be that xenbus initiated suspend won't close the PV connection, in case suspension fails. On PM suspend you seem to always close the connection beforehand, so you will always have to re-negotiate on resume even if suspension failed.

Quoted 5 lines

What I'm mostly worried about is the different approach to ring draining. Ie: either xenbus is changed to freeze the queues and drain the shared rings, or PM uses the already existing logic of not flushing the rings an re-issuing in-flight requests on resume.

Yes, that's needs consideration. I don’t think the same semantic can be suitable for both. E.g. in a xen-suspend we need to freeze with as little processing as possible to avoid dirtying RAM late in the migration cycle, and we know that in-flight data can wait. But in a transition to S4 we need to make sure that at least all the in-flight blkif requests get completed, since they probably contain bits of the guest's memory image and that's not going to get saved any other way.

Paul

Quoted 14 lines · Paul

Quoted 14 lines

-----Original Message-----
From: Roger Pau Monné <[email protected]>
Sent: 20 February 2020 16:49
To: Durrant, Paul <[email protected]>
Cc: Agarwal, Anchal <[email protected]>; Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kamata, Munehisa <[email protected]>; [email protected]; [email protected]; Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected];

Quoted 8 lines

Quoted 8 lines

Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation
For example one necessary difference will be that xenbus initiated suspend won't close the PV connection, in case suspension fails. On PM suspend you seem to always close the connection beforehand, so you will always have to re-negotiate on resume even if suspension failed.

I don't get what you mean, 'suspension failure' during disconnecting frontend from backend? [as in this case we mark frontend closed and then wait for completion] Or do you mean suspension fail in general post bkacend is disconnected from frontend for blkfront?

In case of later, if anything fails after the dpm_suspend(), things need to be thawed or set back up so it should ok to always re-negotitate just to avoid errors.

Quoted 6 lines

Quoted 5 lines

What I'm mostly worried about is the different approach to ring draining. Ie: either xenbus is changed to freeze the queues and drain the shared rings, or PM uses the already existing logic of not flushing the rings an re-issuing in-flight requests on resume.

Quoted 3 lines

Yes, that's needs consideration. I don’t think the same semantic can be suitable for both. E.g. in a xen-suspend we need to freeze with as little processing as possible to avoid dirtying RAM late in the migration cycle, and we know that in-flight data can wait. But in a transition to S4 we need to make sure that at least all the in-flight blkif requests get completed, since they probably contain bits of the guest's memory image and that's not going to get saved any other way.

Paul

I agree with Paul here. Just so as you know, I did try a hacky way in the past to re-queue requests in the past and failed miserably.

I doubt[just from my experimentation]re-queuing the requests will work for PM Hibernation for the same reason Paul mentioned above unless you give me pressing reason why it should work.

Also, won't it effect the migration time if we start waiting for all the inflight requests to complete[last min page faults] ?

Quoted 12 lines

Quoted 1 line

Hopefully what I said above illustrates why it may not be 100% common.

Yes, that's fine. I don't expect it to be 100% common (as I guess that the hooks will have different prototypes), but I expect that routines can be shared, and that the approach taken can be the same.

For example one necessary difference will be that xenbus initiated suspend won't close the PV connection, in case suspension fails. On PM suspend you seem to always close the connection beforehand, so you will always have to re-negotiate on resume even if suspension failed.

Quoted 5 lines

What I'm mostly worried about is the different approach to ring draining. Ie: either xenbus is changed to freeze the queues and drain the shared rings, or PM uses the already existing logic of not flushing the rings an re-issuing in-flight requests on resume.

Yes, that's needs consideration. I don’t think the same semantic can be suitable for both. E.g. in a xen-suspend we need to freeze with as little processing as possible to avoid dirtying RAM late in the migration cycle, and we know that in-flight data can wait. But in a transition to S4 we need to make sure that at least all the in-flight blkif requests get completed, since they probably contain bits of the guest's memory image and that's not going to get saved any other way.

-----Original Message-----
From: Roger Pau Monné <[email protected]>
Sent: 21 February 2020 09:22
To: Durrant, Paul <[email protected]>
Cc: Agarwal, Anchal <[email protected]>; Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kamata, Munehisa <[email protected]>; [email protected]; [email protected]; Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation

Quoted 1 line · Paul

Quoted 1 line

Quoted 1 line

Hopefully what I said above illustrates why it may not be 100%

common.

Quoted 11 lines

Quoted 11 lines

Yes, that's fine. I don't expect it to be 100% common (as I guess that the hooks will have different prototypes), but I expect that routines can be shared, and that the approach taken can be the same.

For example one necessary difference will be that xenbus initiated suspend won't close the PV connection, in case suspension fails. On PM suspend you seem to always close the connection beforehand, so you will always have to re-negotiate on resume even if suspension failed.

Quoted 7 lines

Quoted 5 lines

What I'm mostly worried about is the different approach to ring draining. Ie: either xenbus is changed to freeze the queues and drain the shared rings, or PM uses the already existing logic of not flushing the rings an re-issuing in-flight requests on resume.

Yes, that's needs consideration. I don’t think the same semantic can be

suitable for both. E.g. in a xen-suspend we need to freeze with as little processing as possible to avoid dirtying RAM late in the migration cycle, and we know that in-flight data can wait. But in a transition to S4 we need to make sure that at least all the in-flight blkif requests get completed, since they probably contain bits of the guest's memory image and that's not going to get saved any other way.

Thanks, that makes sense and something along this lines should be added to the commit message IMO.

Wondering about S4, shouldn't we expect the queues to already be empty? As any subsystem that wanted to store something to disk should make sure requests have been successfully completed before suspending.

Thanks, Roger.

Quoted 46 lines

-----Original Message-----
From: Roger Pau Monné <[email protected]>
Sent: 21 February 2020 09:22
To: Durrant, Paul <[email protected]>
Cc: Agarwal, Anchal <[email protected]>; Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kamata, Munehisa <[email protected]>; [email protected]; [email protected]; Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation

Quoted 1 line · Paul

Quoted 1 line

Quoted 1 line

Hopefully what I said above illustrates why it may not be 100%

common.

Quoted 11 lines

Quoted 11 lines

Yes, that's fine. I don't expect it to be 100% common (as I guess that the hooks will have different prototypes), but I expect that routines can be shared, and that the approach taken can be the same.

For example one necessary difference will be that xenbus initiated suspend won't close the PV connection, in case suspension fails. On PM suspend you seem to always close the connection beforehand, so you will always have to re-negotiate on resume even if suspension failed.

Quoted 7 lines

Quoted 5 lines

What I'm mostly worried about is the different approach to ring draining. Ie: either xenbus is changed to freeze the queues and drain the shared rings, or PM uses the already existing logic of not flushing the rings an re-issuing in-flight requests on resume.

Yes, that's needs consideration. I don’t think the same semantic can be

suitable for both. E.g. in a xen-suspend we need to freeze with as little processing as possible to avoid dirtying RAM late in the migration cycle, and we know that in-flight data can wait. But in a transition to S4 we need to make sure that at least all the in-flight blkif requests get completed, since they probably contain bits of the guest's memory image and that's not going to get saved any other way.

Quoted 7 lines

Thanks, that makes sense and something along this lines should be added to the commit message IMO.

Wondering about S4, shouldn't we expect the queues to already be empty? As any subsystem that wanted to store something to disk should make sure requests have been successfully completed before suspending.

What about writing the suspend image itself? Normal filesystem I/O will have been flushed of course, but whatever vestigial kernel actually writes out the hibernation file may well expect a final D0->D3 on the storage device to cause a flush.

Again, I don't know the specifics for Linux (and Windows actually uses an incarnation of the crash kernel to do the job, which brings with it a whole other set of complexity as far as PV drivers go).

-----Original Message-----
From: Roger Pau Monné <[email protected]>
Sent: 21 February 2020 10:22
To: Durrant, Paul <[email protected]>
Cc: Agarwal, Anchal <[email protected]>; Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kamata, Munehisa <[email protected]>; [email protected]; [email protected]; Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation

Quoted 8 lines · Paul

Quoted 8 lines

-----Original Message-----
From: Roger Pau Monné <[email protected]>
Sent: 21 February 2020 09:22
To: Durrant, Paul <[email protected]>
Cc: Agarwal, Anchal <[email protected]>; Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected];

[email protected];

Quoted 6 lines

Quoted 6 lines

[email protected]; Kamata, Munehisa <[email protected]>; [email protected]; [email protected]; Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected];

[email protected];

Quoted 2 lines

Quoted 2 lines

Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add

callbacks

Quoted 9 lines

Quoted 9 lines

for PM suspend and hibernation

Quoted 1 line · Paul

Quoted 1 line

Quoted 1 line

Hopefully what I said above illustrates why it may not be 100%

common.

Quoted 4 lines

Quoted 4 lines

Yes, that's fine. I don't expect it to be 100% common (as I guess that the hooks will have different prototypes), but I expect that routines can be shared, and that the approach taken can be

the

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

same.

For example one necessary difference will be that xenbus initiated suspend won't close the PV connection, in case suspension fails.

On PM

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

suspend you seem to always close the connection beforehand, so you will always have to re-negotiate on resume even if suspension

failed.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

What I'm mostly worried about is the different approach to ring draining. Ie: either xenbus is changed to freeze the queues and

drain

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

the shared rings, or PM uses the already existing logic of not flushing the rings an re-issuing in-flight requests on resume.

Yes, that's needs consideration. I don’t think the same semantic can

be

Quoted 1 line

Quoted 1 line

suitable for both. E.g. in a xen-suspend we need to freeze with as

little

Quoted 1 line

Quoted 1 line

processing as possible to avoid dirtying RAM late in the migration

cycle,

Quoted 3 lines

Quoted 3 lines

and we know that in-flight data can wait. But in a transition to S4 we need to make sure that at least all the in-flight blkif requests get completed, since they probably contain bits of the guest's memory

image

Quoted 10 lines

Quoted 9 lines

and that's not going to get saved any other way.

Thanks, that makes sense and something along this lines should be added to the commit message IMO.

Wondering about S4, shouldn't we expect the queues to already be empty? As any subsystem that wanted to store something to disk should make sure requests have been successfully completed before suspending.

Quoted 4 lines

What about writing the suspend image itself? Normal filesystem I/O will have been flushed of course, but whatever vestigial kernel actually writes out the hibernation file may well expect a final D0->D3 on the storage device to cause a flush.

Hm, I have no idea really. I think whatever writes to the disk before suspend should actually make sure requests have completed, but what you suggest might also be a possibility.

Can you figure out whether there are requests on the ring or in the queue before suspending?

Quoted 4 lines

Again, I don't know the specifics for Linux (and Windows actually uses an incarnation of the crash kernel to do the job, which brings with it a whole other set of complexity as far as PV drivers go).

That seems extremely complex, I'm sure there's a reason for it :).

Thanks, Roger.

Quoted 89 lines

-----Original Message-----
From: Roger Pau Monné <[email protected]>
Sent: 21 February 2020 10:22
To: Durrant, Paul <[email protected]>
Cc: Agarwal, Anchal <[email protected]>; Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kamata, Munehisa <[email protected]>; [email protected]; [email protected]; Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation

Quoted 8 lines · Paul

Quoted 8 lines

-----Original Message-----
From: Roger Pau Monné <[email protected]>
Sent: 21 February 2020 09:22
To: Durrant, Paul <[email protected]>
Cc: Agarwal, Anchal <[email protected]>; Valentin, Eduardo <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected];

[email protected];

Quoted 6 lines

Quoted 6 lines

[email protected]; Kamata, Munehisa <[email protected]>; [email protected]; [email protected]; Singh, Balbir <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected];

[email protected];

Quoted 2 lines

Quoted 2 lines

Woodhouse, David <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add

callbacks

Quoted 9 lines

Quoted 9 lines

for PM suspend and hibernation

Quoted 1 line · Paul

Quoted 1 line

Quoted 1 line

Hopefully what I said above illustrates why it may not be 100%

common.

Quoted 4 lines

Quoted 4 lines

Yes, that's fine. I don't expect it to be 100% common (as I guess that the hooks will have different prototypes), but I expect that routines can be shared, and that the approach taken can be

the

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

same.

For example one necessary difference will be that xenbus initiated suspend won't close the PV connection, in case suspension fails.

On PM

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

suspend you seem to always close the connection beforehand, so you will always have to re-negotiate on resume even if suspension

failed.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

What I'm mostly worried about is the different approach to ring draining. Ie: either xenbus is changed to freeze the queues and

drain

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

the shared rings, or PM uses the already existing logic of not flushing the rings an re-issuing in-flight requests on resume.

Yes, that's needs consideration. I don’t think the same semantic can

be

Quoted 1 line

Quoted 1 line

suitable for both. E.g. in a xen-suspend we need to freeze with as

little

Quoted 1 line

Quoted 1 line

processing as possible to avoid dirtying RAM late in the migration

cycle,

Quoted 3 lines

Quoted 3 lines

and we know that in-flight data can wait. But in a transition to S4 we need to make sure that at least all the in-flight blkif requests get completed, since they probably contain bits of the guest's memory

image

Quoted 14 lines

Quoted 9 lines

and that's not going to get saved any other way.

Thanks, that makes sense and something along this lines should be added to the commit message IMO.

Wondering about S4, shouldn't we expect the queues to already be empty? As any subsystem that wanted to store something to disk should make sure requests have been successfully completed before suspending.

What about writing the suspend image itself? Normal filesystem I/O will have been flushed of course, but whatever vestigial kernel actually writes out the hibernation file may well expect a final D0->D3 on the storage device to cause a flush.

Quoted 6 lines

Hm, I have no idea really. I think whatever writes to the disk before suspend should actually make sure requests have completed, but what you suggest might also be a possibility.

Can you figure out whether there are requests on the ring or in the queue before suspending?

Well there's clearly pending stuff in the ring if rsp_prod != req_prod :-)

As for internal queues, I don't know how blkfront manages that (or whether it has any pending work queue at all).

From: Munehisa Kamata <[email protected]

Add freeze, thaw and restore callbacks for PM suspend and hibernation support. All frontend drivers that needs to use PM_HIBERNATION/PM_SUSPEND events, need to implement these xenbus_driver callbacks.
The freeze handler stops a block-layer queue and disconnect the frontend from the backend while freeing ring_info and associated resources. The restore handler re-allocates ring_info and re-connect to the backend, so the rest of the kernel can continue to use the block device transparently. Also, the handlers are used for both PM suspend and hibernation so that we can keep the existing suspend/resume callbacks for Xen suspend without modification. Before disconnecting from backend, we need to prevent any new IO from being queued and wait for existing IO to complete. Freeze/unfreeze of the queues will guarantee that there are no requests in use on the shared ring.

Note:For older backends,if a backend doesn't have commit'12ea729645ace' xen/blkback: unmap all persistent grants when frontend gets disconnected, the frontend may see massive amount of grant table warning when freeing resources.

[   36.852659] deferring g.e. 0xf9 (pfn 0xffffffffffffffff)
[   36.855089] xen:grant_table: WARNING:e.g. 0x112 still in use!

In this case, persistent grants would need to be disabled.

[Anchal Changelog: Removed timeout/request during blkfront freeze. Fixed major part of the code to work with blk-mq]

Signed-off-by Anchal Agarwal <[email protected]>

Signed-off-by Munehisa Kamata <[email protected]>

---
 drivers/block/xen-blkfront.c | 119 ++++++++++++++++++++++++++++++++---
 1 file changed, 112 insertions(+), 7 deletions(-)

drivers/block/xen-blkfront.c +5 -0

index 478120233750..d715ed3cb69a 100644
@@ -47,6 +47,8 @@
 #include <linux/bitmap.h>
 #include <linux/list.h>
 #include <linux/workqueue.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
 
 #include <xen/xen.h>
 #include <xen/xenbus.h>

@@ -79,6 +81,8 @@ enum blkif_state {
 	BLKIF_STATE_DISCONNECTED,
 	BLKIF_STATE_CONNECTED,
 	BLKIF_STATE_SUSPENDED,
+	BLKIF_STATE_FREEZING,
+	BLKIF_STATE_FROZEN
 };
 
 struct grant {

@@ -220,6 +224,7 @@ struct blkfront_info
 	struct list_head requests;
 	struct bio_list bio_list;
 	struct list_head info_list;
+	struct completion wait_backend_disconnected;
 };
 
 static unsigned int nr_minors;

drivers/block/xen-blkfront.c +1 -0

 static int get_id_from_freelist(struct blkfront_ring_info *rinfo)
 {

@@ -995,6 +1001,7 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size,
 	info->sector_size = sector_size;
 	info->physical_sector_size = physical_sector_size;
 	blkif_set_queue_limits(info);
+	init_completion(&info->wait_backend_disconnected);
 
 	return 0;
 }

drivers/block/xen-blkfront.c +7 -2

 	if (!RING_FULL(&rinfo->ring))
 		blk_mq_start_stopped_hw_queues(rinfo->dev_info->rq, true);
 }

@@ -1341,8 +1350,6 @@ static void blkif_free_ring(struct blkfront_ring_info *rinfo)
 
 static void blkif_free(struct blkfront_info *info, int suspend)
 {
-	unsigned int i;
-
 	/* Prevent new requests being issued until we fix things up. */
 	info->connected = suspend ?
 		BLKIF_STATE_SUSPENDED : BLKIF_STATE_DISCONNECTED;

@@ -1350,6 +1357,13 @@ static void blkif_free(struct blkfront_info *info, int suspend)
 	if (info->rq)
 		blk_mq_stop_hw_queues(info->rq);
 
+	__blkif_free(info);
+}
+
+static void __blkif_free(struct blkfront_info *info)
+{
+	unsigned int i;
+
 	for (i = 0; i < info->nr_rings; i++)
 		blkif_free_ring(&info->rinfo[i]);
 

drivers/block/xen-blkfront.c +10 -1

+
 	list_for_each_entry_safe(req, n, &info->requests, queuelist) {
 		/* Requeue pending requests (flush or discard) */
 		list_del_init(&req->queuelist);

@@ -2359,6 +2379,7 @@ static void blkfront_connect(struct blkfront_info *info)
 
 		return;
 	case BLKIF_STATE_SUSPENDED:
+	case BLKIF_STATE_FROZEN:
 		/*
 		 * If we are recovering from suspension, we need to wait
 		 * for the backend to announce it's features before

@@ -2476,12 +2497,37 @@ static void blkback_changed(struct xenbus_device *dev,
 		break;
 
 	case XenbusStateClosed:
-		if (dev->state == XenbusStateClosed)
+		if (dev->state == XenbusStateClosed) {
+			if (info->connected == BLKIF_STATE_FREEZING) {
+				__blkif_free(info);
+				info->connected = BLKIF_STATE_FROZEN;
+				complete(&info->wait_backend_disconnected);
+				break;
+			}
+

drivers/block/xen-blkfront.c +14 -2

+			dev_dbg(&dev->dev,
+					"ignore the backend's Closed state: %s",
+					dev->nodename);
+			break;
+		}
 		/* fall through */
 	case XenbusStateClosing:
-		if (info)
-			blkfront_closing(info);
+		if (info) {
+			if (info->connected == BLKIF_STATE_FREEZING)
+				xenbus_frontend_closed(dev);
+			else
+				blkfront_closing(info);
+		}
 		break;
 	}
 }

@@ -2625,6 +2671,62 @@ static void blkif_release(struct gendisk *disk, fmode_t mode)
 	mutex_unlock(&blkfront_mutex);
 }
 
+static int blkfront_freeze(struct xenbus_device *dev)
+{
+	unsigned int i;

drivers/block/xen-blkfront.c +27 -0

+
+	for (i = 0; i < info->nr_rings; i++) {
+		rinfo = &info->rinfo[i];
+
+		gnttab_cancel_free_callback(&rinfo->callback);
+		flush_work(&rinfo->work);
+	}
+
+	/* Kick the backend to disconnect */
+	xenbus_switch_state(dev, XenbusStateClosing);
+
+	/*
+	 * We don't want to move forward before the frontend is diconnected
+	 * from the backend cleanly.
+	 */
+	timeout = wait_for_completion_timeout(&info->wait_backend_disconnected,
+					      timeout);
+	if (!timeout) {
+		err = -EBUSY;
+		xenbus_dev_error(dev, err, "Freezing timed out;"
+				 "the device may become inconsistent state");
+	}
+
+	return err;
+}
+
+static int blkfront_restore(struct xenbus_device *dev)

Quoted 62 lines · Anchal Agarwal

From: Munehisa Kamata <[email protected]

Add freeze, thaw and restore callbacks for PM suspend and hibernation support. All frontend drivers that needs to use PM_HIBERNATION/PM_SUSPEND events, need to implement these xenbus_driver callbacks.
The freeze handler stops a block-layer queue and disconnect the frontend from the backend while freeing ring_info and associated resources. The restore handler re-allocates ring_info and re-connect to the backend, so the rest of the kernel can continue to use the block device transparently. Also, the handlers are used for both PM suspend and hibernation so that we can keep the existing suspend/resume callbacks for Xen suspend without modification. Before disconnecting from backend, we need to prevent any new IO from being queued and wait for existing IO to complete. Freeze/unfreeze of the queues will guarantee that there are no requests in use on the shared ring.

Note:For older backends,if a backend doesn't have commit'12ea729645ace' xen/blkback: unmap all persistent grants when frontend gets disconnected, the frontend may see massive amount of grant table warning when freeing resources.

[   36.852659] deferring g.e. 0xf9 (pfn 0xffffffffffffffff)
[   36.855089] xen:grant_table: WARNING:e.g. 0x112 still in use!

In this case, persistent grants would need to be disabled.

[Anchal Changelog: Removed timeout/request during blkfront freeze. Fixed major part of the code to work with blk-mq]

Signed-off-by Anchal Agarwal <[email protected]>

Signed-off-by Munehisa Kamata <[email protected]>

---
 drivers/block/xen-blkfront.c | 119 ++++++++++++++++++++++++++++++++---
 1 file changed, 112 insertions(+), 7 deletions(-)

drivers/block/xen-blkfront.c +5 -0

index 478120233750..d715ed3cb69a 100644
@@ -47,6 +47,8 @@
 #include <linux/bitmap.h>
 #include <linux/list.h>
 #include <linux/workqueue.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
 
 #include <xen/xen.h>
 #include <xen/xenbus.h>

@@ -79,6 +81,8 @@ enum blkif_state {
 	BLKIF_STATE_DISCONNECTED,
 	BLKIF_STATE_CONNECTED,
 	BLKIF_STATE_SUSPENDED,
+	BLKIF_STATE_FREEZING,
+	BLKIF_STATE_FROZEN
 };
 
 struct grant {

@@ -220,6 +224,7 @@ struct blkfront_info
 	struct list_head requests;
 	struct bio_list bio_list;
 	struct list_head info_list;
+	struct completion wait_backend_disconnected;
 };
 

drivers/block/xen-blkfront.c static DEFINE_SPINLOCK(minor_lock); · line 269

Quoted 6 lines

drivers/block/xen-blkfront.c +1 -0

 static unsigned int nr_minors;

@@ -261,6 +266,7 @@ static DEFINE_SPINLOCK(minor_lock);
 static int blkfront_setup_indirect(struct blkfront_ring_info *rinfo);
 static void blkfront_gather_backend_features(struct blkfront_info *info);
 static int negotiate_mq(struct blkfront_info *info);
+static void __blkif_free(struct blkfront_info *info);

I'm not particularly found of adding underscore prefixes to functions, I would rather use a more descriptive name if possible. blkif_free_{queues/rings} maybe?

Quoted 12 lines

drivers/block/xen-blkfront.c +1 -0

 static int get_id_from_freelist(struct blkfront_ring_info *rinfo)
 {

@@ -995,6 +1001,7 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size,
 	info->sector_size = sector_size;
 	info->physical_sector_size = physical_sector_size;
 	blkif_set_queue_limits(info);
+	init_completion(&info->wait_backend_disconnected);
 
 	return 0;
 }

drivers/block/xen-blkfront.c static void xlvbd_release_gendisk(struct blkfront_info *info) · line 1229

Quoted 5 lines

drivers/block/xen-blkfront.c +2 -0

@@ -1218,6 +1225,8 @@ static void xlvbd_release_gendisk(struct blkfront_info *info)
 /* Already hold rinfo->ring_lock. */
 static inline void kick_pending_request_queues_locked(struct blkfront_ring_info *rinfo)
 {
+	if (unlikely(rinfo->dev_info->connected == BLKIF_STATE_FREEZING))
+		return;

Do you really need this check here?

The queue will be frozen and quiesced in blkfront_freeze when the state is set to BLKIF_STATE_FREEZING, and then the call to blk_mq_start_stopped_hw_queues is just a noop as long as the queue is quiesced (see blk_mq_run_hw_queue).

Quoted 27 lines

drivers/block/xen-blkfront.c +7 -2

 	if (!RING_FULL(&rinfo->ring))
 		blk_mq_start_stopped_hw_queues(rinfo->dev_info->rq, true);
 }

@@ -1341,8 +1350,6 @@ static void blkif_free_ring(struct blkfront_ring_info *rinfo)
 
 static void blkif_free(struct blkfront_info *info, int suspend)
 {
-	unsigned int i;
-
 	/* Prevent new requests being issued until we fix things up. */
 	info->connected = suspend ?
 		BLKIF_STATE_SUSPENDED : BLKIF_STATE_DISCONNECTED;

@@ -1350,6 +1357,13 @@ static void blkif_free(struct blkfront_info *info, int suspend)
 	if (info->rq)
 		blk_mq_stop_hw_queues(info->rq);
 
+	__blkif_free(info);
+}
+
+static void __blkif_free(struct blkfront_info *info)
+{
+	unsigned int i;
+
 	for (i = 0; i < info->nr_rings; i++)
 		blkif_free_ring(&info->rinfo[i]);
 

drivers/block/xen-blkfront.c static irqreturn_t blkif_interrupt(int irq, void *dev_id) · line 1571

Quoted 7 lines

drivers/block/xen-blkfront.c +2 -2

@@ -1553,8 +1567,10 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 	struct blkfront_ring_info *rinfo = (struct blkfront_ring_info *)dev_id;
 	struct blkfront_info *info = rinfo->dev_info;
 
-	if (unlikely(info->connected != BLKIF_STATE_CONNECTED))
-		return IRQ_HANDLED;
+	if (unlikely(info->connected != BLKIF_STATE_CONNECTED)) {
+		if (info->connected != BLKIF_STATE_FREEZING)

Please fold this into the previous if condition:

if (unlikely(info->connected != BLKIF_STATE_CONNECTED &&

             info->connected != BLKIF_STATE_FREEZING))
	return IRQ_HANDLED;

Quoted 8 lines

drivers/block/xen-blkfront.c +1 -0

+	}
 
 	spin_lock_irqsave(&rinfo->ring_lock, flags);
  again:

@@ -2020,6 +2036,7 @@ static int blkif_recover(struct blkfront_info *info)
 	struct bio *bio;
 	unsigned int segs;