Re: [git pull] iov_iter fixes

From: Jens Axboe <axboe@kernel.dk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>,
	Pavel Begunkov <asml.silence@gmail.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [git pull] iov_iter fixes
Date: Thu, 9 Sep 2021 16:21:27 -0600	[thread overview]
Message-ID: <ebb7b323-2ae9-9981-cdfd-f0f460be43b3@kernel.dk> (raw)
In-Reply-To: <CAHk-=whoMLW-WP=8DikhfE4xAu_Tw9jDNkdab4RGEWWMagzW8Q@mail.gmail.com>

On 9/9/21 3:56 PM, Linus Torvalds wrote:
> On Thu, Sep 9, 2021 at 2:39 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> OK, one that I immediately found is just doing O_DIRECT to a block
>> device or file on XFS. As pages are mapped and added, the iov_iter is
>> advanced. If we then go and submit and get -EAGAIN, for example, then we
>> return with what we mapped already consumed.
> 
> Ok, that's annoying but understandable. Dave points to a commit that
> removes one of the EAGAIN cases, but apparently not some others.

That one just catches it upfront so we -EAGAIN immediately, which does
make it a lot easier to handle. But yes, that's an example.

The case I mention we basically always consume the whole iov, to the
extent that we can map it to a bio. But there's really no way around
that, we have to map it before we can attempt to do that IO.

> I do kind of wonder if you can't have the exact same case when *some*
> of the IO succeeds, though.
> 
> IOW, can't we have  that
> 
>         ret = io_iter_do_read(req, iter);
> 
> return partial success - and if XFS does that "update iovec on
> failure", I could easily see that same code - or something else -
> having done the exact same thing.
> 
> Put another way: if the iovec isn't guaranteed to be coherent when an
> actual error occurs, then why would it be guaranteed to be coherent
> with a partial success value?
> 
> Because in most cases - I'd argue pretty much all - those "partial
> success" cases are *exactly* the same as the error cases, it's just
> that we had a loop and one or more iterations succeeded before it hit
> the error case.

Right, which is why the reset would be nice, but reexpand + revert at
least works and accomplishes the same even if it doesn't look as pretty.
We do return how much IO was actually done from the various
->read/write_iter() obviously, and that cannot be incorrect. It's just
that the iov_iter doesn't necessarily agree with that view and more (or
all) may have been consumed regardless of the return value. The truncate
was really the part that made it impossible to handle.

-- 
Jens Axboe