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

[Christoph Hellwig <hch@xxxxxx>]

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