On Thu, Dec 01, 2022 at 07:09:54PM +0100, Andreas Gruenbacher wrote: > Hi again, > > [Same thing, but with the patches split correctly this time.] > > we're seeing a race between journaled data writes and the shrinker on > gfs2. What's happening is that gfs2_iomap_page_done() is called after > the page has been unlocked, so try_to_free_buffers() can come in and > free the buffers while gfs2_iomap_page_done() is trying to add them to > the transaction. Not good. > > This is a proposal to change iomap_page_ops so that page_prepare() > prepares the write and grabs the locked page, and page_done() unlocks > and puts that page again. While at it, this also converts the hooks > from pages to folios. > > To move the pagecache_isize_extended() call in iomap_write_end() out of > the way, a new folio_may_straddle_isize() helper is introduced that > takes a locked folio. That is then used when the inode size is updated, > before the folio is unlocked. > > I've also converted the other applicable folio_may_straddle_isize() > users, namely generic_write_end(), ext4_write_end(), and > ext4_journalled_write_end(). > > Any thoughts? I doubt that moving page cache operations from the iomap core to filesystem specific callouts will be acceptible. I recently proposed patches that added page cache walking to an XFS iomap callout to fix a data corruption, but they were NAKd on the basis that iomap is supposed to completely abstract away the folio and page cache manipulations from the filesystem. This patchset seems to be doing the same thing - moving page cache and folio management directly in filesystem specific callouts. Hence I'm going to assume that the same architectural demarcation is going to apply here, too... FYI, there is already significant change committed to the iomap write path in the current XFS tree as a result of the changes I mention - there is stale IOMAP detection which adds a new page ops method and adds new error paths with a locked folio in iomap_write_begin(). And this other data corruption (and performance) fix for handling zeroing over unwritten extents properly: https://lore.kernel.org/linux-xfs/20221201005214.3836105-1-david@xxxxxxxxxxxxx/ changes the way folios are looked up and instantiated in the page cache in iomap_write_begin(). It also adds new error conditions that need to be returned to callers so to implement conditional "folio must be present and dirty" page cache zeroing from iomap_zero_iter(). Those semantics would also have to be supported by gfs2, and that greatly complicates modifying and testing iomap core changes. To avoid all this, can we simple move the ->page_done() callout in the error path and iomap_write_end() to before we unlock the folio? You've already done that for pagecache_isize_extended(), and I can't see anything obvious in the gfs2 ->page_done callout that would cause issues if it is called with a locked dirty folio... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
