Thread (10 messages) 10 messages, 4 authors, 2021-09-10

Re: [PATCH v3 1/3] xen/blkfront: read response from backend only once

From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
Date: 2021-08-02 14:38:34
Also in: lkml, xen-devel

Hi, Juergen!

On 30.07.21 13:38, Juergen Gross wrote:
quoted hunk ↗ jump to hunk
In order to avoid problems in case the backend is modifying a response
on the ring page while the frontend has already seen it, just read the
response into a local buffer in one go and then operate on that buffer
only.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <redacted>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
---
  drivers/block/xen-blkfront.c | 35 ++++++++++++++++++-----------------
  1 file changed, 18 insertions(+), 17 deletions(-)
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index d83fee21f6c5..15e840287734 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1496,7 +1496,7 @@ static bool blkif_completion(unsigned long *id,
  static irqreturn_t blkif_interrupt(int irq, void *dev_id)
  {
  	struct request *req;
-	struct blkif_response *bret;
+	struct blkif_response bret;
  	RING_IDX i, rp;
  	unsigned long flags;
  	struct blkfront_ring_info *rinfo = (struct blkfront_ring_info *)dev_id;
@@ -1513,8 +1513,9 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
  	for (i = rinfo->ring.rsp_cons; i != rp; i++) {
  		unsigned long id;
  
-		bret = RING_GET_RESPONSE(&rinfo->ring, i);
-		id   = bret->id;
+		RING_COPY_RESPONSE(&rinfo->ring, i, &bret);
As per my understanding copying is still not an atomic operation as the request/response

are multi-byte structures in general. IOW, what prevents the backend from modifying the ring while

we are copying?

Thanks,

Oleksandr
quoted hunk ↗ jump to hunk
+		id = bret.id;
+
  		/*
  		 * The backend has messed up and given us an id that we would
  		 * never have given to it (we stamp it up to BLK_RING_SIZE -
@@ -1522,39 +1523,39 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
  		 */
  		if (id >= BLK_RING_SIZE(info)) {
  			WARN(1, "%s: response to %s has incorrect id (%ld)\n",
-			     info->gd->disk_name, op_name(bret->operation), id);
+			     info->gd->disk_name, op_name(bret.operation), id);
  			/* We can't safely get the 'struct request' as
  			 * the id is busted. */
  			continue;
  		}
  		req  = rinfo->shadow[id].request;
  
-		if (bret->operation != BLKIF_OP_DISCARD) {
+		if (bret.operation != BLKIF_OP_DISCARD) {
  			/*
  			 * We may need to wait for an extra response if the
  			 * I/O request is split in 2
  			 */
-			if (!blkif_completion(&id, rinfo, bret))
+			if (!blkif_completion(&id, rinfo, &bret))
  				continue;
  		}
  
  		if (add_id_to_freelist(rinfo, id)) {
  			WARN(1, "%s: response to %s (id %ld) couldn't be recycled!\n",
-			     info->gd->disk_name, op_name(bret->operation), id);
+			     info->gd->disk_name, op_name(bret.operation), id);
  			continue;
  		}
  
-		if (bret->status == BLKIF_RSP_OKAY)
+		if (bret.status == BLKIF_RSP_OKAY)
  			blkif_req(req)->error = BLK_STS_OK;
  		else
  			blkif_req(req)->error = BLK_STS_IOERR;
  
-		switch (bret->operation) {
+		switch (bret.operation) {
  		case BLKIF_OP_DISCARD:
-			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
+			if (unlikely(bret.status == BLKIF_RSP_EOPNOTSUPP)) {
  				struct request_queue *rq = info->rq;
  				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
-					   info->gd->disk_name, op_name(bret->operation));
+					   info->gd->disk_name, op_name(bret.operation));
  				blkif_req(req)->error = BLK_STS_NOTSUPP;
  				info->feature_discard = 0;
  				info->feature_secdiscard = 0;
@@ -1564,15 +1565,15 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
  			break;
  		case BLKIF_OP_FLUSH_DISKCACHE:
  		case BLKIF_OP_WRITE_BARRIER:
-			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
+			if (unlikely(bret.status == BLKIF_RSP_EOPNOTSUPP)) {
  				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
-				       info->gd->disk_name, op_name(bret->operation));
+				       info->gd->disk_name, op_name(bret.operation));
  				blkif_req(req)->error = BLK_STS_NOTSUPP;
  			}
-			if (unlikely(bret->status == BLKIF_RSP_ERROR &&
+			if (unlikely(bret.status == BLKIF_RSP_ERROR &&
  				     rinfo->shadow[id].req.u.rw.nr_segments == 0)) {
  				printk(KERN_WARNING "blkfront: %s: empty %s op failed\n",
-				       info->gd->disk_name, op_name(bret->operation));
+				       info->gd->disk_name, op_name(bret.operation));
  				blkif_req(req)->error = BLK_STS_NOTSUPP;
  			}
  			if (unlikely(blkif_req(req)->error)) {
@@ -1585,9 +1586,9 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
  			fallthrough;
  		case BLKIF_OP_READ:
  		case BLKIF_OP_WRITE:
-			if (unlikely(bret->status != BLKIF_RSP_OKAY))
+			if (unlikely(bret.status != BLKIF_RSP_OKAY))
  				dev_dbg(&info->xbdev->dev, "Bad return from blkdev data "
-					"request: %x\n", bret->status);
+					"request: %x\n", bret.status);
  
  			break;
  		default:
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help