Received-SPF: pass (google.com: domain of linux-kernel+bounces-36570-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240122110722.690223-1-yi.sun@unisoc.com> <20240122110722.690223-3-yi.sun@unisoc.com>
 <20240122154255.GA389442@fedora> <CALpufv0h-sQ4Qfp-Sxd7wME4onMNAMop_gi-np6Dk2R96sba0Q@mail.gmail.com>
 <20240123150924.GD484337@fedora>
In-Reply-To: <20240123150924.GD484337@fedora>
From: yi sun <sunyibuaa@gmail.com>
Date: Wed, 24 Jan 2024 15:38:35 +0800
Message-ID: <CALpufv385oK0+EqYZw1Mk8xxm1Av_jYfcPS0khBi=NSu7qCeHQ@mail.gmail.com>
Subject: Re: [PATCH 2/2] virtio-blk: Ensure no requests in virtqueues before
 deleting vqs.
To: Stefan Hajnoczi <stefanha@redhat.com>
Cc: mst@redhat.com, Yi Sun <yi.sun@unisoc.com>, axboe@kernel.dk, jasowang@redhat.com, 
	xuanzhuo@linux.alibaba.com, pbonzini@redhat.com, 
	virtualization@lists.linux.dev, linux-block@vger.kernel.org, 
	linux-kernel@vger.kernel.org, zhiguo.niu@unisoc.com, hongyu.jin@unisoc.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Yes, I'm using virtio-mmio.

On Tue, Jan 23, 2024 at 11:09=E2=80=AFPM Stefan Hajnoczi <stefanha@redhat.c=
om> wrote:
>
> Hi Michael,
> This could potentially affect other VIRTIO drivers too. Please see
> below.
>
> On Tue, Jan 23, 2024 at 11:27:40AM +0800, yi sun wrote:
> > On Mon, Jan 22, 2024 at 11:43=E2=80=AFPM Stefan Hajnoczi <stefanha@redh=
at.com> wrote:
> > >
> > > On Mon, Jan 22, 2024 at 07:07:22PM +0800, Yi Sun wrote:
> > > > Ensure no remaining requests in virtqueues before resetting vdev an=
d
> > > > deleting virtqueues. Otherwise these requests will never be complet=
ed.
> > > > It may cause the system to become unresponsive. So it is better to =
place
> > > > blk_mq_quiesce_queue() in front of virtio_reset_device().
> > >
> > > QEMU's virtio-blk device implementation completes all requests during
> > > device reset. Most device implementations have to do the same to avoi=
d
> > > leaving dangling requests in flight across device reset.
> > >
> > > Which device implementation are you using and why is it safe for the
> > > device is simply drop requests across device reset?
> > >
> > > Stefan
> >
> > Virtio-blk device implementation completes all requests during device r=
eset, but
> > this can only ensure that the device has finished using virtqueue. We s=
hould
> > also consider the driver's use of virtqueue.
> >
> > I caught such an example. Before del_vqs, the request had been processe=
d by
> > the device, but it had not been processed by the driver. Although I am
> > using kernel5.4,
> > I think this problem may also occur with the latest version of kernel.
> >
> > The debug code I added is as follows:
> > virtblk_freeze()
> > {
> >         vdev reset();
> >         quiesce queue();
> >         if (virtqueue->num_free !=3D 1024) //1024 is the init value.
> >                 BUG_ON(1);
> >         vdev del_vqs();
> > }
> >
> > BUG_ON triggered the dump, the analysis is as follows:
> >
> > There is one request left in request_queue.
> > crash_arm64> struct request ffffff81f0560000 | grep -e state -e __data_=
len
> >   __data_len =3D 20480,
> >   state =3D MQ_RQ_IN_FLIGHT,
> >
> > crash_arm64> vring_virtqueue.packed,last_used_idx,broken,vq 0xffffff808=
6f92900 |
> > grep -e num -e used_wrap_counter -e last_used_idx -e broken -e
> > num_free -e desc_state -e "desc =3D"
> >         num =3D 1024,
> >         desc =3D 0xffffff8085ff8000,
> >       used_wrap_counter =3D false,
> >       desc_state =3D 0xffffff8085610000,
> >   last_used_idx =3D 487,
> >   broken =3D false,
> >     num_free =3D 1017,
> >
> > Find desc based on last_used_idx. Through flags, we can know that the r=
equest
> > has been processed by the device, but it is still in flight state
> > because it has not
> > had time to run virtblk_done().
> > crash_arm> vring_packed_desc ffffff8085ff9e70
> > struct vring_packed_desc {
> >   addr =3D 10474619192,
> >   len =3D 20481,
> >   id =3D 667,
> >   flags =3D 2
> > }
> >
> > I'm using a closed source virtual machine, so I can't see the source
> > code for it,
> > but I'm guessing it's similar to qemu.
> >
> > After the device completes the request, we must also ensure that the dr=
iver can
> > complete the request in virtblk_done().
> >
>
> Okay, I think your approach of waiting for requests before
> virtio_device_reset() makes sense. blk_mq_complete_request() is async
> (might be deferred to an IPI or softirq) so it's not enough for
> virtblk_done() to run before virtio_device_reset() returns. There is no
> guarantee that virtblk_request_done() runs before virtio_device_reset()
> returns.
>
> A separate issue about virtio_device_reset():
>
> Are you using virtio-mmio? virtio-mmio's vm_reset() doesn't offer the
> same guarantees as virtio-pci's reset functions. virtio-pci guarantees
> that irqs sent by the device during the reset operation complete and the
> irq handlers run before virtio_device_reset() returns. virtio-mmio does
> not.
>
> (On top of this, virtio_device_reset() now has
> CONFIG_VIRTIO_HARDEN_NOTIFICATION which changes the semantics. Drivers
> cannot expect to complete any in-flight requests in
> virtio_device_reset() when complied with this config option.)
>
> Other drivers may be affected by these inconsistent
> virtio_device_reset() semantics. I haven't audited the code.
>
> Stefan