[PATCH V3 0/6] null_blk: simplify null_handle_cmd()

[PATCH V3 0/6] null_blk: simplify null_handle_cmd()

[Chaitanya Kulkarni]

Hi,

The core function where we handle the request null_handle_cmd() in the
null_blk does various things based on how null_blk is configured :-

1. Handle throttling.
2. Handle badblocks.
3. Handle Memory backed device operations.
4. Handle Zoned Block device operations.
5. Completion of the requests.

With all the above functionality present in the one function,
null_handle_cmd() is growing and becoming unreasonably lengthy when
we want to add more features such as new Zone requests which is being
worked on (See [1], [2]).

This is a cleanup patch-series which refactors the code in the
null_handle_cmd(). We create a clear interface for each of the above
mentioned functionality which leads to having null_handle_cmd() more
clear and easy to manage with future changes. Please have a look at
NVMe PCIe Driver (nvme_queue_rq()) (See [3]) which has a similar code
structure and nicely structured code for doing various things such as
setting up commands, mapping of the block layer requests, mapping
PRPs/SGLs, handling integrity requests and finally submitting the
NVMe Command etc.

With this patch-series we fix the following issues with the current 
code :-

1. Get rid of the multiple nesting levels (> 2).
2. Easy to read, debug and extend the code for specific feature.
3. Get rid of the various line foldings which are there in the code
   due to multiple level of nesting under if conditions.
4. Precise definition for the null_handle_cmd() and clear error
   handling as helpers are responsible for handling errors.

Please consider this for 5.3.

Cc: hch@lst.de

Regards,
Chaitanya

[1] https://www.spinics.net/lists/linux-block/msg41884.html
[2] https://www.spinics.net/lists/linux-block/msg41883.html
[3] https://github.com/torvalds/linux/blob/master/drivers/nvme/host/pci.c

* Changes from V2:-
1. Adjust the code to latest upstream code.

* Changes from V1:-
1. Move bio vs req code into the callers for the null_handle_cmd() and
   add required arguments to simplify the code in the same function.
2. Get rid of the extra braces for the null_handle_zoned().
3. Get rid of the multiple returns style and the goto.
4. For throttling, code keep the check in the caller.
5. Add uniform code format for setting the cmd->error in the
   null_handle_cmd() and make required code changes so that each
   feature specific function will return blk_status_t value.

Chaitanya Kulkarni (6):
  null_blk: move duplicate code to callers
  null_blk: create a helper for throttling
  null_blk: create a helper for badblocks
  null_blk: create a helper for mem-backed ops
  null_blk: create a helper for zoned devices
  null_blk: create a helper for req completion

 drivers/block/null_blk.h       |  19 ++--
 drivers/block/null_blk_main.c  | 180 +++++++++++++++++++--------------
 drivers/block/null_blk_zoned.c |  23 ++---
 3 files changed, 125 insertions(+), 97 deletions(-)

-- 
2.17.0

[PATCH V3 1/6] null_blk: move duplicate code to callers

[Chaitanya Kulkarni]

This is a preparation patch which moves the duplicate code for sectors
and nr_sectors calculations for bio vs request mode into their
respective callers (null_queue_bio(), null_qeueue_req()). Now the core
function only deals with the respective actions and commands instead of
having to calculte the bio vs req operations and different sector
related variables. We also move the flush command handling at the top
which significantly simplifies the rest of the code.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/null_blk_main.c | 66 +++++++++++------------------------
 1 file changed, 21 insertions(+), 45 deletions(-)

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 99c56d72ff78..7277f2db8ec9 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1133,7 +1133,8 @@ static void null_restart_queue_async(struct nullb *nullb)
 		blk_mq_start_stopped_hw_queues(q, true);
 }
 
-static blk_status_t null_handle_cmd(struct nullb_cmd *cmd)
+static blk_status_t null_handle_cmd(struct nullb_cmd *cmd, sector_t sector,
+				    sector_t nr_sectors, enum req_opf op)
 {
 	struct nullb_device *dev = cmd->nq->dev;
 	struct nullb *nullb = dev->nullb;
@@ -1156,60 +1157,31 @@ static blk_status_t null_handle_cmd(struct nullb_cmd *cmd)
 		}
 	}
 
+	if (op == REQ_OP_FLUSH) {
+		cmd->error = errno_to_blk_status(null_handle_flush(nullb));
+		goto out;
+	}
 	if (nullb->dev->badblocks.shift != -1) {
 		int bad_sectors;
-		sector_t sector, size, first_bad;
-		bool is_flush = true;
-
-		if (dev->queue_mode == NULL_Q_BIO &&
-				bio_op(cmd->bio) != REQ_OP_FLUSH) {
-			is_flush = false;
-			sector = cmd->bio->bi_iter.bi_sector;
-			size = bio_sectors(cmd->bio);
-		}
-		if (dev->queue_mode != NULL_Q_BIO &&
-				req_op(cmd->rq) != REQ_OP_FLUSH) {
-			is_flush = false;
-			sector = blk_rq_pos(cmd->rq);
-			size = blk_rq_sectors(cmd->rq);
-		}
-		if (!is_flush && badblocks_check(&nullb->dev->badblocks, sector,
-				size, &first_bad, &bad_sectors)) {
+		sector_t first_bad;
+
+		if (badblocks_check(&nullb->dev->badblocks, sector, nr_sectors,
+				&first_bad, &bad_sectors)) {
 			cmd->error = BLK_STS_IOERR;
 			goto out;
 		}
 	}
 
 	if (dev->memory_backed) {
-		if (dev->queue_mode == NULL_Q_BIO) {
-			if (bio_op(cmd->bio) == REQ_OP_FLUSH)
-				err = null_handle_flush(nullb);
-			else
-				err = null_handle_bio(cmd);
-		} else {
-			if (req_op(cmd->rq) == REQ_OP_FLUSH)
-				err = null_handle_flush(nullb);
-			else
-				err = null_handle_rq(cmd);
-		}
+		if (dev->queue_mode == NULL_Q_BIO)
+			err = null_handle_bio(cmd);
+		else
+			err = null_handle_rq(cmd);
 	}
+
 	cmd->error = errno_to_blk_status(err);
 
 	if (!cmd->error && dev->zoned) {
-		sector_t sector;
-		unsigned int nr_sectors;
-		enum req_opf op;
-
-		if (dev->queue_mode == NULL_Q_BIO) {
-			op = bio_op(cmd->bio);
-			sector = cmd->bio->bi_iter.bi_sector;
-			nr_sectors = cmd->bio->bi_iter.bi_size >> 9;
-		} else {
-			op = req_op(cmd->rq);
-			sector = blk_rq_pos(cmd->rq);
-			nr_sectors = blk_rq_sectors(cmd->rq);
-		}
-
 		if (op == REQ_OP_WRITE)
 			null_zone_write(cmd, sector, nr_sectors);
 		else if (op == REQ_OP_ZONE_RESET)
@@ -1282,6 +1254,8 @@ static struct nullb_queue *nullb_to_queue(struct nullb *nullb)
 
 static blk_qc_t null_queue_bio(struct request_queue *q, struct bio *bio)
 {
+	sector_t sector = bio->bi_iter.bi_sector;
+	sector_t nr_sectors = bio_sectors(bio);
 	struct nullb *nullb = q->queuedata;
 	struct nullb_queue *nq = nullb_to_queue(nullb);
 	struct nullb_cmd *cmd;
@@ -1289,7 +1263,7 @@ static blk_qc_t null_queue_bio(struct request_queue *q, struct bio *bio)
 	cmd = alloc_cmd(nq, 1);
 	cmd->bio = bio;
 
-	null_handle_cmd(cmd);
+	null_handle_cmd(cmd, sector, nr_sectors, bio_op(bio));
 	return BLK_QC_T_NONE;
 }
 
@@ -1323,6 +1297,8 @@ static blk_status_t null_queue_rq(struct blk_mq_hw_ctx *hctx,
 {
 	struct nullb_cmd *cmd = blk_mq_rq_to_pdu(bd->rq);
 	struct nullb_queue *nq = hctx->driver_data;
+	sector_t nr_sectors = blk_rq_sectors(bd->rq);
+	sector_t sector = blk_rq_pos(bd->rq);
 
 	might_sleep_if(hctx->flags & BLK_MQ_F_BLOCKING);
 
@@ -1351,7 +1327,7 @@ static blk_status_t null_queue_rq(struct blk_mq_hw_ctx *hctx,
 	if (should_timeout_request(bd->rq))
 		return BLK_STS_OK;
 
-	return null_handle_cmd(cmd);
+	return null_handle_cmd(cmd, sector, nr_sectors, req_op(bd->rq));
 }
 
 static const struct blk_mq_ops null_mq_ops = {
-- 
2.17.0

Re: [PATCH V3 1/6] null_blk: move duplicate code to callers

[Christoph Hellwig]

On Tue, Aug 20, 2019 at 11:13:09PM -0700, Chaitanya Kulkarni wrote:
> This is a preparation patch which moves the duplicate code for sectors
> and nr_sectors calculations for bio vs request mode into their
> respective callers (null_queue_bio(), null_qeueue_req()). Now the core
> function only deals with the respective actions and commands instead of
> having to calculte the bio vs req operations and different sector
> related variables. We also move the flush command handling at the top
> which significantly simplifies the rest of the code.
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH V3 2/6] null_blk: create a helper for throttling

[Chaitanya Kulkarni]

This patch creates a helper for handling throttling code in the
null_handle_cmd().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/null_blk_main.c | 39 ++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 7277f2db8ec9..751679fadc9d 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1133,28 +1133,39 @@ static void null_restart_queue_async(struct nullb *nullb)
 		blk_mq_start_stopped_hw_queues(q, true);
 }
 
+static inline blk_status_t null_handle_throttled(struct nullb_cmd *cmd)
+{
+	struct nullb_device *dev = cmd->nq->dev;
+	struct nullb *nullb = dev->nullb;
+	blk_status_t sts = BLK_STS_OK;
+	struct request *rq = cmd->rq;
+
+	if (!hrtimer_active(&nullb->bw_timer))
+		hrtimer_restart(&nullb->bw_timer);
+
+	if (atomic_long_sub_return(blk_rq_bytes(rq), &nullb->cur_bytes) < 0) {
+		null_stop_queue(nullb);
+		/* race with timer */
+		if (atomic_long_read(&nullb->cur_bytes) > 0)
+			null_restart_queue_async(nullb);
+		/* requeue request */
+		sts = BLK_STS_DEV_RESOURCE;
+	}
+	return sts;
+}
+
 static blk_status_t null_handle_cmd(struct nullb_cmd *cmd, sector_t sector,
 				    sector_t nr_sectors, enum req_opf op)
 {
 	struct nullb_device *dev = cmd->nq->dev;
 	struct nullb *nullb = dev->nullb;
+	blk_status_t sts;
 	int err = 0;
 
 	if (test_bit(NULLB_DEV_FL_THROTTLED, &dev->flags)) {
-		struct request *rq = cmd->rq;
-
-		if (!hrtimer_active(&nullb->bw_timer))
-			hrtimer_restart(&nullb->bw_timer);
-
-		if (atomic_long_sub_return(blk_rq_bytes(rq),
-				&nullb->cur_bytes) < 0) {
-			null_stop_queue(nullb);
-			/* race with timer */
-			if (atomic_long_read(&nullb->cur_bytes) > 0)
-				null_restart_queue_async(nullb);
-			/* requeue request */
-			return BLK_STS_DEV_RESOURCE;
-		}
+		sts = null_handle_throttled(cmd);
+		if (sts != BLK_STS_OK)
+			return sts;
 	}
 
 	if (op == REQ_OP_FLUSH) {
-- 
2.17.0

Re: [PATCH V3 2/6] null_blk: create a helper for throttling

[Christoph Hellwig]

On Tue, Aug 20, 2019 at 11:13:10PM -0700, Chaitanya Kulkarni wrote:
> This patch creates a helper for handling throttling code in the
> null_handle_cmd().
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH V3 3/6] null_blk: create a helper for badblocks

[Chaitanya Kulkarni]

This patch creates a helper for handling badblocks code in the
null_handle_cmd().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/null_blk_main.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 751679fadc9d..eefaea1aaa45 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1154,6 +1154,20 @@ static inline blk_status_t null_handle_throttled(struct nullb_cmd *cmd)
 	return sts;
 }
 
+static inline blk_status_t null_handle_badblocks(struct nullb_cmd *cmd,
+						 sector_t sector,
+						 sector_t nr_sectors)
+{
+	struct badblocks *bb = &cmd->nq->dev->badblocks;
+	sector_t first_bad;
+	int bad_sectors;
+
+	if (badblocks_check(bb, sector, nr_sectors, &first_bad, &bad_sectors))
+		return BLK_STS_IOERR;
+
+	return BLK_STS_OK;
+}
+
 static blk_status_t null_handle_cmd(struct nullb_cmd *cmd, sector_t sector,
 				    sector_t nr_sectors, enum req_opf op)
 {
@@ -1172,15 +1186,11 @@ static blk_status_t null_handle_cmd(struct nullb_cmd *cmd, sector_t sector,
 		cmd->error = errno_to_blk_status(null_handle_flush(nullb));
 		goto out;
 	}
-	if (nullb->dev->badblocks.shift != -1) {
-		int bad_sectors;
-		sector_t first_bad;
 
-		if (badblocks_check(&nullb->dev->badblocks, sector, nr_sectors,
-				&first_bad, &bad_sectors)) {
-			cmd->error = BLK_STS_IOERR;
+	if (nullb->dev->badblocks.shift != -1) {
+		cmd->error = null_handle_badblocks(cmd, sector, nr_sectors);
+		if (cmd->error != BLK_STS_OK)
 			goto out;
-		}
 	}
 
 	if (dev->memory_backed) {
-- 
2.17.0

Re: [PATCH V3 3/6] null_blk: create a helper for badblocks

[Christoph Hellwig]

On Tue, Aug 20, 2019 at 11:13:11PM -0700, Chaitanya Kulkarni wrote:
> This patch creates a helper for handling badblocks code in the
> null_handle_cmd().
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  drivers/block/null_blk_main.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH V3 4/6] null_blk: create a helper for mem-backed ops

[Chaitanya Kulkarni]

This patch creates a helper for handling requests when null_blk is
memory backed in the null_handle_cmd(). Although the helper is very
simple right now, it makes the code flow consistent with the rest of
code in the null_handle_cmd() and provides a uniform code structure
for future code.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/null_blk_main.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index eefaea1aaa45..4299274cccfb 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1168,13 +1168,26 @@ static inline blk_status_t null_handle_badblocks(struct nullb_cmd *cmd,
 	return BLK_STS_OK;
 }
 
+static inline blk_status_t null_handle_memory_backed(struct nullb_cmd *cmd,
+						     enum req_opf op)
+{
+	struct nullb_device *dev = cmd->nq->dev;
+	int err;
+
+	if (dev->queue_mode == NULL_Q_BIO)
+		err = null_handle_bio(cmd);
+	else
+		err = null_handle_rq(cmd);
+
+	return errno_to_blk_status(err);
+}
+
 static blk_status_t null_handle_cmd(struct nullb_cmd *cmd, sector_t sector,
 				    sector_t nr_sectors, enum req_opf op)
 {
 	struct nullb_device *dev = cmd->nq->dev;
 	struct nullb *nullb = dev->nullb;
 	blk_status_t sts;
-	int err = 0;
 
 	if (test_bit(NULLB_DEV_FL_THROTTLED, &dev->flags)) {
 		sts = null_handle_throttled(cmd);
@@ -1193,14 +1206,8 @@ static blk_status_t null_handle_cmd(struct nullb_cmd *cmd, sector_t sector,
 			goto out;
 	}
 
-	if (dev->memory_backed) {
-		if (dev->queue_mode == NULL_Q_BIO)
-			err = null_handle_bio(cmd);
-		else
-			err = null_handle_rq(cmd);
-	}
-
-	cmd->error = errno_to_blk_status(err);
+	if (dev->memory_backed)
+		cmd->error = null_handle_memory_backed(cmd, op);
 
 	if (!cmd->error && dev->zoned) {
 		if (op == REQ_OP_WRITE)
-- 
2.17.0

Re: [PATCH V3 4/6] null_blk: create a helper for mem-backed ops

[Christoph Hellwig]

On Tue, Aug 20, 2019 at 11:13:12PM -0700, Chaitanya Kulkarni wrote:
> This patch creates a helper for handling requests when null_blk is
> memory backed in the null_handle_cmd(). Although the helper is very
> simple right now, it makes the code flow consistent with the rest of
> code in the null_handle_cmd() and provides a uniform code structure
> for future code.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH V3 5/6] null_blk: create a helper for zoned devices

[Chaitanya Kulkarni]

This patch creates a helper function for handling zoned block device
operations.

This patch also restructured the code for null_blk_zoned.c and uses the
pattern to return blk_status_t and catch the error in the function
null_handle_cmd() into cmd->error variable instead of setting it up in
the deeper layer just like the way it is done for flush, badblocks and
memory backed case in the null_handle_cmd().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/null_blk.h       | 19 +++++++++++++------
 drivers/block/null_blk_main.c  | 31 +++++++++++++++++++++++--------
 drivers/block/null_blk_zoned.c | 23 ++++++++++-------------
 3 files changed, 46 insertions(+), 27 deletions(-)

diff --git a/drivers/block/null_blk.h b/drivers/block/null_blk.h
index a1b9929bd911..1a8d5a12461f 100644
--- a/drivers/block/null_blk.h
+++ b/drivers/block/null_blk.h
@@ -90,9 +90,9 @@ int null_zone_init(struct nullb_device *dev);
 void null_zone_exit(struct nullb_device *dev);
 int null_zone_report(struct gendisk *disk, sector_t sector,
 		     struct blk_zone *zones, unsigned int *nr_zones);
-void null_zone_write(struct nullb_cmd *cmd, sector_t sector,
-			unsigned int nr_sectors);
-void null_zone_reset(struct nullb_cmd *cmd, sector_t sector);
+blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector,
+			     unsigned int nr_sectors);
+blk_status_t null_zone_reset(struct nullb_cmd *cmd, sector_t sector);
 #else
 static inline int null_zone_init(struct nullb_device *dev)
 {
@@ -106,10 +106,17 @@ static inline int null_zone_report(struct gendisk *disk, sector_t sector,
 {
 	return -EOPNOTSUPP;
 }
-static inline void null_zone_write(struct nullb_cmd *cmd, sector_t sector,
-				   unsigned int nr_sectors)
+static inline blk_status_t null_zone_write(struct nullb_cmd *cmd,
+					   sector_t sector,
+					   unsigned int nr_sectors)
 {
+	return BLK_STS_NOTSUPP;
+}
+
+static inline blk_status_t null_zone_reset(struct nullb_cmd *cmd,
+					   sector_t sector)
+{
+	return BLK_STS_NOTSUPP;
 }
-static inline void null_zone_reset(struct nullb_cmd *cmd, sector_t sector) {}
 #endif /* CONFIG_BLK_DEV_ZONED */
 #endif /* __NULL_BLK_H */
diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 4299274cccfb..501af79bffb2 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1182,6 +1182,26 @@ static inline blk_status_t null_handle_memory_backed(struct nullb_cmd *cmd,
 	return errno_to_blk_status(err);
 }
 
+static inline blk_status_t null_handle_zoned(struct nullb_cmd *cmd,
+					     enum req_opf op, sector_t sector,
+					     sector_t nr_sectors)
+{
+	blk_status_t sts = BLK_STS_OK;
+
+	switch (op) {
+	case REQ_OP_WRITE:
+		sts = null_zone_write(cmd, sector, nr_sectors);
+		break;
+	case REQ_OP_ZONE_RESET:
+		sts = null_zone_reset(cmd, sector);
+		break;
+	default:
+		break;
+	}
+
+	return sts;
+}
+
 static blk_status_t null_handle_cmd(struct nullb_cmd *cmd, sector_t sector,
 				    sector_t nr_sectors, enum req_opf op)
 {
@@ -1209,14 +1229,9 @@ static blk_status_t null_handle_cmd(struct nullb_cmd *cmd, sector_t sector,
 	if (dev->memory_backed)
 		cmd->error = null_handle_memory_backed(cmd, op);
 
-	if (!cmd->error && dev->zoned) {
-		if (op == REQ_OP_WRITE)
-			null_zone_write(cmd, sector, nr_sectors);
-		else if (op == REQ_OP_ZONE_RESET)
-			null_zone_reset(cmd, sector);
-		else if (op == REQ_OP_ZONE_RESET_ALL)
-			null_zone_reset(cmd, 0);
-	}
+	if (!cmd->error && dev->zoned)
+		cmd->error = null_handle_zoned(cmd, op, sector, nr_sectors);
+
 out:
 	/* Complete IO by inline, softirq or timer */
 	switch (dev->irqmode) {
diff --git a/drivers/block/null_blk_zoned.c b/drivers/block/null_blk_zoned.c
index 8c7f5bf81975..4e48b4e088ae 100644
--- a/drivers/block/null_blk_zoned.c
+++ b/drivers/block/null_blk_zoned.c
@@ -84,7 +84,7 @@ int null_zone_report(struct gendisk *disk, sector_t sector,
 	return 0;
 }
 
-void null_zone_write(struct nullb_cmd *cmd, sector_t sector,
+blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector,
 		     unsigned int nr_sectors)
 {
 	struct nullb_device *dev = cmd->nq->dev;
@@ -95,14 +95,12 @@ void null_zone_write(struct nullb_cmd *cmd, sector_t sector,
 	case BLK_ZONE_COND_FULL:
 		/* Cannot write to a full zone */
 		cmd->error = BLK_STS_IOERR;
-		break;
+		return BLK_STS_IOERR;
 	case BLK_ZONE_COND_EMPTY:
 	case BLK_ZONE_COND_IMP_OPEN:
 		/* Writes must be at the write pointer position */
-		if (sector != zone->wp) {
-			cmd->error = BLK_STS_IOERR;
-			break;
-		}
+		if (sector != zone->wp)
+			return BLK_STS_IOERR;
 
 		if (zone->cond == BLK_ZONE_COND_EMPTY)
 			zone->cond = BLK_ZONE_COND_IMP_OPEN;
@@ -115,12 +113,12 @@ void null_zone_write(struct nullb_cmd *cmd, sector_t sector,
 		break;
 	default:
 		/* Invalid zone condition */
-		cmd->error = BLK_STS_IOERR;
-		break;
+		return BLK_STS_IOERR;
 	}
+	return BLK_STS_OK;
 }
 
-void null_zone_reset(struct nullb_cmd *cmd, sector_t sector)
+blk_status_t null_zone_reset(struct nullb_cmd *cmd, sector_t sector)
 {
 	struct nullb_device *dev = cmd->nq->dev;
 	unsigned int zno = null_zone_no(dev, sector);
@@ -137,10 +135,8 @@ void null_zone_reset(struct nullb_cmd *cmd, sector_t sector)
 		}
 		break;
 	case REQ_OP_ZONE_RESET:
-		if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) {
-			cmd->error = BLK_STS_IOERR;
-			return;
-		}
+		if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL)
+			return BLK_STS_IOERR;
 
 		zone->cond = BLK_ZONE_COND_EMPTY;
 		zone->wp = zone->start;
@@ -149,4 +145,5 @@ void null_zone_reset(struct nullb_cmd *cmd, sector_t sector)
 		cmd->error = BLK_STS_NOTSUPP;
 		break;
 	}
+	return BLK_STS_OK;
 }
-- 
2.17.0

Re: [PATCH V3 5/6] null_blk: create a helper for zoned devices

[Christoph Hellwig]

> +static inline blk_status_t null_handle_zoned(struct nullb_cmd *cmd,
> +					     enum req_opf op, sector_t sector,
> +					     sector_t nr_sectors)
> +{

Shouldn't this go into null_blk_zoned.c?  Also the indentation for the
here seems odd.

> +	blk_status_t sts = BLK_STS_OK;
> +
> +	switch (op) {
> +	case REQ_OP_WRITE:
> +		sts = null_zone_write(cmd, sector, nr_sectors);
> +		break;
> +	case REQ_OP_ZONE_RESET:
> +		sts = null_zone_reset(cmd, sector);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return sts;

Why not:

	switch (op) {
	case REQ_OP_WRITE:
		return null_zone_write(cmd, sector, nr_sectors);
	case REQ_OP_ZONE_RESET:
		return null_zone_reset(cmd, sector);
	default:
		return BLK_STS_OK;
}

Re: [PATCH V3 5/6] null_blk: create a helper for zoned devices

[Chaitanya Kulkarni]

Okay, will send the new version..

On 8/21/19 5:58 PM, Christoph Hellwig wrote:
>> +static inline blk_status_t null_handle_zoned(struct nullb_cmd *cmd,
>> +					     enum req_opf op, sector_t sector,
>> +					     sector_t nr_sectors)
>> +{
> Shouldn't this go into null_blk_zoned.c?  Also the indentation for the
> here seems odd.
>
>> +	blk_status_t sts = BLK_STS_OK;
>> +
>> +	switch (op) {
>> +	case REQ_OP_WRITE:
>> +		sts = null_zone_write(cmd, sector, nr_sectors);
>> +		break;
>> +	case REQ_OP_ZONE_RESET:
>> +		sts = null_zone_reset(cmd, sector);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return sts;
> Why not:
>
> 	switch (op) {
> 	case REQ_OP_WRITE:
> 		return null_zone_write(cmd, sector, nr_sectors);
> 	case REQ_OP_ZONE_RESET:
> 		return null_zone_reset(cmd, sector);
> 	default:
> 		return BLK_STS_OK;
> }

[PATCH V3 6/6] null_blk: create a helper for req completion

[Chaitanya Kulkarni]

This patch creates a helper function for handling the request
completion in the null_handle_cmd().

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/null_blk_main.c | 49 +++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 501af79bffb2..fe12ec59b3a6 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1202,6 +1202,32 @@ static inline blk_status_t null_handle_zoned(struct nullb_cmd *cmd,
 	return sts;
 }
 
+static inline void nullb_handle_cmd_completion(struct nullb_cmd *cmd)
+{
+	/* Complete IO by inline, softirq or timer */
+	switch (cmd->nq->dev->irqmode) {
+	case NULL_IRQ_SOFTIRQ:
+		switch (cmd->nq->dev->queue_mode) {
+		case NULL_Q_MQ:
+			blk_mq_complete_request(cmd->rq);
+			break;
+		case NULL_Q_BIO:
+			/*
+			 * XXX: no proper submitting cpu information available.
+			 */
+			end_cmd(cmd);
+			break;
+		}
+		break;
+	case NULL_IRQ_NONE:
+		end_cmd(cmd);
+		break;
+	case NULL_IRQ_TIMER:
+		null_cmd_end_timer(cmd);
+		break;
+	}
+}
+
 static blk_status_t null_handle_cmd(struct nullb_cmd *cmd, sector_t sector,
 				    sector_t nr_sectors, enum req_opf op)
 {
@@ -1233,28 +1259,7 @@ static blk_status_t null_handle_cmd(struct nullb_cmd *cmd, sector_t sector,
 		cmd->error = null_handle_zoned(cmd, op, sector, nr_sectors);
 
 out:
-	/* Complete IO by inline, softirq or timer */
-	switch (dev->irqmode) {
-	case NULL_IRQ_SOFTIRQ:
-		switch (dev->queue_mode)  {
-		case NULL_Q_MQ:
-			blk_mq_complete_request(cmd->rq);
-			break;
-		case NULL_Q_BIO:
-			/*
-			 * XXX: no proper submitting cpu information available.
-			 */
-			end_cmd(cmd);
-			break;
-		}
-		break;
-	case NULL_IRQ_NONE:
-		end_cmd(cmd);
-		break;
-	case NULL_IRQ_TIMER:
-		null_cmd_end_timer(cmd);
-		break;
-	}
+	nullb_handle_cmd_completion(cmd);
 	return BLK_STS_OK;
 }
 
-- 
2.17.0

Re: [PATCH V3 6/6] null_blk: create a helper for req completion

[Christoph Hellwig]

On Tue, Aug 20, 2019 at 11:13:14PM -0700, Chaitanya Kulkarni wrote:
> This patch creates a helper function for handling the request
> completion in the null_handle_cmd().
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  drivers/block/null_blk_main.c | 49 +++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
> index 501af79bffb2..fe12ec59b3a6 100644
> --- a/drivers/block/null_blk_main.c
> +++ b/drivers/block/null_blk_main.c
> @@ -1202,6 +1202,32 @@ static inline blk_status_t null_handle_zoned(struct nullb_cmd *cmd,
>  	return sts;
>  }
>  
> +static inline void nullb_handle_cmd_completion(struct nullb_cmd *cmd)

Maybe cal this nullb_complete_cmd instead?

Otherwise looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH V3 6/6] null_blk: create a helper for req completion

[Chaitanya Kulkarni]

On 8/21/19 5:59 PM, Christoph Hellwig wrote:
> On Tue, Aug 20, 2019 at 11:13:14PM -0700, Chaitanya Kulkarni wrote:
>> This patch creates a helper function for handling the request
>> completion in the null_handle_cmd().
>>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>> ---
>>   drivers/block/null_blk_main.c | 49 +++++++++++++++++++----------------
>>   1 file changed, 27 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
>> index 501af79bffb2..fe12ec59b3a6 100644
>> --- a/drivers/block/null_blk_main.c
>> +++ b/drivers/block/null_blk_main.c
>> @@ -1202,6 +1202,32 @@ static inline blk_status_t null_handle_zoned(struct nullb_cmd *cmd,
>>   	return sts;
>>   }
>>   
>> +static inline void nullb_handle_cmd_completion(struct nullb_cmd *cmd)
> Maybe cal this nullb_complete_cmd instead?

Okay.

Jens do you want me to send new series with above fix or it can be done

at the time of applying the patch to minimize the emails ? either way 
I'm fine.

>
> Otherwise looks fine:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH V3 6/6] null_blk: create a helper for req completion

[Chaitanya Kulkarni]

Please ignore, I'll send a new version.

On 8/21/19 7:17 PM, Chaitanya Kulkarni wrote:
> On 8/21/19 5:59 PM, Christoph Hellwig wrote:
>> On Tue, Aug 20, 2019 at 11:13:14PM -0700, Chaitanya Kulkarni wrote:
>>> This patch creates a helper function for handling the request
>>> completion in the null_handle_cmd().
>>>
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>>> ---
>>>    drivers/block/null_blk_main.c | 49 +++++++++++++++++++----------------
>>>    1 file changed, 27 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
>>> index 501af79bffb2..fe12ec59b3a6 100644
>>> --- a/drivers/block/null_blk_main.c
>>> +++ b/drivers/block/null_blk_main.c
>>> @@ -1202,6 +1202,32 @@ static inline blk_status_t null_handle_zoned(struct nullb_cmd *cmd,
>>>    	return sts;
>>>    }
>>>    
>>> +static inline void nullb_handle_cmd_completion(struct nullb_cmd *cmd)
>> Maybe cal this nullb_complete_cmd instead?
> Okay.
>
> Jens do you want me to send new series with above fix or it can be done
>
> at the time of applying the patch to minimize the emails ? either way
> I'm fine.
>
>> Otherwise looks fine:
>>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>