linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Anchal Agarwal <anchalag@amazon.com>
Cc: <tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>,
	<hpa@zytor.com>, <x86@kernel.org>, <boris.ostrovsky@oracle.com>,
	<jgross@suse.com>, <linux-pm@vger.kernel.org>,
	<linux-mm@kvack.org>, <kamatam@amazon.com>,
	<sstabellini@kernel.org>, <konrad.wilk@oracle.com>,
	<axboe@kernel.dk>, <davem@davemloft.net>, <rjw@rjwysocki.net>,
	<len.brown@intel.com>, <pavel@ucw.cz>, <peterz@infradead.org>,
	<eduval@amazon.com>, <sblbir@amazon.com>,
	<xen-devel@lists.xenproject.org>, <vkuznets@redhat.com>,
	<netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<dwmw@amazon.co.uk>, <fllinden@amaozn.com>,
	<benh@kernel.crashing.org>
Subject: Re: [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation
Date: Tue, 18 Feb 2020 10:16:11 +0100	[thread overview]
Message-ID: <20200218091611.GN4679@Air-de-Roger> (raw)
In-Reply-To: <20200217230553.GA8100@dev-dsk-anchalag-2a-9c2d1d96.us-west-2.amazon.com>

On Mon, Feb 17, 2020 at 11:05:53PM +0000, Anchal Agarwal wrote:
> 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. 

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.

> > > +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?

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).

> > 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.

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

> 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.


  reply	other threads:[~2020-02-18  9:16 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-14 23:21 [RFC RESEND PATCH v3 00/12] Enable PM hibernation on guest VMs Anchal Agarwal
2020-02-14 23:22 ` [RFC PATCH v3 01/12] xen/manage: keep track of the on-going suspend mode Anchal Agarwal
2020-02-14 23:23 ` [RFC PATCH v3 02/12] xenbus: add freeze/thaw/restore callbacks support Anchal Agarwal
2020-02-14 23:23 ` [RFC PATCH v3 03/12] x86/xen: Introduce new function to map HYPERVISOR_shared_info on Resume Anchal Agarwal
2020-02-14 23:24 ` [RFC PATCH v3 04/12] x86/xen: add system core suspend and resume callbacks Anchal Agarwal
2020-02-14 23:24 ` [RFC PATCH v3 05/12] xen-netfront: add callbacks for PM suspend and hibernation support Anchal Agarwal
2020-02-14 23:25 ` [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation Anchal Agarwal
2020-02-17 10:05   ` Roger Pau Monné
2020-02-17 23:05     ` Anchal Agarwal
2020-02-18  9:16       ` Roger Pau Monné [this message]
2020-02-19 18:04         ` Anchal Agarwal
2020-02-20  8:39           ` Roger Pau Monné
2020-02-20  8:54             ` [Xen-devel] " Durrant, Paul
2020-02-20 15:45               ` Roger Pau Monné
2020-02-20 16:23                 ` Durrant, Paul
2020-02-20 16:48                   ` Roger Pau Monné
2020-02-20 17:01                     ` Durrant, Paul
2020-02-21  0:49                       ` Anchal Agarwal
2020-02-21  9:47                         ` Roger Pau Monné
2020-02-21  9:22                       ` Roger Pau Monné
2020-02-21  9:56                         ` Durrant, Paul
2020-02-21 10:21                           ` Roger Pau Monné
2020-02-21 10:33                             ` Durrant, Paul
2020-02-21 11:51                               ` Roger Pau Monné
2020-02-21 14:24   ` Roger Pau Monné
2020-03-06 18:40     ` Anchal Agarwal
2020-03-09  9:54       ` Roger Pau Monné
     [not found]         ` <FA688A68-5372-4757-B075-A69A45671CB9@amazon.com>
     [not found]           ` <20200312090435.GK24449@Air-de-Roger.citrite.net>
2020-03-13 17:21             ` Anchal Agarwal
2020-02-14 23:25 ` [RFC PATCH v3 07/12] genirq: Shutdown irq chips in suspend/resume during hibernation Anchal Agarwal
2020-03-06 23:03   ` Thomas Gleixner
2020-03-09 22:37     ` [EXTERNAL][RFC " Anchal Agarwal
2020-02-14 23:26 ` [RFC PATCH v3 08/12] xen/time: introduce xen_{save,restore}_steal_clock Anchal Agarwal
2020-02-14 23:27 ` [RFC PATCH v3 09/12] x86/xen: save and restore steal clock Anchal Agarwal
2020-02-14 23:27 ` [RFC PATCH v3 10/12] xen: Introduce wrapper for save/restore sched clock offset Anchal Agarwal
2020-02-14 23:27 ` [RFC PATCH v3 11/12] xen: Update sched clock offset to avoid system instability in hibernation Anchal Agarwal
2020-02-14 23:28 ` [RFC PATCH v3 12/12] PM / hibernate: update the resume offset on SNAPSHOT_SET_SWAP_AREA Anchal Agarwal
  -- strict thread matches above, loose matches on Subject: below --
2020-02-12 22:32 [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation Anchal Agarwal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200218091611.GN4679@Air-de-Roger \
    --to=roger.pau@citrix.com \
    --cc=anchalag@amazon.com \
    --cc=axboe@kernel.dk \
    --cc=benh@kernel.crashing.org \
    --cc=boris.ostrovsky@oracle.com \
    --cc=bp@alien8.de \
    --cc=davem@davemloft.net \
    --cc=dwmw@amazon.co.uk \
    --cc=eduval@amazon.com \
    --cc=fllinden@amaozn.com \
    --cc=hpa@zytor.com \
    --cc=jgross@suse.com \
    --cc=kamatam@amazon.com \
    --cc=konrad.wilk@oracle.com \
    --cc=len.brown@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=pavel@ucw.cz \
    --cc=peterz@infradead.org \
    --cc=rjw@rjwysocki.net \
    --cc=sblbir@amazon.com \
    --cc=sstabellini@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=vkuznets@redhat.com \
    --cc=x86@kernel.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).