Re: [PATCH v3 2/9] block: Micro-optimize blk_req_needs_zone_write_lock()

[Christoph Hellwig <hch@xxxxxx>]

On Mon, Apr 24, 2023 at 01:33:22PM -0700, Bart Van Assche wrote:
> @@ -367,8 +367,9 @@ static inline void blk_mq_clear_mq_map(struct blk_mq_queue_map *qmap)
>  static inline struct blk_plug *blk_mq_plug( struct bio *bio)
>  {
>  	/* Zoned block device write operation case: do not plug the BIO */
> -	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) &&
> -	    bdev_op_is_zoned_write(bio->bi_bdev, bio_op(bio)))
> +	if ((bio_op(bio) == REQ_OP_WRITE ||
> +	     bio_op(bio) == REQ_OP_WRITE_ZEROES) &&
> +	    disk_zone_is_seq(bio->bi_bdev->bd_disk, bio->bi_iter.bi_sector))
>  		return NULL;

I find this a bit hard to hard to read.  Why not:

	if (disk_zone_is_seq(bio->bi_bdev->bd_disk, bio->bi_iter.bi_sector)) {
	  	/*
		 * Do not plug for writes that require zone locking.
		 */
		if (bio_op(bio) == REQ_OP_WRITE ||
		    bio_op(bio) == REQ_OP_WRITE_ZEROES)
			return NULL;
	}

>  bool blk_req_needs_zone_write_lock(struct request *rq)
>  {
> -	if (!rq->q->disk->seq_zones_wlock)
> -		return false;
> -
> -	if (bdev_op_is_zoned_write(rq->q->disk->part0, req_op(rq)))
> -		return blk_rq_zone_is_seq(rq);
> -
> -	return false;
> +	return rq->q->disk->seq_zones_wlock &&
> +		(req_op(rq) == REQ_OP_WRITE ||
> +		 req_op(rq) == REQ_OP_WRITE_ZEROES) &&
> +		blk_rq_zone_is_seq(rq);

Similaly here.  The old version did flow much better, so I'd prefer
something like:


	if (!rq->q->disk->seq_zones_wlock || !blk_rq_zone_is_seq(rq))
		return false;
	return req_op(rq) == REQ_OP_WRITE || req_op(rq) == REQ_OP_WRITE_ZEROES);

I also wonder if the check that and op is write or write zeroes, that
is needs zone locking would be useful instead of dupliating it all
over.  That is instead of removing bdev_op_is_zoned_write
keep a op_is_zoned_write without the bdev_is_zoned check.