| Message ID | 20230808171310.112878-3-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Block nowait fix | expand |
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; }
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.
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).
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>
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; }
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(-)