On Fri, Jun 11, 2021 at 04:35:25PM +0100, David Howells wrote:
> Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> > Yes. I do that kind of thing in iomap. What you're doing there looks
> > a bit like offset_in_folio(), but I don't understand the problem with
> > just checking pos against i_size directly.
>
> pos can be in the middle of a page. If i_size is at, say, the same point in
> the middle of that page and the page isn't currently in the cache, then we'll
> just clear the entire page and not read the bottom part of it (ie. the bit
> prior to the EOF).
The comment says that's expected, though. I think the comment is wrong;
consider the case where the page size is 4kB, file is 5kB long and
we do a 1kB write at offset=6kB. The existing CEPH code (prior to
netfs) will zero out 4-6KB and 7-8kB, which is wrong.
Anyway, looking at netfs_write_begin(), it's wrong too, in a bunch of
ways. You don't need to zero out the part of the page you're going to
copy into. And the condition is overly complicated which makes it
hard to know what's going on. Setting aside the is_cache_enabled part,
I think you want:
if (offset == 0 && len >= thp_size(page))
goto have_page_no_wait;
if (page_offset(page) >= size) {
zero_user_segments(page, 0, offset,
offset + len, thp_size(page));
goto have_page_no_wait;
}
... read the interesting chunks of page ...
