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

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

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.

Indeed.
AMD guys, can't device survive with reordered notifications?
Basically just drop a notification if you see index
going back?


-- 
MST