Re: [PATCH] AFS: Implement shared-writable mmap [try #2]

[David Howells <dhowells@redhat.com>]

Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> I would strongly suggest you used (0, PAGE_CACHE_SIZE) for the range,

That tells prepare_write() that the region to be modified is the whole page -
which is incorrect.  We're going to change a little bit of it.

Hmmm... Thinking about it again, I probably shouldn't be using
afs_prepare_write() at all.  afs_prepare_write() does two things:

 (1) Fills in the bits around the edges of the region to be modified if the
     page is not up to date.

 (2) Records the region of the page to be modified.

If afs_prepare_write() function is invoked by write(), then the region in (2)
is known, and thus the edges are known.

But if it's called from page_mkwrite(), we don't know where the edges are, so
we have to refresh the entire page if it's not up to date, and then we have to
record that the entire page needs writing back as we don't know which bits
have changed.

> and have your nopage function DTRT.

That assumes nopage() will be called in all cases - which it won't.

> Rather than add this (not always correct) comment about the VM workings, I'd
> just add a directive in the page_mkwrite API documentation that the filesystem
> is to return 0 if the page has been truncated.

I still want to put a comment in my code to say *why* I'm doing this.

> Minor issue: you can just check for `if (!page->mapping)` for truncation,
> which is the usual signal to tell the reader you're checking for truncate.

That's inconsistent with other core code, truncate_complete_page() for
example.

> Then you can remove the comment...

The comment stays, though I'm willing to improve it.

David
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/