| Message ID | 20221213194833.1636649-1-agruenba@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | iomap: Move page_done callback under the folio lock | expand |
On Tue, Dec 13, 2022 at 08:48:33PM +0100, Andreas Gruenbacher wrote: > Hi Darrick, > > I'd like to get the following iomap change into this merge window. This > only affects gfs2, so I can push it as part of the gfs2 updates if you > don't mind, provided that I'll get your Reviewed-by confirmation. > Otherwise, if you'd prefer to pass this through the xfs tree, could you > please take it? I don't mind you pushing changes to ->page_done through the gfs2 tree, but don't you need to move the other callsite at the bottom of iomap_write_begin? --D > Thanks, > Andreas > > -- > > Move the ->page_done() call in iomap_write_end() under the folio lock. > This closes a race between journaled data writes and the shrinker in > 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 current transaction. The folio lock prevents that from happening. > > The only user of ->page_done() is gfs2, so other filesystems are not > affected. > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> > --- > fs/iomap/buffered-io.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 91ee0b308e13..476c9ed1b333 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -714,12 +714,12 @@ static size_t iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len, > i_size_write(iter->inode, pos + ret); > iter->iomap.flags |= IOMAP_F_SIZE_CHANGED; > } > + if (page_ops && page_ops->page_done) > + page_ops->page_done(iter->inode, pos, ret, &folio->page); > folio_unlock(folio); > > if (old_size < pos) > pagecache_isize_extended(iter->inode, old_size, pos); > - if (page_ops && page_ops->page_done) > - page_ops->page_done(iter->inode, pos, ret, &folio->page); > folio_put(folio); > > if (ret < len) > -- > 2.38.1 >
On Tue, Dec 13, 2022 at 9:03 PM Darrick J. Wong <djwong@kernel.org> wrote: > On Tue, Dec 13, 2022 at 08:48:33PM +0100, Andreas Gruenbacher wrote: > > Hi Darrick, > > > > I'd like to get the following iomap change into this merge window. This > > only affects gfs2, so I can push it as part of the gfs2 updates if you > > don't mind, provided that I'll get your Reviewed-by confirmation. > > Otherwise, if you'd prefer to pass this through the xfs tree, could you > > please take it? > > I don't mind you pushing changes to ->page_done through the gfs2 tree, > but don't you need to move the other callsite at the bottom of > iomap_write_begin? No, in the failure case in iomap_write_begin(), the folio isn't relevant because it's not being written to. Thanks for paying attention, Andreas > --D > > > Thanks, > > Andreas > > > > -- > > > > Move the ->page_done() call in iomap_write_end() under the folio lock. > > This closes a race between journaled data writes and the shrinker in > > 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 current transaction. The folio lock prevents that from happening. > > > > The only user of ->page_done() is gfs2, so other filesystems are not > > affected. > > > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> > > --- > > fs/iomap/buffered-io.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > > index 91ee0b308e13..476c9ed1b333 100644 > > --- a/fs/iomap/buffered-io.c > > +++ b/fs/iomap/buffered-io.c > > @@ -714,12 +714,12 @@ static size_t iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len, > > i_size_write(iter->inode, pos + ret); > > iter->iomap.flags |= IOMAP_F_SIZE_CHANGED; > > } > > + if (page_ops && page_ops->page_done) > > + page_ops->page_done(iter->inode, pos, ret, &folio->page); > > folio_unlock(folio); > > > > if (old_size < pos) > > pagecache_isize_extended(iter->inode, old_size, pos); > > - if (page_ops && page_ops->page_done) > > - page_ops->page_done(iter->inode, pos, ret, &folio->page); > > folio_put(folio); > > > > if (ret < len) > > -- > > 2.38.1 > > >
On Tue, Dec 13, 2022 at 12:03:41PM -0800, Darrick J. Wong wrote: > On Tue, Dec 13, 2022 at 08:48:33PM +0100, Andreas Gruenbacher wrote: > > Hi Darrick, > > > > I'd like to get the following iomap change into this merge window. This > > only affects gfs2, so I can push it as part of the gfs2 updates if you > > don't mind, provided that I'll get your Reviewed-by confirmation. > > Otherwise, if you'd prefer to pass this through the xfs tree, could you > > please take it? > > I don't mind you pushing changes to ->page_done through the gfs2 tree, > but don't you need to move the other callsite at the bottom of > iomap_write_begin? Yes. And if we touch this anyway it really should switch to passing a folio, which also nicely breaks any in progress code (if there is any) and makes them notice the change. That being said, do you mean 6.2 with "this window"? Unless the gfs2 changes are a critical bug fix, I don't think Linux will take them if applied after 6.1 was released.
On Wed, Dec 14, 2022 at 10:07 AM Christoph Hellwig <hch@infradead.org> wrote: > On Tue, Dec 13, 2022 at 12:03:41PM -0800, Darrick J. Wong wrote: > > On Tue, Dec 13, 2022 at 08:48:33PM +0100, Andreas Gruenbacher wrote: > > > Hi Darrick, > > > > > > I'd like to get the following iomap change into this merge window. This > > > only affects gfs2, so I can push it as part of the gfs2 updates if you > > > don't mind, provided that I'll get your Reviewed-by confirmation. > > > Otherwise, if you'd prefer to pass this through the xfs tree, could you > > > please take it? > > > > I don't mind you pushing changes to ->page_done through the gfs2 tree, > > but don't you need to move the other callsite at the bottom of > > iomap_write_begin? > > Yes. I assume you mean yes to the former, because the ->page_done() call in iomap_write_begin() really doesn't need to be moved. > And if we touch this anyway it really should switch to passing > a folio, which also nicely breaks any in progress code (if there is any) > and makes them notice the change. Okay. > That being said, do you mean 6.2 with "this window"? Unless the gfs2 > changes are a critical bug fix, I don't think Linux will take them if > applied after 6.1 was released. Yes, I really mean the merge window that is currently open. Thanks, Andreas
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 91ee0b308e13..476c9ed1b333 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -714,12 +714,12 @@ static size_t iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len, i_size_write(iter->inode, pos + ret); iter->iomap.flags |= IOMAP_F_SIZE_CHANGED; } + if (page_ops && page_ops->page_done) + page_ops->page_done(iter->inode, pos, ret, &folio->page); folio_unlock(folio); if (old_size < pos) pagecache_isize_extended(iter->inode, old_size, pos); - if (page_ops && page_ops->page_done) - page_ops->page_done(iter->inode, pos, ret, &folio->page); folio_put(folio); if (ret < len)
Hi Darrick, I'd like to get the following iomap change into this merge window. This only affects gfs2, so I can push it as part of the gfs2 updates if you don't mind, provided that I'll get your Reviewed-by confirmation. Otherwise, if you'd prefer to pass this through the xfs tree, could you please take it? Thanks, Andreas -- Move the ->page_done() call in iomap_write_end() under the folio lock. This closes a race between journaled data writes and the shrinker in 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 current transaction. The folio lock prevents that from happening. The only user of ->page_done() is gfs2, so other filesystems are not affected. Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> --- fs/iomap/buffered-io.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)