[PATCH 0/2] Block nowait fix

[PATCH 0/2] Block nowait fix

[Jens Axboe]

Hi,

Patch 1 is just something I came across while debugging an issue,
killing old and unused code.

Patch 2 fixes an issue with IOCB_HIPRI -> REQ_POLLED, where we always
set REQ_NOWAIT as well. We should not be combining these two flags, they
are separate properties and it's up to the caller to do the right thing
and set one or both as they see fit.

-- 
Jens Axboe

[PATCH 1/2] block: get rid of unused plug->nowait flag

[Jens Axboe]

This was introduced to add a plug based way of signaling nowait issues,
but we have since moved on from that. Kill the old dead code, nobody is
setting it anymore.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-core.c       | 6 ------
 include/linux/blkdev.h | 1 -
 2 files changed, 7 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 90de50082146..9866468c72a2 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -722,14 +722,9 @@ void submit_bio_noacct(struct bio *bio)
 	struct block_device *bdev = bio->bi_bdev;
 	struct request_queue *q = bdev_get_queue(bdev);
 	blk_status_t status = BLK_STS_IOERR;
-	struct blk_plug *plug;
 
 	might_sleep();
 
-	plug = blk_mq_plug(bio);
-	if (plug && plug->nowait)
-		bio->bi_opf |= REQ_NOWAIT;
-
 	/*
 	 * For a REQ_NOWAIT based request, return -EOPNOTSUPP
 	 * if queue does not support NOWAIT.
@@ -1059,7 +1054,6 @@ void blk_start_plug_nr_ios(struct blk_plug *plug, unsigned short nr_ios)
 	plug->rq_count = 0;
 	plug->multiple_queues = false;
 	plug->has_elevator = false;
-	plug->nowait = false;
 	INIT_LIST_HEAD(&plug->cb_list);
 
 	/*
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index ed44a997f629..87d94be7825a 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -969,7 +969,6 @@ struct blk_plug {
 
 	bool multiple_queues;
 	bool has_elevator;
-	bool nowait;
 
 	struct list_head cb_list; /* md requires an unplug callback */
 };
-- 
2.40.1

[PATCH 2/2] block: don't make REQ_POLLED imply REQ_NOWAIT

[Jens Axboe]

Normally these two flags do go together, as the issuer of polled IO
generally cannot wait for resources that will get freed as part of IO
completion. This is because that very task is the one that will complete
the request and free those resources, hence that would introduce a
deadlock.

But it is possible to have someone else issue the polled IO, eg via
io_uring if the request is punted to io-wq. For that case, it's fine to
have the task block on IO submission, as it is not the same task that
will be completing the IO.

It's completely up to the caller to ask for both polled and nowait IO
separately! If we don't allow polled IO where IOCB_NOWAIT isn't set in
the kiocb, then we can run into repeated -EAGAIN submissions and not
make any progress.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/bio.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/bio.h b/include/linux/bio.h
index c4f5b5228105..11984ed29cb8 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -791,7 +791,7 @@ static inline int bio_integrity_add_page(struct bio *bio, struct page *page,
 static inline void bio_set_polled(struct bio *bio, struct kiocb *kiocb)
 {
 	bio->bi_opf |= REQ_POLLED;
-	if (!is_sync_kiocb(kiocb))
+	if (kiocb->ki_flags & IOCB_NOWAIT)
 		bio->bi_opf |= REQ_NOWAIT;
 }
 
-- 
2.40.1

Re: [PATCH 2/2] block: don't make REQ_POLLED imply REQ_NOWAIT

[Jens Axboe]

On 8/8/23 11:13?AM, Jens Axboe wrote:
> Normally these two flags do go together, as the issuer of polled IO
> generally cannot wait for resources that will get freed as part of IO
> completion. This is because that very task is the one that will complete
> the request and free those resources, hence that would introduce a
> deadlock.
> 
> But it is possible to have someone else issue the polled IO, eg via
> io_uring if the request is punted to io-wq. For that case, it's fine to
> have the task block on IO submission, as it is not the same task that
> will be completing the IO.
> 
> It's completely up to the caller to ask for both polled and nowait IO
> separately! If we don't allow polled IO where IOCB_NOWAIT isn't set in
> the kiocb, then we can run into repeated -EAGAIN submissions and not
> make any progress.
  
Looks like I forgot to update when adding the first half of this...
Here's the full patch 2/2:

commit 50bd4aa84442442c87e669d72d1a6d0b01c332a8
Author: Jens Axboe <axboe@kernel.dk>
Date:   Tue Aug 8 11:06:17 2023 -0600

    block: don't make REQ_POLLED imply REQ_NOWAIT
    
    Normally these two flags do go together, as the issuer of polled IO
    generally cannot wait for resources that will get freed as part of IO
    completion. This is because that very task is the one that will complete
    the request and free those resources, hence that would introduce a
    deadlock.
    
    But it is possible to have someone else issue the polled IO, eg via
    io_uring if the request is punted to io-wq. For that case, it's fine to
    have the task block on IO submission, as it is not the same task that
    will be completing the IO.
    
    It's completely up to the caller to ask for both polled and nowait IO
    separately! If we don't allow polled IO where IOCB_NOWAIT isn't set in
    the kiocb, then we can run into repeated -EAGAIN submissions and not
    make any progress.
    
    Signed-off-by: Jens Axboe <axboe@kernel.dk>

diff --git a/block/fops.c b/block/fops.c
index a286bf3325c5..838ffada5341 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -358,13 +358,14 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb,
 		task_io_account_write(bio->bi_iter.bi_size);
 	}
 
+	if (iocb->ki_flags & IOCB_NOWAIT)
+		bio->bi_opf |= REQ_NOWAIT;
+
 	if (iocb->ki_flags & IOCB_HIPRI) {
-		bio->bi_opf |= REQ_POLLED | REQ_NOWAIT;
+		bio->bi_opf |= REQ_POLLED;
 		submit_bio(bio);
 		WRITE_ONCE(iocb->private, bio);
 	} else {
-		if (iocb->ki_flags & IOCB_NOWAIT)
-			bio->bi_opf |= REQ_NOWAIT;
 		submit_bio(bio);
 	}
 	return -EIOCBQUEUED;
diff --git a/include/linux/bio.h b/include/linux/bio.h
index c4f5b5228105..11984ed29cb8 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -791,7 +791,7 @@ static inline int bio_integrity_add_page(struct bio *bio, struct page *page,
 static inline void bio_set_polled(struct bio *bio, struct kiocb *kiocb)
 {
 	bio->bi_opf |= REQ_POLLED;
-	if (!is_sync_kiocb(kiocb))
+	if (kiocb->ki_flags & IOCB_NOWAIT)
 		bio->bi_opf |= REQ_NOWAIT;
 }
 

-- 
Jens Axboe

Re: [PATCH 1/2] block: get rid of unused plug->nowait flag

[Chaitanya Kulkarni]

On 8/8/2023 10:13 AM, Jens Axboe wrote:
> This was introduced to add a plug based way of signaling nowait issues,
> but we have since moved on from that. Kill the old dead code, nobody is
> setting it anymore.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---

Looks good to me.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 1/2] block: get rid of unused plug->nowait flag

[Bart Van Assche]

On 8/8/23 10:13, Jens Axboe wrote:
> This was introduced to add a plug based way of signaling nowait issues,
> but we have since moved on from that. Kill the old dead code, nobody is
> setting it anymore.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 2/2] block: don't make REQ_POLLED imply REQ_NOWAIT

[Bart Van Assche]

On 8/8/23 10:13, Jens Axboe wrote:
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index c4f5b5228105..11984ed29cb8 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -791,7 +791,7 @@ static inline int bio_integrity_add_page(struct bio *bio, struct page *page,
>   static inline void bio_set_polled(struct bio *bio, struct kiocb *kiocb)
>   {
>   	bio->bi_opf |= REQ_POLLED;
> -	if (!is_sync_kiocb(kiocb))
> +	if (kiocb->ki_flags & IOCB_NOWAIT)
>   		bio->bi_opf |= REQ_NOWAIT;
>   }

The implementation of bio_set_polled() is short and that function has
only one caller. Has it been considered to inline that function into
its caller?

Thanks,

Bart.

Re: [PATCH 2/2] block: don't make REQ_POLLED imply REQ_NOWAIT

[Jens Axboe]

On 8/8/23 3:58?PM, Bart Van Assche wrote:
> On 8/8/23 10:13, Jens Axboe wrote:
>> diff --git a/include/linux/bio.h b/include/linux/bio.h
>> index c4f5b5228105..11984ed29cb8 100644
>> --- a/include/linux/bio.h
>> +++ b/include/linux/bio.h
>> @@ -791,7 +791,7 @@ static inline int bio_integrity_add_page(struct bio *bio, struct page *page,
>>   static inline void bio_set_polled(struct bio *bio, struct kiocb *kiocb)
>>   {
>>       bio->bi_opf |= REQ_POLLED;
>> -    if (!is_sync_kiocb(kiocb))
>> +    if (kiocb->ki_flags & IOCB_NOWAIT)
>>           bio->bi_opf |= REQ_NOWAIT;
>>   }
> 
> The implementation of bio_set_polled() is short and that function has
> only one caller. Has it been considered to inline that function into
> its caller?

I think it should probably just go away, but wanted to leave that for a
non-functional cleanup (which can wait for 6.6).

-- 
Jens Axboe

Re: [PATCH 2/2] block: don't make REQ_POLLED imply REQ_NOWAIT

[Bart Van Assche]

On 8/8/23 10:13, Jens Axboe wrote:
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -791,7 +791,7 @@ static inline int bio_integrity_add_page(struct bio *bio, struct page *page,
>   static inline void bio_set_polled(struct bio *bio, struct kiocb *kiocb)
>   {
>   	bio->bi_opf |= REQ_POLLED;
> -	if (!is_sync_kiocb(kiocb))
> +	if (kiocb->ki_flags & IOCB_NOWAIT)
>   		bio->bi_opf |= REQ_NOWAIT;
>   }

Reviewed-by: Bart Van Assche <bvanassche@acm.org>