linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Halil Pasic <pasic@linux.ibm.com>
To: "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>
Cc: Halil Pasic <pasic@linux.ibm.com>, Jens Axboe <axboe@kernel.dk>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	virtualization@lists.linux-foundation.org,
	linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-s390@vger.kernel.org,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Viktor Mihajlovski <mihajlov@linux.ibm.com>,
	Cornelia Huck <cohuck@redhat.com>, Ram Pai <linuxram@us.ibm.com>,
	Thiago Jung Bauermann <bauerman@linux.ibm.com>,
	"Lendacky, Thomas" <Thomas.Lendacky@amd.com>
Subject: [PATCH 1/2] virtio-blk: fix hw_queue stopped on arbitrary error
Date: Thu, 13 Feb 2020 13:37:27 +0100	[thread overview]
Message-ID: <20200213123728.61216-2-pasic@linux.ibm.com> (raw)
In-Reply-To: <20200213123728.61216-1-pasic@linux.ibm.com>

Since nobody else is going to restart our hw_queue for us, the
blk_mq_start_stopped_hw_queues() is in virtblk_done() is not sufficient
necessarily sufficient to ensure that the queue will get started again.
In case of global resource outage (-ENOMEM because mapping failure,
because of swiotlb full) our virtqueue may be empty and we can get
stuck with a stopped hw_queue.

Let us not stop the queue on arbitrary errors, but only on -EONSPC which
indicates a full virtqueue, where the hw_queue is guaranteed to get
started by virtblk_done() before when it makes sense to carry on
submitting requests. Let us also remove a stale comment.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Cc: Jens Axboe <axboe@kernel.dk>
Fixes: f7728002c1c7 ("virtio_ring: fix return code on DMA mapping fails")
---

I'm in doubt with regards to the Fixes tag. The thing is, virtio-blk
probably made an assumption on virtqueue_add: the failure is either
because the virtqueue is full, or the failure is fatal. In both cases it
seems acceptable to stop the queue, although the fatal case is arguable.
Since commit f7728002c1c7 it the dma mapping has failed returns -ENOMEM
and not -EIO, and thus we have a recoverable failure that ain't
virtqueue full. So I lean towards to a fixes tag that references that
commit, although it ain't broken. Alternatively one could say 'Fixes:
e467cde23818 ("Block driver using virtio.")', if the aforementioned
assumption shouldn't have made in the first place.
---
 drivers/block/virtio_blk.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 54158766334b..adfe43f5ffe4 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -245,10 +245,12 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
 	err = virtblk_add_req(vblk->vqs[qid].vq, vbr, vbr->sg, num);
 	if (err) {
 		virtqueue_kick(vblk->vqs[qid].vq);
-		blk_mq_stop_hw_queue(hctx);
+		/* Don't stop the queue if -ENOMEM: we may have failed to
+		 * bounce the buffer due to global resource outage.
+		 */
+		if (err == -ENOSPC)
+			blk_mq_stop_hw_queue(hctx);
 		spin_unlock_irqrestore(&vblk->vqs[qid].lock, flags);
-		/* Out of mem doesn't actually happen, since we fall back
-		 * to direct descriptors */
 		if (err == -ENOMEM || err == -ENOSPC)
 			return BLK_STS_DEV_RESOURCE;
 		return BLK_STS_IOERR;
-- 
2.17.1


  reply	other threads:[~2020-02-13 12:38 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-13 12:37 [PATCH 0/2] virtio-blk: improve handling of DMA mapping failures Halil Pasic
2020-02-13 12:37 ` Halil Pasic [this message]
2020-02-14 18:20   ` [PATCH 1/2] virtio-blk: fix hw_queue stopped on arbitrary error dongli.zhang
2020-02-17 13:08     ` Halil Pasic
2020-02-18  2:21   ` Ming Lei
2020-02-18 12:35     ` Halil Pasic
2020-02-19  1:46       ` Ming Lei
2020-02-19 15:42         ` Halil Pasic
2020-02-13 12:37 ` [PATCH 2/2] virtio-blk: improve virtqueue error to BLK_STS Halil Pasic
2020-02-19 15:11 ` [PATCH 0/2] virtio-blk: improve handling of DMA mapping failures Stefan Hajnoczi
2020-03-03 14:12 ` Halil Pasic
2020-03-03 14:49   ` Michael S. Tsirkin
2020-03-03 17:02     ` Halil Pasic

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200213123728.61216-2-pasic@linux.ibm.com \
    --to=pasic@linux.ibm.com \
    --cc=Thomas.Lendacky@amd.com \
    --cc=axboe@kernel.dk \
    --cc=bauerman@linux.ibm.com \
    --cc=borntraeger@de.ibm.com \
    --cc=cohuck@redhat.com \
    --cc=jasowang@redhat.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=linuxram@us.ibm.com \
    --cc=mihajlov@linux.ibm.com \
    --cc=mst@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=stefanha@redhat.com \
    --cc=virtualization@lists.linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).