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

Commit Message

Chaitanya Kulkarni June 29, 2019, 5:04 a.m. UTC

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

Signed-off-by: Chaitanya Kulkarni <[e-mail]>
---
 drivers/block/null_blk_main.c | 62 ++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 23 deletions(-)

Comments

Christoph Hellwig July 1, 2019, 6:29 a.m. UTC | #1

> +	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

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) {