* [PATCHSET 0/3] Add ability to save/restore iov_iter state
@ 2021-09-10 18:25 Jens Axboe
2021-09-10 18:25 ` [PATCH 1/3] iov_iter: add helper to save " Jens Axboe
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Jens Axboe @ 2021-09-10 18:25 UTC (permalink / raw)
To: io-uring, linux-fsdevel; +Cc: torvalds, viro
Hi,
Linus didn't particularly love the iov_iter->truncated addition and how
it was used, and it was hard to disagree with that. Instead of relying
on tracking ->truncated, add a few pieces of state so we can safely
handle partial or errored read/write attempts (that we want to retry).
Then we can get rid of the iov_iter addition, and at the same time
handle cases that weren't handled correctly before.
I've run this through vectored read/write with io_uring on the commonly
problematic cases (dm and low depth SCSI device) which trigger these
conditions often, and it seems to pass muster.
For a discussion on this topic, see the thread here:
https://lore.kernel.org/linux-fsdevel/CAHk-=wiacKV4Gh-MYjteU0LwNBSGpWrK-Ov25HdqB1ewinrFPg@mail.gmail.com/
You can find these patches here:
https://git.kernel.dk/cgit/linux-block/log/?h=iov_iter
--
Jens Axboe
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] iov_iter: add helper to save iov_iter state 2021-09-10 18:25 [PATCHSET 0/3] Add ability to save/restore iov_iter state Jens Axboe @ 2021-09-10 18:25 ` Jens Axboe 2021-09-10 18:50 ` Al Viro 2021-09-10 18:25 ` [PATCH 2/3] io_uring: use iov_iter state save/restore helpers Jens Axboe ` (2 subsequent siblings) 3 siblings, 1 reply; 9+ messages in thread From: Jens Axboe @ 2021-09-10 18:25 UTC (permalink / raw) To: io-uring, linux-fsdevel; +Cc: torvalds, viro, Jens Axboe In an ideal world, when someone is passed an iov_iter and returns X bytes, then X bytes would have been consumed/advanced from the iov_iter. But we have use cases that always consume the entire iterator, a few examples of that are iomap and bdev O_DIRECT. This means we cannot rely on the state of the iov_iter once we've called ->read_iter() or ->write_iter(). This would be easier if we didn't always have to deal with truncate of the iov_iter, as rewinding would be trivial without that. We recently added a commit to track the truncate state, but that grew the iov_iter by 8 bytes and wasn't the best solution. Implement a helper to save enough of the iov_iter state to sanely restore it after we've called the read/write iterator helpers. This currently only works for IOVEC/BVEC/KVEC as that's all we need, support for other iterator types are left as an exercise for the reader. Link: https://lore.kernel.org/linux-fsdevel/CAHk-=wiacKV4Gh-MYjteU0LwNBSGpWrK-Ov25HdqB1ewinrFPg@mail.gmail.com/ Signed-off-by: Jens Axboe <axboe@kernel.dk> --- include/linux/uio.h | 16 ++++++++++++++++ lib/iov_iter.c | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/include/linux/uio.h b/include/linux/uio.h index 5265024e8b90..6eaedae5ea2f 100644 --- a/include/linux/uio.h +++ b/include/linux/uio.h @@ -27,6 +27,12 @@ enum iter_type { ITER_DISCARD, }; +struct iov_iter_state { + size_t iov_offset; + size_t count; + unsigned long nr_segs; +}; + struct iov_iter { u8 iter_type; bool data_source; @@ -55,6 +61,14 @@ static inline enum iter_type iov_iter_type(const struct iov_iter *i) return i->iter_type; } +static inline void iov_iter_save_state(struct iov_iter *iter, + struct iov_iter_state *state) +{ + state->iov_offset = iter->iov_offset; + state->count = iter->count; + state->nr_segs = iter->nr_segs; +} + static inline bool iter_is_iovec(const struct iov_iter *i) { return iov_iter_type(i) == ITER_IOVEC; @@ -233,6 +247,8 @@ ssize_t iov_iter_get_pages(struct iov_iter *i, struct page **pages, ssize_t iov_iter_get_pages_alloc(struct iov_iter *i, struct page ***pages, size_t maxsize, size_t *start); int iov_iter_npages(const struct iov_iter *i, int maxpages); +void iov_iter_restore(struct iov_iter *i, struct iov_iter_state *state, + ssize_t did_bytes); const void *dup_iter(struct iov_iter *new, struct iov_iter *old, gfp_t flags); diff --git a/lib/iov_iter.c b/lib/iov_iter.c index f2d50d69a6c3..280dbcc523e5 100644 --- a/lib/iov_iter.c +++ b/lib/iov_iter.c @@ -1972,3 +1972,39 @@ int import_single_range(int rw, void __user *buf, size_t len, return 0; } EXPORT_SYMBOL(import_single_range); + +/** + * iov_iter_restore() - Restore a &struct iov_iter to the same state as when + * iov_iter_save_state() was called. + * + * @i: &struct iov_iter to restore + * @state: state to restore from + * @did_bytes: bytes to advance @i after restoring it + * + * Used after iov_iter_save_state() to bring restore @i, if operations may + * have advanced it. If @did_bytes is a positive value, then after restoring + * @i it is advanced accordingly. This is useful for handling short reads or + * writes for retry, if lower down the stack @i was advanced further than the + * returned value. If @did_bytes is negative (eg an error), then only the + * state restore is done. + * + * Note: only works on ITER_IOVEC, ITER_BVEC, and ITER_KVEC + */ +void iov_iter_restore(struct iov_iter *i, struct iov_iter_state *state, + ssize_t did_bytes) +{ + if (WARN_ON_ONCE(!iov_iter_is_bvec(i) && !iter_is_iovec(i)) && + !iov_iter_is_kvec(i)) + return; + i->iov_offset = state->iov_offset; + i->count = state->count; + /* + * For the *vec iters, nr_segs + iov is constant - if we increment + * the vec, then we also decrement the nr_segs count. Hence we don't + * need to track both of these, just one is enough and we can deduct + * the other from that. ITER_{BVEC,IOVEC,KVEC} all have their pointers + * unionized, so we don't need to handle them individually. + */ + i->iov -= state->nr_segs - i->nr_segs; + i->nr_segs = state->nr_segs; +} -- 2.33.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] iov_iter: add helper to save iov_iter state 2021-09-10 18:25 ` [PATCH 1/3] iov_iter: add helper to save " Jens Axboe @ 2021-09-10 18:50 ` Al Viro 2021-09-10 19:15 ` [PATCH v2 " Jens Axboe 0 siblings, 1 reply; 9+ messages in thread From: Al Viro @ 2021-09-10 18:50 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring, linux-fsdevel, torvalds On Fri, Sep 10, 2021 at 12:25:34PM -0600, Jens Axboe wrote: > +void iov_iter_restore(struct iov_iter *i, struct iov_iter_state *state, > + ssize_t did_bytes) > +{ > + if (WARN_ON_ONCE(!iov_iter_is_bvec(i) && !iter_is_iovec(i)) && > + !iov_iter_is_kvec(i)) > + return; > + i->iov_offset = state->iov_offset; > + i->count = state->count; > + /* > + * For the *vec iters, nr_segs + iov is constant - if we increment > + * the vec, then we also decrement the nr_segs count. Hence we don't > + * need to track both of these, just one is enough and we can deduct > + * the other from that. ITER_{BVEC,IOVEC,KVEC} all have their pointers > + * unionized, so we don't need to handle them individually. > + */ > + i->iov -= state->nr_segs - i->nr_segs; > + i->nr_segs = state->nr_segs; > +} No. You can have iovec and kvec handled jointly (struct iovec and struct kvec always have the same size). You can *not* do that to bvec - check sizeof of struct bio_vec and struct iovec on 32bit targets. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/3] iov_iter: add helper to save iov_iter state 2021-09-10 18:50 ` Al Viro @ 2021-09-10 19:15 ` Jens Axboe 0 siblings, 0 replies; 9+ messages in thread From: Jens Axboe @ 2021-09-10 19:15 UTC (permalink / raw) To: Al Viro; +Cc: io-uring, linux-fsdevel, torvalds In an ideal world, when someone is passed an iov_iter and returns X bytes, then X bytes would have been consumed/advanced from the iov_iter. But we have use cases that always consume the entire iterator, a few examples of that are iomap and bdev O_DIRECT. This means we cannot rely on the state of the iov_iter once we've called ->read_iter() or ->write_iter(). This would be easier if we didn't always have to deal with truncate of the iov_iter, as rewinding would be trivial without that. We recently added a commit to track the truncate state, but that grew the iov_iter by 8 bytes and wasn't the best solution. Implement a helper to save enough of the iov_iter state to sanely restore it after we've called the read/write iterator helpers. This currently only works for IOVEC/BVEC/KVEC as that's all we need, support for other iterator types are left as an exercise for the reader. Link: https://lore.kernel.org/linux-fsdevel/CAHk-=wiacKV4Gh-MYjteU0LwNBSGpWrK-Ov25HdqB1ewinrFPg@mail.gmail.com/ Signed-off-by: Jens Axboe <axboe@kernel.dk> --- V2: Don't assume bvec is the same size as iovec/kvec. Add a special case for it, and a BUILD_BUG_ON() for iovec/kvec sizing as well. diff --git a/include/linux/uio.h b/include/linux/uio.h index 5265024e8b90..6eaedae5ea2f 100644 --- a/include/linux/uio.h +++ b/include/linux/uio.h @@ -27,6 +27,12 @@ enum iter_type { ITER_DISCARD, }; +struct iov_iter_state { + size_t iov_offset; + size_t count; + unsigned long nr_segs; +}; + struct iov_iter { u8 iter_type; bool data_source; @@ -55,6 +61,14 @@ static inline enum iter_type iov_iter_type(const struct iov_iter *i) return i->iter_type; } +static inline void iov_iter_save_state(struct iov_iter *iter, + struct iov_iter_state *state) +{ + state->iov_offset = iter->iov_offset; + state->count = iter->count; + state->nr_segs = iter->nr_segs; +} + static inline bool iter_is_iovec(const struct iov_iter *i) { return iov_iter_type(i) == ITER_IOVEC; @@ -233,6 +247,8 @@ ssize_t iov_iter_get_pages(struct iov_iter *i, struct page **pages, ssize_t iov_iter_get_pages_alloc(struct iov_iter *i, struct page ***pages, size_t maxsize, size_t *start); int iov_iter_npages(const struct iov_iter *i, int maxpages); +void iov_iter_restore(struct iov_iter *i, struct iov_iter_state *state, + ssize_t did_bytes); const void *dup_iter(struct iov_iter *new, struct iov_iter *old, gfp_t flags); diff --git a/lib/iov_iter.c b/lib/iov_iter.c index f2d50d69a6c3..b46173eb61c7 100644 --- a/lib/iov_iter.c +++ b/lib/iov_iter.c @@ -1972,3 +1972,45 @@ int import_single_range(int rw, void __user *buf, size_t len, return 0; } EXPORT_SYMBOL(import_single_range); + +/** + * iov_iter_restore() - Restore a &struct iov_iter to the same state as when + * iov_iter_save_state() was called. + * + * @i: &struct iov_iter to restore + * @state: state to restore from + * @did_bytes: bytes to advance @i after restoring it + * + * Used after iov_iter_save_state() to bring restore @i, if operations may + * have advanced it. If @did_bytes is a positive value, then after restoring + * @i it is advanced accordingly. This is useful for handling short reads or + * writes for retry, if lower down the stack @i was advanced further than the + * returned value. If @did_bytes is negative (eg an error), then only the + * state restore is done. + * + * Note: only works on ITER_IOVEC, ITER_BVEC, and ITER_KVEC + */ +void iov_iter_restore(struct iov_iter *i, struct iov_iter_state *state, + ssize_t did_bytes) +{ + if (WARN_ON_ONCE(!iov_iter_is_bvec(i) && !iter_is_iovec(i)) && + !iov_iter_is_kvec(i)) + return; + i->iov_offset = state->iov_offset; + i->count = state->count; + /* + * For the *vec iters, nr_segs + iov is constant - if we increment + * the vec, then we also decrement the nr_segs count. Hence we don't + * need to track both of these, just one is enough and we can deduct + * the other from that. ITER_KVEC and ITER_IOVEC are the same struct + * size, so we can just increment the iov pointer as they are unionzed. + * ITER_BVEC _may_ be the same size on some archs, but on others it is + * not. Be safe and handle it separately. + */ + BUILD_BUG_ON(sizeof(struct iovec) != sizeof(struct kvec)); + if (iov_iter_is_bvec(i)) + i->bvec -= state->nr_segs - i->nr_segs; + else + i->iov -= state->nr_segs - i->nr_segs; + i->nr_segs = state->nr_segs; +} ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] io_uring: use iov_iter state save/restore helpers 2021-09-10 18:25 [PATCHSET 0/3] Add ability to save/restore iov_iter state Jens Axboe 2021-09-10 18:25 ` [PATCH 1/3] iov_iter: add helper to save " Jens Axboe @ 2021-09-10 18:25 ` Jens Axboe 2021-09-10 18:25 ` [PATCH 3/3] Revert "iov_iter: track truncated size" Jens Axboe 2021-09-13 22:43 ` [PATCHSET 0/3] Add ability to save/restore iov_iter state Jens Axboe 3 siblings, 0 replies; 9+ messages in thread From: Jens Axboe @ 2021-09-10 18:25 UTC (permalink / raw) To: io-uring, linux-fsdevel; +Cc: torvalds, viro, Jens Axboe Get rid of the need to do re-expand and revert on an iterator when we encounter a short IO, or failure that warrants a retry. Use the new state save/restore helpers instead. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- fs/io_uring.c | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index 855ea544807f..84e33f751372 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -712,6 +712,7 @@ struct io_async_rw { struct iovec fast_iov[UIO_FASTIOV]; const struct iovec *free_iovec; struct iov_iter iter; + struct iov_iter_state iter_state; size_t bytes_done; struct wait_page_queue wpq; }; @@ -2608,8 +2609,7 @@ static bool io_resubmit_prep(struct io_kiocb *req) if (!rw) return !io_req_prep_async(req); - /* may have left rw->iter inconsistent on -EIOCBQUEUED */ - iov_iter_revert(&rw->iter, req->result - iov_iter_count(&rw->iter)); + iov_iter_restore(&rw->iter, &rw->iter_state, 0); return true; } @@ -3437,19 +3437,22 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) struct kiocb *kiocb = &req->rw.kiocb; struct iov_iter __iter, *iter = &__iter; struct io_async_rw *rw = req->async_data; - ssize_t io_size, ret, ret2; bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; + struct iov_iter_state __state, *state; + ssize_t ret, ret2; if (rw) { iter = &rw->iter; + state = &rw->iter_state; iovec = NULL; } else { ret = io_import_iovec(READ, req, &iovec, iter, !force_nonblock); if (ret < 0) return ret; + state = &__state; } - io_size = iov_iter_count(iter); - req->result = io_size; + req->result = iov_iter_count(iter); + iov_iter_save_state(iter, state); /* Ensure we clear previously set non-block flag */ if (!force_nonblock) @@ -3463,7 +3466,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) return ret ?: -EAGAIN; } - ret = rw_verify_area(READ, req->file, io_kiocb_ppos(kiocb), io_size); + ret = rw_verify_area(READ, req->file, io_kiocb_ppos(kiocb), state->count); if (unlikely(ret)) { kfree(iovec); return ret; @@ -3479,18 +3482,17 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) /* no retry on NONBLOCK nor RWF_NOWAIT */ if (req->flags & REQ_F_NOWAIT) goto done; - /* some cases will consume bytes even on error returns */ - iov_iter_reexpand(iter, iter->count + iter->truncated); - iov_iter_revert(iter, io_size - iov_iter_count(iter)); ret = 0; } else if (ret == -EIOCBQUEUED) { goto out_free; - } else if (ret <= 0 || ret == io_size || !force_nonblock || + } else if (ret <= 0 || ret == state->count || !force_nonblock || (req->flags & REQ_F_NOWAIT) || !need_read_all(req)) { /* read all, failed, already did sync or don't want to retry */ goto done; } + iov_iter_restore(iter, state, ret); + ret2 = io_setup_async_rw(req, iovec, inline_vecs, iter, true); if (ret2) return ret2; @@ -3501,7 +3503,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) iter = &rw->iter; do { - io_size -= ret; + state->count -= ret; rw->bytes_done += ret; /* if we can retry, do so with the callbacks armed */ if (!io_rw_should_retry(req)) { @@ -3520,7 +3522,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) return 0; /* we got some bytes, but not all. retry. */ kiocb->ki_flags &= ~IOCB_WAITQ; - } while (ret > 0 && ret < io_size); + } while (ret > 0 && ret < state->count); done: kiocb_done(kiocb, ret, issue_flags); out_free: @@ -3543,19 +3545,23 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags) struct kiocb *kiocb = &req->rw.kiocb; struct iov_iter __iter, *iter = &__iter; struct io_async_rw *rw = req->async_data; - ssize_t ret, ret2, io_size; bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; + struct iov_iter_state __state, *state; + ssize_t ret, ret2; if (rw) { iter = &rw->iter; + state = &rw->iter_state; iovec = NULL; } else { ret = io_import_iovec(WRITE, req, &iovec, iter, !force_nonblock); if (ret < 0) return ret; + state = &__state; } - io_size = iov_iter_count(iter); - req->result = io_size; + req->result = iov_iter_count(iter); + iov_iter_save_state(iter, state); + ret2 = 0; /* Ensure we clear previously set non-block flag */ if (!force_nonblock) @@ -3572,7 +3578,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags) (req->flags & REQ_F_ISREG)) goto copy_iov; - ret = rw_verify_area(WRITE, req->file, io_kiocb_ppos(kiocb), io_size); + ret = rw_verify_area(WRITE, req->file, io_kiocb_ppos(kiocb), state->count); if (unlikely(ret)) goto out_free; @@ -3619,9 +3625,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags) kiocb_done(kiocb, ret2, issue_flags); } else { copy_iov: - /* some cases will consume bytes even on error returns */ - iov_iter_reexpand(iter, iter->count + iter->truncated); - iov_iter_revert(iter, io_size - iov_iter_count(iter)); + iov_iter_restore(iter, state, ret2); ret = io_setup_async_rw(req, iovec, inline_vecs, iter, false); return ret ?: -EAGAIN; } -- 2.33.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] Revert "iov_iter: track truncated size" 2021-09-10 18:25 [PATCHSET 0/3] Add ability to save/restore iov_iter state Jens Axboe 2021-09-10 18:25 ` [PATCH 1/3] iov_iter: add helper to save " Jens Axboe 2021-09-10 18:25 ` [PATCH 2/3] io_uring: use iov_iter state save/restore helpers Jens Axboe @ 2021-09-10 18:25 ` Jens Axboe 2021-09-13 22:43 ` [PATCHSET 0/3] Add ability to save/restore iov_iter state Jens Axboe 3 siblings, 0 replies; 9+ messages in thread From: Jens Axboe @ 2021-09-10 18:25 UTC (permalink / raw) To: io-uring, linux-fsdevel; +Cc: torvalds, viro, Jens Axboe This reverts commit 2112ff5ce0c1128fe7b4d19cfe7f2b8ce5b595fa. We no longer need to track the truncation count, the one user that did need it has been converted to using iov_iter_restore() instead. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- include/linux/uio.h | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/include/linux/uio.h b/include/linux/uio.h index 6eaedae5ea2f..7c8aeacc6fa7 100644 --- a/include/linux/uio.h +++ b/include/linux/uio.h @@ -53,7 +53,6 @@ struct iov_iter { }; loff_t xarray_start; }; - size_t truncated; }; static inline enum iter_type iov_iter_type(const struct iov_iter *i) @@ -271,10 +270,8 @@ static inline void iov_iter_truncate(struct iov_iter *i, u64 count) * conversion in assignement is by definition greater than all * values of size_t, including old i->count. */ - if (i->count > count) { - i->truncated += i->count - count; + if (i->count > count) i->count = count; - } } /* @@ -283,7 +280,6 @@ static inline void iov_iter_truncate(struct iov_iter *i, u64 count) */ static inline void iov_iter_reexpand(struct iov_iter *i, size_t count) { - i->truncated -= count - i->count; i->count = count; } -- 2.33.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCHSET 0/3] Add ability to save/restore iov_iter state 2021-09-10 18:25 [PATCHSET 0/3] Add ability to save/restore iov_iter state Jens Axboe ` (2 preceding siblings ...) 2021-09-10 18:25 ` [PATCH 3/3] Revert "iov_iter: track truncated size" Jens Axboe @ 2021-09-13 22:43 ` Jens Axboe 2021-09-13 23:23 ` Linus Torvalds 3 siblings, 1 reply; 9+ messages in thread From: Jens Axboe @ 2021-09-13 22:43 UTC (permalink / raw) To: io-uring, linux-fsdevel; +Cc: torvalds, viro On 9/10/21 12:25 PM, Jens Axboe wrote: > Hi, > > Linus didn't particularly love the iov_iter->truncated addition and how > it was used, and it was hard to disagree with that. Instead of relying > on tracking ->truncated, add a few pieces of state so we can safely > handle partial or errored read/write attempts (that we want to retry). > > Then we can get rid of the iov_iter addition, and at the same time > handle cases that weren't handled correctly before. > > I've run this through vectored read/write with io_uring on the commonly > problematic cases (dm and low depth SCSI device) which trigger these > conditions often, and it seems to pass muster. > > For a discussion on this topic, see the thread here: > > https://lore.kernel.org/linux-fsdevel/CAHk-=wiacKV4Gh-MYjteU0LwNBSGpWrK-Ov25HdqB1ewinrFPg@mail.gmail.com/ > > You can find these patches here: > > https://git.kernel.dk/cgit/linux-block/log/?h=iov_iter Al, Linus, are you OK with this? I think we should get this in for 5.15. I didn't resend the whole series, just a v2 of patch 1/3 to fix that bvec vs iovec issue. Let me know if you want the while thing resent. -- Jens Axboe ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCHSET 0/3] Add ability to save/restore iov_iter state 2021-09-13 22:43 ` [PATCHSET 0/3] Add ability to save/restore iov_iter state Jens Axboe @ 2021-09-13 23:23 ` Linus Torvalds 2021-09-14 1:54 ` Jens Axboe 0 siblings, 1 reply; 9+ messages in thread From: Linus Torvalds @ 2021-09-13 23:23 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring, linux-fsdevel, Al Viro On Mon, Sep 13, 2021 at 3:43 PM Jens Axboe <axboe@kernel.dk> wrote: > > Al, Linus, are you OK with this? I think we should get this in for 5.15. > I didn't resend the whole series, just a v2 of patch 1/3 to fix that bvec > vs iovec issue. Let me know if you want the while thing resent. So I'm ok with the iov_iter side, but the io_uring side seems still positively buggy, and very confused. It also messes with the state in bad ways and has internal knowledge. And some of it looks completely bogus. For example, I see state->count -= ret; rw->bytes_done += ret; and I go "that's BS". There's no way it's ok to start messing with the byte count inside the state like that. That just means that the state is now no longer the saved state, and it's some random garbage. I also think that the "bytes_done += ret" is a big hint there: any time you restore the iovec state, you should then forward it by "bytes_done". But that's not what the code does. Instead, it will now restore the iovec styate with the *wrong* number of bytes remaining, but will start from the beginning of the iovec. So I think the fs/io_uring.c use of this state buffer is completely wrong. What *may* be the right thing to do is to (a) not mess with state->count (b) when you restore the state you always use iov_iter_restore(iter, state, bytes_done); to actually restore the *correct* state. Because modifying the iovec save state like that cannot be right, and if it's right it's still too ugly and fragile for words. That save state should be treated as a snapshot, not as a random buffer that you can make arbitrary changes to. See what I'm saying? I'd like Al to take a look at the io_uring.c usage too, since this was just my reaction from looking at that diff a bit more. Linus ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCHSET 0/3] Add ability to save/restore iov_iter state 2021-09-13 23:23 ` Linus Torvalds @ 2021-09-14 1:54 ` Jens Axboe 0 siblings, 0 replies; 9+ messages in thread From: Jens Axboe @ 2021-09-14 1:54 UTC (permalink / raw) To: Linus Torvalds; +Cc: io-uring, linux-fsdevel, Al Viro On 9/13/21 5:23 PM, Linus Torvalds wrote: > On Mon, Sep 13, 2021 at 3:43 PM Jens Axboe <axboe@kernel.dk> wrote: >> >> Al, Linus, are you OK with this? I think we should get this in for 5.15. >> I didn't resend the whole series, just a v2 of patch 1/3 to fix that bvec >> vs iovec issue. Let me know if you want the while thing resent. > > So I'm ok with the iov_iter side, but the io_uring side seems still > positively buggy, and very confused. > > It also messes with the state in bad ways and has internal knowledge. > And some of it looks completely bogus. > > For example, I see > > state->count -= ret; > rw->bytes_done += ret; > > and I go "that's BS". There's no way it's ok to start messing with the > byte count inside the state like that. That just means that the state > is now no longer the saved state, and it's some random garbage. > > I also think that the "bytes_done += ret" is a big hint there: any > time you restore the iovec state, you should then forward it by > "bytes_done". But that's not what the code does. > > Instead, it will now restore the iovec styate with the *wrong* number > of bytes remaining, but will start from the beginning of the iovec. > > So I think the fs/io_uring.c use of this state buffer is completely wrong. > > What *may* be the right thing to do is to > > (a) not mess with state->count > > (b) when you restore the state you always use > > iov_iter_restore(iter, state, bytes_done); > > to actually restore the *correct* state. > > Because modifying the iovec save state like that cannot be right, and > if it's right it's still too ugly and fragile for words. That save > state should be treated as a snapshot, not as a random buffer that you > can make arbitrary changes to. > > See what I'm saying? OK, for the do while loop itself, I do think we should be more consistent and that would also get rid of the state->count manipulation. I do agree that messing with that state is not something that should be done, and we can do this more cleanly and consistently instead. Once we hit the do {} while loop, state should be &rw->state and we can consistently handle it that way. Let me rework that bit and run the tests, and I'll post a v2 tomorrow. Thanks for taking a closer look. -- Jens Axboe ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-09-14 1:54 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-10 18:25 [PATCHSET 0/3] Add ability to save/restore iov_iter state Jens Axboe 2021-09-10 18:25 ` [PATCH 1/3] iov_iter: add helper to save " Jens Axboe 2021-09-10 18:50 ` Al Viro 2021-09-10 19:15 ` [PATCH v2 " Jens Axboe 2021-09-10 18:25 ` [PATCH 2/3] io_uring: use iov_iter state save/restore helpers Jens Axboe 2021-09-10 18:25 ` [PATCH 3/3] Revert "iov_iter: track truncated size" Jens Axboe 2021-09-13 22:43 ` [PATCHSET 0/3] Add ability to save/restore iov_iter state Jens Axboe 2021-09-13 23:23 ` Linus Torvalds 2021-09-14 1:54 ` Jens Axboe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).