lkml.org 
[lkml]   [2020]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation
On Mon, Feb 17, 2020 at 11:05:09AM +0100, Roger Pau Monné wrote:
> On Fri, Feb 14, 2020 at 11:25:34PM +0000, Anchal Agarwal wrote:
> > From: Munehisa Kamata <kamatam@amazon.com
> >
> > 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.
Yes, AFAIK in xen initiated suspension backend takes care of it.
>
> 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.
>
> > 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 <anchalag@amazon.com>
> > Signed-off-by: Munehisa Kamata <kamatam@amazon.com>
> > ---
> > drivers/block/xen-blkfront.c | 119 ++++++++++++++++++++++++++++++++---
> > 1 file changed, 112 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> > index 478120233750..d715ed3cb69a 100644
> > --- a/drivers/block/xen-blkfront.c
> > +++ b/drivers/block/xen-blkfront.c
> > @@ -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];
> > +
> > + 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?
> 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.

Do you have any particular scenario in mind which may cause resume to fail?
> Thanks, Roger.

\
 
 \ /
  Last update: 2020-02-18 00:06    [W:0.142 / U:3.368 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site