Cc: Erez Zadok <ezk@...>, Dave Hansen <haveblue@...>, Pekka Enberg <penberg@...>, Ryan Finnie <ryan@...>, Andrew Morton <akpm@...>, <linux-kernel@...>, <linux-fsdevel@...>, <cjwatson@...>, <linux-mm@...>
In message <Pine.LNX.4.64.0711051358440.7629@blonde.wat.veritas.com>, Hugh Dickins writes:
Hugh, my code looks like:
if (wbc->for_reclaim) {
set_page_dirty(lower_page);
unlock_page(lower_page);
goto out_release;
}
BUG_ON(!lower_mapping->a_ops->writepage);
clear_page_dirty_for_io(lower_page); /* emulate VFS behavior */
err = lower_mapping->a_ops->writepage(lower_page, wbc);
Do you mean I should set_page_dirty(lower_page) unconditionally before
clear_page_dirty_for_io? (I already do that in the 'if' statement above it.)
I looked at all mainline filesystems's ->writepage to see what, if any, they
do with their page's uptodate flag. Most f/s don't touch the flag one way
or another.
cifs_writepage sets the uptodate flag unconditionally: why?
ecryptfs_writepage has a legit reason: if encrypting the page failed, it doesn't want
anyone to use it, so it clears its page's uptodate flag (else it sets it as
uptodate).
hostfs_writepage clears pageuptodate if it failed to write_file(), which I'm
not sure if it makes sense or not.
ntfs_writepage goes as far as doing BUG_ON(!PageUptodate(page)) which
indicates to me that the page passed to ->writepage should always be
uptodate. Is that a fair statement?
smb_writepage pretty much unconditionally calls SetPageUptodate(page). Why?
Is there a reason smbfs and cifs both do this unconditionally? If so, then
why is ntfs calling BUG_ON if the page isn't uptodate? Either that BUG_ON
in ntfs is redundant, or cifs/smbfs's SetPageUptodate is redundant, but they
can't both be right.
And finally, unionfs clears the uptodate flag on error from the lower
->writepage, and otherwise sets the flag on success from the lower
->writepage. My gut feeling is that unionfs shouldn't change the page
uptodate flag at all: if the VFS passes unionfs_writepage a page which isn't
uptodate, then the VFS has a serious problem b/c it'd be asking a f/s to
write out a page which isn't up-to-date, right? Otherwise, whether
unionfs_writepage manages to write the lower page or not, why should that
invalidate the state of the unionfs page itself? Come to think of it, I
think clearing pageuptodate on error from ->writepage(lower_page) may be
bad. Imagine if after such a failed unionfs_writepage, I get a
unionfs_readpage: that ->readpage will get data from the lower f/s page and
copy it *over* the unionfs page, even if the upper page's data was more
recent prior to the failed call to unionfs_writepage. IOW, we could be
reverting a user-visible mmap'ed page to a previous on-disk version. What
do you think: could this happen? Anyway, I'll run some exhaustive testing
next and see what happens if I don't set/clear the uptodate flag in
unionfs_writepage.
OK.
[...]
Thanks. I'll look more closely into these issues and your patches, and post
my findings.
Erez.
-
