[03/12] writeback: Factor should_writeback_folio() out of write_cache_pages()
Commit Message
Reduce write_cache_pages() by about 30 lines; much of it is commentary,
but it all bundles nicely into an obvious function.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
mm/page-writeback.c | 60 +++++++++++++++++++++++++--------------------
1 file changed, 33 insertions(+), 27 deletions(-)
Comments
> + if (folio_test_writeback(folio)) {
> + if (wbc->sync_mode != WB_SYNC_NONE)
> + folio_wait_writeback(folio);
> + else
> + return false;
> + }
Please reorder this to avoid the else and return earlier while you're
at it:
if (folio_test_writeback(folio)) {
if (wbc->sync_mode == WB_SYNC_NONE)
return false;
folio_wait_writeback(folio);
}
(that's what actually got me started on my little cleanup spree while
checking some details of the writeback waiting..)
> + BUG_ON(folio_test_writeback(folio));
> + if (!folio_clear_dirty_for_io(folio))
> + return false;
> +
> + return true;
..
return folio_clear_dirty_for_io(folio);
?
On Mon, Jun 26, 2023 at 09:12:07PM -0700, Christoph Hellwig wrote:
> > + if (folio_test_writeback(folio)) {
> > + if (wbc->sync_mode != WB_SYNC_NONE)
> > + folio_wait_writeback(folio);
> > + else
> > + return false;
> > + }
>
> Please reorder this to avoid the else and return earlier while you're
> at it:
>
> if (folio_test_writeback(folio)) {
> if (wbc->sync_mode == WB_SYNC_NONE)
> return false;
> folio_wait_writeback(folio);
> }
Sure, that makes sense.
> (that's what actually got me started on my little cleanup spree while
> checking some details of the writeback waiting..)
This might be a good point to share that I'm considering (eventually)
not taking the folio lock here.
My plan looks something like this (not fully baked):
truncation (and similar) paths currently lock the folio, They would both
lock the folio _and_ claim that they were doing writeback on the folio.
Filesystems would receive the folio from the writeback iterator with
the writeback flag already set.
This allows, eg, folio mapping/unmapping to take place completely
independent of writeback. That seems like a good thing; I can't see
why the two should be connected.
> > + BUG_ON(folio_test_writeback(folio));
> > + if (!folio_clear_dirty_for_io(folio))
> > + return false;
> > +
> > + return true;
>
> ..
>
> return folio_clear_dirty_for_io(folio);
>
> ?
I did consider that, but there's a nice symmetry to the code the way it's
currently written, and that took precedence in my mind over "fewer lines
of code". There's nothing intrinsic about folio_clear_dirty_for_io()
being the last condition to be checked (is there? We have to
redirty_for_io if we decide to not start writeback), so it seemed to
make sense to leave space to add more conditions.
On Tue, Jun 27, 2023 at 12:16:34PM +0100, Matthew Wilcox wrote:
> This might be a good point to share that I'm considering (eventually)
> not taking the folio lock here.
>
> My plan looks something like this (not fully baked):
>
> truncation (and similar) paths currently lock the folio, They would both
> lock the folio _and_ claim that they were doing writeback on the folio.
>
> Filesystems would receive the folio from the writeback iterator with
> the writeback flag already set.
>
>
> This allows, eg, folio mapping/unmapping to take place completely
> independent of writeback. That seems like a good thing; I can't see
> why the two should be connected.
Ah, i_size is a problem. With an extending write, i_size is updated
while holding the folio lock. If we're writing back a partial folio,
we zero the tail. That must not race with an extending write. So
either we'd need to take both the folio lock & wb_lock when updating
i_size, or we'd need to take both the lock and wb_lock when writing
back the last page of a file.
@@ -2394,6 +2394,37 @@ static void writeback_get_batch(struct address_space *mapping,
&wbc->fbatch);
}
+static bool should_writeback_folio(struct address_space *mapping,
+ struct writeback_control *wbc, struct folio *folio)
+{
+ /*
+ * Folio truncated or invalidated. We can freely skip it then,
+ * even for data integrity operations: the folio has disappeared
+ * concurrently, so there could be no real expectation of this
+ * data integrity operation even if there is now a new, dirty
+ * folio at the same pagecache index.
+ */
+ if (unlikely(folio->mapping != mapping))
+ return false;
+
+ /* Did somebody write it for us? */
+ if (!folio_test_dirty(folio))
+ return false;
+
+ if (folio_test_writeback(folio)) {
+ if (wbc->sync_mode != WB_SYNC_NONE)
+ folio_wait_writeback(folio);
+ else
+ return false;
+ }
+
+ BUG_ON(folio_test_writeback(folio));
+ if (!folio_clear_dirty_for_io(folio))
+ return false;
+
+ return true;
+}
+
/**
* write_cache_pages - walk the list of dirty pages of the given address space and write all of them.
* @mapping: address space structure to write
@@ -2461,38 +2492,13 @@ int write_cache_pages(struct address_space *mapping,
wbc->done_index = folio->index;
folio_lock(folio);
-
- /*
- * Page truncated or invalidated. We can freely skip it
- * then, even for data integrity operations: the page
- * has disappeared concurrently, so there could be no
- * real expectation of this data integrity operation
- * even if there is now a new, dirty page at the same
- * pagecache address.
- */
- if (unlikely(folio->mapping != mapping)) {
-continue_unlock:
+ if (!should_writeback_folio(mapping, wbc, folio)) {
folio_unlock(folio);
continue;
}
- if (!folio_test_dirty(folio)) {
- /* someone wrote it for us */
- goto continue_unlock;
- }
-
- if (folio_test_writeback(folio)) {
- if (wbc->sync_mode != WB_SYNC_NONE)
- folio_wait_writeback(folio);
- else
- goto continue_unlock;
- }
-
- BUG_ON(folio_test_writeback(folio));
- if (!folio_clear_dirty_for_io(folio))
- goto continue_unlock;
-
trace_wbc_writepage(wbc, inode_to_bdi(mapping->host));
+
error = writepage(folio, wbc, data);
if (unlikely(error)) {
/*