| Message ID | f287a7882a4c4576e90e55ecc5ab8bf634579afd.1741090631.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/1] iomap: propagate nowait to block layer | expand |
On Tue, Mar 04, 2025 at 12:18:07PM +0000, Pavel Begunkov wrote: > ((dio->flags & IOMAP_DIO_NEED_SYNC) && !use_fua) || > - ((dio->flags & IOMAP_DIO_WRITE) && pos >= i_size_read(inode))) > + ((dio->flags & IOMAP_DIO_WRITE) && pos >= i_size_read(inode))) { > dio->flags &= ~IOMAP_DIO_CALLER_COMP; > > + if (!is_sync_kiocb(dio->iocb) && > + (dio->iocb->ki_flags & IOCB_NOWAIT)) > + return -EAGAIN; Black magic without comments explaining it. > + if (!is_sync_kiocb(dio->iocb) && (dio->iocb->ki_flags & IOCB_NOWAIT)) { > + /* > + * This is nonblocking IO, and we might need to allocate > + * multiple bios. In this case, as we cannot guarantee that > + * one of the sub bios will not fail getting issued FOR NOWAIT > + * and as error results are coalesced across all of them, ask > + * for a retry of this from blocking context. > + */ > + if (bio_iov_vecs_to_alloc(dio->submit.iter, BIO_MAX_VECS + 1) > > + BIO_MAX_VECS) This is not very accurate in times of multi-page bvecs and large order folios all over. I think you really need to byte the bullet and support for early returns from the non-blocking bio submission path.
On 3/4/25 16:07, Christoph Hellwig wrote: > On Tue, Mar 04, 2025 at 12:18:07PM +0000, Pavel Begunkov wrote: >> ((dio->flags & IOMAP_DIO_NEED_SYNC) && !use_fua) || >> - ((dio->flags & IOMAP_DIO_WRITE) && pos >= i_size_read(inode))) >> + ((dio->flags & IOMAP_DIO_WRITE) && pos >= i_size_read(inode))) { >> dio->flags &= ~IOMAP_DIO_CALLER_COMP; >> >> + if (!is_sync_kiocb(dio->iocb) && >> + (dio->iocb->ki_flags & IOCB_NOWAIT)) >> + return -EAGAIN; > > Black magic without comments explaining it. I can copy the comment from below if you wish. >> + if (!is_sync_kiocb(dio->iocb) && (dio->iocb->ki_flags & IOCB_NOWAIT)) { >> + /* >> + * This is nonblocking IO, and we might need to allocate >> + * multiple bios. In this case, as we cannot guarantee that >> + * one of the sub bios will not fail getting issued FOR NOWAIT >> + * and as error results are coalesced across all of them, ask >> + * for a retry of this from blocking context. >> + */ >> + if (bio_iov_vecs_to_alloc(dio->submit.iter, BIO_MAX_VECS + 1) > >> + BIO_MAX_VECS) > > This is not very accurate in times of multi-page bvecs and large order > folios all over. bio_iov_vecs_to_alloc() can overestimate, i.e. the check might return -EAGAIN in more cases than required but not the other way around, that should be enough for a fix such as this patch. Or did I maybe misunderstood you? > I think you really need to byte the bullet and support for early returns > from the non-blocking bio submission path. Assuming you're suggesting to implement that, I can't say I'm excited by the idea of reworking a non trivial chunk of block layer to fix a problem and then porting it up to some 5.x, especially since it was already attempted before by someone and ultimately got reverted.
On Tue, Mar 04, 2025 at 04:41:40PM +0000, Pavel Begunkov wrote: > bio_iov_vecs_to_alloc() can overestimate, i.e. the check might return > -EAGAIN in more cases than required but not the other way around, > that should be enough for a fix such as this patch. Or did I maybe > misunderstood you? No you didn;t but we need to do this properly. > Assuming you're suggesting to implement that, I can't say I'm excited by > the idea of reworking a non trivial chunk of block layer to fix a problem > and then porting it up to some 5.x, especially since it was already > attempted before by someone and ultimately got reverted. Stop whining. Backporting does not matter for upstream development, and I'm pretty sure you'd qualify for a job where you don't have to do this if you actually care and don't just like to complain.
On 3/4/25 9:59 AM, Christoph Hellwig wrote: > Stop whining. Backporting does not matter for upstream development, > and I'm pretty sure you'd qualify for a job where you don't have to do > this if you actually care and don't just like to complain. Sorry, but wtf is this? Can we please keep it factual. You may not need to care about backporting, but those of us supporting stable and actual production certainly do. Not that this should drive upstream development in any way, it's entirely unrelated to the problem at hand.
On 3/4/25 16:59, Christoph Hellwig wrote: > On Tue, Mar 04, 2025 at 04:41:40PM +0000, Pavel Begunkov wrote: >> bio_iov_vecs_to_alloc() can overestimate, i.e. the check might return >> -EAGAIN in more cases than required but not the other way around, >> that should be enough for a fix such as this patch. Or did I maybe >> misunderstood you? > > No you didn;t but we need to do this properly. > >> Assuming you're suggesting to implement that, I can't say I'm excited by >> the idea of reworking a non trivial chunk of block layer to fix a problem >> and then porting it up to some 5.x, especially since it was already >> attempted before by someone and ultimately got reverted. > > Stop whining. Backporting does not matter for upstream development, > and I'm pretty sure you'd qualify for a job where you don't have to do > this if you actually care and don't just like to complain. That's an interesting choice of words. Dear Christoph, I'm afraid you don't realise that you're the one whining at a person who actually tries to fix it. I'd appreciate if you stop this bullshit, especially if you're not willing to fix it yourself. If you do, please, be my guest and prove me wrong. Stable does exist, and people do care about it, and people do care about problems being fixed and not delayed because your desire for someone else to do some random grand rework for you. If there are _actual_ problems with the patch, please let me know. Some of more rare cases being not as efficient is not a problem.
On Tue, Mar 04, 2025 at 04:41:40PM +0000, Pavel Begunkov wrote: > On 3/4/25 16:07, Christoph Hellwig wrote: > > On Tue, Mar 04, 2025 at 12:18:07PM +0000, Pavel Begunkov wrote: > > > ((dio->flags & IOMAP_DIO_NEED_SYNC) && !use_fua) || > > > - ((dio->flags & IOMAP_DIO_WRITE) && pos >= i_size_read(inode))) > > > + ((dio->flags & IOMAP_DIO_WRITE) && pos >= i_size_read(inode))) { > > > dio->flags &= ~IOMAP_DIO_CALLER_COMP; > > > + if (!is_sync_kiocb(dio->iocb) && > > > + (dio->iocb->ki_flags & IOCB_NOWAIT)) > > > + return -EAGAIN; > > > > Black magic without comments explaining it. > > I can copy the comment from below if you wish. > > > > + if (!is_sync_kiocb(dio->iocb) && (dio->iocb->ki_flags & IOCB_NOWAIT)) { > > > + /* > > > + * This is nonblocking IO, and we might need to allocate > > > + * multiple bios. In this case, as we cannot guarantee that > > > + * one of the sub bios will not fail getting issued FOR NOWAIT > > > + * and as error results are coalesced across all of them, ask > > > + * for a retry of this from blocking context. > > > + */ > > > + if (bio_iov_vecs_to_alloc(dio->submit.iter, BIO_MAX_VECS + 1) > > > > + BIO_MAX_VECS) > > > > This is not very accurate in times of multi-page bvecs and large order > > folios all over. > > bio_iov_vecs_to_alloc() can overestimate, i.e. the check might return > -EAGAIN in more cases than required but not the other way around, > that should be enough for a fix such as this patch. Or did I maybe > misunderstood you? > > > I think you really need to byte the bullet and support for early returns > > from the non-blocking bio submission path. > > Assuming you're suggesting to implement that, I can't say I'm excited by > the idea of reworking a non trivial chunk of block layer to fix a problem > and then porting it up to some 5.x, especially since it was already > attempted before by someone and ultimately got reverted. [I'm going to ignore the sarcasm downthread because I don't like it and will not participate in prolonging that.] So don't. XFS LTS generally doesn't pull large chunks of new code into old kernels, we just tell people they need to keep moving forward if they want new code, or even bug fixes that get really involved. You want an XFS that doesn't allocate xfs_bufs from reclaim? Well, you have to move to 6.12, we're not going to backport a ton of super invasive changes to 6.6, let alone 5.x. We don't let old kernel source dictate changes to new kernels. --D > -- > Pavel Begunkov > >
On 3/4/25 19:22, Darrick J. Wong wrote: ... >> Assuming you're suggesting to implement that, I can't say I'm excited by >> the idea of reworking a non trivial chunk of block layer to fix a problem >> and then porting it up to some 5.x, especially since it was already >> attempted before by someone and ultimately got reverted. Clarification: the mentioned work was reverted or pulled out _upstream_, it wasn't about back porting. > [I'm going to ignore the sarcasm downthread because I don't like it and > will not participate in prolonging that.] > > So don't. XFS LTS generally doesn't pull large chunks of new code into I agree, and that's why I'm trying to have a small fix. I think this patch is concise if you disregard comments taking some lines. And Christoph even of confirmed that the main check in the patch does what's intended, i.e. disallowing setups where multiple bios would be generated from the iterator. > old kernels, we just tell people they need to keep moving forward if > they want new code, or even bug fixes that get really involved. You It's still a broken io_uring uapi promise though, and I'd still need to address it in older kernels somehow. For example we can have a patch like this, and IMHO it'd be the ideal option. Another option is to push all io_uring filesystem / iomap requests to the slow path (where blocking is possible) and have a meaningful perf regression for those who still use fs+io_uring direct IO. And I don't put any dramaticism into it, it's essentially what users who detect the problem already do, either that but from the user space or disabling io_uring all together. Even then it'd leave the question how to fix it upstream, I don't see the full scope, but it has non trivial nuances and might likely turn out to be a very involving project and take a lot of time I don't have at the moment. Darrick, any thoughts on the patch? Is there any problem why it wouldn't work? > want an XFS that doesn't allocate xfs_bufs from reclaim? Well, you have > to move to 6.12, we're not going to backport a ton of super invasive > changes to 6.6, let alone 5.x. > > We don't let old kernel source dictate changes to new kernels.
On Tue, Mar 04, 2025 at 12:18:07PM +0000, Pavel Begunkov wrote: > There are reports of high io_uring submission latency for ext4 and xfs, > which is due to iomap not propagating nowait flag to the block layer > resulting in waiting for IO during tag allocation. > > Because of how errors are propagated back, we can't set REQ_NOWAIT > for multi bio IO, in this case return -EAGAIN and let the caller to > handle it, for example, it can reissue it from a blocking context. > It's aligned with how raw bdev direct IO handles it. > > Cc: stable@vger.kernel.org > Link: https://github.com/axboe/liburing/issues/826#issuecomment-2674131870 > Reported-by: wu lei <uwydoc@gmail.com> > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > > v2: > Fail multi-bio nowait submissions > > fs/iomap/direct-io.c | 26 +++++++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-) > > diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c > index b521eb15759e..07c336fdf4f0 100644 > --- a/fs/iomap/direct-io.c > +++ b/fs/iomap/direct-io.c > @@ -363,9 +363,14 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter, > */ > if (need_zeroout || > ((dio->flags & IOMAP_DIO_NEED_SYNC) && !use_fua) || > - ((dio->flags & IOMAP_DIO_WRITE) && pos >= i_size_read(inode))) > + ((dio->flags & IOMAP_DIO_WRITE) && pos >= i_size_read(inode))) { > dio->flags &= ~IOMAP_DIO_CALLER_COMP; > > + if (!is_sync_kiocb(dio->iocb) && > + (dio->iocb->ki_flags & IOCB_NOWAIT)) > + return -EAGAIN; > + } How are we getting here with IOCB_NOWAIT IO? This is either sub-block unaligned write IO, it is a write IO that requires allocation (i.e. write beyond EOF), or we are doing a O_DSYNC write on hardware that doesn't support REQ_FUA. The first 2 cases should have already been filtered out by the filesystem before we ever get here. The latter doesn't require multiple IOs in IO submission - the O_DSYNC IO submission (if any is required) occurs from data IO completion context, and so it will not block IO submission at all. So what type of IO in what mapping condition is triggering the need to return EAGAIN here? > + > /* > * The rules for polled IO completions follow the guidelines as the > * ones we set for inline and deferred completions. If none of those > @@ -374,6 +379,23 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter, > if (!(dio->flags & (IOMAP_DIO_INLINE_COMP|IOMAP_DIO_CALLER_COMP))) > dio->iocb->ki_flags &= ~IOCB_HIPRI; > > + bio_opf = iomap_dio_bio_opflags(dio, iomap, use_fua, atomic); > + > + if (!is_sync_kiocb(dio->iocb) && (dio->iocb->ki_flags & IOCB_NOWAIT)) { > + /* > + * This is nonblocking IO, and we might need to allocate > + * multiple bios. In this case, as we cannot guarantee that > + * one of the sub bios will not fail getting issued FOR NOWAIT > + * and as error results are coalesced across all of them, ask > + * for a retry of this from blocking context. > + */ > + if (bio_iov_vecs_to_alloc(dio->submit.iter, BIO_MAX_VECS + 1) > > + BIO_MAX_VECS) > + return -EAGAIN; > + > + bio_opf |= REQ_NOWAIT; > + } Ok, so this allows a max sized bio to be used. So, what, 1MB on 4kB page size is the maximum IO size for IOCB_NOWAIT now? I bet that's not documented anywhere.... Ah. This doesn't fix the problem at all. Say, for exmaple, I have a hardware storage device with a max hardware IO size of 128kB. This is from the workstation I'm typing this email on: $ cat /sys/block/nvme0n1/queue/max_hw_sectors_kb 128 $ cat /sys/block/nvme0n1/queue/max_segments 33 $ We build a 1MB bio above, set REQ_NOWAIT, then: submit_bio .... blk_mq_submit_bio __bio_split_to_limits(bio, &q->limits, &nr_segs); bio_split_rw() ..... split: ..... /* * We can't sanely support splitting for a REQ_NOWAIT bio. End it * with EAGAIN if splitting is required and return an error pointer. */ if (bio->bi_opf & REQ_NOWAIT) return -EAGAIN; So, REQ_NOWAIT effectively limits bio submission to the maximum single IO size of the underlying storage. So, we can't use REQ_NOWAIT without actually looking at request queue limits before we start building the IO - yuk. REQ_NOWAIT still feels completely unusable to me.... -Dave.
On 3/4/25 21:11, Dave Chinner wrote: ... >> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c >> index b521eb15759e..07c336fdf4f0 100644 >> --- a/fs/iomap/direct-io.c >> +++ b/fs/iomap/direct-io.c >> @@ -363,9 +363,14 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter, >> */ >> if (need_zeroout || >> ((dio->flags & IOMAP_DIO_NEED_SYNC) && !use_fua) || >> - ((dio->flags & IOMAP_DIO_WRITE) && pos >= i_size_read(inode))) >> + ((dio->flags & IOMAP_DIO_WRITE) && pos >= i_size_read(inode))) { >> dio->flags &= ~IOMAP_DIO_CALLER_COMP; >> >> + if (!is_sync_kiocb(dio->iocb) && >> + (dio->iocb->ki_flags & IOCB_NOWAIT)) >> + return -EAGAIN; >> + } > > How are we getting here with IOCB_NOWAIT IO? This is either > sub-block unaligned write IO, it is a write IO that requires > allocation (i.e. write beyond EOF), or we are doing a O_DSYNC write > on hardware that doesn't support REQ_FUA. > > The first 2 cases should have already been filtered out by the > filesystem before we ever get here. The latter doesn't require > multiple IOs in IO submission - the O_DSYNC IO submission (if any is > required) occurs from data IO completion context, and so it will not > block IO submission at all. > > So what type of IO in what mapping condition is triggering the need > to return EAGAIN here? I didn't hit it but neither could prove that it's impossible. I'll drop the hunk since you're saying it shouldn't be needed. >> /* >> * The rules for polled IO completions follow the guidelines as the >> * ones we set for inline and deferred completions. If none of those >> @@ -374,6 +379,23 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter, >> if (!(dio->flags & (IOMAP_DIO_INLINE_COMP|IOMAP_DIO_CALLER_COMP))) >> dio->iocb->ki_flags &= ~IOCB_HIPRI; >> >> + bio_opf = iomap_dio_bio_opflags(dio, iomap, use_fua, atomic); >> + >> + if (!is_sync_kiocb(dio->iocb) && (dio->iocb->ki_flags & IOCB_NOWAIT)) { >> + /* >> + * This is nonblocking IO, and we might need to allocate >> + * multiple bios. In this case, as we cannot guarantee that >> + * one of the sub bios will not fail getting issued FOR NOWAIT >> + * and as error results are coalesced across all of them, ask >> + * for a retry of this from blocking context. >> + */ >> + if (bio_iov_vecs_to_alloc(dio->submit.iter, BIO_MAX_VECS + 1) > >> + BIO_MAX_VECS) >> + return -EAGAIN; >> + >> + bio_opf |= REQ_NOWAIT; >> + } > > Ok, so this allows a max sized bio to be used. So, what, 1MB on 4kB > page size is the maximum IO size for IOCB_NOWAIT now? I bet that's > not documented anywhere.... > > Ah. This doesn't fix the problem at all. > > Say, for exmaple, I have a hardware storage device with a max > hardware IO size of 128kB. This is from the workstation I'm typing > this email on: > > $ cat /sys/block/nvme0n1/queue/max_hw_sectors_kb > 128 > $ cat /sys/block/nvme0n1/queue/max_segments > 33 > $ > > We build a 1MB bio above, set REQ_NOWAIT, then: > > submit_bio > .... > blk_mq_submit_bio > __bio_split_to_limits(bio, &q->limits, &nr_segs); > bio_split_rw() > ..... > split: > ..... > /* > * We can't sanely support splitting for a REQ_NOWAIT bio. End it > * with EAGAIN if splitting is required and return an error pointer. > */ > if (bio->bi_opf & REQ_NOWAIT) > return -EAGAIN; > > > So, REQ_NOWAIT effectively limits bio submission to the maximum > single IO size of the underlying storage. So, we can't use > REQ_NOWAIT without actually looking at request queue limits before > we start building the IO - yuk. > > REQ_NOWAIT still feels completely unusable to me.... Not great but better than not using the async path at all (i.e. executing by a kernel worker) as far as io_uring concerned. It should cover a good share of users, and io_uring has a fallback path, so userspace won't even know about the error. The overhead per byte is less biting for 128K as well. The patch already limits the change to async users only, but if you're concerned about non-io_uring users, I can try to narrow it to io_uring requests only, even though I don't think it's a good way. Are you only concerned about the size being too restrictive or do you see any other problems?
On Tue, Mar 04, 2025 at 10:36:16AM -0700, Jens Axboe wrote: > stable and actual production certainly do. Not that this should drive > upstream development in any way, it's entirely unrelated to the problem > at hand. And that's exactly what I'm saying. Do the right thing instead of whining about backports to old kernels.
On Tue, Mar 04, 2025 at 05:54:51PM +0000, Pavel Begunkov wrote: > That's an interesting choice of words. Dear Christoph, I'm afraid you > don't realise that you're the one whining at a person who actually tries > to fix it. I'd appreciate if you stop this bullshit, especially if you're > not willing to fix it yourself. If you do, please, be my guest and prove > me wrong. I'm not bullshiting. I put my money where my mouth is, and you're complaining.
On Tue, Mar 04, 2025 at 10:47:48PM +0000, Pavel Begunkov wrote: > Are you only concerned about the size being too restrictive or do you > see any other problems? You can't know the size beforehand. A stacking block driver can split the I/O for absolutely any reason it wants, and there is no requirement to propagate that up to the queue limits. That's why we need to be able to synchronously return the wouldblock error to get this right.
On 3/4/25 4:26 PM, Christoph Hellwig wrote: > On Tue, Mar 04, 2025 at 10:36:16AM -0700, Jens Axboe wrote: >> stable and actual production certainly do. Not that this should drive >> upstream development in any way, it's entirely unrelated to the problem >> at hand. > > And that's exactly what I'm saying. Do the right thing instead of > whining about backports to old kernels. Yep we agree on that, that's obvious. What I'm objecting to is your delivery, which was personal rather than factual, which you should imho apologize for. It's not that hard to stay on topic and avoid random statements like that, which are entirely useless, counterproductive, and just bullshit to be honest. And honestly pretty tiring that this needs to be said, still. Really.
On Tue, Mar 04, 2025 at 04:43:29PM -0700, Jens Axboe wrote: > On 3/4/25 4:26 PM, Christoph Hellwig wrote: > > On Tue, Mar 04, 2025 at 10:36:16AM -0700, Jens Axboe wrote: > >> stable and actual production certainly do. Not that this should drive > >> upstream development in any way, it's entirely unrelated to the problem > >> at hand. > > > > And that's exactly what I'm saying. Do the right thing instead of > > whining about backports to old kernels. > > Yep we agree on that, that's obvious. What I'm objecting to is your > delivery, which was personal rather than factual, which you should imho > apologize for. I thus sincerly apologize for flaming Pavel for whining about backporting, but I'd still prefer he would have taken up that proposal on technical grounds as I see absolutely no alternatively to synchronously returning an error. > And honestly pretty tiring that this needs to be said, still. Really. An I'm really tired of folks whining about backporting instead of staying ontopic.
On Tue, Mar 04, 2025 at 08:35:52PM +0000, Pavel Begunkov wrote: > Clarification: the mentioned work was reverted or pulled out _upstream_, > it wasn't about back porting. I don't think we ever tried synchronous reporting of wouldblock errors, but maybe i'm just too old and confused by now. > lines. And Christoph even of confirmed that the main check in the patch > does what's intended, I absolutely did not. > Another option is to push all io_uring filesystem / iomap requests > to the slow path (where blocking is possible) and have a meaningful > perf regression for those who still use fs+io_uring direct IO. And > I don't put any dramaticism into it, it's essentially what users > who detect the problem already do, either that but from the user > space or disabling io_uring all together. If you don't want to do synchronous wouldblock errors that's your only option. I think it would suck badly, but it's certainly easier to backport.
On 3/4/25 23:49, Christoph Hellwig wrote: > On Tue, Mar 04, 2025 at 04:43:29PM -0700, Jens Axboe wrote: >> On 3/4/25 4:26 PM, Christoph Hellwig wrote: >>> On Tue, Mar 04, 2025 at 10:36:16AM -0700, Jens Axboe wrote: >>>> stable and actual production certainly do. Not that this should drive >>>> upstream development in any way, it's entirely unrelated to the problem >>>> at hand. >>> >>> And that's exactly what I'm saying. Do the right thing instead of >>> whining about backports to old kernels. >> >> Yep we agree on that, that's obvious. What I'm objecting to is your >> delivery, which was personal rather than factual, which you should imho >> apologize for. > > I thus sincerly apologize for flaming Pavel for whining about Again, pretty interesting choice of words, because it was more akin to a tantrum. I hope you don't think you'd be taken seriously after that, I can't, and not only from this conversation. > backporting, but I'd still prefer he would have taken up that proposal > on technical grounds as I see absolutely no alternatively to > synchronously returning an error. The "You have to do X" hardly reveals any details to claim "technical grounds". Take stable out of the question, and I would still think that a simple understandable fix for upstream is better than jumping to a major rework right away. >> And honestly pretty tiring that this needs to be said, still. Really. > > An I'm really tired of folks whining about backporting instead of > staying ontopic.
On 3/5/25 00:14, Pavel Begunkov wrote: > On 3/4/25 23:49, Christoph Hellwig wrote: >> On Tue, Mar 04, 2025 at 04:43:29PM -0700, Jens Axboe wrote: >>> On 3/4/25 4:26 PM, Christoph Hellwig wrote: >>>> On Tue, Mar 04, 2025 at 10:36:16AM -0700, Jens Axboe wrote: >>>>> stable and actual production certainly do. Not that this should drive >>>>> upstream development in any way, it's entirely unrelated to the problem >>>>> at hand. >>>> >>>> And that's exactly what I'm saying. Do the right thing instead of >>>> whining about backports to old kernels. >>> >>> Yep we agree on that, that's obvious. What I'm objecting to is your >>> delivery, which was personal rather than factual, which you should imho >>> apologize for. >> >> I thus sincerly apologize for flaming Pavel for whining about > > Again, pretty interesting choice of words, because it was more akin > to a tantrum. I hope you don't think you'd be taken seriously after > that, I can't, and not only from this conversation. > >> backporting, but I'd still prefer he would have taken up that proposal >> on technical grounds as I see absolutely no alternatively to >> synchronously returning an error. > > The "You have to do X" hardly reveals any details to claim "technical > grounds". Take stable out of the question, and I would still think > that a simple understandable fix for upstream is better than jumping > to a major rework right away. That is _if_ there is a simple fix, but your argument was that it's "not very accurate", which is not very useful. >>> And honestly pretty tiring that this needs to be said, still. Really. >> >> An I'm really tired of folks whining about backporting instead of >> staying ontopic.
On 3/5/25 00:01, Christoph Hellwig wrote: > On Tue, Mar 04, 2025 at 08:35:52PM +0000, Pavel Begunkov wrote: >> Clarification: the mentioned work was reverted or pulled out _upstream_, >> it wasn't about back porting. > > I don't think we ever tried synchronous reporting of wouldblock errors, > but maybe i'm just too old and confused by now. It's not something recent. After some digging I think the one I remember is https://lore.kernel.org/all/20190717150445.1131-2-axboe@kernel.dk/ Remove by commit 7b6620d7db566a46f49b4b9deab9fa061fd4b59b Author: Jens Axboe <axboe@kernel.dk> Date: Thu Aug 15 11:09:16 2019 -0600 block: remove REQ_NOWAIT_INLINE >> lines. And Christoph even of confirmed that the main check in the patch >> does what's intended, > > I absolutely did not. > >> Another option is to push all io_uring filesystem / iomap requests >> to the slow path (where blocking is possible) and have a meaningful >> perf regression for those who still use fs+io_uring direct IO. And >> I don't put any dramaticism into it, it's essentially what users >> who detect the problem already do, either that but from the user >> space or disabling io_uring all together. > > If you don't want to do synchronous wouldblock errors that's your > only option. I think it would suck badly, but it's certainly easier > to backport. Is there some intrinsic difference of iomap from the block file in block/fops.c? Or is that one broken?
On Tue, Mar 04, 2025 at 10:47:48PM +0000, Pavel Begunkov wrote: > On 3/4/25 21:11, Dave Chinner wrote: > > > @@ -374,6 +379,23 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter, > > > if (!(dio->flags & (IOMAP_DIO_INLINE_COMP|IOMAP_DIO_CALLER_COMP))) > > > dio->iocb->ki_flags &= ~IOCB_HIPRI; > > > + bio_opf = iomap_dio_bio_opflags(dio, iomap, use_fua, atomic); > > > + > > > + if (!is_sync_kiocb(dio->iocb) && (dio->iocb->ki_flags & IOCB_NOWAIT)) { > > > + /* > > > + * This is nonblocking IO, and we might need to allocate > > > + * multiple bios. In this case, as we cannot guarantee that > > > + * one of the sub bios will not fail getting issued FOR NOWAIT > > > + * and as error results are coalesced across all of them, ask > > > + * for a retry of this from blocking context. > > > + */ > > > + if (bio_iov_vecs_to_alloc(dio->submit.iter, BIO_MAX_VECS + 1) > > > > + BIO_MAX_VECS) > > > + return -EAGAIN; > > > + > > > + bio_opf |= REQ_NOWAIT; > > > + } > > > > Ok, so this allows a max sized bio to be used. So, what, 1MB on 4kB > > page size is the maximum IO size for IOCB_NOWAIT now? I bet that's > > not documented anywhere.... > > > > Ah. This doesn't fix the problem at all. > > > > Say, for exmaple, I have a hardware storage device with a max > > hardware IO size of 128kB. This is from the workstation I'm typing > > this email on: > > > > $ cat /sys/block/nvme0n1/queue/max_hw_sectors_kb > > 128 > > $ cat /sys/block/nvme0n1/queue/max_segments > > 33 > > $ > > > > We build a 1MB bio above, set REQ_NOWAIT, then: > > > > submit_bio > > .... > > blk_mq_submit_bio > > __bio_split_to_limits(bio, &q->limits, &nr_segs); > > bio_split_rw() > > ..... > > split: > > ..... > > /* > > * We can't sanely support splitting for a REQ_NOWAIT bio. End it > > * with EAGAIN if splitting is required and return an error pointer. > > */ > > if (bio->bi_opf & REQ_NOWAIT) > > return -EAGAIN; > > > > > > So, REQ_NOWAIT effectively limits bio submission to the maximum > > single IO size of the underlying storage. So, we can't use > > REQ_NOWAIT without actually looking at request queue limits before > > we start building the IO - yuk. > > > > REQ_NOWAIT still feels completely unusable to me.... > > Not great but better than not using the async path at all (i.e. executing > by a kernel worker) as far as io_uring concerned. I really don't care about what io_uring thinks or does. If the block layer REQ_NOWAIT semantics are unusable for non-blocking IO submission, then that's the problem that needs fixing. This isn't a problem we can (or should) try to work around in the iomap layer. > It should cover a good > share of users, and io_uring has a fallback path, so userspace won't even > know about the error. The overhead per byte is less biting for 128K as well. That 128kb limit I demonstrated is not a fixed number. Stacking block devices (e.g. software RAID devices) can split bios at any sector offset within the submitted bio. For example: we have RAID5 witha 64kB chunk size, so max REQ_NOWAIT io size is 64kB according to the queue limits. However, if we do a 64kB IO at a 60kB chunk offset, that bio is going to be split into a 4kB bio and a 60kB bio because they are issued to different physical devices..... There is no way the bio submitter can know that this behaviour will occur, nor should they even be attempting to predict when/if such splitting may occur. > The patch already limits the change to async users only, but if you're > concerned about non-io_uring users, I can try to narrow it to io_uring > requests only, even though I don't think it's a good way. I'm concerned about any calling context that sets IOCB_NOWAIT. I don't care if it's io_uring, AIO or a synchronous preadv2(RWF_NOWAIT) call: we still have the same issue that REQ_NOWAIT submission side -EAGAIN is only reported through IO completion callbacks. > Are you only concerned about the size being too restrictive or do you > see any other problems? I'm concerned abou the fact that REQ_NOWAIT is not usable as it stands. We've identified bio chaining as an issue, now bio splitting is an issue, and I'm sure if we look further there will be other cases that are issues (e.g. bounce buffers). The underlying problem here is that bio submission errors are reported through bio completion mechanisms, not directly back to the submitting context. Fix that problem in the block layer API, and then iomap can use REQ_NOWAIT without having to care about what the block layer is doing under the covers. -Dave.
On Wed, Mar 05, 2025 at 12:45:52AM +0000, Pavel Begunkov wrote: > It's not something recent. After some digging I think the > one I remember is > > https://lore.kernel.org/all/20190717150445.1131-2-axboe@kernel.dk/ > > Remove by > > commit 7b6620d7db566a46f49b4b9deab9fa061fd4b59b > Author: Jens Axboe <axboe@kernel.dk> > Date: Thu Aug 15 11:09:16 2019 -0600 > > block: remove REQ_NOWAIT_INLINE That does indeed look pretty broken, mostly because it doesn't actually return the errors inline as return values but tries to emulate it. > > If you don't want to do synchronous wouldblock errors that's your > > only option. I think it would suck badly, but it's certainly easier > > to backport. > > Is there some intrinsic difference of iomap from the block file > in block/fops.c? Or is that one broken? block/fops.c has roughly the same issue, but it's not anywhere near as sever, as block/fops.c doesn't have any file system state that gets confused by the async BLK_STS_AGAIN. But unless io_uring knows how to rewind the state for that case it probably also doesn't work properly. It might be worth to look into testcases that actually exercise the nowait I/O on block devices with annoyingly small limits or that split a lot (like RAID0).
On Wed, Mar 05, 2025 at 12:19:46PM +1100, Dave Chinner wrote: > I really don't care about what io_uring thinks or does. If the block > layer REQ_NOWAIT semantics are unusable for non-blocking IO > submission, then that's the problem that needs fixing. This isn't a > problem we can (or should) try to work around in the iomap layer. Agreed. The problem are the block layer semantics. iomap/xfs really just is the messenger here. > For example: we have RAID5 witha 64kB chunk size, so max REQ_NOWAIT > io size is 64kB according to the queue limits. However, if we do a > 64kB IO at a 60kB chunk offset, that bio is going to be split into a > 4kB bio and a 60kB bio because they are issued to different physical > devices..... > > There is no way the bio submitter can know that this behaviour will > occur, nor should they even be attempting to predict when/if such > splitting may occur. And for something that has a real block allocator it could also be entirely dynamic. But I'm not sure if dm-thinp or bcache do anything like that at the moment. > > Are you only concerned about the size being too restrictive or do you > > see any other problems? > > I'm concerned abou the fact that REQ_NOWAIT is not usable as it > stands. We've identified bio chaining as an issue, now bio splitting > is an issue, and I'm sure if we look further there will be other > cases that are issues (e.g. bounce buffers). > > The underlying problem here is that bio submission errors are > reported through bio completion mechanisms, not directly back to the > submitting context. Fix that problem in the block layer API, and > then iomap can use REQ_NOWAIT without having to care about what the > block layer is doing under the covers. Exactly. Either they need to be reported synchronously, or maybe we need a block layer hook in bio_endio that retries the given bio on a workqueue without ever bubbling up to the caller. But allowing delayed BLK_STS_AGAIN is going to mess up any non-trivial caller. But even for the plain block device is will cause duplicate I/O where some blocks have already been read/written and then will get resubmitted. I'm not sure that breaks any atomicity assumptions as we don't really give explicit ones for block devices (except maybe for the new RWF_ATOMIC flag?), but it certainly is unexpected and suboptimal.
diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c index b521eb15759e..07c336fdf4f0 100644 --- a/fs/iomap/direct-io.c +++ b/fs/iomap/direct-io.c @@ -363,9 +363,14 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter, */ if (need_zeroout || ((dio->flags & IOMAP_DIO_NEED_SYNC) && !use_fua) || - ((dio->flags & IOMAP_DIO_WRITE) && pos >= i_size_read(inode))) + ((dio->flags & IOMAP_DIO_WRITE) && pos >= i_size_read(inode))) { dio->flags &= ~IOMAP_DIO_CALLER_COMP; + if (!is_sync_kiocb(dio->iocb) && + (dio->iocb->ki_flags & IOCB_NOWAIT)) + return -EAGAIN; + } + /* * The rules for polled IO completions follow the guidelines as the * ones we set for inline and deferred completions. If none of those @@ -374,6 +379,23 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter, if (!(dio->flags & (IOMAP_DIO_INLINE_COMP|IOMAP_DIO_CALLER_COMP))) dio->iocb->ki_flags &= ~IOCB_HIPRI; + bio_opf = iomap_dio_bio_opflags(dio, iomap, use_fua, atomic); + + if (!is_sync_kiocb(dio->iocb) && (dio->iocb->ki_flags & IOCB_NOWAIT)) { + /* + * This is nonblocking IO, and we might need to allocate + * multiple bios. In this case, as we cannot guarantee that + * one of the sub bios will not fail getting issued FOR NOWAIT + * and as error results are coalesced across all of them, ask + * for a retry of this from blocking context. + */ + if (bio_iov_vecs_to_alloc(dio->submit.iter, BIO_MAX_VECS + 1) > + BIO_MAX_VECS) + return -EAGAIN; + + bio_opf |= REQ_NOWAIT; + } + if (need_zeroout) { /* zero out from the start of the block to the write offset */ pad = pos & (fs_block_size - 1); @@ -383,8 +405,6 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter, goto out; } - bio_opf = iomap_dio_bio_opflags(dio, iomap, use_fua, atomic); - nr_pages = bio_iov_vecs_to_alloc(dio->submit.iter, BIO_MAX_VECS); do { size_t n;
There are reports of high io_uring submission latency for ext4 and xfs, which is due to iomap not propagating nowait flag to the block layer resulting in waiting for IO during tag allocation. Because of how errors are propagated back, we can't set REQ_NOWAIT for multi bio IO, in this case return -EAGAIN and let the caller to handle it, for example, it can reissue it from a blocking context. It's aligned with how raw bdev direct IO handles it. Cc: stable@vger.kernel.org Link: https://github.com/axboe/liburing/issues/826#issuecomment-2674131870 Reported-by: wu lei <uwydoc@gmail.com> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- v2: Fail multi-bio nowait submissions fs/iomap/direct-io.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)