[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [qemu-s390x] [Qemu-devel] [PATCH v4 1/6] vfio-ccw: make it safe to a

From: Eric Farman
Subject: Re: [qemu-s390x] [Qemu-devel] [PATCH v4 1/6] vfio-ccw: make it safe to access channel programs
Date: Wed, 10 Apr 2019 22:59:41 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1

On 4/9/19 7:34 PM, Halil Pasic wrote:
On Mon, 8 Apr 2019 19:07:47 +0200
Cornelia Huck <address@hidden> wrote:

On Mon, 8 Apr 2019 13:02:12 -0400
Farhan Ali <address@hidden> wrote:

On 03/01/2019 04:38 AM, Cornelia Huck wrote:
When we get a solicited interrupt, the start function may have
been cleared by a csch, but we still have a channel program
structure allocated. Make it safe to call the cp accessors in
any case, so we can call them unconditionally.

While at it, also make sure that functions called from other parts
of the code return gracefully if the channel program structure
has not been initialized (even though that is a bug in the caller).

Reviewed-by: Eric Farman<address@hidden>
Signed-off-by: Cornelia Huck<address@hidden>

Hi Connie,

My series of fixes for vfio-ccw depends on this patch as I would like to
call cp_free unconditionally :) (I had developed my code on top of your

Could we pick this patch up as well when/if you pick up my patch series?
I am in the process of sending out a v2.

Regarding this patch we could merge it as a stand alone patch, separate
from this series. And also the patch LGTM

Reviewed-by: Farhan Ali <address@hidden>

Actually, I wanted to ask how people felt about merging this whole
series for the next release :) It would be one thing less on my plate...

Sorry I was not able to spend any significant amount of time on this

Gave the combined set (this + Farhans fio-ccw fixes for kernel
stacktraces v2) it a bit of smoke testing after some minor adjustments
to make it compile:

--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -13,6 +13,7 @@
  #include <linux/vfio.h>
  #include <linux/mdev.h>
  #include <linux/nospec.h>
+#include <linux/slab.h>
#include "vfio_ccw_private.h"

Hrm... Taking today's master, and the two series you mention (slight adjustment to apply patch 3 of Connie's series, because part of it was split out a few weeks ago), and I don't encounter this. Tried switching between SLUB/SLAB, but still compiles fine.

I'm just running fio on a pass-through DASD and on some virto-blk disks
in parallel. My QEMU is today's vfio-ccw-caps from your repo.

I see stuff like this:
qemu-git: vfio-ccw: wirte I/O region failed with errno=16[1811/7332/0 iops] 
[eta 26m:34s]

Without knowing what the I/O was that failed, this is a guessing game. But I encountered something similar just now running fio.

2019-04-11T02:06:09.524838Z qemu-system-s390x: vfio-ccw: wirte I/O region failed with errno=16

[ 422.931458] dasd-eckd 0.0.ca8d: An error occurred in the DASD device driver, reason=14 00000000730bbe9a [ 553.741554] dasd-eckd 0.0.ca8e: An error occurred in the DASD device driver, reason=14 00000000e59b81da [ 554.761552] dasd-eckd 0.0.ca8d: An error occurred in the DASD device driver, reason=14 00000000cdf4fb4e [ 554.921518] dasd-eckd 0.0.ca8b: An error occurred in the DASD device driver, reason=14 0000000068775082 [ 555.271556] dasd-eckd 0.0.ca8d: ERP 00000000cdf4fb4e has run out of retries and failed
[  555.271786] dasd(eckd): I/O status report for device 0.0.ca8d:
dasd(eckd): in req: 00000000cdf4fb4e CC:00 FC:00 AC:00 SC:00 DS:00 CS:00 RC:-16
               dasd(eckd): device 0.0.ca8d: Failing CCW:           (null)
               dasd(eckd): SORRY - NO VALID SENSE AVAILABLE
[  555.272214] dasd(eckd): Related CP in req: 00000000cdf4fb4e
               dasd(eckd): CCW 000000006434c30f: 03400000 00000000 DAT:
               dasd(eckd): CCW 000000007a65f7e0: 08000000 70E5B700 DAT:
[  555.272508] dasd(eckd):......

From the associated I/O, I think this is fixed by a series I am nearly ready to send for review. I'll try again with those fixes on top of the two series here, and report back.

[Thread 0x3ff75890910 (LWP 43803) exited]/7932KB/0KB /s] [1930/7932/0 iops] 
[eta 26m:33s]
[Thread 0x3ff6b7b7910 (LWP 43800) exited]/8030KB/0KB /s] [2031/8030/0 iops] 
[eta 26m:32s]
dasd-eckd 0.0.1234: An error occurred in the DASD device driver, reason=14 
INFO: task kworker/u6:1:26 blocked for more than 122 seconds.ps] [eta 
23m:26s]eta 23m:25s]
       Not tainted 5.1.0-rc3-00217-g6ab18dc #598
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kworker/u6:1    D    0    26      2 0x00000000
Workqueue: writeback wb_workfn (flush-94:0)
Call Trace:
([<0000000000ae23f2>] __schedule+0x4fa/0xc98)
  [<0000000000ae2bda>] schedule+0x4a/0xb0
  [<00000000001b30ec>] io_schedule+0x2c/0x50
  [<000000000071cc9c>] blk_mq_get_tag+0x1bc/0x310
  [<000000000071571c>] blk_mq_get_request+0x1a4/0x4a8
  [<0000000000719d38>] blk_mq_make_request+0x100/0x728
  [<000000000070aa0a>] generic_make_request+0x26a/0x478
  [<000000000070ac76>] submit_bio+0x5e/0x178
  [<00000000004cfa2c>] ext4_io_submit+0x74/0x88
  [<00000000004cfd32>] ext4_bio_write_page+0x2d2/0x4c8
  [<00000000004aa5b4>] mpage_submit_page+0x74/0xa8
  [<00000000004aa676>] mpage_process_page_bufs+0x8e/0x1b8
  [<00000000004aa9bc>] mpage_prepare_extent_to_map+0x21c/0x390
  [<00000000004b063c>] ext4_writepages+0x4bc/0x11a0
  [<000000000032ef7a>] do_writepages+0x3a/0xf0
  [<0000000000416226>] __writeback_single_inode+0x86/0x7a0
  [<0000000000417154>] writeback_sb_inodes+0x2cc/0x550
  [<0000000000417476>] __writeback_inodes_wb+0x9e/0xe8
  [<00000000004179e0>] wb_writeback+0x468/0x598
  [<0000000000418780>] wb_workfn+0x3b8/0x710
  [<0000000000199322>] process_one_work+0x25a/0x668
  [<000000000019977a>] worker_thread+0x4a/0x428
  [<00000000001a1ae8>] kthread+0x150/0x170
  [<0000000000aeadda>] kernel_thread_starter+0x6/0xc
  [<0000000000aeadd4>] kernel_thread_starter+0x0/0xc
4 locks held by kworker/u6:1/26:
  #0: 00000000792cf224 ((wq_completion)writeback){+.+.}, at: 
  #1: 000000009888c0e5 ((work_completion)(&(&wb->dwork)->work)){+.+.}, at: 
  #2: 000000002bfb76f0 (&type->s_umount_key#29){++++}, at: 
  #3: 00000000ff47fe1d (&sbi->s_journal_flag_rwsem){.+.+}, at: 

Since I haven't had the time to keep up lately, I will just trust Eric
and Farhan on whether this should be merged or not. From a quick look at
the code, and a quick stroll through my remaining memories, I think, there
are a couple of things, that I myself would try to solve differently. But
that is not a valid reason to hold this up.

I would like to spare the hustle of revisiting my old comments for everyone.
 From the stability and utility perspective I'm pretty convinced we are
better off than without the patches in question.

I agree, both series are an improvement.  I'll focus on both tomorrow.

 - Eric

If it is good enough for Eric and Farhan, I have no objections against merging.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]