[PATCH] block/blk-ioprio: Skip zoned writes that are not append operations

[PATCH] block/blk-ioprio: Skip zoned writes that are not append operations

[Bart Van Assche]

If REQ_OP_WRITE or REQ_OP_WRITE_ZEROES operations for the same zone
originate from different cgroups that could result in different
priorities being assigned to these operations. Do not modify the I/O
priority of these write operations to prevent that these would be
executed in the wrong order when using the mq-deadline I/O
scheduler.

Cc: Damien Le Moal <dlemoal@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/blk-ioprio.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/block/blk-ioprio.c b/block/blk-ioprio.c
index 4051fada01f1..09ce083a0e3a 100644
--- a/block/blk-ioprio.c
+++ b/block/blk-ioprio.c
@@ -192,6 +192,17 @@ void blkcg_set_ioprio(struct bio *bio)
 	if (!blkcg || blkcg->prio_policy == POLICY_NO_CHANGE)
 		return;
 
+	/*
+	 * If REQ_OP_WRITE or REQ_OP_WRITE_ZEROES operations for the same zone
+	 * originate from different cgroups that could result in different
+	 * priorities being assigned to these operations. Do not modify the I/O
+	 * priority of these write operations to prevent that these would be
+	 * executed in the wrong order when using the mq-deadline I/O
+	 * scheduler.
+	 */
+	if (bdev_op_is_zoned_write(bio->bi_bdev, bio_op(bio)))
+		return;
+
 	if (blkcg->prio_policy == POLICY_PROMOTE_TO_RT ||
 	    blkcg->prio_policy == POLICY_NONE_TO_RT) {
 		/*

Re: [PATCH] block/blk-ioprio: Skip zoned writes that are not append operations

[Damien Le Moal]

On 12/12/23 08:14, Bart Van Assche wrote:
> If REQ_OP_WRITE or REQ_OP_WRITE_ZEROES operations for the same zone
> originate from different cgroups that could result in different
> priorities being assigned to these operations. Do not modify the I/O
> priority of these write operations to prevent that these would be
> executed in the wrong order when using the mq-deadline I/O

...to prevent them from being executed in the wrong...

> scheduler.
> 
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  block/blk-ioprio.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/block/blk-ioprio.c b/block/blk-ioprio.c
> index 4051fada01f1..09ce083a0e3a 100644
> --- a/block/blk-ioprio.c
> +++ b/block/blk-ioprio.c
> @@ -192,6 +192,17 @@ void blkcg_set_ioprio(struct bio *bio)
>  	if (!blkcg || blkcg->prio_policy == POLICY_NO_CHANGE)
>  		return;
>  
> +	/*
> +	 * If REQ_OP_WRITE or REQ_OP_WRITE_ZEROES operations for the same zone
> +	 * originate from different cgroups that could result in different
> +	 * priorities being assigned to these operations. Do not modify the I/O
> +	 * priority of these write operations to prevent that these would be
> +	 * executed in the wrong order when using the mq-deadline I/O
> +	 * scheduler.
> +	 */
> +	if (bdev_op_is_zoned_write(bio->bi_bdev, bio_op(bio)))

Ideally, we want the bio equivalent of blk_rq_is_seq_zoned_write() here so that
writes to conventional zones are not affected (these can be reordered).

> +		return;
> +
>  	if (blkcg->prio_policy == POLICY_PROMOTE_TO_RT ||
>  	    blkcg->prio_policy == POLICY_NONE_TO_RT) {
>  		/*

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] block/blk-ioprio: Skip zoned writes that are not append operations

[Bart Van Assche]

On 12/11/23 15:31, Damien Le Moal wrote:
> On 12/12/23 08:14, Bart Van Assche wrote:
>> +	/*
>> +	 * If REQ_OP_WRITE or REQ_OP_WRITE_ZEROES operations for the same zone
>> +	 * originate from different cgroups that could result in different
>> +	 * priorities being assigned to these operations. Do not modify the I/O
>> +	 * priority of these write operations to prevent that these would be
>> +	 * executed in the wrong order when using the mq-deadline I/O
>> +	 * scheduler.
>> +	 */
>> +	if (bdev_op_is_zoned_write(bio->bi_bdev, bio_op(bio)))
> 
> Ideally, we want the bio equivalent of blk_rq_is_seq_zoned_write() here so that
> writes to conventional zones are not affected (these can be reordered).
  How about the patch below?

Thanks,

Bart.


[PATCH] block/blk-ioprio: Skip zoned writes that are not append operations

If REQ_OP_WRITE or REQ_OP_WRITE_ZEROES operations for the same zone
originate from different cgroups that could result in different priorities
being assigned to these operations. Do not modify the I/O priority of
these write operations to prevent them from being executed in the wrong
order when using the mq-deadline I/O scheduler.

Cc: Damien Le Moal <dlemoal@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
  block/blk-ioprio.c     | 11 +++++++++++
  include/linux/blk-mq.h | 17 +++++++++++++++++
  2 files changed, 28 insertions(+)

diff --git a/block/blk-ioprio.c b/block/blk-ioprio.c
index 4051fada01f1..96b46d34e3d6 100644
--- a/block/blk-ioprio.c
+++ b/block/blk-ioprio.c
@@ -192,6 +192,17 @@ void blkcg_set_ioprio(struct bio *bio)
  	if (!blkcg || blkcg->prio_policy == POLICY_NO_CHANGE)
  		return;

+	/*
+	 * If REQ_OP_WRITE or REQ_OP_WRITE_ZEROES operations for the same zone
+	 * originate from different cgroups that could result in different
+	 * priorities being assigned to these operations. Do not modify the I/O
+	 * priority of these write operations to prevent that these would be
+	 * executed in the wrong order when using the mq-deadline I/O
+	 * scheduler.
+	 */
+	if (blk_bio_is_seq_zoned_write(bio))
+		return;
+
  	if (blkcg->prio_policy == POLICY_PROMOTE_TO_RT ||
  	    blkcg->prio_policy == POLICY_NONE_TO_RT) {
  		/*
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 1ab3081c82ed..90907d9001c0 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -1149,6 +1149,18 @@ static inline unsigned int blk_rq_zone_no(struct request *rq)
  	return disk_zone_no(rq->q->disk, blk_rq_pos(rq));
  }

+/**
+ * blk_bio_is_seq_zoned_write() - Check if @bio requires write serialization.
+ * @bio: Bio to examine.
+ *
+ * Note: REQ_OP_ZONE_APPEND bios do not require serialization.
+ */
+static inline bool blk_bio_is_seq_zoned_write(struct bio *bio)
+{
+	return op_needs_zoned_write_locking(bio_op(bio)) &&
+		disk_zone_is_seq(bio->bi_disk, bio.bi_iter.bi_sector);
+}
+
  static inline unsigned int blk_rq_zone_is_seq(struct request *rq)
  {
  	return disk_zone_is_seq(rq->q->disk, blk_rq_pos(rq));
@@ -1196,6 +1208,11 @@ static inline bool blk_req_can_dispatch_to_zone(struct request *rq)
  	return !blk_req_zone_is_write_locked(rq);
  }
  #else /* CONFIG_BLK_DEV_ZONED */
+static inline bool blk_bio_is_seq_zoned_write(struct bio *bio)
+{
+	return false;
+}
+
  static inline bool blk_rq_is_seq_zoned_write(struct request *rq)
  {
  	return false;

Re: [PATCH] block/blk-ioprio: Skip zoned writes that are not append operations

[Damien Le Moal]

On 12/12/23 09:11, Bart Van Assche wrote:
> On 12/11/23 15:31, Damien Le Moal wrote:
>> On 12/12/23 08:14, Bart Van Assche wrote:
>>> +	/*
>>> +	 * If REQ_OP_WRITE or REQ_OP_WRITE_ZEROES operations for the same zone
>>> +	 * originate from different cgroups that could result in different
>>> +	 * priorities being assigned to these operations. Do not modify the I/O
>>> +	 * priority of these write operations to prevent that these would be
>>> +	 * executed in the wrong order when using the mq-deadline I/O
>>> +	 * scheduler.
>>> +	 */
>>> +	if (bdev_op_is_zoned_write(bio->bi_bdev, bio_op(bio)))
>>
>> Ideally, we want the bio equivalent of blk_rq_is_seq_zoned_write() here so that
>> writes to conventional zones are not affected (these can be reordered).
>   How about the patch below?
> 
> Thanks,
> 
> Bart.
> 
> 
> [PATCH] block/blk-ioprio: Skip zoned writes that are not append operations
> 
> If REQ_OP_WRITE or REQ_OP_WRITE_ZEROES operations for the same zone
> originate from different cgroups that could result in different priorities
> being assigned to these operations. Do not modify the I/O priority of
> these write operations to prevent them from being executed in the wrong
> order when using the mq-deadline I/O scheduler.
> 
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   block/blk-ioprio.c     | 11 +++++++++++
>   include/linux/blk-mq.h | 17 +++++++++++++++++
>   2 files changed, 28 insertions(+)
> 
> diff --git a/block/blk-ioprio.c b/block/blk-ioprio.c
> index 4051fada01f1..96b46d34e3d6 100644
> --- a/block/blk-ioprio.c
> +++ b/block/blk-ioprio.c
> @@ -192,6 +192,17 @@ void blkcg_set_ioprio(struct bio *bio)
>   	if (!blkcg || blkcg->prio_policy == POLICY_NO_CHANGE)
>   		return;
> 
> +	/*
> +	 * If REQ_OP_WRITE or REQ_OP_WRITE_ZEROES operations for the same zone
> +	 * originate from different cgroups that could result in different
> +	 * priorities being assigned to these operations. Do not modify the I/O
> +	 * priority of these write operations to prevent that these would be
> +	 * executed in the wrong order when using the mq-deadline I/O
> +	 * scheduler.
> +	 */
> +	if (blk_bio_is_seq_zoned_write(bio))
> +		return;
> +
>   	if (blkcg->prio_policy == POLICY_PROMOTE_TO_RT ||
>   	    blkcg->prio_policy == POLICY_NONE_TO_RT) {
>   		/*
> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> index 1ab3081c82ed..90907d9001c0 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -1149,6 +1149,18 @@ static inline unsigned int blk_rq_zone_no(struct request *rq)
>   	return disk_zone_no(rq->q->disk, blk_rq_pos(rq));
>   }
> 
> +/**
> + * blk_bio_is_seq_zoned_write() - Check if @bio requires write serialization.
> + * @bio: Bio to examine.
> + *
> + * Note: REQ_OP_ZONE_APPEND bios do not require serialization.
> + */
> +static inline bool blk_bio_is_seq_zoned_write(struct bio *bio)
> +{
> +	return op_needs_zoned_write_locking(bio_op(bio)) &&
> +		disk_zone_is_seq(bio->bi_disk, bio.bi_iter.bi_sector);

Given that disk_zone_is_seq() always return false for regular devices, I think
reversing the test order is better:

	return disk_zone_is_seq(bio->bi_disk, bio.bi_iter.bi_sector) &&
	       op_needs_zoned_write_locking(bio_op(bio));

> +}
> +
>   static inline unsigned int blk_rq_zone_is_seq(struct request *rq)
>   {
>   	return disk_zone_is_seq(rq->q->disk, blk_rq_pos(rq));
> @@ -1196,6 +1208,11 @@ static inline bool blk_req_can_dispatch_to_zone(struct request *rq)
>   	return !blk_req_zone_is_write_locked(rq);
>   }
>   #else /* CONFIG_BLK_DEV_ZONED */
> +static inline bool blk_bio_is_seq_zoned_write(struct bio *bio)
> +{
> +	return false;
> +}
> +
>   static inline bool blk_rq_is_seq_zoned_write(struct request *rq)
>   {
>   	return false;
> 

-- 
Damien Le Moal
Western Digital Research

Re: Re: [PATCH] block/blk-ioprio: Skip zoned writes that are not append operations

[kernel test robot]

Hi Bart,

kernel test robot noticed the following build errors:

[auto build test ERROR on axboe-block/for-next]
[also build test ERROR on linus/master v6.7-rc5 next-20231212]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bart-Van-Assche/Re-PATCH-block-blk-ioprio-Skip-zoned-writes-that-are-not-append-operations/20231212-081223
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
patch link:    https://lore.kernel.org/r/d660cc31-a5be-47f2-9fdf-ba4bf5106226%40acm.org
patch subject: Re: [PATCH] block/blk-ioprio: Skip zoned writes that are not append operations
config: x86_64-rhel-8.3-func (https://download.01.org/0day-ci/archive/20231213/202312130548.cusrlw1s-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231213/202312130548.cusrlw1s-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312130548.cusrlw1s-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/t10-pi.h:6,
                    from include/scsi/scsi_cmnd.h:7,
                    from drivers/scsi/scsi.c:61:
   include/linux/blk-mq.h: In function 'blk_bio_is_seq_zoned_write':
>> include/linux/blk-mq.h:1161:37: error: 'struct bio' has no member named 'bi_disk'
    1161 |                 disk_zone_is_seq(bio->bi_disk, bio.bi_iter.bi_sector);
         |                                     ^~
>> include/linux/blk-mq.h:1161:51: error: 'bio' is a pointer; did you mean to use '->'?
    1161 |                 disk_zone_is_seq(bio->bi_disk, bio.bi_iter.bi_sector);
         |                                                   ^
         |                                                   ->


vim +1161 include/linux/blk-mq.h

  1151	
  1152	/**
  1153	 * blk_bio_is_seq_zoned_write() - Check if @bio requires write serialization.
  1154	 * @bio: Bio to examine.
  1155	 *
  1156	 * Note: REQ_OP_ZONE_APPEND bios do not require serialization.
  1157	 */
  1158	static inline bool blk_bio_is_seq_zoned_write(struct bio *bio)
  1159	{
  1160		return op_needs_zoned_write_locking(bio_op(bio)) &&
> 1161			disk_zone_is_seq(bio->bi_disk, bio.bi_iter.bi_sector);
  1162	}
  1163	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki