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

[Nick Piggin <nickpiggin@yahoo.com.au>]

David Howells wrote:
> 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.

You don't know how much you're going to change, but it could be anything
in the range of 0 to PAGE_CACHE_SIZE. Clearly a (0, PAGE_CACHE_SIZE)
range is the right range to pass in.


> 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.

Oh god you're doing ClearPageUptodate directly on pagecache pages?

In general (modulo bugs and crazy filesystems), you're not allowed to have
!uptodate pages mapped into user addresses because that implies the user
would be allowed to see garbage.

If you follow that rule, then you can never have a !uptodate page be passed
into page_mkwrite (unless it has just been truncated, in which case you
catch that case anyway).


> > and have your nopage function DTRT.
>
>
> That assumes nopage() will be called in all cases - which it won't.

No, just assumes that nopage brings the page uptodate and people use the
correct invalidation functions to invalidate pagecache, so you never have
!uptodate pages that are mapped.


> > 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.

The one you put there was wrong.


> > 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.

Your filesystem internally moves pages between mappings like tmpfs?


> > Then you can remove the comment...
>
>
> The comment stays, though I'm willing to improve it.

Well I don't really care, so I'll just concentrate on trying to get the
code fixed.

--
SUSE Labs, Novell Inc.
-
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/