Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Kanchan Joshi <joshi.k@samsung.com>
To:     axboe@kernel.dk, Damien.LeMoal@wdc.com
Cc:     linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
        selvakuma.s1@samsung.com, nj.shetty@samsung.com,
        javier.gonz@samsung.com, Kanchan Joshi <joshi.k@samsung.com>
Subject: [PATCH] null_blk: synchronization fix for zoned device
Date:   Thu, 24 Sep 2020 11:51:36 +0530
Message-Id: <20200924062136.47019-1-joshi.k@samsung.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 105P
References: <CGME20200924062620epcas5p2f0468a1f41d4177ce15727c519bfbad5@epcas5p2.samsung.com>
Precedence: bulk

Parallel write,read,zone-mgmt operations accessing/altering zone state
and write-pointer may get into race. Avoid the situation by using a new
spinlock for zoned device.
Concurrent zone-appends (on a zone) returning same write-pointer issue
is also avoided using this lock.

Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
---
 drivers/block/null_blk.h       |  1 +
 drivers/block/null_blk_zoned.c | 84 +++++++++++++++++++++++-----------
 2 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/drivers/block/null_blk.h b/drivers/block/null_blk.h
index daed4a9c3436..b3f4d62e7c38 100644
--- a/drivers/block/null_blk.h
+++ b/drivers/block/null_blk.h
@@ -44,6 +44,7 @@ struct nullb_device {
 	unsigned int nr_zones;
 	struct blk_zone *zones;
 	sector_t zone_size_sects;
+	spinlock_t zlock;
 
 	unsigned long size; /* device size in MB */
 	unsigned long completion_nsec; /* time in ns to complete a request */
diff --git a/drivers/block/null_blk_zoned.c b/drivers/block/null_blk_zoned.c
index 3d25c9ad2383..04fbf267703a 100644
--- a/drivers/block/null_blk_zoned.c
+++ b/drivers/block/null_blk_zoned.c
@@ -45,6 +45,7 @@ int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q)
 	if (!dev->zones)
 		return -ENOMEM;
 
+	spin_lock_init(&dev->zlock);
 	if (dev->zone_nr_conv >= dev->nr_zones) {
 		dev->zone_nr_conv = dev->nr_zones - 1;
 		pr_info("changed the number of conventional zones to %u",
@@ -124,6 +125,7 @@ int null_report_zones(struct gendisk *disk, sector_t sector,
 	nr_zones = min(nr_zones, dev->nr_zones - first_zone);
 	trace_nullb_report_zones(nullb, nr_zones);
 
+	spin_lock_irq(&dev->zlock);
 	for (i = 0; i < nr_zones; i++) {
 		/*
 		 * Stacked DM target drivers will remap the zone information by
@@ -134,10 +136,13 @@ int null_report_zones(struct gendisk *disk, sector_t sector,
 		memcpy(&zone, &dev->zones[first_zone + i],
 		       sizeof(struct blk_zone));
 		error = cb(&zone, i, data);
-		if (error)
+		if (error) {
+			spin_unlock_irq(&dev->zlock);
 			return error;
+		}
 	}
 
+	spin_unlock_irq(&dev->zlock);
 	return nr_zones;
 }
 
@@ -147,16 +152,24 @@ size_t null_zone_valid_read_len(struct nullb *nullb,
 	struct nullb_device *dev = nullb->dev;
 	struct blk_zone *zone = &dev->zones[null_zone_no(dev, sector)];
 	unsigned int nr_sectors = len >> SECTOR_SHIFT;
+	size_t ret = 0;
 
+	spin_lock_irq(&dev->zlock);
 	/* Read must be below the write pointer position */
 	if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL ||
-	    sector + nr_sectors <= zone->wp)
-		return len;
+	    sector + nr_sectors <= zone->wp) {
+		ret = len;
+		goto out_unlock;
+	}
 
 	if (sector > zone->wp)
-		return 0;
+		goto out_unlock;
+
+	ret = (zone->wp - sector) << SECTOR_SHIFT;
 
-	return (zone->wp - sector) << SECTOR_SHIFT;
+out_unlock:
+	spin_unlock_irq(&dev->zlock);
+	return ret;
 }
 
 static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector,
@@ -165,17 +178,19 @@ static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector,
 	struct nullb_device *dev = cmd->nq->dev;
 	unsigned int zno = null_zone_no(dev, sector);
 	struct blk_zone *zone = &dev->zones[zno];
-	blk_status_t ret;
+	blk_status_t ret = BLK_STS_OK;
 
 	trace_nullb_zone_op(cmd, zno, zone->cond);
 
 	if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL)
 		return null_process_cmd(cmd, REQ_OP_WRITE, sector, nr_sectors);
 
+	spin_lock_irq(&dev->zlock);
 	switch (zone->cond) {
 	case BLK_ZONE_COND_FULL:
 		/* Cannot write to a full zone */
-		return BLK_STS_IOERR;
+		ret = BLK_STS_IOERR;
+		break;
 	case BLK_ZONE_COND_EMPTY:
 	case BLK_ZONE_COND_IMP_OPEN:
 	case BLK_ZONE_COND_EXP_OPEN:
@@ -193,27 +208,33 @@ static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector,
 			else
 				cmd->rq->__sector = sector;
 		} else if (sector != zone->wp) {
-			return BLK_STS_IOERR;
+			ret = BLK_STS_IOERR;
+			break;
 		}
 
-		if (zone->wp + nr_sectors > zone->start + zone->capacity)
-			return BLK_STS_IOERR;
+		if (zone->wp + nr_sectors > zone->start + zone->capacity) {
+			ret = BLK_STS_IOERR;
+			break;
+		}
 
 		if (zone->cond != BLK_ZONE_COND_EXP_OPEN)
 			zone->cond = BLK_ZONE_COND_IMP_OPEN;
 
 		ret = null_process_cmd(cmd, REQ_OP_WRITE, sector, nr_sectors);
 		if (ret != BLK_STS_OK)
-			return ret;
+			break;
 
 		zone->wp += nr_sectors;
 		if (zone->wp == zone->start + zone->capacity)
 			zone->cond = BLK_ZONE_COND_FULL;
-		return BLK_STS_OK;
+		break;
 	default:
 		/* Invalid zone condition */
-		return BLK_STS_IOERR;
+		ret = BLK_STS_IOERR;
 	}
+
+	spin_unlock_irq(&dev->zlock);
+	return ret;
 }
 
 static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_opf op,
@@ -223,7 +244,9 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_opf op,
 	unsigned int zone_no = null_zone_no(dev, sector);
 	struct blk_zone *zone = &dev->zones[zone_no];
 	size_t i;
+	blk_status_t ret = BLK_STS_OK;
 
+	spin_lock_irq(&dev->zlock);
 	switch (op) {
 	case REQ_OP_ZONE_RESET_ALL:
 		for (i = 0; i < dev->nr_zones; i++) {
@@ -234,25 +257,29 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_opf op,
 		}
 		break;
 	case REQ_OP_ZONE_RESET:
-		if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL)
-			return BLK_STS_IOERR;
+		if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) {
+			ret = BLK_STS_IOERR;
+			break;
+		}
 
 		zone->cond = BLK_ZONE_COND_EMPTY;
 		zone->wp = zone->start;
 		break;
 	case REQ_OP_ZONE_OPEN:
-		if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL)
-			return BLK_STS_IOERR;
-		if (zone->cond == BLK_ZONE_COND_FULL)
-			return BLK_STS_IOERR;
+		if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL ||
+				zone->cond == BLK_ZONE_COND_FULL) {
+			ret = BLK_STS_IOERR;
+			break;
+		}
 
 		zone->cond = BLK_ZONE_COND_EXP_OPEN;
 		break;
 	case REQ_OP_ZONE_CLOSE:
-		if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL)
-			return BLK_STS_IOERR;
-		if (zone->cond == BLK_ZONE_COND_FULL)
-			return BLK_STS_IOERR;
+		if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL ||
+				zone->cond == BLK_ZONE_COND_FULL) {
+			ret = BLK_STS_IOERR;
+			break;
+		}
 
 		if (zone->wp == zone->start)
 			zone->cond = BLK_ZONE_COND_EMPTY;
@@ -260,18 +287,21 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_opf op,
 			zone->cond = BLK_ZONE_COND_CLOSED;
 		break;
 	case REQ_OP_ZONE_FINISH:
-		if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL)
-			return BLK_STS_IOERR;
+		if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) {
+			ret = BLK_STS_IOERR;
+			break;
+		}
 
 		zone->cond = BLK_ZONE_COND_FULL;
 		zone->wp = zone->start + zone->len;
 		break;
 	default:
-		return BLK_STS_NOTSUPP;
+		ret = BLK_STS_NOTSUPP;
 	}
 
+	spin_unlock_irq(&dev->zlock);
 	trace_nullb_zone_op(cmd, zone_no, zone->cond);
-	return BLK_STS_OK;
+	return ret;
 }
 
 blk_status_t null_process_zoned_cmd(struct nullb_cmd *cmd, enum req_opf op,
-- 
2.25.1