* [PATCH 1/6] mm/writeback: Move __set_page_dirty() to core mm
2021-06-15 16:23 [PATCH 0/6] Further set_page_dirty cleanups Matthew Wilcox (Oracle)
@ 2021-06-15 16:23 ` Matthew Wilcox (Oracle)
2021-06-15 16:35 ` Christoph Hellwig
` (2 more replies)
2021-06-15 16:23 ` [PATCH 2/6] mm/writeback: Use __set_page_dirty in __set_page_dirty_nobuffers Matthew Wilcox (Oracle)
` (4 subsequent siblings)
5 siblings, 3 replies; 27+ messages in thread
From: Matthew Wilcox (Oracle) @ 2021-06-15 16:23 UTC (permalink / raw)
To: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro,
Greg Kroah-Hartman, linux-mm, linux-fsdevel, linux-kernel
Cc: Matthew Wilcox (Oracle)
Nothing in __set_page_dirty() is specific to buffer_head, so
move it to mm/page-writeback.c. That removes the only caller of
account_page_dirtied() outside of page-writeback.c, so make it static.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
fs/buffer.c | 24 ------------------------
include/linux/mm.h | 1 -
mm/page-writeback.c | 27 ++++++++++++++++++++++++++-
3 files changed, 26 insertions(+), 26 deletions(-)
diff --git a/fs/buffer.c b/fs/buffer.c
index 8c02e44f16f2..f5384cff7e0c 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -588,30 +588,6 @@ void mark_buffer_dirty_inode(struct buffer_head *bh, struct inode *inode)
}
EXPORT_SYMBOL(mark_buffer_dirty_inode);
-/*
- * Mark the page dirty, and set it dirty in the page cache, and mark the inode
- * dirty.
- *
- * If warn is true, then emit a warning if the page is not uptodate and has
- * not been truncated.
- *
- * The caller must hold lock_page_memcg().
- */
-void __set_page_dirty(struct page *page, struct address_space *mapping,
- int warn)
-{
- unsigned long flags;
-
- xa_lock_irqsave(&mapping->i_pages, flags);
- if (page->mapping) { /* Race with truncate? */
- WARN_ON_ONCE(warn && !PageUptodate(page));
- account_page_dirtied(page, mapping);
- __xa_set_mark(&mapping->i_pages, page_index(page),
- PAGECACHE_TAG_DIRTY);
- }
- xa_unlock_irqrestore(&mapping->i_pages, flags);
-}
-
/*
* Add a page to the dirty page list.
*
diff --git a/include/linux/mm.h b/include/linux/mm.h
index d64f8a1284d9..1086b556961a 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1858,7 +1858,6 @@ int __set_page_dirty_nobuffers(struct page *page);
int __set_page_dirty_no_writeback(struct page *page);
int redirty_page_for_writepage(struct writeback_control *wbc,
struct page *page);
-void account_page_dirtied(struct page *page, struct address_space *mapping);
void account_page_cleaned(struct page *page, struct address_space *mapping,
struct bdi_writeback *wb);
int set_page_dirty(struct page *page);
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index f6ee085767cb..0c2c8355f97f 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2420,7 +2420,8 @@ int __set_page_dirty_no_writeback(struct page *page)
*
* NOTE: This relies on being atomic wrt interrupts.
*/
-void account_page_dirtied(struct page *page, struct address_space *mapping)
+static void account_page_dirtied(struct page *page,
+ struct address_space *mapping)
{
struct inode *inode = mapping->host;
@@ -2461,6 +2462,30 @@ void account_page_cleaned(struct page *page, struct address_space *mapping,
}
}
+/*
+ * Mark the page dirty, and set it dirty in the page cache, and mark the inode
+ * dirty.
+ *
+ * If warn is true, then emit a warning if the page is not uptodate and has
+ * not been truncated.
+ *
+ * The caller must hold lock_page_memcg().
+ */
+void __set_page_dirty(struct page *page, struct address_space *mapping,
+ int warn)
+{
+ unsigned long flags;
+
+ xa_lock_irqsave(&mapping->i_pages, flags);
+ if (page->mapping) { /* Race with truncate? */
+ WARN_ON_ONCE(warn && !PageUptodate(page));
+ account_page_dirtied(page, mapping);
+ __xa_set_mark(&mapping->i_pages, page_index(page),
+ PAGECACHE_TAG_DIRTY);
+ }
+ xa_unlock_irqrestore(&mapping->i_pages, flags);
+}
+
/*
* For address_spaces which do not use buffers. Just tag the page as dirty in
* the xarray.
--
2.30.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] mm/writeback: Move __set_page_dirty() to core mm
2021-06-15 16:23 ` [PATCH 1/6] mm/writeback: Move __set_page_dirty() to core mm Matthew Wilcox (Oracle)
@ 2021-06-15 16:35 ` Christoph Hellwig
2021-06-15 17:17 ` Greg Kroah-Hartman
2021-06-16 16:14 ` Matthew Wilcox
2 siblings, 0 replies; 27+ messages in thread
From: Christoph Hellwig @ 2021-06-15 16:35 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro,
Greg Kroah-Hartman, linux-mm, linux-fsdevel, linux-kernel
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] mm/writeback: Move __set_page_dirty() to core mm
2021-06-15 16:23 ` [PATCH 1/6] mm/writeback: Move __set_page_dirty() to core mm Matthew Wilcox (Oracle)
2021-06-15 16:35 ` Christoph Hellwig
@ 2021-06-15 17:17 ` Greg Kroah-Hartman
2021-06-16 16:14 ` Matthew Wilcox
2 siblings, 0 replies; 27+ messages in thread
From: Greg Kroah-Hartman @ 2021-06-15 17:17 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro, linux-mm,
linux-fsdevel, linux-kernel
On Tue, Jun 15, 2021 at 05:23:37PM +0100, Matthew Wilcox (Oracle) wrote:
> Nothing in __set_page_dirty() is specific to buffer_head, so
> move it to mm/page-writeback.c. That removes the only caller of
> account_page_dirtied() outside of page-writeback.c, so make it static.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> fs/buffer.c | 24 ------------------------
> include/linux/mm.h | 1 -
> mm/page-writeback.c | 27 ++++++++++++++++++++++++++-
> 3 files changed, 26 insertions(+), 26 deletions(-)
>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] mm/writeback: Move __set_page_dirty() to core mm
2021-06-15 16:23 ` [PATCH 1/6] mm/writeback: Move __set_page_dirty() to core mm Matthew Wilcox (Oracle)
2021-06-15 16:35 ` Christoph Hellwig
2021-06-15 17:17 ` Greg Kroah-Hartman
@ 2021-06-16 16:14 ` Matthew Wilcox
2 siblings, 0 replies; 27+ messages in thread
From: Matthew Wilcox @ 2021-06-16 16:14 UTC (permalink / raw)
To: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro,
Greg Kroah-Hartman, linux-mm, linux-fsdevel, linux-kernel
On Tue, Jun 15, 2021 at 05:23:37PM +0100, Matthew Wilcox (Oracle) wrote:
> -/*
> - * Mark the page dirty, and set it dirty in the page cache, and mark the inode
> - * dirty.
> - *
> - * If warn is true, then emit a warning if the page is not uptodate and has
> - * not been truncated.
> - *
> - * The caller must hold lock_page_memcg().
> - */
Checking against my folio tree, I found a bit of extra documentation
that I had added and didn't make it into this submission. Let me know
if it's useful and if so I can submit it as a fixup patch:
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 73b937955cc1..2072787d9b44 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2466,7 +2466,11 @@ void account_page_cleaned(struct page *page, struct addre
ss_space *mapping,
* If warn is true, then emit a warning if the page is not uptodate and has
* not been truncated.
*
- * The caller must hold lock_page_memcg().
+ * The caller must hold lock_page_memcg(). Most callers have the page
+ * locked. A few have the page blocked from truncation through other
+ * means (eg zap_page_range() has it mapped and is holding the page table
+ * lock). This can also be called from mark_buffer_dirty(), which I
+ * cannot prove is always protected against truncate.
*/
void __set_page_dirty(struct page *page, struct address_space *mapping,
int warn)
... it's a bit "notes to self", so perhaps someone can clean it up.
In particular, someone who knows the buffer code better than I do can
prove that mark_buffer_dirty() is always protected against truncate.
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 2/6] mm/writeback: Use __set_page_dirty in __set_page_dirty_nobuffers
2021-06-15 16:23 [PATCH 0/6] Further set_page_dirty cleanups Matthew Wilcox (Oracle)
2021-06-15 16:23 ` [PATCH 1/6] mm/writeback: Move __set_page_dirty() to core mm Matthew Wilcox (Oracle)
@ 2021-06-15 16:23 ` Matthew Wilcox (Oracle)
2021-06-15 16:35 ` Christoph Hellwig
2021-06-15 17:18 ` Greg Kroah-Hartman
2021-06-15 16:23 ` [PATCH 3/6] iomap: Use __set_page_dirty_nobuffers Matthew Wilcox (Oracle)
` (3 subsequent siblings)
5 siblings, 2 replies; 27+ messages in thread
From: Matthew Wilcox (Oracle) @ 2021-06-15 16:23 UTC (permalink / raw)
To: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro,
Greg Kroah-Hartman, linux-mm, linux-fsdevel, linux-kernel
Cc: Matthew Wilcox (Oracle)
This is fundamentally the same code, so just call it instead of
duplicating it.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
mm/page-writeback.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 0c2c8355f97f..980a6cb9cbd9 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2503,20 +2503,12 @@ int __set_page_dirty_nobuffers(struct page *page)
lock_page_memcg(page);
if (!TestSetPageDirty(page)) {
struct address_space *mapping = page_mapping(page);
- unsigned long flags;
if (!mapping) {
unlock_page_memcg(page);
return 1;
}
-
- xa_lock_irqsave(&mapping->i_pages, flags);
- BUG_ON(page_mapping(page) != mapping);
- WARN_ON_ONCE(!PagePrivate(page) && !PageUptodate(page));
- account_page_dirtied(page, mapping);
- __xa_set_mark(&mapping->i_pages, page_index(page),
- PAGECACHE_TAG_DIRTY);
- xa_unlock_irqrestore(&mapping->i_pages, flags);
+ __set_page_dirty(page, mapping, !PagePrivate(page));
unlock_page_memcg(page);
if (mapping->host) {
--
2.30.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 2/6] mm/writeback: Use __set_page_dirty in __set_page_dirty_nobuffers
2021-06-15 16:23 ` [PATCH 2/6] mm/writeback: Use __set_page_dirty in __set_page_dirty_nobuffers Matthew Wilcox (Oracle)
@ 2021-06-15 16:35 ` Christoph Hellwig
2021-06-15 17:18 ` Greg Kroah-Hartman
1 sibling, 0 replies; 27+ messages in thread
From: Christoph Hellwig @ 2021-06-15 16:35 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro,
Greg Kroah-Hartman, linux-mm, linux-fsdevel, linux-kernel
On Tue, Jun 15, 2021 at 05:23:38PM +0100, Matthew Wilcox (Oracle) wrote:
> This is fundamentally the same code, so just call it instead of
> duplicating it.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/6] mm/writeback: Use __set_page_dirty in __set_page_dirty_nobuffers
2021-06-15 16:23 ` [PATCH 2/6] mm/writeback: Use __set_page_dirty in __set_page_dirty_nobuffers Matthew Wilcox (Oracle)
2021-06-15 16:35 ` Christoph Hellwig
@ 2021-06-15 17:18 ` Greg Kroah-Hartman
1 sibling, 0 replies; 27+ messages in thread
From: Greg Kroah-Hartman @ 2021-06-15 17:18 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro, linux-mm,
linux-fsdevel, linux-kernel
On Tue, Jun 15, 2021 at 05:23:38PM +0100, Matthew Wilcox (Oracle) wrote:
> This is fundamentally the same code, so just call it instead of
> duplicating it.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> mm/page-writeback.c | 10 +---------
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 3/6] iomap: Use __set_page_dirty_nobuffers
2021-06-15 16:23 [PATCH 0/6] Further set_page_dirty cleanups Matthew Wilcox (Oracle)
2021-06-15 16:23 ` [PATCH 1/6] mm/writeback: Move __set_page_dirty() to core mm Matthew Wilcox (Oracle)
2021-06-15 16:23 ` [PATCH 2/6] mm/writeback: Use __set_page_dirty in __set_page_dirty_nobuffers Matthew Wilcox (Oracle)
@ 2021-06-15 16:23 ` Matthew Wilcox (Oracle)
2021-06-15 16:37 ` Christoph Hellwig
2021-06-15 17:19 ` Greg Kroah-Hartman
2021-06-15 16:23 ` [PATCH 4/6] fs: Remove anon_set_page_dirty() Matthew Wilcox (Oracle)
` (2 subsequent siblings)
5 siblings, 2 replies; 27+ messages in thread
From: Matthew Wilcox (Oracle) @ 2021-06-15 16:23 UTC (permalink / raw)
To: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro,
Greg Kroah-Hartman, linux-mm, linux-fsdevel, linux-kernel
Cc: Matthew Wilcox (Oracle)
The only difference between iomap_set_page_dirty() and
__set_page_dirty_nobuffers() is that the latter includes a debugging
check that a !Uptodate page has private data.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
fs/gfs2/aops.c | 2 +-
fs/iomap/buffered-io.c | 27 +--------------------------
fs/xfs/xfs_aops.c | 2 +-
fs/zonefs/super.c | 2 +-
include/linux/iomap.h | 1 -
5 files changed, 4 insertions(+), 30 deletions(-)
diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 50dd1771d00c..746b78c3a91d 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -784,7 +784,7 @@ static const struct address_space_operations gfs2_aops = {
.writepages = gfs2_writepages,
.readpage = gfs2_readpage,
.readahead = gfs2_readahead,
- .set_page_dirty = iomap_set_page_dirty,
+ .set_page_dirty = __set_page_dirty_nobuffers,
.releasepage = iomap_releasepage,
.invalidatepage = iomap_invalidatepage,
.bmap = gfs2_bmap,
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 2bf4778f2098..41da4f14c00b 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -640,31 +640,6 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
return status;
}
-int
-iomap_set_page_dirty(struct page *page)
-{
- struct address_space *mapping = page_mapping(page);
- int newly_dirty;
-
- if (unlikely(!mapping))
- return !TestSetPageDirty(page);
-
- /*
- * Lock out page's memcg migration to keep PageDirty
- * synchronized with per-memcg dirty page counters.
- */
- lock_page_memcg(page);
- newly_dirty = !TestSetPageDirty(page);
- if (newly_dirty)
- __set_page_dirty(page, mapping, 0);
- unlock_page_memcg(page);
-
- if (newly_dirty)
- __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
- return newly_dirty;
-}
-EXPORT_SYMBOL_GPL(iomap_set_page_dirty);
-
static size_t __iomap_write_end(struct inode *inode, loff_t pos, size_t len,
size_t copied, struct page *page)
{
@@ -684,7 +659,7 @@ static size_t __iomap_write_end(struct inode *inode, loff_t pos, size_t len,
if (unlikely(copied < len && !PageUptodate(page)))
return 0;
iomap_set_range_uptodate(page, offset_in_page(pos), len);
- iomap_set_page_dirty(page);
+ __set_page_dirty_nobuffers(page);
return copied;
}
diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 826caa6b4a5a..a335d79dcff8 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -561,7 +561,7 @@ const struct address_space_operations xfs_address_space_operations = {
.readahead = xfs_vm_readahead,
.writepage = xfs_vm_writepage,
.writepages = xfs_vm_writepages,
- .set_page_dirty = iomap_set_page_dirty,
+ .set_page_dirty = __set_page_dirty_nobuffers,
.releasepage = iomap_releasepage,
.invalidatepage = iomap_invalidatepage,
.bmap = xfs_vm_bmap,
diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
index cd145d318b17..3aacf016c7c2 100644
--- a/fs/zonefs/super.c
+++ b/fs/zonefs/super.c
@@ -185,7 +185,7 @@ static const struct address_space_operations zonefs_file_aops = {
.readahead = zonefs_readahead,
.writepage = zonefs_writepage,
.writepages = zonefs_writepages,
- .set_page_dirty = iomap_set_page_dirty,
+ .set_page_dirty = __set_page_dirty_nobuffers,
.releasepage = iomap_releasepage,
.invalidatepage = iomap_invalidatepage,
.migratepage = iomap_migrate_page,
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index c87d0cb0de6d..479c1da3e221 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -159,7 +159,6 @@ ssize_t iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *from,
const struct iomap_ops *ops);
int iomap_readpage(struct page *page, const struct iomap_ops *ops);
void iomap_readahead(struct readahead_control *, const struct iomap_ops *ops);
-int iomap_set_page_dirty(struct page *page);
int iomap_is_partially_uptodate(struct page *page, unsigned long from,
unsigned long count);
int iomap_releasepage(struct page *page, gfp_t gfp_mask);
--
2.30.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 3/6] iomap: Use __set_page_dirty_nobuffers
2021-06-15 16:23 ` [PATCH 3/6] iomap: Use __set_page_dirty_nobuffers Matthew Wilcox (Oracle)
@ 2021-06-15 16:37 ` Christoph Hellwig
2021-06-15 17:19 ` Greg Kroah-Hartman
1 sibling, 0 replies; 27+ messages in thread
From: Christoph Hellwig @ 2021-06-15 16:37 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro,
Greg Kroah-Hartman, linux-mm, linux-fsdevel, linux-kernel
On Tue, Jun 15, 2021 at 05:23:39PM +0100, Matthew Wilcox (Oracle) wrote:
> The only difference between iomap_set_page_dirty() and
> __set_page_dirty_nobuffers() is that the latter includes a debugging
> check that a !Uptodate page has private data.
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/6] iomap: Use __set_page_dirty_nobuffers
2021-06-15 16:23 ` [PATCH 3/6] iomap: Use __set_page_dirty_nobuffers Matthew Wilcox (Oracle)
2021-06-15 16:37 ` Christoph Hellwig
@ 2021-06-15 17:19 ` Greg Kroah-Hartman
2021-06-15 17:32 ` Matthew Wilcox
1 sibling, 1 reply; 27+ messages in thread
From: Greg Kroah-Hartman @ 2021-06-15 17:19 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro, linux-mm,
linux-fsdevel, linux-kernel
On Tue, Jun 15, 2021 at 05:23:39PM +0100, Matthew Wilcox (Oracle) wrote:
> The only difference between iomap_set_page_dirty() and
> __set_page_dirty_nobuffers() is that the latter includes a debugging
> check that a !Uptodate page has private data.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> fs/gfs2/aops.c | 2 +-
> fs/iomap/buffered-io.c | 27 +--------------------------
> fs/xfs/xfs_aops.c | 2 +-
> fs/zonefs/super.c | 2 +-
> include/linux/iomap.h | 1 -
> 5 files changed, 4 insertions(+), 30 deletions(-)
>
> diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
> index 50dd1771d00c..746b78c3a91d 100644
> --- a/fs/gfs2/aops.c
> +++ b/fs/gfs2/aops.c
> @@ -784,7 +784,7 @@ static const struct address_space_operations gfs2_aops = {
> .writepages = gfs2_writepages,
> .readpage = gfs2_readpage,
> .readahead = gfs2_readahead,
> - .set_page_dirty = iomap_set_page_dirty,
> + .set_page_dirty = __set_page_dirty_nobuffers,
Using __ functions in structures in different modules feels odd to me.
Why not just have iomap_set_page_dirty be a #define to this function now
if you want to do this?
Or take the __ off of the function name?
Anyway, logic here is fine, but feels odd.
greg k-h
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/6] iomap: Use __set_page_dirty_nobuffers
2021-06-15 17:19 ` Greg Kroah-Hartman
@ 2021-06-15 17:32 ` Matthew Wilcox
2021-06-15 17:34 ` Christoph Hellwig
0 siblings, 1 reply; 27+ messages in thread
From: Matthew Wilcox @ 2021-06-15 17:32 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro, linux-mm,
linux-fsdevel, linux-kernel
On Tue, Jun 15, 2021 at 07:19:59PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 15, 2021 at 05:23:39PM +0100, Matthew Wilcox (Oracle) wrote:
> Using __ functions in structures in different modules feels odd to me.
> Why not just have iomap_set_page_dirty be a #define to this function now
> if you want to do this?
>
> Or take the __ off of the function name?
>
> Anyway, logic here is fine, but feels odd.
heh, that was how I did it the first time. Then I thought that it was
better to follow Christoph's patch:
static const struct address_space_operations adfs_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
(etc)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/6] iomap: Use __set_page_dirty_nobuffers
2021-06-15 17:32 ` Matthew Wilcox
@ 2021-06-15 17:34 ` Christoph Hellwig
2021-06-15 18:04 ` Greg Kroah-Hartman
2021-06-15 18:13 ` Matthew Wilcox
0 siblings, 2 replies; 27+ messages in thread
From: Christoph Hellwig @ 2021-06-15 17:34 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Greg Kroah-Hartman, Christoph Hellwig, Andrew Morton, Jan Kara,
Al Viro, linux-mm, linux-fsdevel, linux-kernel
On Tue, Jun 15, 2021 at 06:32:37PM +0100, Matthew Wilcox wrote:
> On Tue, Jun 15, 2021 at 07:19:59PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jun 15, 2021 at 05:23:39PM +0100, Matthew Wilcox (Oracle) wrote:
> > Using __ functions in structures in different modules feels odd to me.
> > Why not just have iomap_set_page_dirty be a #define to this function now
> > if you want to do this?
> >
> > Or take the __ off of the function name?
> >
> > Anyway, logic here is fine, but feels odd.
>
> heh, that was how I did it the first time. Then I thought that it was
> better to follow Christoph's patch:
>
> static const struct address_space_operations adfs_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> (etc)
Eventually everything around set_page_dirty should be changed to operate
on folios, and that will be a good time to come up with a sane
naming scheme without introducing extra churn.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/6] iomap: Use __set_page_dirty_nobuffers
2021-06-15 17:34 ` Christoph Hellwig
@ 2021-06-15 18:04 ` Greg Kroah-Hartman
2021-06-15 18:13 ` Matthew Wilcox
1 sibling, 0 replies; 27+ messages in thread
From: Greg Kroah-Hartman @ 2021-06-15 18:04 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Matthew Wilcox, Andrew Morton, Jan Kara, Al Viro, linux-mm,
linux-fsdevel, linux-kernel
On Tue, Jun 15, 2021 at 07:34:53PM +0200, Christoph Hellwig wrote:
> On Tue, Jun 15, 2021 at 06:32:37PM +0100, Matthew Wilcox wrote:
> > On Tue, Jun 15, 2021 at 07:19:59PM +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Jun 15, 2021 at 05:23:39PM +0100, Matthew Wilcox (Oracle) wrote:
> > > Using __ functions in structures in different modules feels odd to me.
> > > Why not just have iomap_set_page_dirty be a #define to this function now
> > > if you want to do this?
> > >
> > > Or take the __ off of the function name?
> > >
> > > Anyway, logic here is fine, but feels odd.
> >
> > heh, that was how I did it the first time. Then I thought that it was
> > better to follow Christoph's patch:
> >
> > static const struct address_space_operations adfs_aops = {
> > + .set_page_dirty = __set_page_dirty_buffers,
> > (etc)
>
> Eventually everything around set_page_dirty should be changed to operate
> on folios, and that will be a good time to come up with a sane
> naming scheme without introducing extra churn.
Ok, that's fine, I don't normally touch these files, so it's not an
issue for me :)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/6] iomap: Use __set_page_dirty_nobuffers
2021-06-15 17:34 ` Christoph Hellwig
2021-06-15 18:04 ` Greg Kroah-Hartman
@ 2021-06-15 18:13 ` Matthew Wilcox
2021-06-16 6:50 ` Greg Kroah-Hartman
1 sibling, 1 reply; 27+ messages in thread
From: Matthew Wilcox @ 2021-06-15 18:13 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Greg Kroah-Hartman, Andrew Morton, Jan Kara, Al Viro, linux-mm,
linux-fsdevel, linux-kernel
On Tue, Jun 15, 2021 at 07:34:53PM +0200, Christoph Hellwig wrote:
> On Tue, Jun 15, 2021 at 06:32:37PM +0100, Matthew Wilcox wrote:
> > On Tue, Jun 15, 2021 at 07:19:59PM +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Jun 15, 2021 at 05:23:39PM +0100, Matthew Wilcox (Oracle) wrote:
> > > Using __ functions in structures in different modules feels odd to me.
> > > Why not just have iomap_set_page_dirty be a #define to this function now
> > > if you want to do this?
> > >
> > > Or take the __ off of the function name?
> > >
> > > Anyway, logic here is fine, but feels odd.
> >
> > heh, that was how I did it the first time. Then I thought that it was
> > better to follow Christoph's patch:
> >
> > static const struct address_space_operations adfs_aops = {
> > + .set_page_dirty = __set_page_dirty_buffers,
> > (etc)
>
> Eventually everything around set_page_dirty should be changed to operate
> on folios, and that will be a good time to come up with a sane
> naming scheme without introducing extra churn.
The way it currently looks in my tree ...
set_page_dirty(page) is a thin wrapper that calls folio_mark_dirty(folio).
folio_mark_dirty() calls a_ops->dirty_folio(mapping, folio) (which
returns bool).
__set_page_dirty_nobuffers() becomes filemap_dirty_folio()
__set_page_dirty_buffers() becomes block_dirty_folio()
__set_page_dirty_no_writeback() becomes dirty_folio_no_writeback()
Now I look at it, maybe that last should be nowb_dirty_folio().
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/6] iomap: Use __set_page_dirty_nobuffers
2021-06-15 18:13 ` Matthew Wilcox
@ 2021-06-16 6:50 ` Greg Kroah-Hartman
2021-06-16 16:28 ` Matthew Wilcox
0 siblings, 1 reply; 27+ messages in thread
From: Greg Kroah-Hartman @ 2021-06-16 6:50 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro, linux-mm,
linux-fsdevel, linux-kernel
On Tue, Jun 15, 2021 at 07:13:16PM +0100, Matthew Wilcox wrote:
> On Tue, Jun 15, 2021 at 07:34:53PM +0200, Christoph Hellwig wrote:
> > On Tue, Jun 15, 2021 at 06:32:37PM +0100, Matthew Wilcox wrote:
> > > On Tue, Jun 15, 2021 at 07:19:59PM +0200, Greg Kroah-Hartman wrote:
> > > > On Tue, Jun 15, 2021 at 05:23:39PM +0100, Matthew Wilcox (Oracle) wrote:
> > > > Using __ functions in structures in different modules feels odd to me.
> > > > Why not just have iomap_set_page_dirty be a #define to this function now
> > > > if you want to do this?
> > > >
> > > > Or take the __ off of the function name?
> > > >
> > > > Anyway, logic here is fine, but feels odd.
> > >
> > > heh, that was how I did it the first time. Then I thought that it was
> > > better to follow Christoph's patch:
> > >
> > > static const struct address_space_operations adfs_aops = {
> > > + .set_page_dirty = __set_page_dirty_buffers,
> > > (etc)
> >
> > Eventually everything around set_page_dirty should be changed to operate
> > on folios, and that will be a good time to come up with a sane
> > naming scheme without introducing extra churn.
>
> The way it currently looks in my tree ...
>
> set_page_dirty(page) is a thin wrapper that calls folio_mark_dirty(folio).
> folio_mark_dirty() calls a_ops->dirty_folio(mapping, folio) (which
> returns bool).
> __set_page_dirty_nobuffers() becomes filemap_dirty_folio()
> __set_page_dirty_buffers() becomes block_dirty_folio()
> __set_page_dirty_no_writeback() becomes dirty_folio_no_writeback()
>
> Now I look at it, maybe that last should be nowb_dirty_folio().
Not to be a pain, but you are mixing "folio" at the front and back of
the api name? We messed up in the driver core with this for some things
(get_device() being one), I would recommend just sticking with one
naming scheme now as you are getting to pick what you want to use.
So perhaps for the above:
folio_mark_dirty()
folio_dirty_no_writeback()
folio_dirty_filemap()
folio_dirty_block()
much like "set_page" is used today?
Anyway, just bikeshedding, it's your code, your choice :)
thanks for doing this work overall.
greg k-h
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/6] iomap: Use __set_page_dirty_nobuffers
2021-06-16 6:50 ` Greg Kroah-Hartman
@ 2021-06-16 16:28 ` Matthew Wilcox
2021-06-16 16:35 ` Greg Kroah-Hartman
0 siblings, 1 reply; 27+ messages in thread
From: Matthew Wilcox @ 2021-06-16 16:28 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro, linux-mm,
linux-fsdevel, linux-kernel
On Wed, Jun 16, 2021 at 08:50:40AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 15, 2021 at 07:13:16PM +0100, Matthew Wilcox wrote:
> > On Tue, Jun 15, 2021 at 07:34:53PM +0200, Christoph Hellwig wrote:
> > > Eventually everything around set_page_dirty should be changed to operate
> > > on folios, and that will be a good time to come up with a sane
> > > naming scheme without introducing extra churn.
> >
> > The way it currently looks in my tree ...
> >
> > set_page_dirty(page) is a thin wrapper that calls folio_mark_dirty(folio).
> > folio_mark_dirty() calls a_ops->dirty_folio(mapping, folio) (which
> > returns bool).
> > __set_page_dirty_nobuffers() becomes filemap_dirty_folio()
> > __set_page_dirty_buffers() becomes block_dirty_folio()
> > __set_page_dirty_no_writeback() becomes dirty_folio_no_writeback()
> >
> > Now I look at it, maybe that last should be nowb_dirty_folio().
>
> Not to be a pain, but you are mixing "folio" at the front and back of
> the api name? We messed up in the driver core with this for some things
> (get_device() being one), I would recommend just sticking with one
> naming scheme now as you are getting to pick what you want to use.
That is mostly what I'm doing. eg,
get_page -> folio_get
lock_page -> folio_lock
PageUptodate -> folio_uptodate
set_page_dirty -> folio_mark_dirty
What I haven't dealt with yet is the naming of the
address_space_operations. My thinking with those is that they should
be verb_folio, since they _aren't_ the functions that get called.
ie it looks like this:
folio_mark_dirty()
aops->dirty_folio()
ext4_dirty_folio()
buffer_dirty_folio()
I actually see the inconsistency here as a good thing -- these are
implementations of the aop, so foo_verb_folio() means you're doing
something weird and internal instead of going through the vfs/mm.
That implies doing things like renaming ->readpage to ->read_folio, but
if we're changing the API from passing a struct page to a struct folio,
that can all be done at the same time with no additional disruption.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/6] iomap: Use __set_page_dirty_nobuffers
2021-06-16 16:28 ` Matthew Wilcox
@ 2021-06-16 16:35 ` Greg Kroah-Hartman
0 siblings, 0 replies; 27+ messages in thread
From: Greg Kroah-Hartman @ 2021-06-16 16:35 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro, linux-mm,
linux-fsdevel, linux-kernel
On Wed, Jun 16, 2021 at 05:28:14PM +0100, Matthew Wilcox wrote:
> On Wed, Jun 16, 2021 at 08:50:40AM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jun 15, 2021 at 07:13:16PM +0100, Matthew Wilcox wrote:
> > > On Tue, Jun 15, 2021 at 07:34:53PM +0200, Christoph Hellwig wrote:
> > > > Eventually everything around set_page_dirty should be changed to operate
> > > > on folios, and that will be a good time to come up with a sane
> > > > naming scheme without introducing extra churn.
> > >
> > > The way it currently looks in my tree ...
> > >
> > > set_page_dirty(page) is a thin wrapper that calls folio_mark_dirty(folio).
> > > folio_mark_dirty() calls a_ops->dirty_folio(mapping, folio) (which
> > > returns bool).
> > > __set_page_dirty_nobuffers() becomes filemap_dirty_folio()
> > > __set_page_dirty_buffers() becomes block_dirty_folio()
> > > __set_page_dirty_no_writeback() becomes dirty_folio_no_writeback()
> > >
> > > Now I look at it, maybe that last should be nowb_dirty_folio().
> >
> > Not to be a pain, but you are mixing "folio" at the front and back of
> > the api name? We messed up in the driver core with this for some things
> > (get_device() being one), I would recommend just sticking with one
> > naming scheme now as you are getting to pick what you want to use.
>
> That is mostly what I'm doing. eg,
>
> get_page -> folio_get
> lock_page -> folio_lock
> PageUptodate -> folio_uptodate
> set_page_dirty -> folio_mark_dirty
Nice.
> What I haven't dealt with yet is the naming of the
> address_space_operations. My thinking with those is that they should
> be verb_folio, since they _aren't_ the functions that get called.
> ie it looks like this:
>
> folio_mark_dirty()
> aops->dirty_folio()
> ext4_dirty_folio()
> buffer_dirty_folio()
>
> I actually see the inconsistency here as a good thing -- these are
> implementations of the aop, so foo_verb_folio() means you're doing
> something weird and internal instead of going through the vfs/mm.
>
> That implies doing things like renaming ->readpage to ->read_folio, but
> if we're changing the API from passing a struct page to a struct folio,
> that can all be done at the same time with no additional disruption.
Ok, as long as there's a reason for the naming scheme, I'm happy as
hopefully it will make sense to others as well.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 4/6] fs: Remove anon_set_page_dirty()
2021-06-15 16:23 [PATCH 0/6] Further set_page_dirty cleanups Matthew Wilcox (Oracle)
` (2 preceding siblings ...)
2021-06-15 16:23 ` [PATCH 3/6] iomap: Use __set_page_dirty_nobuffers Matthew Wilcox (Oracle)
@ 2021-06-15 16:23 ` Matthew Wilcox (Oracle)
2021-06-15 16:37 ` Christoph Hellwig
2021-06-15 16:23 ` [PATCH 5/6] fs: Remove noop_set_page_dirty() Matthew Wilcox (Oracle)
2021-06-15 16:23 ` [PATCH 6/6] mm: Move page dirtying prototypes from mm.h Matthew Wilcox (Oracle)
5 siblings, 1 reply; 27+ messages in thread
From: Matthew Wilcox (Oracle) @ 2021-06-15 16:23 UTC (permalink / raw)
To: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro,
Greg Kroah-Hartman, linux-mm, linux-fsdevel, linux-kernel
Cc: Matthew Wilcox (Oracle)
Use __set_page_dirty_no_writeback() instead. This will set the dirty
bit on the page, which will be used to avoid calling set_page_dirty()
in the future. It will have no effect on actually writing the page
back, as the pages are not on any LRU lists.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
fs/libfs.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/fs/libfs.c b/fs/libfs.c
index 2d7f086b93d6..3fdd89b156d6 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1217,19 +1217,10 @@ void kfree_link(void *p)
}
EXPORT_SYMBOL(kfree_link);
-/*
- * nop .set_page_dirty method so that people can use .page_mkwrite on
- * anon inodes.
- */
-static int anon_set_page_dirty(struct page *page)
-{
- return 0;
-};
-
struct inode *alloc_anon_inode(struct super_block *s)
{
static const struct address_space_operations anon_aops = {
- .set_page_dirty = anon_set_page_dirty,
+ .set_page_dirty = __set_page_dirty_no_writeback,
};
struct inode *inode = new_inode_pseudo(s);
--
2.30.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 4/6] fs: Remove anon_set_page_dirty()
2021-06-15 16:23 ` [PATCH 4/6] fs: Remove anon_set_page_dirty() Matthew Wilcox (Oracle)
@ 2021-06-15 16:37 ` Christoph Hellwig
0 siblings, 0 replies; 27+ messages in thread
From: Christoph Hellwig @ 2021-06-15 16:37 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro,
Greg Kroah-Hartman, linux-mm, linux-fsdevel, linux-kernel
On Tue, Jun 15, 2021 at 05:23:40PM +0100, Matthew Wilcox (Oracle) wrote:
> Use __set_page_dirty_no_writeback() instead. This will set the dirty
> bit on the page, which will be used to avoid calling set_page_dirty()
> in the future. It will have no effect on actually writing the page
> back, as the pages are not on any LRU lists.
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 5/6] fs: Remove noop_set_page_dirty()
2021-06-15 16:23 [PATCH 0/6] Further set_page_dirty cleanups Matthew Wilcox (Oracle)
` (3 preceding siblings ...)
2021-06-15 16:23 ` [PATCH 4/6] fs: Remove anon_set_page_dirty() Matthew Wilcox (Oracle)
@ 2021-06-15 16:23 ` Matthew Wilcox (Oracle)
2021-06-15 16:38 ` Christoph Hellwig
` (2 more replies)
2021-06-15 16:23 ` [PATCH 6/6] mm: Move page dirtying prototypes from mm.h Matthew Wilcox (Oracle)
5 siblings, 3 replies; 27+ messages in thread
From: Matthew Wilcox (Oracle) @ 2021-06-15 16:23 UTC (permalink / raw)
To: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro,
Greg Kroah-Hartman, linux-mm, linux-fsdevel, linux-kernel
Cc: Matthew Wilcox (Oracle)
Use __set_page_dirty_no_writeback() instead. This will set the dirty
bit on the page, which will be used to avoid calling set_page_dirty()
in the future. It will have no effect on actually writing the page
back, as the pages are not on any LRU lists.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
drivers/dax/device.c | 2 +-
fs/ext2/inode.c | 2 +-
fs/ext4/inode.c | 2 +-
fs/fuse/dax.c | 2 +-
fs/libfs.c | 16 ----------------
fs/xfs/xfs_aops.c | 2 +-
include/linux/fs.h | 1 -
7 files changed, 5 insertions(+), 22 deletions(-)
diff --git a/drivers/dax/device.c b/drivers/dax/device.c
index db92573c94e8..dd8222a42808 100644
--- a/drivers/dax/device.c
+++ b/drivers/dax/device.c
@@ -337,7 +337,7 @@ static unsigned long dax_get_unmapped_area(struct file *filp,
}
static const struct address_space_operations dev_dax_aops = {
- .set_page_dirty = noop_set_page_dirty,
+ .set_page_dirty = __set_page_dirty_no_writeback,
.invalidatepage = noop_invalidatepage,
};
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index bf41f579ed3e..dadb121beb22 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -992,7 +992,7 @@ const struct address_space_operations ext2_nobh_aops = {
static const struct address_space_operations ext2_dax_aops = {
.writepages = ext2_dax_writepages,
.direct_IO = noop_direct_IO,
- .set_page_dirty = noop_set_page_dirty,
+ .set_page_dirty = __set_page_dirty_no_writeback,
.invalidatepage = noop_invalidatepage,
};
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index f44800361a38..6dd58c14ef1f 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3701,7 +3701,7 @@ static const struct address_space_operations ext4_da_aops = {
static const struct address_space_operations ext4_dax_aops = {
.writepages = ext4_dax_writepages,
.direct_IO = noop_direct_IO,
- .set_page_dirty = noop_set_page_dirty,
+ .set_page_dirty = __set_page_dirty_no_writeback,
.bmap = ext4_bmap,
.invalidatepage = noop_invalidatepage,
.swap_activate = ext4_iomap_swap_activate,
diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
index ff99ab2a3c43..515ad0895345 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -1329,7 +1329,7 @@ bool fuse_dax_inode_alloc(struct super_block *sb, struct fuse_inode *fi)
static const struct address_space_operations fuse_dax_file_aops = {
.writepages = fuse_dax_writepages,
.direct_IO = noop_direct_IO,
- .set_page_dirty = noop_set_page_dirty,
+ .set_page_dirty = __set_page_dirty_no_writeback,
.invalidatepage = noop_invalidatepage,
};
diff --git a/fs/libfs.c b/fs/libfs.c
index 3fdd89b156d6..51b4de3b3447 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1171,22 +1171,6 @@ int noop_fsync(struct file *file, loff_t start, loff_t end, int datasync)
}
EXPORT_SYMBOL(noop_fsync);
-int noop_set_page_dirty(struct page *page)
-{
- /*
- * Unlike __set_page_dirty_no_writeback that handles dirty page
- * tracking in the page object, dax does all dirty tracking in
- * the inode address_space in response to mkwrite faults. In the
- * dax case we only need to worry about potentially dirty CPU
- * caches, not dirty page cache pages to write back.
- *
- * This callback is defined to prevent fallback to
- * __set_page_dirty_buffers() in set_page_dirty().
- */
- return 0;
-}
-EXPORT_SYMBOL_GPL(noop_set_page_dirty);
-
void noop_invalidatepage(struct page *page, unsigned int offset,
unsigned int length)
{
diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index a335d79dcff8..cb4e0fcf4c76 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -575,7 +575,7 @@ const struct address_space_operations xfs_address_space_operations = {
const struct address_space_operations xfs_dax_aops = {
.writepages = xfs_dax_writepages,
.direct_IO = noop_direct_IO,
- .set_page_dirty = noop_set_page_dirty,
+ .set_page_dirty = __set_page_dirty_no_writeback,
.invalidatepage = noop_invalidatepage,
.swap_activate = xfs_iomap_swapfile_activate,
};
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ef5c1d93994c..9dfd1e15b0e1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3416,7 +3416,6 @@ extern int simple_rename(struct user_namespace *, struct inode *,
extern void simple_recursive_removal(struct dentry *,
void (*callback)(struct dentry *));
extern int noop_fsync(struct file *, loff_t, loff_t, int);
-extern int noop_set_page_dirty(struct page *page);
extern void noop_invalidatepage(struct page *page, unsigned int offset,
unsigned int length);
extern ssize_t noop_direct_IO(struct kiocb *iocb, struct iov_iter *iter);
--
2.30.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 5/6] fs: Remove noop_set_page_dirty()
2021-06-15 16:23 ` [PATCH 5/6] fs: Remove noop_set_page_dirty() Matthew Wilcox (Oracle)
@ 2021-06-15 16:38 ` Christoph Hellwig
2021-06-16 18:10 ` kernel test robot
2021-06-16 22:33 ` kernel test robot
2 siblings, 0 replies; 27+ messages in thread
From: Christoph Hellwig @ 2021-06-15 16:38 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro,
Greg Kroah-Hartman, linux-mm, linux-fsdevel, linux-kernel
On Tue, Jun 15, 2021 at 05:23:41PM +0100, Matthew Wilcox (Oracle) wrote:
> Use __set_page_dirty_no_writeback() instead. This will set the dirty
> bit on the page, which will be used to avoid calling set_page_dirty()
> in the future. It will have no effect on actually writing the page
> back, as the pages are not on any LRU lists.
Looks sane to me, but I'd really like to have Dan to look over this..
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 5/6] fs: Remove noop_set_page_dirty()
2021-06-15 16:23 ` [PATCH 5/6] fs: Remove noop_set_page_dirty() Matthew Wilcox (Oracle)
2021-06-15 16:38 ` Christoph Hellwig
@ 2021-06-16 18:10 ` kernel test robot
2021-06-16 18:12 ` Matthew Wilcox
2021-06-16 22:33 ` kernel test robot
2 siblings, 1 reply; 27+ messages in thread
From: kernel test robot @ 2021-06-16 18:10 UTC (permalink / raw)
To: Matthew Wilcox (Oracle),
Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro,
Greg Kroah-Hartman, linux-fsdevel, linux-kernel
Cc: kbuild-all, Linux Memory Management List, Matthew Wilcox (Oracle)
[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]
Hi "Matthew,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on next-20210615]
[cannot apply to hnaz-linux-mm/master xfs-linux/for-next fuse/for-next linus/master v5.13-rc6 v5.13-rc5 v5.13-rc4 v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Matthew-Wilcox-Oracle/Further-set_page_dirty-cleanups/20210616-205504
base: 19ae1f2bd9c091059f80646604ccef8a1e614f57
config: m68k-defconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/9367b704615caea2752bf1ec461f19e891936c55
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Matthew-Wilcox-Oracle/Further-set_page_dirty-cleanups/20210616-205504
git checkout 9367b704615caea2752bf1ec461f19e891936c55
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "__set_page_dirty_no_writeback" [fs/xfs/xfs.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 17164 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 5/6] fs: Remove noop_set_page_dirty()
2021-06-16 18:10 ` kernel test robot
@ 2021-06-16 18:12 ` Matthew Wilcox
0 siblings, 0 replies; 27+ messages in thread
From: Matthew Wilcox @ 2021-06-16 18:12 UTC (permalink / raw)
To: kernel test robot
Cc: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro,
Greg Kroah-Hartman, linux-fsdevel, linux-kernel, kbuild-all,
Linux Memory Management List
On Thu, Jun 17, 2021 at 02:10:04AM +0800, kernel test robot wrote:
> >> ERROR: modpost: "__set_page_dirty_no_writeback" [fs/xfs/xfs.ko] undefined!
Thanks; Andrew already fixed this one.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 5/6] fs: Remove noop_set_page_dirty()
2021-06-15 16:23 ` [PATCH 5/6] fs: Remove noop_set_page_dirty() Matthew Wilcox (Oracle)
2021-06-15 16:38 ` Christoph Hellwig
2021-06-16 18:10 ` kernel test robot
@ 2021-06-16 22:33 ` kernel test robot
2 siblings, 0 replies; 27+ messages in thread
From: kernel test robot @ 2021-06-16 22:33 UTC (permalink / raw)
To: Matthew Wilcox (Oracle),
Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro,
Greg Kroah-Hartman, linux-fsdevel, linux-kernel
Cc: kbuild-all, Linux Memory Management List, Matthew Wilcox (Oracle)
[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]
Hi "Matthew,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on next-20210615]
[cannot apply to hnaz-linux-mm/master xfs-linux/for-next fuse/for-next linus/master v5.13-rc6 v5.13-rc5 v5.13-rc4 v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Matthew-Wilcox-Oracle/Further-set_page_dirty-cleanups/20210616-205504
base: 19ae1f2bd9c091059f80646604ccef8a1e614f57
config: i386-randconfig-a015-20210615 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/9367b704615caea2752bf1ec461f19e891936c55
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Matthew-Wilcox-Oracle/Further-set_page_dirty-cleanups/20210616-205504
git checkout 9367b704615caea2752bf1ec461f19e891936c55
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "__set_page_dirty_no_writeback" [fs/ext4/ext4.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36680 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] mm: Move page dirtying prototypes from mm.h
2021-06-15 16:23 [PATCH 0/6] Further set_page_dirty cleanups Matthew Wilcox (Oracle)
` (4 preceding siblings ...)
2021-06-15 16:23 ` [PATCH 5/6] fs: Remove noop_set_page_dirty() Matthew Wilcox (Oracle)
@ 2021-06-15 16:23 ` Matthew Wilcox (Oracle)
2021-06-15 16:39 ` Christoph Hellwig
5 siblings, 1 reply; 27+ messages in thread
From: Matthew Wilcox (Oracle) @ 2021-06-15 16:23 UTC (permalink / raw)
To: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro,
Greg Kroah-Hartman, linux-mm, linux-fsdevel, linux-kernel
Cc: Matthew Wilcox (Oracle)
These functions implement the address_space ->set_page_dirty operation
and should live in pagemap.h, not mm.h so that the rest of the kernel
doesn't get funny ideas about calling them directly.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
fs/fuse/dax.c | 1 +
fs/zonefs/super.c | 2 +-
include/linux/mm.h | 3 ---
include/linux/pagemap.h | 4 ++++
4 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
index 515ad0895345..fb733eb5aead 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -9,6 +9,7 @@
#include <linux/delay.h>
#include <linux/dax.h>
#include <linux/uio.h>
+#include <linux/pagemap.h>
#include <linux/pfn_t.h>
#include <linux/iomap.h>
#include <linux/interval_tree.h>
diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
index 3aacf016c7c2..dbf03635869c 100644
--- a/fs/zonefs/super.c
+++ b/fs/zonefs/super.c
@@ -5,7 +5,7 @@
* Copyright (C) 2019 Western Digital Corporation or its affiliates.
*/
#include <linux/module.h>
-#include <linux/fs.h>
+#include <linux/pagemap.h>
#include <linux/magic.h>
#include <linux/iomap.h>
#include <linux/init.h>
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 1086b556961a..5608f75ecc80 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1853,9 +1853,6 @@ extern int try_to_release_page(struct page * page, gfp_t gfp_mask);
extern void do_invalidatepage(struct page *page, unsigned int offset,
unsigned int length);
-void __set_page_dirty(struct page *, struct address_space *, int warn);
-int __set_page_dirty_nobuffers(struct page *page);
-int __set_page_dirty_no_writeback(struct page *page);
int redirty_page_for_writepage(struct writeback_control *wbc,
struct page *page);
void account_page_cleaned(struct page *page, struct address_space *mapping,
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 5f0582de24e7..090e45616172 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -752,6 +752,10 @@ int wait_on_page_writeback_killable(struct page *page);
extern void end_page_writeback(struct page *page);
void wait_for_stable_page(struct page *page);
+void __set_page_dirty(struct page *, struct address_space *, int warn);
+int __set_page_dirty_nobuffers(struct page *page);
+int __set_page_dirty_no_writeback(struct page *page);
+
void page_endio(struct page *page, bool is_write, int err);
/**
--
2.30.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 6/6] mm: Move page dirtying prototypes from mm.h
2021-06-15 16:23 ` [PATCH 6/6] mm: Move page dirtying prototypes from mm.h Matthew Wilcox (Oracle)
@ 2021-06-15 16:39 ` Christoph Hellwig
0 siblings, 0 replies; 27+ messages in thread
From: Christoph Hellwig @ 2021-06-15 16:39 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Christoph Hellwig, Andrew Morton, Jan Kara, Al Viro,
Greg Kroah-Hartman, linux-mm, linux-fsdevel, linux-kernel
On Tue, Jun 15, 2021 at 05:23:42PM +0100, Matthew Wilcox (Oracle) wrote:
> These functions implement the address_space ->set_page_dirty operation
> and should live in pagemap.h, not mm.h so that the rest of the kernel
> doesn't get funny ideas about calling them directly.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 27+ messages in thread