[PATCH v3 0/2] plugging cleanup v3

[PATCH v3 0/2] plugging cleanup v3

[Pankaj Raghav]

Hi Jens,

1st patch modifies blk_mq_plug() function to disable plugging for
 write and write zeroes in zoned block devices.

2nd patch uses the blk_mq_plug function in the block layer consistently.

The patches are based on next-20220923.

Changes since v2:
- Enhance the commit log for the 2nd patch (Christoph)

Changes since v1:
- Explicltly check only for write and write zeroes as they require zone
  locking in blk_mq_plug
- create a new helper to check for ops that require zone locking for
  zoned devices and also reuse it in blk_req_needs_zone_write_lock

Pankaj Raghav (2):
  block: adapt blk_mq_plug() to not plug for writes that require a zone
    lock
  block: use blk_mq_plug() wrapper consistently in the block layer

 block/blk-core.c       | 2 +-
 block/blk-mq.c         | 6 ++++--
 block/blk-mq.h         | 3 ++-
 block/blk-zoned.c      | 9 +++------
 include/linux/blkdev.h | 9 +++++++++
 5 files changed, 19 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH v3 1/2] block: adapt blk_mq_plug() to not plug for writes that require a zone lock

[Pankaj Raghav]

The current implementation of blk_mq_plug() disables plugging for all
operations that involves a transfer to the device as we just check if
the last bit in op_is_write() function.

Modify blk_mq_plug() to disable plugging only for REQ_OP_WRITE and
REQ_OP_WRITE_ZEROS as they might require a zone lock.

Suggested-by: Christoph Hellwig <hch@lst.de>
Suggested-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
---
 block/blk-mq.h         | 3 ++-
 block/blk-zoned.c      | 9 +++------
 include/linux/blkdev.h | 9 +++++++++
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/block/blk-mq.h b/block/blk-mq.h
index 8ca453ac243d..0b2870839cdd 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -312,7 +312,8 @@ 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 (bdev_is_zoned(bio->bi_bdev) && op_is_write(bio_op(bio)))
+	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) &&
+	    bdev_op_is_zoned_write(bio->bi_bdev, bio_op(bio)))
 		return NULL;
 
 	/*
diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index a264621d4905..db829401d8d0 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -63,13 +63,10 @@ bool blk_req_needs_zone_write_lock(struct request *rq)
 	if (!rq->q->disk->seq_zones_wlock)
 		return false;
 
-	switch (req_op(rq)) {
-	case REQ_OP_WRITE_ZEROES:
-	case REQ_OP_WRITE:
+	if (bdev_op_is_zoned_write(rq->q->disk->part0, req_op(rq)))
 		return blk_rq_zone_is_seq(rq);
-	default:
-		return false;
-	}
+
+	return false;
 }
 EXPORT_SYMBOL_GPL(blk_req_needs_zone_write_lock);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 8038c5fbde40..74bc30c680d6 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1300,6 +1300,15 @@ static inline bool bdev_is_zoned(struct block_device *bdev)
 	return false;
 }
 
+static inline bool bdev_op_is_zoned_write(struct block_device *bdev,
+					  blk_opf_t op)
+{
+	if (!bdev_is_zoned(bdev))
+		return false;
+
+	return op == REQ_OP_WRITE || op == REQ_OP_WRITE_ZEROES;
+}
+
 static inline sector_t bdev_zone_sectors(struct block_device *bdev)
 {
 	struct request_queue *q = bdev_get_queue(bdev);
-- 
2.25.1

[PATCH v3 2/2] block: use blk_mq_plug() wrapper consistently in the block layer

[Pankaj Raghav]

Use blk_mq_plug() wrapper to get the plug instead of directly accessing
it in the block layer.

Either of the changes should not have led to a bug in zoned devices:

- blk_execute_rq_nowait:
  Only passthrough requests can use this function, and plugging can be
  performed on those requests in zoned devices. So no issues directly
  accessing the plug.

- blk_flush_plug in bio_poll:
  As we don't plug the requests that require a zone lock in the first
  place, flushing should not have any impact. So no issues directly
  accessing the plug.

This is just a cleanup patch to use this wrapper to get the plug
consistently across the block layer.

Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
---
 block/blk-core.c | 2 +-
 block/blk-mq.c   | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 203be672da52..d0e97de216db 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -850,7 +850,7 @@ int bio_poll(struct bio *bio, struct io_comp_batch *iob, unsigned int flags)
 	    !test_bit(QUEUE_FLAG_POLL, &q->queue_flags))
 		return 0;
 
-	blk_flush_plug(current->plug, false);
+	blk_flush_plug(blk_mq_plug(bio), false);
 
 	if (bio_queue_enter(bio))
 		return 0;
diff --git a/block/blk-mq.c b/block/blk-mq.c
index c11949d66163..5bf245c4bf0a 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1209,12 +1209,14 @@ static void blk_add_rq_to_plug(struct blk_plug *plug, struct request *rq)
  */
 void blk_execute_rq_nowait(struct request *rq, bool at_head)
 {
+	struct blk_plug *plug = blk_mq_plug(rq->bio);
+
 	WARN_ON(irqs_disabled());
 	WARN_ON(!blk_rq_is_passthrough(rq));
 
 	blk_account_io_start(rq);
-	if (current->plug)
-		blk_add_rq_to_plug(current->plug, rq);
+	if (plug)
+		blk_add_rq_to_plug(plug, rq);
 	else
 		blk_mq_sched_insert_request(rq, at_head, true, false);
 }
-- 
2.25.1

Re: [PATCH v3 1/2] block: adapt blk_mq_plug() to not plug for writes that require a zone lock

[Damien Le Moal]

On 9/29/22 16:47, Pankaj Raghav wrote:
> The current implementation of blk_mq_plug() disables plugging for all
> operations that involves a transfer to the device as we just check if
> the last bit in op_is_write() function.
> 
> Modify blk_mq_plug() to disable plugging only for REQ_OP_WRITE and
> REQ_OP_WRITE_ZEROS as they might require a zone lock.
> 
> Suggested-by: Christoph Hellwig <hch@lst.de>
> Suggested-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

> ---
>  block/blk-mq.h         | 3 ++-
>  block/blk-zoned.c      | 9 +++------
>  include/linux/blkdev.h | 9 +++++++++
>  3 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/block/blk-mq.h b/block/blk-mq.h
> index 8ca453ac243d..0b2870839cdd 100644
> --- a/block/blk-mq.h
> +++ b/block/blk-mq.h
> @@ -312,7 +312,8 @@ 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 (bdev_is_zoned(bio->bi_bdev) && op_is_write(bio_op(bio)))
> +	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) &&
> +	    bdev_op_is_zoned_write(bio->bi_bdev, bio_op(bio)))
>  		return NULL;
>  
>  	/*
> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> index a264621d4905..db829401d8d0 100644
> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c
> @@ -63,13 +63,10 @@ bool blk_req_needs_zone_write_lock(struct request *rq)
>  	if (!rq->q->disk->seq_zones_wlock)
>  		return false;
>  
> -	switch (req_op(rq)) {
> -	case REQ_OP_WRITE_ZEROES:
> -	case REQ_OP_WRITE:
> +	if (bdev_op_is_zoned_write(rq->q->disk->part0, req_op(rq)))
>  		return blk_rq_zone_is_seq(rq);
> -	default:
> -		return false;
> -	}
> +
> +	return false;
>  }
>  EXPORT_SYMBOL_GPL(blk_req_needs_zone_write_lock);
>  
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 8038c5fbde40..74bc30c680d6 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1300,6 +1300,15 @@ static inline bool bdev_is_zoned(struct block_device *bdev)
>  	return false;
>  }
>  
> +static inline bool bdev_op_is_zoned_write(struct block_device *bdev,
> +					  blk_opf_t op)
> +{
> +	if (!bdev_is_zoned(bdev))
> +		return false;
> +
> +	return op == REQ_OP_WRITE || op == REQ_OP_WRITE_ZEROES;
> +}
> +
>  static inline sector_t bdev_zone_sectors(struct block_device *bdev)
>  {
>  	struct request_queue *q = bdev_get_queue(bdev);

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 2/2] block: use blk_mq_plug() wrapper consistently in the block layer

[Damien Le Moal]

On 9/29/22 16:47, Pankaj Raghav wrote:
> Use blk_mq_plug() wrapper to get the plug instead of directly accessing
> it in the block layer.
> 
> Either of the changes should not have led to a bug in zoned devices:
> 
> - blk_execute_rq_nowait:
>   Only passthrough requests can use this function, and plugging can be
>   performed on those requests in zoned devices. So no issues directly
>   accessing the plug.
> 
> - blk_flush_plug in bio_poll:
>   As we don't plug the requests that require a zone lock in the first
>   place, flushing should not have any impact. So no issues directly
>   accessing the plug.
> 
> This is just a cleanup patch to use this wrapper to get the plug
> consistently across the block layer.
> 
> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>


> ---
>  block/blk-core.c | 2 +-
>  block/blk-mq.c   | 6 ++++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 203be672da52..d0e97de216db 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -850,7 +850,7 @@ int bio_poll(struct bio *bio, struct io_comp_batch *iob, unsigned int flags)
>  	    !test_bit(QUEUE_FLAG_POLL, &q->queue_flags))
>  		return 0;
>  
> -	blk_flush_plug(current->plug, false);
> +	blk_flush_plug(blk_mq_plug(bio), false);
>  
>  	if (bio_queue_enter(bio))
>  		return 0;
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index c11949d66163..5bf245c4bf0a 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1209,12 +1209,14 @@ static void blk_add_rq_to_plug(struct blk_plug *plug, struct request *rq)
>   */
>  void blk_execute_rq_nowait(struct request *rq, bool at_head)
>  {
> +	struct blk_plug *plug = blk_mq_plug(rq->bio);
> +
>  	WARN_ON(irqs_disabled());
>  	WARN_ON(!blk_rq_is_passthrough(rq));
>  
>  	blk_account_io_start(rq);
> -	if (current->plug)
> -		blk_add_rq_to_plug(current->plug, rq);
> +	if (plug)
> +		blk_add_rq_to_plug(plug, rq);
>  	else
>  		blk_mq_sched_insert_request(rq, at_head, true, false);
>  }

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 1/2] block: adapt blk_mq_plug() to not plug for writes that require a zone lock

[Johannes Thumshirn]

On 29.09.22 09:49, Pankaj Raghav wrote:
Nit: (but I think Jens can fix this up when applying if he thinks so 
as well)

> The current implementation of blk_mq_plug() disables plugging for all
> operations that involves a transfer to the device as we just check if

                         to a zoned device ~^


Otherwise,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v3 2/2] block: use blk_mq_plug() wrapper consistently in the block layer

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v3 2/2] block: use blk_mq_plug() wrapper consistently in the block layer

[Jens Axboe]

On 9/29/22 1:47 AM, Pankaj Raghav wrote:
> Use blk_mq_plug() wrapper to get the plug instead of directly accessing
> it in the block layer.
> 
> Either of the changes should not have led to a bug in zoned devices:
> 
> - blk_execute_rq_nowait:
>   Only passthrough requests can use this function, and plugging can be
>   performed on those requests in zoned devices. So no issues directly
>   accessing the plug.
> 
> - blk_flush_plug in bio_poll:
>   As we don't plug the requests that require a zone lock in the first
>   place, flushing should not have any impact. So no issues directly
>   accessing the plug.
> 
> This is just a cleanup patch to use this wrapper to get the plug
> consistently across the block layer.

While I did suggest to make this consistent and in principle it's
the right thing to do, it also irks me to add extra checks to paths
where we know that it's just extra pointless code. Maybe we can
just comment these two spots? Basically each of the sections above
could just go into the appropriate file.

-- 
Jens Axboe

Re: [PATCH v3 2/2] block: use blk_mq_plug() wrapper consistently in the block layer

[Pankaj Raghav]

>> Either of the changes should not have led to a bug in zoned devices:
>>
>> - blk_execute_rq_nowait:
>>   Only passthrough requests can use this function, and plugging can be
>>   performed on those requests in zoned devices. So no issues directly
>>   accessing the plug.
>>
>> - blk_flush_plug in bio_poll:
>>   As we don't plug the requests that require a zone lock in the first
>>   place, flushing should not have any impact. So no issues directly
>>   accessing the plug.
>>
>> This is just a cleanup patch to use this wrapper to get the plug
>> consistently across the block layer.
> 
> While I did suggest to make this consistent and in principle it's
> the right thing to do, it also irks me to add extra checks to paths
> where we know that it's just extra pointless code. Maybe we can
> just comment these two spots? Basically each of the sections above
> could just go into the appropriate file.
> 
The checks should go away, and the plug could be inlined by the compiler if
we don't have CONFIG_BLK_DEV_ZONED. Otherwise, I do agree with you that it
is a pointless check.

I am fine with either, though I prefer what this patch is doing. So if you
feel strongly against calling the blk_mq_plug function, I can turn this
patch into just adding comments as you suggested.

Re: [PATCH v3 2/2] block: use blk_mq_plug() wrapper consistently in the block layer

[Jens Axboe]

On 9/29/22 7:41 AM, Pankaj Raghav wrote:
>>> Either of the changes should not have led to a bug in zoned devices:
>>>
>>> - blk_execute_rq_nowait:
>>>   Only passthrough requests can use this function, and plugging can be
>>>   performed on those requests in zoned devices. So no issues directly
>>>   accessing the plug.
>>>
>>> - blk_flush_plug in bio_poll:
>>>   As we don't plug the requests that require a zone lock in the first
>>>   place, flushing should not have any impact. So no issues directly
>>>   accessing the plug.
>>>
>>> This is just a cleanup patch to use this wrapper to get the plug
>>> consistently across the block layer.
>>
>> While I did suggest to make this consistent and in principle it's
>> the right thing to do, it also irks me to add extra checks to paths
>> where we know that it's just extra pointless code. Maybe we can
>> just comment these two spots? Basically each of the sections above
>> could just go into the appropriate file.
>>
> The checks should go away, and the plug could be inlined by the
> compiler if we don't have CONFIG_BLK_DEV_ZONED. Otherwise, I do agree
> with you that it is a pointless check.

But that's my general complaint with the argument that "it doesn't
matter if this feature isn't configured" - distros enable features.
Anything that uses IS_ENABLED() is handy for testing, but assuming it
all pretty much gets enabled in the distro kernel, it does absolutely
nothing to help the added overhead. It's something that gets done to
create the illusion that an added feature CAN have zero core overhead,
while in reality that's utterly wrong.

> I am fine with either, though I prefer what this patch is doing. So if
> you feel strongly against calling the blk_mq_plug function, I can turn
> this patch into just adding comments as you suggested.

I think we should. I can pick patch 1 here, and then you can send a
patch 2 for that when you have time.

-- 
Jens Axboe

Re: (subset) [PATCH v3 0/2] plugging cleanup v3

[Jens Axboe]

On Thu, 29 Sep 2022 09:47:43 +0200, Pankaj Raghav wrote:
> 1st patch modifies blk_mq_plug() function to disable plugging for
>  write and write zeroes in zoned block devices.
> 
> 2nd patch uses the blk_mq_plug function in the block layer consistently.
> 
> The patches are based on next-20220923.
> 
> [...]

Applied, thanks!

[1/2] block: adapt blk_mq_plug() to not plug for writes that require a zone lock
      commit: 8cafdb5ab94cda3ebb0975be16e2d564a05132ea

Best regards,
-- 
Jens Axboe

Re: [PATCH v3 2/2] block: use blk_mq_plug() wrapper consistently in the block layer

[Pankaj Raghav]

On 2022-09-29 15:45, Jens Axboe wrote:
> On 9/29/22 7:41 AM, Pankaj Raghav wrote:
>>>> Either of the changes should not have led to a bug in zoned devices:
>>>>
>>>> - blk_execute_rq_nowait:
>>>>   Only passthrough requests can use this function, and plugging can be
>>>>   performed on those requests in zoned devices. So no issues directly
>>>>   accessing the plug.
>>>>
>>>> - blk_flush_plug in bio_poll:
>>>>   As we don't plug the requests that require a zone lock in the first
>>>>   place, flushing should not have any impact. So no issues directly
>>>>   accessing the plug.
>>>>
>>>> This is just a cleanup patch to use this wrapper to get the plug
>>>> consistently across the block layer.
>>>
>>> While I did suggest to make this consistent and in principle it's
>>> the right thing to do, it also irks me to add extra checks to paths
>>> where we know that it's just extra pointless code. Maybe we can
>>> just comment these two spots? Basically each of the sections above
>>> could just go into the appropriate file.
>>>
>> The checks should go away, and the plug could be inlined by the
>> compiler if we don't have CONFIG_BLK_DEV_ZONED. Otherwise, I do agree
>> with you that it is a pointless check.
> 
> But that's my general complaint with the argument that "it doesn't
> matter if this feature isn't configured" - distros enable features.
> Anything that uses IS_ENABLED() is handy for testing, but assuming it
> all pretty much gets enabled in the distro kernel, it does absolutely
> nothing to help the added overhead. It's something that gets done to
> create the illusion that an added feature CAN have zero core overhead,
> while in reality that's utterly wrong.
> Got it!
>> I am fine with either, though I prefer what this patch is doing. So if
>> you feel strongly against calling the blk_mq_plug function, I can turn
>> this patch into just adding comments as you suggested.
> 
> I think we should. I can pick patch 1 here, and then you can send a
> patch 2 for that when you have time.
> 

I will do it right away as a separate patch! Thanks.