On Tue, Sep 14, 2021 at 7:18 AM Jens Axboe <axboe@xxxxxxxxx> wrote:
>
>
> + iov_iter_restore(iter, state);
> +
...
> rw->bytes_done += ret;
> + iov_iter_advance(iter, ret);
> + if (!iov_iter_count(iter))
> + break;
> + iov_iter_save_state(iter, state);
Ok, so now you keep iovb_iter and the state always in sync by just
always resetting the iter back and then walking it forward explicitly
- and re-saving the state.
That seems safe, if potentially unnecessarily expensive.
I guess re-walking lots of iovec entries is actually very unlikely in
practice, so maybe this "stupid brute-force" model is the right one.
I do find the odd "use __state vs rw->state" to be very confusing,
though. Particularly in io_read(), where you do this:
+ iov_iter_restore(iter, state);
+
ret2 = io_setup_async_rw(req, iovec, inline_vecs, iter, true);
if (ret2)
return ret2;
iovec = NULL;
rw = req->async_data;
- /* now use our persistent iterator, if we aren't already */
- iter = &rw->iter;
+ /* now use our persistent iterator and state, if we aren't already */
+ if (iter != &rw->iter) {
+ iter = &rw->iter;
+ state = &rw->iter_state;
+ }
do {
- io_size -= ret;
rw->bytes_done += ret;
+ iov_iter_advance(iter, ret);
+ if (!iov_iter_count(iter))
+ break;
+ iov_iter_save_state(iter, state);
Note how it first does that iov_iter_restore() on iter/state, buit
then it *replaces&* the iter/state pointers, and then it does
iov_iter_advance() on the replacement ones.
I don't see how that could be right. You're doing iov_iter_advance()
on something else than the one you restored to the original values.
And if it is right, it's sure confusing as hell.
Linus
