* [PATCH] iomap: Move page_done callback under the folio lock @ 2022-12-13 19:48 Andreas Gruenbacher 2022-12-13 20:03 ` Darrick J. Wong 0 siblings, 1 reply; 8+ messages in thread From: Andreas Gruenbacher @ 2022-12-13 19:48 UTC (permalink / raw) To: Darrick J . Wong Cc: Andreas Gruenbacher, Christoph Hellwig, linux-xfs, linux-fsdevel 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(-) 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] iomap: Move page_done callback under the folio lock 2022-12-13 19:48 [PATCH] iomap: Move page_done callback under the folio lock Andreas Gruenbacher @ 2022-12-13 20:03 ` Darrick J. Wong 2022-12-13 20:15 ` Andreas Gruenbacher 2022-12-14 7:39 ` Christoph Hellwig 0 siblings, 2 replies; 8+ messages in thread From: Darrick J. Wong @ 2022-12-13 20:03 UTC (permalink / raw) To: Andreas Gruenbacher; +Cc: Christoph Hellwig, linux-xfs, linux-fsdevel 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iomap: Move page_done callback under the folio lock 2022-12-13 20:03 ` Darrick J. Wong @ 2022-12-13 20:15 ` Andreas Gruenbacher 2022-12-14 7:39 ` Christoph Hellwig 1 sibling, 0 replies; 8+ messages in thread From: Andreas Gruenbacher @ 2022-12-13 20:15 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs, linux-fsdevel 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 > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iomap: Move page_done callback under the folio lock 2022-12-13 20:03 ` Darrick J. Wong 2022-12-13 20:15 ` Andreas Gruenbacher @ 2022-12-14 7:39 ` Christoph Hellwig 2022-12-14 10:23 ` Andreas Gruenbacher 2022-12-14 10:24 ` [PATCH v2] " Andreas Gruenbacher 1 sibling, 2 replies; 8+ messages in thread From: Christoph Hellwig @ 2022-12-14 7:39 UTC (permalink / raw) To: Darrick J. Wong Cc: Andreas Gruenbacher, Christoph Hellwig, linux-xfs, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iomap: Move page_done callback under the folio lock 2022-12-14 7:39 ` Christoph Hellwig @ 2022-12-14 10:23 ` Andreas Gruenbacher 2022-12-14 10:24 ` [PATCH v2] " Andreas Gruenbacher 1 sibling, 0 replies; 8+ messages in thread From: Andreas Gruenbacher @ 2022-12-14 10:23 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Darrick J. Wong, linux-xfs, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] iomap: Move page_done callback under the folio lock 2022-12-14 7:39 ` Christoph Hellwig 2022-12-14 10:23 ` Andreas Gruenbacher @ 2022-12-14 10:24 ` Andreas Gruenbacher 2022-12-15 20:13 ` Andreas Gruenbacher 1 sibling, 1 reply; 8+ messages in thread From: Andreas Gruenbacher @ 2022-12-14 10:24 UTC (permalink / raw) To: Christoph Hellwig, Darrick J. Wong Cc: Andreas Gruenbacher, linux-xfs, linux-fsdevel 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 current user of ->page_done() is gfs2, so other filesystems are not affected. Still, to catch out any new users, switch from page to folio in ->page_done(). Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> --- fs/gfs2/bmap.c | 7 ++++--- fs/iomap/buffered-io.c | 4 ++-- include/linux/iomap.h | 10 +++++----- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c index e7537fd305dd..c4ee47f8e499 100644 --- a/fs/gfs2/bmap.c +++ b/fs/gfs2/bmap.c @@ -968,14 +968,15 @@ static int gfs2_iomap_page_prepare(struct inode *inode, loff_t pos, } static void gfs2_iomap_page_done(struct inode *inode, loff_t pos, - unsigned copied, struct page *page) + unsigned copied, struct folio *folio) { struct gfs2_trans *tr = current->journal_info; struct gfs2_inode *ip = GFS2_I(inode); struct gfs2_sbd *sdp = GFS2_SB(inode); - if (page && !gfs2_is_stuffed(ip)) - gfs2_page_add_databufs(ip, page, offset_in_page(pos), copied); + if (folio && !gfs2_is_stuffed(ip)) + gfs2_page_add_databufs(ip, &folio->page, offset_in_page(pos), + copied); if (tr->tr_num_buf_new) __mark_inode_dirty(inode, I_DIRTY_DATASYNC); diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 91ee0b308e13..d988c1bedf70 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); 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) diff --git a/include/linux/iomap.h b/include/linux/iomap.h index 238a03087e17..bd6d80453726 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -116,18 +116,18 @@ static inline bool iomap_inline_data_valid(const struct iomap *iomap) /* * When a filesystem sets page_ops in an iomap mapping it returns, page_prepare - * and page_done will be called for each page written to. This only applies to - * buffered writes as unbuffered writes will not typically have pages + * and page_done will be called for each folio written to. This only applies + * to buffered writes as unbuffered writes will not typically have folios * associated with them. * * When page_prepare succeeds, page_done will always be called to do any - * cleanup work necessary. In that page_done call, @page will be NULL if the - * associated page could not be obtained. + * cleanup work necessary. In that page_done call, @folio will be NULL if the + * associated folio could not be obtained. */ struct iomap_page_ops { int (*page_prepare)(struct inode *inode, loff_t pos, unsigned len); void (*page_done)(struct inode *inode, loff_t pos, unsigned copied, - struct page *page); + struct folio *folio); }; /* -- 2.38.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] iomap: Move page_done callback under the folio lock 2022-12-14 10:24 ` [PATCH v2] " Andreas Gruenbacher @ 2022-12-15 20:13 ` Andreas Gruenbacher 2022-12-16 8:22 ` Christoph Hellwig 0 siblings, 1 reply; 8+ messages in thread From: Andreas Gruenbacher @ 2022-12-15 20:13 UTC (permalink / raw) To: Christoph Hellwig, Darrick J. Wong; +Cc: linux-xfs, linux-fsdevel On Wed, Dec 14, 2022 at 11:24 AM Andreas Gruenbacher <agruenba@redhat.com> wrote: > > 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 current user of ->page_done() is gfs2, so other filesystems are > not affected. Still, to catch out any new users, switch from page to > folio in ->page_done(). > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> > --- > fs/gfs2/bmap.c | 7 ++++--- > fs/iomap/buffered-io.c | 4 ++-- > include/linux/iomap.h | 10 +++++----- > 3 files changed, 11 insertions(+), 10 deletions(-) > > diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c > index e7537fd305dd..c4ee47f8e499 100644 > --- a/fs/gfs2/bmap.c > +++ b/fs/gfs2/bmap.c > @@ -968,14 +968,15 @@ static int gfs2_iomap_page_prepare(struct inode *inode, loff_t pos, > } > > static void gfs2_iomap_page_done(struct inode *inode, loff_t pos, > - unsigned copied, struct page *page) > + unsigned copied, struct folio *folio) > { > struct gfs2_trans *tr = current->journal_info; > struct gfs2_inode *ip = GFS2_I(inode); > struct gfs2_sbd *sdp = GFS2_SB(inode); > > - if (page && !gfs2_is_stuffed(ip)) > - gfs2_page_add_databufs(ip, page, offset_in_page(pos), copied); > + if (folio && !gfs2_is_stuffed(ip)) > + gfs2_page_add_databufs(ip, &folio->page, offset_in_page(pos), > + copied); > > if (tr->tr_num_buf_new) > __mark_inode_dirty(inode, I_DIRTY_DATASYNC); This is still screwed up. We really need to unlock the page before calling into __mark_inode_dirty() and ending the transaction. The current page_done() hook would force us to then re-lock the page just so that the caller can unlock it again. This just doesn't make sense, particularly since the page_prepare and page_done hooks only exist to allow gfs2 to do data journaling via iomap. I'll follow up with a more useful approach ... > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 91ee0b308e13..d988c1bedf70 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); > 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) > diff --git a/include/linux/iomap.h b/include/linux/iomap.h > index 238a03087e17..bd6d80453726 100644 > --- a/include/linux/iomap.h > +++ b/include/linux/iomap.h > @@ -116,18 +116,18 @@ static inline bool iomap_inline_data_valid(const struct iomap *iomap) > > /* > * When a filesystem sets page_ops in an iomap mapping it returns, page_prepare > - * and page_done will be called for each page written to. This only applies to > - * buffered writes as unbuffered writes will not typically have pages > + * and page_done will be called for each folio written to. This only applies > + * to buffered writes as unbuffered writes will not typically have folios > * associated with them. > * > * When page_prepare succeeds, page_done will always be called to do any > - * cleanup work necessary. In that page_done call, @page will be NULL if the > - * associated page could not be obtained. > + * cleanup work necessary. In that page_done call, @folio will be NULL if the > + * associated folio could not be obtained. > */ > struct iomap_page_ops { > int (*page_prepare)(struct inode *inode, loff_t pos, unsigned len); > void (*page_done)(struct inode *inode, loff_t pos, unsigned copied, > - struct page *page); > + struct folio *folio); > }; > > /* > -- > 2.38.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] iomap: Move page_done callback under the folio lock 2022-12-15 20:13 ` Andreas Gruenbacher @ 2022-12-16 8:22 ` Christoph Hellwig 0 siblings, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2022-12-16 8:22 UTC (permalink / raw) To: Andreas Gruenbacher Cc: Christoph Hellwig, Darrick J. Wong, linux-xfs, linux-fsdevel On Thu, Dec 15, 2022 at 09:13:50PM +0100, Andreas Gruenbacher wrote: > This is still screwed up. We really need to unlock the page before > calling into __mark_inode_dirty() and ending the transaction. The > current page_done() hook would force us to then re-lock the page just > so that the caller can unlock it again. This just doesn't make sense, > particularly since the page_prepare and page_done hooks only exist to > allow gfs2 to do data journaling via iomap. I'll follow up with a more > useful approach ... Yes. And it would make sense to include the gfs2 patches. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-12-16 8:22 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-12-13 19:48 [PATCH] iomap: Move page_done callback under the folio lock Andreas Gruenbacher 2022-12-13 20:03 ` Darrick J. Wong 2022-12-13 20:15 ` Andreas Gruenbacher 2022-12-14 7:39 ` Christoph Hellwig 2022-12-14 10:23 ` Andreas Gruenbacher 2022-12-14 10:24 ` [PATCH v2] " Andreas Gruenbacher 2022-12-15 20:13 ` Andreas Gruenbacher 2022-12-16 8:22 ` Christoph Hellwig
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.