* [PATCH v2] virtio_pmem: Check device status before requesting flush
@ 2024-08-15 1:03 Philip Chen
0 siblings, 0 replies; 6+ messages in thread
From: Philip Chen @ 2024-08-15 1:03 UTC (permalink / raw)
To: Pankaj Gupta, Dan Williams, Vishal Verma, Dave Jiang, Ira Weiny
Cc: virtualization, nvdimm, linux-kernel, Philip Chen
If a pmem device is in a bad status, the driver side could wait for
host ack forever in virtio_pmem_flush(), causing the system to hang.
Signed-off-by: Philip Chen <philipchen@chromium.org>
---
Change since v1:
- Remove change id from the patch description
drivers/nvdimm/nd_virtio.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
index 35c8fbbba10e..3b4d07aa8447 100644
--- a/drivers/nvdimm/nd_virtio.c
+++ b/drivers/nvdimm/nd_virtio.c
@@ -44,6 +44,15 @@ static int virtio_pmem_flush(struct nd_region *nd_region)
unsigned long flags;
int err, err1;
+ /*
+ * Don't bother to send the request to the device if the device is not
+ * acticated.
+ */
+ if (vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_NEEDS_RESET) {
+ dev_info(&vdev->dev, "virtio pmem device needs a reset\n");
+ return -EIO;
+ }
+
might_sleep();
req_data = kmalloc(sizeof(*req_data), GFP_KERNEL);
if (!req_data)
--
2.46.0.76.ge559c4bf1a-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2] virtio_pmem: Check device status before requesting flush
@ 2024-08-20 17:22 Philip Chen
2024-08-20 20:01 ` Dave Jiang
0 siblings, 1 reply; 6+ messages in thread
From: Philip Chen @ 2024-08-20 17:22 UTC (permalink / raw)
To: Pankaj Gupta, Dan Williams, Vishal Verma, Dave Jiang, Ira Weiny
Cc: virtualization, nvdimm, linux-kernel, Philip Chen
If a pmem device is in a bad status, the driver side could wait for
host ack forever in virtio_pmem_flush(), causing the system to hang.
So add a status check in the beginning of virtio_pmem_flush() to return
early if the device is not activated.
Signed-off-by: Philip Chen <philipchen@chromium.org>
---
v2:
- Remove change id from the patch description
- Add more details to the patch description
drivers/nvdimm/nd_virtio.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
index 35c8fbbba10e..97addba06539 100644
--- a/drivers/nvdimm/nd_virtio.c
+++ b/drivers/nvdimm/nd_virtio.c
@@ -44,6 +44,15 @@ static int virtio_pmem_flush(struct nd_region *nd_region)
unsigned long flags;
int err, err1;
+ /*
+ * Don't bother to submit the request to the device if the device is
+ * not acticated.
+ */
+ if (vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_NEEDS_RESET) {
+ dev_info(&vdev->dev, "virtio pmem device needs a reset\n");
+ return -EIO;
+ }
+
might_sleep();
req_data = kmalloc(sizeof(*req_data), GFP_KERNEL);
if (!req_data)
--
2.46.0.184.g6999bdac58-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] virtio_pmem: Check device status before requesting flush
2024-08-20 17:22 [PATCH v2] virtio_pmem: Check device status before requesting flush Philip Chen
@ 2024-08-20 20:01 ` Dave Jiang
2024-08-21 2:48 ` Philip Chen
0 siblings, 1 reply; 6+ messages in thread
From: Dave Jiang @ 2024-08-20 20:01 UTC (permalink / raw)
To: Philip Chen, Pankaj Gupta, Dan Williams, Vishal Verma, Ira Weiny
Cc: virtualization, nvdimm, linux-kernel
On 8/20/24 10:22 AM, Philip Chen wrote:
> If a pmem device is in a bad status, the driver side could wait for
> host ack forever in virtio_pmem_flush(), causing the system to hang.
>
> So add a status check in the beginning of virtio_pmem_flush() to return
> early if the device is not activated.
>
> Signed-off-by: Philip Chen <philipchen@chromium.org>
> ---
>
> v2:
> - Remove change id from the patch description
> - Add more details to the patch description
>
> drivers/nvdimm/nd_virtio.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
> index 35c8fbbba10e..97addba06539 100644
> --- a/drivers/nvdimm/nd_virtio.c
> +++ b/drivers/nvdimm/nd_virtio.c
> @@ -44,6 +44,15 @@ static int virtio_pmem_flush(struct nd_region *nd_region)
> unsigned long flags;
> int err, err1;
>
> + /*
> + * Don't bother to submit the request to the device if the device is
> + * not acticated.
s/acticated/activated/
> + */
> + if (vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_NEEDS_RESET) {
> + dev_info(&vdev->dev, "virtio pmem device needs a reset\n");
> + return -EIO;
> + }
> +
> might_sleep();
> req_data = kmalloc(sizeof(*req_data), GFP_KERNEL);
> if (!req_data)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] virtio_pmem: Check device status before requesting flush
2024-08-20 20:01 ` Dave Jiang
@ 2024-08-21 2:48 ` Philip Chen
2024-08-21 20:37 ` Ira Weiny
0 siblings, 1 reply; 6+ messages in thread
From: Philip Chen @ 2024-08-21 2:48 UTC (permalink / raw)
To: Dave Jiang
Cc: Pankaj Gupta, Dan Williams, Vishal Verma, Ira Weiny,
virtualization, nvdimm, linux-kernel
Hi,
On Tue, Aug 20, 2024 at 1:01 PM Dave Jiang <dave.jiang@intel.com> wrote:
>
>
>
> On 8/20/24 10:22 AM, Philip Chen wrote:
> > If a pmem device is in a bad status, the driver side could wait for
> > host ack forever in virtio_pmem_flush(), causing the system to hang.
> >
> > So add a status check in the beginning of virtio_pmem_flush() to return
> > early if the device is not activated.
> >
> > Signed-off-by: Philip Chen <philipchen@chromium.org>
> > ---
> >
> > v2:
> > - Remove change id from the patch description
> > - Add more details to the patch description
> >
> > drivers/nvdimm/nd_virtio.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
> > index 35c8fbbba10e..97addba06539 100644
> > --- a/drivers/nvdimm/nd_virtio.c
> > +++ b/drivers/nvdimm/nd_virtio.c
> > @@ -44,6 +44,15 @@ static int virtio_pmem_flush(struct nd_region *nd_region)
> > unsigned long flags;
> > int err, err1;
> >
> > + /*
> > + * Don't bother to submit the request to the device if the device is
> > + * not acticated.
>
> s/acticated/activated/
Thanks for the review.
I'll fix this typo in v3.
In addition to this typo, does anyone have any other concerns?
>
> > + */
> > + if (vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_NEEDS_RESET) {
> > + dev_info(&vdev->dev, "virtio pmem device needs a reset\n");
> > + return -EIO;
> > + }
> > +
> > might_sleep();
> > req_data = kmalloc(sizeof(*req_data), GFP_KERNEL);
> > if (!req_data)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] virtio_pmem: Check device status before requesting flush
2024-08-21 2:48 ` Philip Chen
@ 2024-08-21 20:37 ` Ira Weiny
2024-08-21 21:30 ` Philip Chen
0 siblings, 1 reply; 6+ messages in thread
From: Ira Weiny @ 2024-08-21 20:37 UTC (permalink / raw)
To: Philip Chen, Dave Jiang
Cc: Pankaj Gupta, Dan Williams, Vishal Verma, Ira Weiny,
virtualization, nvdimm, linux-kernel
Philip Chen wrote:
> Hi,
>
> On Tue, Aug 20, 2024 at 1:01 PM Dave Jiang <dave.jiang@intel.com> wrote:
> >
> >
> >
> > On 8/20/24 10:22 AM, Philip Chen wrote:
> > > If a pmem device is in a bad status, the driver side could wait for
> > > host ack forever in virtio_pmem_flush(), causing the system to hang.
> > >
> > > So add a status check in the beginning of virtio_pmem_flush() to return
> > > early if the device is not activated.
> > >
> > > Signed-off-by: Philip Chen <philipchen@chromium.org>
> > > ---
> > >
> > > v2:
> > > - Remove change id from the patch description
> > > - Add more details to the patch description
> > >
> > > drivers/nvdimm/nd_virtio.c | 9 +++++++++
> > > 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
> > > index 35c8fbbba10e..97addba06539 100644
> > > --- a/drivers/nvdimm/nd_virtio.c
> > > +++ b/drivers/nvdimm/nd_virtio.c
> > > @@ -44,6 +44,15 @@ static int virtio_pmem_flush(struct nd_region *nd_region)
> > > unsigned long flags;
> > > int err, err1;
> > >
> > > + /*
> > > + * Don't bother to submit the request to the device if the device is
> > > + * not acticated.
> >
> > s/acticated/activated/
>
> Thanks for the review.
> I'll fix this typo in v3.
>
> In addition to this typo, does anyone have any other concerns?
I'm not super familiar with the virtio-pmem workings and the needs reset
flag is barely used.
Did you actually experience this hang? How was this found? What is the
user visible issue and how critical is it?
Thanks,
Ira
>
> >
> > > + */
> > > + if (vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_NEEDS_RESET) {
> > > + dev_info(&vdev->dev, "virtio pmem device needs a reset\n");
> > > + return -EIO;
> > > + }
> > > +
> > > might_sleep();
> > > req_data = kmalloc(sizeof(*req_data), GFP_KERNEL);
> > > if (!req_data)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] virtio_pmem: Check device status before requesting flush
2024-08-21 20:37 ` Ira Weiny
@ 2024-08-21 21:30 ` Philip Chen
0 siblings, 0 replies; 6+ messages in thread
From: Philip Chen @ 2024-08-21 21:30 UTC (permalink / raw)
To: Ira Weiny
Cc: Dave Jiang, Pankaj Gupta, Dan Williams, Vishal Verma,
virtualization, nvdimm, linux-kernel
Hi
On Wed, Aug 21, 2024 at 1:37 PM Ira Weiny <ira.weiny@intel.com> wrote:
>
> Philip Chen wrote:
> > Hi,
> >
> > On Tue, Aug 20, 2024 at 1:01 PM Dave Jiang <dave.jiang@intel.com> wrote:
> > >
> > >
> > >
> > > On 8/20/24 10:22 AM, Philip Chen wrote:
> > > > If a pmem device is in a bad status, the driver side could wait for
> > > > host ack forever in virtio_pmem_flush(), causing the system to hang.
> > > >
> > > > So add a status check in the beginning of virtio_pmem_flush() to return
> > > > early if the device is not activated.
> > > >
> > > > Signed-off-by: Philip Chen <philipchen@chromium.org>
> > > > ---
> > > >
> > > > v2:
> > > > - Remove change id from the patch description
> > > > - Add more details to the patch description
> > > >
> > > > drivers/nvdimm/nd_virtio.c | 9 +++++++++
> > > > 1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
> > > > index 35c8fbbba10e..97addba06539 100644
> > > > --- a/drivers/nvdimm/nd_virtio.c
> > > > +++ b/drivers/nvdimm/nd_virtio.c
> > > > @@ -44,6 +44,15 @@ static int virtio_pmem_flush(struct nd_region *nd_region)
> > > > unsigned long flags;
> > > > int err, err1;
> > > >
> > > > + /*
> > > > + * Don't bother to submit the request to the device if the device is
> > > > + * not acticated.
> > >
> > > s/acticated/activated/
> >
> > Thanks for the review.
> > I'll fix this typo in v3.
> >
> > In addition to this typo, does anyone have any other concerns?
>
> I'm not super familiar with the virtio-pmem workings and the needs reset
> flag is barely used.
>
> Did you actually experience this hang? How was this found? What is the
> user visible issue and how critical is it?
Yes, I experienced the problem when trying to enable hibernation for a VM.
In the typical hibernation flow, the kernel would try to:
(1) freeze the processes
(2) freeze the devices
(3) take a snapshot of the memory
(4) thaw the devices
(5) write the snapshot to the storage
(6) power off the system (or perform platform-specific action)
In my case, I see VMM fail to re-activate the virtio_pmem device in (4).
(And therefore the virtio_pmem device side sets VIRTIO_CONFIG_S_NEEDS_RESET.)
As a result, when the kernel tries to power off the virtio_pmem device
in (6), the system would hang in virtio_pmem_flush() if this patch is
not added.
To fix the root cause of this issue, I sent another patch to add
freeze/restore PM callbacks to the virtio_pmem driver:
https://lore.kernel.org/lkml/20240815004617.2325269-1-philipchen@chromium.org/
(Please also help review that patch.)
However, I think this patch is still helpful since the system
shouldn't hang in virtio_pmem_flush() regardless of the device state.
>
> Thanks,
> Ira
>
> >
> > >
> > > > + */
> > > > + if (vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_NEEDS_RESET) {
> > > > + dev_info(&vdev->dev, "virtio pmem device needs a reset\n");
> > > > + return -EIO;
> > > > + }
> > > > +
> > > > might_sleep();
> > > > req_data = kmalloc(sizeof(*req_data), GFP_KERNEL);
> > > > if (!req_data)
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-08-21 21:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-20 17:22 [PATCH v2] virtio_pmem: Check device status before requesting flush Philip Chen
2024-08-20 20:01 ` Dave Jiang
2024-08-21 2:48 ` Philip Chen
2024-08-21 20:37 ` Ira Weiny
2024-08-21 21:30 ` Philip Chen
-- strict thread matches above, loose matches on Subject: below --
2024-08-15 1:03 Philip Chen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox