On Fri, Aug 06, 2021 at 02:42:37PM +0100, David Howells wrote:

Matthew Wilcox [off-list ref] wrote:

quoted

quoted

It's fairly important to be able to do streaming writes without having
to read the old contents for some loads. And read-modify-write cycles
are death for performance, so you really want to coalesce writes until
you have the whole page.

I completely agree with you.  The context you're missing is that Dave
wants to do RMW twice.  He doesn't do the delaying SetPageUptodate dance.

Actually, I do the delaying of SetPageUptodate in the new write helpers that
I'm working on - at least to some extent.  For a write of any particular size
(which may be more than a page), I only read the first and last pages affected
if they're not completely changed by the write.  Note that I have my own
version of generic_perform_write() that allows me to eliminate write_begin and
write_end for any filesystem using it.

No, that is very much not the same thing.  Look at what NFS does, like
Linus said.  Consider this test program:

	fd = open();
	lseek(fd, 5, SEEK_SET);
	write(fd, buf, 3);
	write(fd, buf2, 10);
	write(fd, buf3, 2);
	close(fd);

You're going to do an RMW.  NFS keeps track of which bytes are dirty,
and writes only those bytes to the server (when that page is eventually
written-back).  So yes, it's using the page cache, but it's not doing
an unnecessary read from the server.

It has occurred to me that I don't actually need the pages to be uptodate and
completely filled out.  I'm tracking which bits are dirty - I could defer
reading the missing bits till someone wants to read or mmap.

But that kind of screws with local caching.  The local cache might need to
track the missing bits, and we are likely to be using blocks larger than a
page.

There's nothing to cache.  Pages which are !Uptodate aren't going to get
locally cached.

Basically, there are a lot of scenarios where not having fully populated pages
sucks.  And for streaming writes, wouldn't it be better if you used DIO
writes?

DIO can't do sub-512-byte writes.

quoted

If the write is less than the whole page, AFS, Ceph and anybody else
using netfs_write_begin() will first read the entire page in and mark
it Uptodate.

Indeed - but that function is set to be replaced.  What you're missing is that
if someone then tries to read the partially modified page, you may have to do
two reads from the server.

NFS doesn't.  It writes back the dirty data from the page and then
does a single read of the entire page.  And as I said later on, using
->is_partially_uptodate can avoid that for some cases.

quoted

Then he wants to track which parts of the page are dirty (at byte
granularity) and send only those bytes to the server in a write request.

Yes.  Because other constraints may apply, for example the handling of
conflicting third-party writes.  The question here is how much we care about
that - and that's why I'm trying to write back only what's changed where
possible.

If you care about conflicting writes from different clients, you really
need to establish a cache ownership model.  Or treat the page-cache as
write-through.

quoted

quoted

That said, I suspect it's also *very* filesystem-specific, to the
point where it might not be worth trying to do in some generic manner.

It certainly doesn't make sense for block filesystems.  Since they
can only do I/O on block boundaries, a sub-block write has to read in
the surrounding block, and once you're doing that, you might as well
read in the whole page.

I'm not trying to do this for block filesystems!  However, a block filesystem
- or even a blockdev - might be involved in terms of the local cache.

You might not be trying to do anything for block filesystems, but we
should think about what makes sense for block filesystems as well as
network filesystems.

quoted

Tracking sub-page dirty bits still makes sense.  It's on my to-do
list for iomap.

/me blinks

"bits" as in parts of a page or "bits" as in the PG_dirty bits on the pages
contributing to a folio?

Perhaps I should have said "Tracking dirtiness on a sub-page basis".
Right now, that looks like a block bitmap, but maybe it should be a
range-based data structure.