On Thu, Mar 17, 2022 at 8:04 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> So how about we do something like this:
>
> - Make folio_start_writeback() and set_page_writeback() return void,
> fixing up AFS and NFS.
> - Add a folio_wait_start_writeback() to use in the VFS
> - Remove the calls to set_page_writeback() in the filesystems
That sounds lovely, but it does worry me a bit. Not just the odd
'keepwrite' thing, but also the whole ordering between the folio bit
and the tagging bits. Does the ordering possibly matter?
That whole "xyz_writeback_keepwrite()" thing seems odd. It's used in
only one place (the folio version isn't used at all):
ext4_writepage():
ext4_walk_page_buffers() fails:
redirty_page_for_writepage(wbc, page);
keep_towrite = true;
ext4_bio_write_page().
which just looks odd. Why does it even try to continue to do the
writepage when the page buffer thing has failed?
In the regular write path (ie ext4_write_begin()), a
ext4_walk_page_buffers() failure is fatal or causes a retry). Why is
ext4_writepage() any different? Particularly since it wants to keep
the page dirty, then trying to do the writeback just seems wrong.
So this code is all a bit odd, I suspect there are decades of "people
continued to do what they historically did" changes, and it is all
worrisome.
Your cleanup sounds like the right thing, but I also think that
getting rid of that 'keepwrite' thing would also be the right thing.
And it all worries me.
Linus
