Re: [PATCH] virtio_blk: always post notifications under the lock

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jan 22, 2025 at 05:45:28PM +0000, Boyer, Andrew wrote:
> 
> 
> > On Jan 22, 2025, at 10:13 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > 
> > 
> > On Wed, Jan 22, 2025 at 02:44:50PM +0000, Boyer, Andrew wrote:
> >> 
> >> 
> >>    On Jan 9, 2025, at 8:42 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> 
> >>    On Thu, Jan 09, 2025 at 01:01:20PM +0100, Christian Borntraeger wrote:
> >> 
> >> 
> >>        Am 07.01.25 um 19:25 schrieb Andrew Boyer:
> >> 
> >>            Commit af8ececda185 ("virtio: add VIRTIO_F_NOTIFICATION_DATA
> >>            feature
> >>            support") added notification data support to the core virtio driver
> >>            code. When this feature is enabled, the notification includes the
> >>            updated producer index for the queue. Thus it is now critical that
> >>            notifications arrive in order.
> >> 
> >>            The virtio_blk driver has historically not worried about
> >>            notification
> >>            ordering. Modify it so that the prepare and kick steps are both
> >>            done
> >>            under the vq lock.
> >> 
> >>            Signed-off-by: Andrew Boyer <andrew.boyer@amd.com>
> >>            Reviewed-by: Brett Creeley <brett.creeley@amd.com>
> >>            Fixes: af8ececda185 ("virtio: add VIRTIO_F_NOTIFICATION_DATA
> >>            feature support")
> >>            Cc: Viktor Prutyanov <viktor@daynix.com>
> >>            Cc: virtualization@lists.linux.dev
> >>            Cc: linux-block@vger.kernel.org
> >>            ---
> >>             drivers/block/virtio_blk.c | 19 ++++---------------
> >>             1 file changed, 4 insertions(+), 15 deletions(-)
> >> 
> >>            diff --git a/drivers/block/virtio_blk.c b/drivers/block/
> >>            virtio_blk.c
> >>            index 3efe378f1386..14d9e66bb844 100644
> >>            --- a/drivers/block/virtio_blk.c
> >>            +++ b/drivers/block/virtio_blk.c
> >>            @@ -379,14 +379,10 @@ static void virtio_commit_rqs(struct
> >>            blk_mq_hw_ctx *hctx)
> >>             {
> >>               struct virtio_blk *vblk = hctx->queue->queuedata;
> >>               struct virtio_blk_vq *vq = &vblk->vqs[hctx->queue_num];
> >>            -   bool kick;
> >>               spin_lock_irq(&vq->lock);
> >>            -   kick = virtqueue_kick_prepare(vq->vq);
> >>            +   virtqueue_kick(vq->vq);
> >>               spin_unlock_irq(&vq->lock);
> >>            -
> >>            -   if (kick)
> >>            -           virtqueue_notify(vq->vq);
> >>             }
> >> 
> >> 
> >>        I would assume this will be a performance nightmare for normal IO.
> >> 
> >> 
> >> 
> >> 
> >> Hello Michael and Christian and Jason,
> >> Thank you for taking a look.
> >> 
> >> Is the performance concern that the vmexit might lead to the underlying virtual
> >> storage stack doing the work immediately? Any other job posting to the same
> >> queue would presumably be blocked on a vmexit when it goes to attempt its own
> >> notification. That would be almost the same as having the other job block on a
> >> lock during the operation, although I guess if you are skipping notifications
> >> somehow it would look different.
> >> 
> >> I don't have any sort of setup where I can try it but I would appreciate it if
> >> someone else could.
> >> 
> >> 
> >>    Hmm. Not good, notify can be very slow, holding a lock is a bad idea.
> >>    Basically, virtqueue_notify must work ouside of locks, this
> >>    means af8ececda185 is broken and we did not notice.
> >> 
> >>    Let's fix it please.
> >> 
> >> 
> >> With so many broken kernels already in the wild, I think disabling
> >> F_NOTIFICATION_DATA for virtio-blk would be a reasonable solution.
> > 
> > Some devices might fail feature negotiation then.
> > I am not sure they are broken, devices might simply be able to
> > handle out of order values.
> > 
> 
> A driver which does not support F_NOTIFICATION_DATA should just clear that bit. Surely devices which support it would also support not enabling it? Otherwise pre-6.4 kernels wouldn't work at all.
> 
> > 
> >> 
> >>    Try some kind of compare and swap scheme where we detect that index
> >>    was updated since? Will allow skipping a notification, too.
> >> 
> >> 
> >> Do you have an idea of how this might be done? Anything I've come up with
> >> involves a lock.
> >> 
> >> Would it be doable to have a lock for the vq management stuff
> >> and a second one to post notifications?
> > 
> > 
> > and only for when F_NOTIFICATION_DATA is set. not terrible ok I think.
> > 
> >> 
> >>    AMD guys, can't device survive with reordered notifications?
> >>    Basically just drop a notification if you see index
> >>    going back?
> >> 
> >> 
> >> This is the driver lying to us about the state of the queue; it's not going to
> >> be possible for us to work around it in hardware. For starters, how would we
> >> detect queue wrap around?
> >> 
> >> Thank you,
> >> Andrew
> > 
> > The index is a running value for split, for wrap arounds, there is
> > a special bit for that. No?
> > 
> 
> This is a hardware block used for many different interfaces and devices. When the notification write comes through, the doorbell block updates the queue state and schedules the queue for work. If a second notification comes in and overwrites that update before the queue is able to run (going backwards but not wrapping), we'll have no way of detecting it.
> 
> -Andrew

Do you not have programmable hardware that can compare current queue state
and the doorbell *before* overwriting it?