[PATCH 0/5] null_blk: simplify null_handle_cmd()

[PATCH 0/5] 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.

Regards,
Chaitanya

Chaitanya Kulkarni (5):
  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_main.c | 189 +++++++++++++++++++++-------------
 1 file changed, 120 insertions(+), 69 deletions(-)

-- 
2.21.0

[PATCH 1/5] 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 | 43 +++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 99328ded60d1..98e2985f57fc 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1133,28 +1133,41 @@ 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 inline blk_status_t null_handle_throttled(struct nullb_cmd *cmd)
 {
 	struct nullb_device *dev = cmd->nq->dev;
 	struct nullb *nullb = dev->nullb;
-	int err = 0;
+	blk_status_t sts = BLK_STS_OK;
+	struct request *rq = cmd->rq;
 
-	if (test_bit(NULLB_DEV_FL_THROTTLED, &dev->flags)) {
-		struct request *rq = cmd->rq;
+	if (!test_bit(NULLB_DEV_FL_THROTTLED, &dev->flags))
+		goto out;
 
-		if (!hrtimer_active(&nullb->bw_timer))
-			hrtimer_restart(&nullb->bw_timer);
+	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;
-		}
+	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;
 	}
+out:
+	return sts;
+}
+
+static blk_status_t null_handle_cmd(struct nullb_cmd *cmd)
+{
+	struct nullb_device *dev = cmd->nq->dev;
+	struct nullb *nullb = dev->nullb;
+	blk_status_t sts;
+	int err = 0;
+
+	sts = null_handle_throttled(cmd);
+	if (sts != BLK_STS_OK)
+		return sts;
 
 	if (nullb->dev->badblocks.shift != -1) {
 		int bad_sectors;
-- 
2.21.0

Re: [PATCH 1/5] null_blk: create a helper for throttling

[Christoph Hellwig]

> +	if (!test_bit(NULLB_DEV_FL_THROTTLED, &dev->flags))
> +		goto out;

Maybe keep this check in the caller to make it more obvious
when it actually does something?

[PATCH 2/5] 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 | 62 ++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 23 deletions(-)

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 98e2985f57fc..80c30bcf024f 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1158,6 +1158,42 @@ 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)
+{
+	struct nullb_device *dev = cmd->nq->dev;
+	struct badblocks *bb = &dev->badblocks;
+	sector_t sector, size, first_bad;
+	blk_status_t sts = BLK_STS_OK;
+	bool is_flush = true;
+	int bad_sectors;
+
+	if (dev->nullb->dev->badblocks.shift == -1)
+		goto out;
+
+	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)
+		goto out;
+
+	if (badblocks_check(bb, sector, size, &first_bad, &bad_sectors)) {
+		cmd->error = BLK_STS_IOERR;
+		sts = BLK_STS_IOERR;
+	}
+out:
+	return sts;
+}
+
 static blk_status_t null_handle_cmd(struct nullb_cmd *cmd)
 {
 	struct nullb_device *dev = cmd->nq->dev;
@@ -1169,29 +1205,9 @@ static blk_status_t null_handle_cmd(struct nullb_cmd *cmd)
 	if (sts != BLK_STS_OK)
 		return sts;
 
-	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)) {
-			cmd->error = BLK_STS_IOERR;
-			goto out;
-		}
-	}
+	sts = null_handle_badblocks(cmd);
+	if (sts != BLK_STS_OK)
+		goto out;
 
 	if (dev->memory_backed) {
 		if (dev->queue_mode == NULL_Q_BIO) {
-- 
2.21.0

Re: [PATCH 2/5] null_blk: create a helper for badblocks

[Christoph Hellwig]

> +	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)
> +		goto out;

This isn't really new in your patch, but looks very odd.  Why not:

	if (dev->queue_mode == NULL_Q_BIO) {
		if (bio_op(cmd->bio) == REQ_OP_FLUSH)
			return BLK_STS_OK;
		sector = cmd->bio->bi_iter.bi_sector;
		size = bio_sectors(cmd->bio);
	} else {
		if (req_op(cmd->rq) == REQ_OP_FLUSH)
			return BLK_STS_OK;
		sector = blk_rq_pos(cmd->rq);
		size = blk_rq_sectors(cmd->rq);
	}

> +	if (badblocks_check(bb, sector, size, &first_bad, &bad_sectors)) {
> +		cmd->error = BLK_STS_IOERR;
> +		sts = BLK_STS_IOERR;
> +	}
> +out:
> +	return sts;

Also I find the idea of a goto label that just does a return rather odd.
Please just return directly to make it obvious what is going on.

But in general for the whole series:  I'd much prefer moving the
bio vs request handling out of null_handle_cmd and into the callers
rather than hiding them one layer deeper in helpers.  Patch 1 is
a good help for that, and anything else factoring out common code,
but code for request vs bio should much rather move to the callers.

[PATCH 3/5] 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().

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

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 80c30bcf024f..e75d187c7393 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1194,10 +1194,29 @@ static inline blk_status_t null_handle_badblocks(struct nullb_cmd *cmd)
 	return sts;
 }
 
+static inline int nullb_handle_memory_backed(struct nullb_cmd *cmd)
+{
+	struct nullb_device *dev = cmd->nq->dev;
+
+	if (!dev->memory_backed)
+		return 0;
+
+	if (dev->queue_mode == NULL_Q_BIO) {
+		if (bio_op(cmd->bio) == REQ_OP_FLUSH)
+			return null_handle_flush(dev->nullb);
+
+		return null_handle_bio(cmd);
+	}
+
+	if (req_op(cmd->rq) == REQ_OP_FLUSH)
+		return null_handle_flush(dev->nullb);
+
+	return null_handle_rq(cmd);
+}
+
 static blk_status_t null_handle_cmd(struct nullb_cmd *cmd)
 {
 	struct nullb_device *dev = cmd->nq->dev;
-	struct nullb *nullb = dev->nullb;
 	blk_status_t sts;
 	int err = 0;
 
@@ -1209,19 +1228,7 @@ static blk_status_t null_handle_cmd(struct nullb_cmd *cmd)
 	if (sts != BLK_STS_OK)
 		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);
-		}
-	}
+	err = nullb_handle_memory_backed(cmd);
 	cmd->error = errno_to_blk_status(err);
 
 	if (!cmd->error && dev->zoned) {
-- 
2.21.0

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

[Chaitanya Kulkarni]

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

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

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index e75d187c7393..824392681a28 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1214,6 +1214,34 @@ static inline int nullb_handle_memory_backed(struct nullb_cmd *cmd)
 	return null_handle_rq(cmd);
 }
 
+static inline void nullb_handle_zoned(struct nullb_cmd *cmd)
+{
+	unsigned int nr_sectors;
+	sector_t sector;
+	req_opf op;
+
+	if (cmd->nq->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);
+	}
+
+	switch ((op)) {
+	case REQ_OP_WRITE:
+		null_zone_write(cmd, sector, nr_sectors);
+		break;
+	case REQ_OP_ZONE_RESET:
+		null_zone_reset(cmd, sector);
+		break;
+	default:
+		break;
+	}
+}
+
 static blk_status_t null_handle_cmd(struct nullb_cmd *cmd)
 {
 	struct nullb_device *dev = cmd->nq->dev;
@@ -1231,26 +1259,8 @@ static blk_status_t null_handle_cmd(struct nullb_cmd *cmd)
 	err = nullb_handle_memory_backed(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)
-			null_zone_reset(cmd, sector);
-	}
+	if (!cmd->error && dev->zoned)
+		nullb_handle_zoned(cmd);
 out:
 	/* Complete IO by inline, softirq or timer */
 	switch (dev->irqmode) {
-- 
2.21.0

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

[Christoph Hellwig]

On Fri, Jun 28, 2019 at 10:04:41PM -0700, Chaitanya Kulkarni wrote:
> This patch creates a helper function for handling zoned block device
> operations.
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  drivers/block/null_blk_main.c | 50 +++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
> index e75d187c7393..824392681a28 100644
> --- a/drivers/block/null_blk_main.c
> +++ b/drivers/block/null_blk_main.c
> @@ -1214,6 +1214,34 @@ static inline int nullb_handle_memory_backed(struct nullb_cmd *cmd)
>  	return null_handle_rq(cmd);
>  }
>  
> +static inline void nullb_handle_zoned(struct nullb_cmd *cmd)
> +{
> +	unsigned int nr_sectors;
> +	sector_t sector;
> +	req_opf op;
> +
> +	if (cmd->nq->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);
> +	}

And this checks for the same info as the previous one.
You probably just want to pass sector and nr_sectors as an argument
to null_handle_cmd early in the series.

> +	switch ((op)) {

No need for the double braces.

[PATCH 5/5] 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().

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

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 824392681a28..c5343d514c66 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1242,30 +1242,12 @@ static inline void nullb_handle_zoned(struct nullb_cmd *cmd)
 	}
 }
 
-static blk_status_t null_handle_cmd(struct nullb_cmd *cmd)
+static inline void nullb_handle_cmd_completion(struct nullb_cmd *cmd)
 {
-	struct nullb_device *dev = cmd->nq->dev;
-	blk_status_t sts;
-	int err = 0;
-
-	sts = null_handle_throttled(cmd);
-	if (sts != BLK_STS_OK)
-		return sts;
-
-	sts = null_handle_badblocks(cmd);
-	if (sts != BLK_STS_OK)
-		goto out;
-
-	err = nullb_handle_memory_backed(cmd);
-	cmd->error = errno_to_blk_status(err);
-
-	if (!cmd->error && dev->zoned)
-		nullb_handle_zoned(cmd);
-out:
 	/* Complete IO by inline, softirq or timer */
-	switch (dev->irqmode) {
+	switch (cmd->nq->dev->irqmode) {
 	case NULL_IRQ_SOFTIRQ:
-		switch (dev->queue_mode)  {
+		switch (cmd->nq->dev->queue_mode)  {
 		case NULL_Q_MQ:
 			blk_mq_complete_request(cmd->rq);
 			break;
@@ -1284,6 +1266,29 @@ static blk_status_t null_handle_cmd(struct nullb_cmd *cmd)
 		null_cmd_end_timer(cmd);
 		break;
 	}
+}
+
+static blk_status_t null_handle_cmd(struct nullb_cmd *cmd)
+{
+	struct nullb_device *dev = cmd->nq->dev;
+	blk_status_t sts;
+	int err = 0;
+
+	sts = null_handle_throttled(cmd);
+	if (sts != BLK_STS_OK)
+		return sts;
+
+	sts = null_handle_badblocks(cmd);
+	if (sts != BLK_STS_OK)
+		goto out;
+
+	err = nullb_handle_memory_backed(cmd);
+	cmd->error = errno_to_blk_status(err);
+
+	if (!cmd->error && dev->zoned)
+		nullb_handle_zoned(cmd);
+out:
+	nullb_handle_cmd_completion(cmd);
 	return BLK_STS_OK;
 }
 
-- 
2.21.0

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

[Christoph Hellwig]

On Fri, Jun 28, 2019 at 10:04:42PM -0700, Chaitanya Kulkarni wrote:
> This patch creates a helper function for handling the request
> completion in the null_handle_cmd().
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Looks good:

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