Messages in this thread | | | Date | Tue, 27 Jun 2023 12:16:34 +0100 | From | Matthew Wilcox <> | Subject | Re: [PATCH 03/12] writeback: Factor should_writeback_folio() out of write_cache_pages() |
| |
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.
| |