2021-06-14 06:17:21

by Christoph Hellwig

[permalink] [raw]
Subject: remove the implicit .set_page_dirty default

Hi all,

this series cleans up a few lose ends around ->set_page_dirty, most
importantly removes the default to the buffer head based on if no
method is wired up.


2021-06-14 06:17:42

by Christoph Hellwig

[permalink] [raw]
Subject: [PATCH 3/3] mm: require ->set_page_dirty to be explicitly wire up

Remove the CONFIG_BLOCK default to __set_page_dirty_buffers and just
wire that method up for the missing instances.

Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/adfs/inode.c | 1 +
fs/affs/file.c | 2 ++
fs/bfs/file.c | 1 +
fs/block_dev.c | 1 +
fs/exfat/inode.c | 1 +
fs/ext2/inode.c | 2 ++
fs/fat/inode.c | 1 +
fs/gfs2/meta_io.c | 2 ++
fs/hfs/inode.c | 2 ++
fs/hfsplus/inode.c | 2 ++
fs/hpfs/file.c | 1 +
fs/jfs/inode.c | 1 +
fs/minix/inode.c | 1 +
fs/nilfs2/mdt.c | 1 +
fs/ocfs2/aops.c | 1 +
fs/omfs/file.c | 1 +
fs/sysv/itree.c | 1 +
fs/udf/file.c | 1 +
fs/udf/inode.c | 1 +
fs/ufs/inode.c | 1 +
mm/page-writeback.c | 18 ++++--------------
21 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/fs/adfs/inode.c b/fs/adfs/inode.c
index fb7ee026d101..adbb3a1edcbf 100644
--- a/fs/adfs/inode.c
+++ b/fs/adfs/inode.c
@@ -73,6 +73,7 @@ static sector_t _adfs_bmap(struct address_space *mapping, sector_t block)
}

static const struct address_space_operations adfs_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = adfs_readpage,
.writepage = adfs_writepage,
.write_begin = adfs_write_begin,
diff --git a/fs/affs/file.c b/fs/affs/file.c
index d91b0133d95d..75ebd2b576ca 100644
--- a/fs/affs/file.c
+++ b/fs/affs/file.c
@@ -453,6 +453,7 @@ static sector_t _affs_bmap(struct address_space *mapping, sector_t block)
}

const struct address_space_operations affs_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = affs_readpage,
.writepage = affs_writepage,
.write_begin = affs_write_begin,
@@ -833,6 +834,7 @@ static int affs_write_end_ofs(struct file *file, struct address_space *mapping,
}

const struct address_space_operations affs_aops_ofs = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = affs_readpage_ofs,
//.writepage = affs_writepage_ofs,
.write_begin = affs_write_begin_ofs,
diff --git a/fs/bfs/file.c b/fs/bfs/file.c
index 0dceefc54b48..7f8544abf636 100644
--- a/fs/bfs/file.c
+++ b/fs/bfs/file.c
@@ -188,6 +188,7 @@ static sector_t bfs_bmap(struct address_space *mapping, sector_t block)
}

const struct address_space_operations bfs_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = bfs_readpage,
.writepage = bfs_writepage,
.write_begin = bfs_write_begin,
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 6cc4d4cfe0c2..eb34f5c357cf 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1754,6 +1754,7 @@ static int blkdev_writepages(struct address_space *mapping,
}

static const struct address_space_operations def_blk_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = blkdev_readpage,
.readahead = blkdev_readahead,
.writepage = blkdev_writepage,
diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c
index 1803ef3220fd..ca37d4344361 100644
--- a/fs/exfat/inode.c
+++ b/fs/exfat/inode.c
@@ -491,6 +491,7 @@ int exfat_block_truncate_page(struct inode *inode, loff_t from)
}

static const struct address_space_operations exfat_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = exfat_readpage,
.readahead = exfat_readahead,
.writepage = exfat_writepage,
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 68178b2234bd..bf41f579ed3e 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -961,6 +961,7 @@ ext2_dax_writepages(struct address_space *mapping, struct writeback_control *wbc
}

const struct address_space_operations ext2_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = ext2_readpage,
.readahead = ext2_readahead,
.writepage = ext2_writepage,
@@ -975,6 +976,7 @@ const struct address_space_operations ext2_aops = {
};

const struct address_space_operations ext2_nobh_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = ext2_readpage,
.readahead = ext2_readahead,
.writepage = ext2_nobh_writepage,
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index bab9b202b496..de0c9b013a85 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -342,6 +342,7 @@ int fat_block_truncate_page(struct inode *inode, loff_t from)
}

static const struct address_space_operations fat_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = fat_readpage,
.readahead = fat_readahead,
.writepage = fat_writepage,
diff --git a/fs/gfs2/meta_io.c b/fs/gfs2/meta_io.c
index d68184ebbfdd..7c9619997355 100644
--- a/fs/gfs2/meta_io.c
+++ b/fs/gfs2/meta_io.c
@@ -89,11 +89,13 @@ static int gfs2_aspace_writepage(struct page *page, struct writeback_control *wb
}

const struct address_space_operations gfs2_meta_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.writepage = gfs2_aspace_writepage,
.releasepage = gfs2_releasepage,
};

const struct address_space_operations gfs2_rgrp_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.writepage = gfs2_aspace_writepage,
.releasepage = gfs2_releasepage,
};
diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index 3fc5cb346586..4a95a92546a0 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -159,6 +159,7 @@ static int hfs_writepages(struct address_space *mapping,
}

const struct address_space_operations hfs_btree_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = hfs_readpage,
.writepage = hfs_writepage,
.write_begin = hfs_write_begin,
@@ -168,6 +169,7 @@ const struct address_space_operations hfs_btree_aops = {
};

const struct address_space_operations hfs_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = hfs_readpage,
.writepage = hfs_writepage,
.write_begin = hfs_write_begin,
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index 8ea447e5c470..70e8374ddac4 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -156,6 +156,7 @@ static int hfsplus_writepages(struct address_space *mapping,
}

const struct address_space_operations hfsplus_btree_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = hfsplus_readpage,
.writepage = hfsplus_writepage,
.write_begin = hfsplus_write_begin,
@@ -165,6 +166,7 @@ const struct address_space_operations hfsplus_btree_aops = {
};

const struct address_space_operations hfsplus_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = hfsplus_readpage,
.writepage = hfsplus_writepage,
.write_begin = hfsplus_write_begin,
diff --git a/fs/hpfs/file.c b/fs/hpfs/file.c
index 077c25128eb7..c3a49aacf20a 100644
--- a/fs/hpfs/file.c
+++ b/fs/hpfs/file.c
@@ -196,6 +196,7 @@ static int hpfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
}

const struct address_space_operations hpfs_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = hpfs_readpage,
.writepage = hpfs_writepage,
.readahead = hpfs_readahead,
diff --git a/fs/jfs/inode.c b/fs/jfs/inode.c
index 6f65bfa9f18d..3663dd5a23bc 100644
--- a/fs/jfs/inode.c
+++ b/fs/jfs/inode.c
@@ -356,6 +356,7 @@ static ssize_t jfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
}

const struct address_space_operations jfs_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = jfs_readpage,
.readahead = jfs_readahead,
.writepage = jfs_writepage,
diff --git a/fs/minix/inode.c b/fs/minix/inode.c
index a532a99bbe81..a71f1cf894b9 100644
--- a/fs/minix/inode.c
+++ b/fs/minix/inode.c
@@ -442,6 +442,7 @@ static sector_t minix_bmap(struct address_space *mapping, sector_t block)
}

static const struct address_space_operations minix_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = minix_readpage,
.writepage = minix_writepage,
.write_begin = minix_write_begin,
diff --git a/fs/nilfs2/mdt.c b/fs/nilfs2/mdt.c
index c0361ce45f62..97769fe4d588 100644
--- a/fs/nilfs2/mdt.c
+++ b/fs/nilfs2/mdt.c
@@ -434,6 +434,7 @@ nilfs_mdt_write_page(struct page *page, struct writeback_control *wbc)


static const struct address_space_operations def_mdt_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.writepage = nilfs_mdt_write_page,
};

diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index 1294925ac94a..b3517de178ff 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -2454,6 +2454,7 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
}

const struct address_space_operations ocfs2_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = ocfs2_readpage,
.readahead = ocfs2_readahead,
.writepage = ocfs2_writepage,
diff --git a/fs/omfs/file.c b/fs/omfs/file.c
index 11e733aab25d..89725b15a64b 100644
--- a/fs/omfs/file.c
+++ b/fs/omfs/file.c
@@ -372,6 +372,7 @@ const struct inode_operations omfs_file_inops = {
};

const struct address_space_operations omfs_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = omfs_readpage,
.readahead = omfs_readahead,
.writepage = omfs_writepage,
diff --git a/fs/sysv/itree.c b/fs/sysv/itree.c
index 8b2e99b7bc9f..749385015a8d 100644
--- a/fs/sysv/itree.c
+++ b/fs/sysv/itree.c
@@ -495,6 +495,7 @@ static sector_t sysv_bmap(struct address_space *mapping, sector_t block)
}

const struct address_space_operations sysv_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = sysv_readpage,
.writepage = sysv_writepage,
.write_begin = sysv_write_begin,
diff --git a/fs/udf/file.c b/fs/udf/file.c
index 2846dcd92197..1baff8ddb754 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -125,6 +125,7 @@ static int udf_adinicb_write_end(struct file *file, struct address_space *mappin
}

const struct address_space_operations udf_adinicb_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = udf_adinicb_readpage,
.writepage = udf_adinicb_writepage,
.write_begin = udf_adinicb_write_begin,
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 0dd2f93ac048..4917670860a0 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -235,6 +235,7 @@ static sector_t udf_bmap(struct address_space *mapping, sector_t block)
}

const struct address_space_operations udf_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = udf_readpage,
.readahead = udf_readahead,
.writepage = udf_writepage,
diff --git a/fs/ufs/inode.c b/fs/ufs/inode.c
index debc282c1bb4..ac628de69601 100644
--- a/fs/ufs/inode.c
+++ b/fs/ufs/inode.c
@@ -526,6 +526,7 @@ static sector_t ufs_bmap(struct address_space *mapping, sector_t block)
}

const struct address_space_operations ufs_aops = {
+ .set_page_dirty = __set_page_dirty_buffers,
.readpage = ufs_readpage,
.writepage = ufs_writepage,
.write_begin = ufs_write_begin,
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 0062d5c57d41..081fa02236c2 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -32,7 +32,6 @@
#include <linux/sysctl.h>
#include <linux/cpu.h>
#include <linux/syscalls.h>
-#include <linux/buffer_head.h> /* __set_page_dirty_buffers */
#include <linux/pagevec.h>
#include <linux/timer.h>
#include <linux/sched/rt.h>
@@ -2546,13 +2545,9 @@ EXPORT_SYMBOL(redirty_page_for_writepage);
/*
* Dirty a page.
*
- * For pages with a mapping this should be done under the page lock
- * for the benefit of asynchronous memory errors who prefer a consistent
- * dirty state. This rule can be broken in some special cases,
- * but should be better not to.
- *
- * If the mapping doesn't provide a set_page_dirty a_op, then
- * just fall through and assume that it wants buffer_heads.
+ * For pages with a mapping this should be done under the page lock for the
+ * benefit of asynchronous memory errors who prefer a consistent dirty state.
+ * This rule can be broken in some special cases, but should be better not to.
*/
int set_page_dirty(struct page *page)
{
@@ -2560,7 +2555,6 @@ int set_page_dirty(struct page *page)

page = compound_head(page);
if (likely(mapping)) {
- int (*spd)(struct page *) = mapping->a_ops->set_page_dirty;
/*
* readahead/lru_deactivate_page could remain
* PG_readahead/PG_reclaim due to race with end_page_writeback
@@ -2573,11 +2567,7 @@ int set_page_dirty(struct page *page)
*/
if (PageReclaim(page))
ClearPageReclaim(page);
-#ifdef CONFIG_BLOCK
- if (!spd)
- spd = __set_page_dirty_buffers;
-#endif
- return (*spd)(page);
+ return mapping->a_ops->set_page_dirty(page);
}
if (!PageDirty(page)) {
if (!TestSetPageDirty(page))
--
2.30.2

2021-06-14 06:17:50

by Christoph Hellwig

[permalink] [raw]
Subject: [PATCH 2/3] fs: move ramfs_aops to libfs

Move the ramfs aops to libfs and reuse them for kernfs and configfs.
Thosw two did not wire up ->set_page_dirty before and now get
__set_page_dirty_no_writeback, which is the right one for no-writeback
address_space usage.

Drop the now unused exports of the libfs helpers only used for
ramfs-style pagecache usage.

Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/configfs/inode.c | 8 +-------
fs/kernfs/inode.c | 8 +-------
fs/libfs.c | 17 +++++++++++++----
fs/ramfs/inode.c | 9 +--------
include/linux/fs.h | 5 +----
5 files changed, 17 insertions(+), 30 deletions(-)

diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
index eb5ec3e46283..b601610e9907 100644
--- a/fs/configfs/inode.c
+++ b/fs/configfs/inode.c
@@ -28,12 +28,6 @@
static struct lock_class_key default_group_class[MAX_LOCK_DEPTH];
#endif

-static const struct address_space_operations configfs_aops = {
- .readpage = simple_readpage,
- .write_begin = simple_write_begin,
- .write_end = simple_write_end,
-};
-
static const struct inode_operations configfs_inode_operations ={
.setattr = configfs_setattr,
};
@@ -114,7 +108,7 @@ struct inode *configfs_new_inode(umode_t mode, struct configfs_dirent *sd,
struct inode * inode = new_inode(s);
if (inode) {
inode->i_ino = get_next_ino();
- inode->i_mapping->a_ops = &configfs_aops;
+ inode->i_mapping->a_ops = &ram_aops;
inode->i_op = &configfs_inode_operations;

if (sd->s_iattr) {
diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index d73950fc3d57..26f2aa3586f9 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -17,12 +17,6 @@

#include "kernfs-internal.h"

-static const struct address_space_operations kernfs_aops = {
- .readpage = simple_readpage,
- .write_begin = simple_write_begin,
- .write_end = simple_write_end,
-};
-
static const struct inode_operations kernfs_iops = {
.permission = kernfs_iop_permission,
.setattr = kernfs_iop_setattr,
@@ -203,7 +197,7 @@ static void kernfs_init_inode(struct kernfs_node *kn, struct inode *inode)
{
kernfs_get(kn);
inode->i_private = kn;
- inode->i_mapping->a_ops = &kernfs_aops;
+ inode->i_mapping->a_ops = &ram_aops;
inode->i_op = &kernfs_iops;
inode->i_generation = kernfs_gen(kn);

diff --git a/fs/libfs.c b/fs/libfs.c
index e9b29c6ffccb..2d7f086b93d6 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -512,7 +512,7 @@ int simple_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
}
EXPORT_SYMBOL(simple_setattr);

-int simple_readpage(struct file *file, struct page *page)
+static int simple_readpage(struct file *file, struct page *page)
{
clear_highpage(page);
flush_dcache_page(page);
@@ -520,7 +520,6 @@ int simple_readpage(struct file *file, struct page *page)
unlock_page(page);
return 0;
}
-EXPORT_SYMBOL(simple_readpage);

int simple_write_begin(struct file *file, struct address_space *mapping,
loff_t pos, unsigned len, unsigned flags,
@@ -568,7 +567,7 @@ EXPORT_SYMBOL(simple_write_begin);
*
* Use *ONLY* with simple_readpage()
*/
-int simple_write_end(struct file *file, struct address_space *mapping,
+static int simple_write_end(struct file *file, struct address_space *mapping,
loff_t pos, unsigned len, unsigned copied,
struct page *page, void *fsdata)
{
@@ -597,7 +596,17 @@ int simple_write_end(struct file *file, struct address_space *mapping,

return copied;
}
-EXPORT_SYMBOL(simple_write_end);
+
+/*
+ * Provides ramfs-style behavior: data in the pagecache, but no writeback.
+ */
+const struct address_space_operations ram_aops = {
+ .readpage = simple_readpage,
+ .write_begin = simple_write_begin,
+ .write_end = simple_write_end,
+ .set_page_dirty = __set_page_dirty_no_writeback,
+};
+EXPORT_SYMBOL(ram_aops);

/*
* the inodes created here are not hashed. If you use iunique to generate
diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
index 9ebd17d7befb..65e7e56005b8 100644
--- a/fs/ramfs/inode.c
+++ b/fs/ramfs/inode.c
@@ -53,13 +53,6 @@ struct ramfs_fs_info {
static const struct super_operations ramfs_ops;
static const struct inode_operations ramfs_dir_inode_operations;

-static const struct address_space_operations ramfs_aops = {
- .readpage = simple_readpage,
- .write_begin = simple_write_begin,
- .write_end = simple_write_end,
- .set_page_dirty = __set_page_dirty_no_writeback,
-};
-
struct inode *ramfs_get_inode(struct super_block *sb,
const struct inode *dir, umode_t mode, dev_t dev)
{
@@ -68,7 +61,7 @@ struct inode *ramfs_get_inode(struct super_block *sb,
if (inode) {
inode->i_ino = get_next_ino();
inode_init_owner(&init_user_ns, inode, dir, mode);
- inode->i_mapping->a_ops = &ramfs_aops;
+ inode->i_mapping->a_ops = &ram_aops;
mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
mapping_set_unevictable(inode->i_mapping);
inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c3c88fdb9b2a..869909345420 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3422,13 +3422,10 @@ 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);
extern int simple_empty(struct dentry *);
-extern int simple_readpage(struct file *file, struct page *page);
extern int simple_write_begin(struct file *file, struct address_space *mapping,
loff_t pos, unsigned len, unsigned flags,
struct page **pagep, void **fsdata);
-extern int simple_write_end(struct file *file, struct address_space *mapping,
- loff_t pos, unsigned len, unsigned copied,
- struct page *page, void *fsdata);
+extern const struct address_space_operations ram_aops;
extern int always_delete_dentry(const struct dentry *);
extern struct inode *alloc_anon_inode(struct super_block *);
extern int simple_nosetlease(struct file *, long, struct file_lock **, void **);
--
2.30.2

2021-06-14 06:19:26

by Christoph Hellwig

[permalink] [raw]
Subject: [PATCH 1/3] fs: unexport __set_page_dirty

__set_page_dirty is only used by built-in code.

Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/buffer.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index ea48c01fb76b..3d18831c7ad8 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -611,7 +611,6 @@ void __set_page_dirty(struct page *page, struct address_space *mapping,
}
xa_unlock_irqrestore(&mapping->i_pages, flags);
}
-EXPORT_SYMBOL_GPL(__set_page_dirty);

/*
* Add a page to the dirty page list.
--
2.30.2

2021-06-14 06:25:39

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 1/3] fs: unexport __set_page_dirty

On Mon, Jun 14, 2021 at 08:15:10AM +0200, Christoph Hellwig wrote:
> __set_page_dirty is only used by built-in code.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> fs/buffer.c | 1 -
> 1 file changed, 1 deletion(-)

Reviewed-by: Greg Kroah-Hartman <[email protected]>

2021-06-14 06:27:25

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 3/3] mm: require ->set_page_dirty to be explicitly wire up

On Mon, Jun 14, 2021 at 08:15:12AM +0200, Christoph Hellwig wrote:
> Remove the CONFIG_BLOCK default to __set_page_dirty_buffers and just
> wire that method up for the missing instances.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> fs/adfs/inode.c | 1 +
> fs/affs/file.c | 2 ++
> fs/bfs/file.c | 1 +
> fs/block_dev.c | 1 +
> fs/exfat/inode.c | 1 +
> fs/ext2/inode.c | 2 ++
> fs/fat/inode.c | 1 +
> fs/gfs2/meta_io.c | 2 ++
> fs/hfs/inode.c | 2 ++
> fs/hfsplus/inode.c | 2 ++
> fs/hpfs/file.c | 1 +
> fs/jfs/inode.c | 1 +
> fs/minix/inode.c | 1 +
> fs/nilfs2/mdt.c | 1 +
> fs/ocfs2/aops.c | 1 +
> fs/omfs/file.c | 1 +
> fs/sysv/itree.c | 1 +
> fs/udf/file.c | 1 +
> fs/udf/inode.c | 1 +
> fs/ufs/inode.c | 1 +
> mm/page-writeback.c | 18 ++++--------------
> 21 files changed, 29 insertions(+), 14 deletions(-)
>


Reviewed-by: Greg Kroah-Hartman <[email protected]>

2021-06-14 06:28:24

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 2/3] fs: move ramfs_aops to libfs

On Mon, Jun 14, 2021 at 08:15:11AM +0200, Christoph Hellwig wrote:
> Move the ramfs aops to libfs and reuse them for kernfs and configfs.
> Thosw two did not wire up ->set_page_dirty before and now get
> __set_page_dirty_no_writeback, which is the right one for no-writeback
> address_space usage.
>
> Drop the now unused exports of the libfs helpers only used for
> ramfs-style pagecache usage.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> fs/configfs/inode.c | 8 +-------
> fs/kernfs/inode.c | 8 +-------
> fs/libfs.c | 17 +++++++++++++----
> fs/ramfs/inode.c | 9 +--------
> include/linux/fs.h | 5 +----
> 5 files changed, 17 insertions(+), 30 deletions(-)
>


Reviewed-by: Greg Kroah-Hartman <[email protected]>

2021-06-14 09:44:17

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH 1/3] fs: unexport __set_page_dirty

On Mon 14-06-21 08:15:10, Christoph Hellwig wrote:
> __set_page_dirty is only used by built-in code.
>
> Signed-off-by: Christoph Hellwig <[email protected]>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

Honza

> ---
> fs/buffer.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index ea48c01fb76b..3d18831c7ad8 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -611,7 +611,6 @@ void __set_page_dirty(struct page *page, struct address_space *mapping,
> }
> xa_unlock_irqrestore(&mapping->i_pages, flags);
> }
> -EXPORT_SYMBOL_GPL(__set_page_dirty);
>
> /*
> * Add a page to the dirty page list.
> --
> 2.30.2
>
--
Jan Kara <[email protected]>
SUSE Labs, CR

2021-06-14 09:49:18

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH 2/3] fs: move ramfs_aops to libfs

On Mon 14-06-21 08:15:11, Christoph Hellwig wrote:
> Move the ramfs aops to libfs and reuse them for kernfs and configfs.
> Thosw two did not wire up ->set_page_dirty before and now get
> __set_page_dirty_no_writeback, which is the right one for no-writeback
> address_space usage.
>
> Drop the now unused exports of the libfs helpers only used for
> ramfs-style pagecache usage.
>
> Signed-off-by: Christoph Hellwig <[email protected]>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

Honza

> ---
> fs/configfs/inode.c | 8 +-------
> fs/kernfs/inode.c | 8 +-------
> fs/libfs.c | 17 +++++++++++++----
> fs/ramfs/inode.c | 9 +--------
> include/linux/fs.h | 5 +----
> 5 files changed, 17 insertions(+), 30 deletions(-)
>
> diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
> index eb5ec3e46283..b601610e9907 100644
> --- a/fs/configfs/inode.c
> +++ b/fs/configfs/inode.c
> @@ -28,12 +28,6 @@
> static struct lock_class_key default_group_class[MAX_LOCK_DEPTH];
> #endif
>
> -static const struct address_space_operations configfs_aops = {
> - .readpage = simple_readpage,
> - .write_begin = simple_write_begin,
> - .write_end = simple_write_end,
> -};
> -
> static const struct inode_operations configfs_inode_operations ={
> .setattr = configfs_setattr,
> };
> @@ -114,7 +108,7 @@ struct inode *configfs_new_inode(umode_t mode, struct configfs_dirent *sd,
> struct inode * inode = new_inode(s);
> if (inode) {
> inode->i_ino = get_next_ino();
> - inode->i_mapping->a_ops = &configfs_aops;
> + inode->i_mapping->a_ops = &ram_aops;
> inode->i_op = &configfs_inode_operations;
>
> if (sd->s_iattr) {
> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> index d73950fc3d57..26f2aa3586f9 100644
> --- a/fs/kernfs/inode.c
> +++ b/fs/kernfs/inode.c
> @@ -17,12 +17,6 @@
>
> #include "kernfs-internal.h"
>
> -static const struct address_space_operations kernfs_aops = {
> - .readpage = simple_readpage,
> - .write_begin = simple_write_begin,
> - .write_end = simple_write_end,
> -};
> -
> static const struct inode_operations kernfs_iops = {
> .permission = kernfs_iop_permission,
> .setattr = kernfs_iop_setattr,
> @@ -203,7 +197,7 @@ static void kernfs_init_inode(struct kernfs_node *kn, struct inode *inode)
> {
> kernfs_get(kn);
> inode->i_private = kn;
> - inode->i_mapping->a_ops = &kernfs_aops;
> + inode->i_mapping->a_ops = &ram_aops;
> inode->i_op = &kernfs_iops;
> inode->i_generation = kernfs_gen(kn);
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index e9b29c6ffccb..2d7f086b93d6 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -512,7 +512,7 @@ int simple_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
> }
> EXPORT_SYMBOL(simple_setattr);
>
> -int simple_readpage(struct file *file, struct page *page)
> +static int simple_readpage(struct file *file, struct page *page)
> {
> clear_highpage(page);
> flush_dcache_page(page);
> @@ -520,7 +520,6 @@ int simple_readpage(struct file *file, struct page *page)
> unlock_page(page);
> return 0;
> }
> -EXPORT_SYMBOL(simple_readpage);
>
> int simple_write_begin(struct file *file, struct address_space *mapping,
> loff_t pos, unsigned len, unsigned flags,
> @@ -568,7 +567,7 @@ EXPORT_SYMBOL(simple_write_begin);
> *
> * Use *ONLY* with simple_readpage()
> */
> -int simple_write_end(struct file *file, struct address_space *mapping,
> +static int simple_write_end(struct file *file, struct address_space *mapping,
> loff_t pos, unsigned len, unsigned copied,
> struct page *page, void *fsdata)
> {
> @@ -597,7 +596,17 @@ int simple_write_end(struct file *file, struct address_space *mapping,
>
> return copied;
> }
> -EXPORT_SYMBOL(simple_write_end);
> +
> +/*
> + * Provides ramfs-style behavior: data in the pagecache, but no writeback.
> + */
> +const struct address_space_operations ram_aops = {
> + .readpage = simple_readpage,
> + .write_begin = simple_write_begin,
> + .write_end = simple_write_end,
> + .set_page_dirty = __set_page_dirty_no_writeback,
> +};
> +EXPORT_SYMBOL(ram_aops);
>
> /*
> * the inodes created here are not hashed. If you use iunique to generate
> diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
> index 9ebd17d7befb..65e7e56005b8 100644
> --- a/fs/ramfs/inode.c
> +++ b/fs/ramfs/inode.c
> @@ -53,13 +53,6 @@ struct ramfs_fs_info {
> static const struct super_operations ramfs_ops;
> static const struct inode_operations ramfs_dir_inode_operations;
>
> -static const struct address_space_operations ramfs_aops = {
> - .readpage = simple_readpage,
> - .write_begin = simple_write_begin,
> - .write_end = simple_write_end,
> - .set_page_dirty = __set_page_dirty_no_writeback,
> -};
> -
> struct inode *ramfs_get_inode(struct super_block *sb,
> const struct inode *dir, umode_t mode, dev_t dev)
> {
> @@ -68,7 +61,7 @@ struct inode *ramfs_get_inode(struct super_block *sb,
> if (inode) {
> inode->i_ino = get_next_ino();
> inode_init_owner(&init_user_ns, inode, dir, mode);
> - inode->i_mapping->a_ops = &ramfs_aops;
> + inode->i_mapping->a_ops = &ram_aops;
> mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
> mapping_set_unevictable(inode->i_mapping);
> inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index c3c88fdb9b2a..869909345420 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3422,13 +3422,10 @@ 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);
> extern int simple_empty(struct dentry *);
> -extern int simple_readpage(struct file *file, struct page *page);
> extern int simple_write_begin(struct file *file, struct address_space *mapping,
> loff_t pos, unsigned len, unsigned flags,
> struct page **pagep, void **fsdata);
> -extern int simple_write_end(struct file *file, struct address_space *mapping,
> - loff_t pos, unsigned len, unsigned copied,
> - struct page *page, void *fsdata);
> +extern const struct address_space_operations ram_aops;
> extern int always_delete_dentry(const struct dentry *);
> extern struct inode *alloc_anon_inode(struct super_block *);
> extern int simple_nosetlease(struct file *, long, struct file_lock **, void **);
> --
> 2.30.2
>
--
Jan Kara <[email protected]>
SUSE Labs, CR

2021-06-14 09:52:42

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH 3/3] mm: require ->set_page_dirty to be explicitly wire up

On Mon 14-06-21 08:15:12, Christoph Hellwig wrote:
> Remove the CONFIG_BLOCK default to __set_page_dirty_buffers and just
> wire that method up for the missing instances.
>
> Signed-off-by: Christoph Hellwig <[email protected]>

Make sense. Did you somehow autogenerate this? If this patch would race
with addition of new aops struct, we'd get null-ptr-defer out of that so
maybe providing the script would be better. But other than that the changes
look good to me. You can add:

Reviewed-by: Jan Kara <[email protected]>


Honza

> ---
> fs/adfs/inode.c | 1 +
> fs/affs/file.c | 2 ++
> fs/bfs/file.c | 1 +
> fs/block_dev.c | 1 +
> fs/exfat/inode.c | 1 +
> fs/ext2/inode.c | 2 ++
> fs/fat/inode.c | 1 +
> fs/gfs2/meta_io.c | 2 ++
> fs/hfs/inode.c | 2 ++
> fs/hfsplus/inode.c | 2 ++
> fs/hpfs/file.c | 1 +
> fs/jfs/inode.c | 1 +
> fs/minix/inode.c | 1 +
> fs/nilfs2/mdt.c | 1 +
> fs/ocfs2/aops.c | 1 +
> fs/omfs/file.c | 1 +
> fs/sysv/itree.c | 1 +
> fs/udf/file.c | 1 +
> fs/udf/inode.c | 1 +
> fs/ufs/inode.c | 1 +
> mm/page-writeback.c | 18 ++++--------------
> 21 files changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/fs/adfs/inode.c b/fs/adfs/inode.c
> index fb7ee026d101..adbb3a1edcbf 100644
> --- a/fs/adfs/inode.c
> +++ b/fs/adfs/inode.c
> @@ -73,6 +73,7 @@ static sector_t _adfs_bmap(struct address_space *mapping, sector_t block)
> }
>
> static const struct address_space_operations adfs_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = adfs_readpage,
> .writepage = adfs_writepage,
> .write_begin = adfs_write_begin,
> diff --git a/fs/affs/file.c b/fs/affs/file.c
> index d91b0133d95d..75ebd2b576ca 100644
> --- a/fs/affs/file.c
> +++ b/fs/affs/file.c
> @@ -453,6 +453,7 @@ static sector_t _affs_bmap(struct address_space *mapping, sector_t block)
> }
>
> const struct address_space_operations affs_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = affs_readpage,
> .writepage = affs_writepage,
> .write_begin = affs_write_begin,
> @@ -833,6 +834,7 @@ static int affs_write_end_ofs(struct file *file, struct address_space *mapping,
> }
>
> const struct address_space_operations affs_aops_ofs = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = affs_readpage_ofs,
> //.writepage = affs_writepage_ofs,
> .write_begin = affs_write_begin_ofs,
> diff --git a/fs/bfs/file.c b/fs/bfs/file.c
> index 0dceefc54b48..7f8544abf636 100644
> --- a/fs/bfs/file.c
> +++ b/fs/bfs/file.c
> @@ -188,6 +188,7 @@ static sector_t bfs_bmap(struct address_space *mapping, sector_t block)
> }
>
> const struct address_space_operations bfs_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = bfs_readpage,
> .writepage = bfs_writepage,
> .write_begin = bfs_write_begin,
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 6cc4d4cfe0c2..eb34f5c357cf 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1754,6 +1754,7 @@ static int blkdev_writepages(struct address_space *mapping,
> }
>
> static const struct address_space_operations def_blk_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = blkdev_readpage,
> .readahead = blkdev_readahead,
> .writepage = blkdev_writepage,
> diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c
> index 1803ef3220fd..ca37d4344361 100644
> --- a/fs/exfat/inode.c
> +++ b/fs/exfat/inode.c
> @@ -491,6 +491,7 @@ int exfat_block_truncate_page(struct inode *inode, loff_t from)
> }
>
> static const struct address_space_operations exfat_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = exfat_readpage,
> .readahead = exfat_readahead,
> .writepage = exfat_writepage,
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index 68178b2234bd..bf41f579ed3e 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -961,6 +961,7 @@ ext2_dax_writepages(struct address_space *mapping, struct writeback_control *wbc
> }
>
> const struct address_space_operations ext2_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = ext2_readpage,
> .readahead = ext2_readahead,
> .writepage = ext2_writepage,
> @@ -975,6 +976,7 @@ const struct address_space_operations ext2_aops = {
> };
>
> const struct address_space_operations ext2_nobh_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = ext2_readpage,
> .readahead = ext2_readahead,
> .writepage = ext2_nobh_writepage,
> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index bab9b202b496..de0c9b013a85 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -342,6 +342,7 @@ int fat_block_truncate_page(struct inode *inode, loff_t from)
> }
>
> static const struct address_space_operations fat_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = fat_readpage,
> .readahead = fat_readahead,
> .writepage = fat_writepage,
> diff --git a/fs/gfs2/meta_io.c b/fs/gfs2/meta_io.c
> index d68184ebbfdd..7c9619997355 100644
> --- a/fs/gfs2/meta_io.c
> +++ b/fs/gfs2/meta_io.c
> @@ -89,11 +89,13 @@ static int gfs2_aspace_writepage(struct page *page, struct writeback_control *wb
> }
>
> const struct address_space_operations gfs2_meta_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .writepage = gfs2_aspace_writepage,
> .releasepage = gfs2_releasepage,
> };
>
> const struct address_space_operations gfs2_rgrp_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .writepage = gfs2_aspace_writepage,
> .releasepage = gfs2_releasepage,
> };
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index 3fc5cb346586..4a95a92546a0 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -159,6 +159,7 @@ static int hfs_writepages(struct address_space *mapping,
> }
>
> const struct address_space_operations hfs_btree_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = hfs_readpage,
> .writepage = hfs_writepage,
> .write_begin = hfs_write_begin,
> @@ -168,6 +169,7 @@ const struct address_space_operations hfs_btree_aops = {
> };
>
> const struct address_space_operations hfs_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = hfs_readpage,
> .writepage = hfs_writepage,
> .write_begin = hfs_write_begin,
> diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
> index 8ea447e5c470..70e8374ddac4 100644
> --- a/fs/hfsplus/inode.c
> +++ b/fs/hfsplus/inode.c
> @@ -156,6 +156,7 @@ static int hfsplus_writepages(struct address_space *mapping,
> }
>
> const struct address_space_operations hfsplus_btree_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = hfsplus_readpage,
> .writepage = hfsplus_writepage,
> .write_begin = hfsplus_write_begin,
> @@ -165,6 +166,7 @@ const struct address_space_operations hfsplus_btree_aops = {
> };
>
> const struct address_space_operations hfsplus_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = hfsplus_readpage,
> .writepage = hfsplus_writepage,
> .write_begin = hfsplus_write_begin,
> diff --git a/fs/hpfs/file.c b/fs/hpfs/file.c
> index 077c25128eb7..c3a49aacf20a 100644
> --- a/fs/hpfs/file.c
> +++ b/fs/hpfs/file.c
> @@ -196,6 +196,7 @@ static int hpfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> }
>
> const struct address_space_operations hpfs_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = hpfs_readpage,
> .writepage = hpfs_writepage,
> .readahead = hpfs_readahead,
> diff --git a/fs/jfs/inode.c b/fs/jfs/inode.c
> index 6f65bfa9f18d..3663dd5a23bc 100644
> --- a/fs/jfs/inode.c
> +++ b/fs/jfs/inode.c
> @@ -356,6 +356,7 @@ static ssize_t jfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
> }
>
> const struct address_space_operations jfs_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = jfs_readpage,
> .readahead = jfs_readahead,
> .writepage = jfs_writepage,
> diff --git a/fs/minix/inode.c b/fs/minix/inode.c
> index a532a99bbe81..a71f1cf894b9 100644
> --- a/fs/minix/inode.c
> +++ b/fs/minix/inode.c
> @@ -442,6 +442,7 @@ static sector_t minix_bmap(struct address_space *mapping, sector_t block)
> }
>
> static const struct address_space_operations minix_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = minix_readpage,
> .writepage = minix_writepage,
> .write_begin = minix_write_begin,
> diff --git a/fs/nilfs2/mdt.c b/fs/nilfs2/mdt.c
> index c0361ce45f62..97769fe4d588 100644
> --- a/fs/nilfs2/mdt.c
> +++ b/fs/nilfs2/mdt.c
> @@ -434,6 +434,7 @@ nilfs_mdt_write_page(struct page *page, struct writeback_control *wbc)
>
>
> static const struct address_space_operations def_mdt_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .writepage = nilfs_mdt_write_page,
> };
>
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index 1294925ac94a..b3517de178ff 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -2454,6 +2454,7 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
> }
>
> const struct address_space_operations ocfs2_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = ocfs2_readpage,
> .readahead = ocfs2_readahead,
> .writepage = ocfs2_writepage,
> diff --git a/fs/omfs/file.c b/fs/omfs/file.c
> index 11e733aab25d..89725b15a64b 100644
> --- a/fs/omfs/file.c
> +++ b/fs/omfs/file.c
> @@ -372,6 +372,7 @@ const struct inode_operations omfs_file_inops = {
> };
>
> const struct address_space_operations omfs_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = omfs_readpage,
> .readahead = omfs_readahead,
> .writepage = omfs_writepage,
> diff --git a/fs/sysv/itree.c b/fs/sysv/itree.c
> index 8b2e99b7bc9f..749385015a8d 100644
> --- a/fs/sysv/itree.c
> +++ b/fs/sysv/itree.c
> @@ -495,6 +495,7 @@ static sector_t sysv_bmap(struct address_space *mapping, sector_t block)
> }
>
> const struct address_space_operations sysv_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = sysv_readpage,
> .writepage = sysv_writepage,
> .write_begin = sysv_write_begin,
> diff --git a/fs/udf/file.c b/fs/udf/file.c
> index 2846dcd92197..1baff8ddb754 100644
> --- a/fs/udf/file.c
> +++ b/fs/udf/file.c
> @@ -125,6 +125,7 @@ static int udf_adinicb_write_end(struct file *file, struct address_space *mappin
> }
>
> const struct address_space_operations udf_adinicb_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = udf_adinicb_readpage,
> .writepage = udf_adinicb_writepage,
> .write_begin = udf_adinicb_write_begin,
> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index 0dd2f93ac048..4917670860a0 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -235,6 +235,7 @@ static sector_t udf_bmap(struct address_space *mapping, sector_t block)
> }
>
> const struct address_space_operations udf_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = udf_readpage,
> .readahead = udf_readahead,
> .writepage = udf_writepage,
> diff --git a/fs/ufs/inode.c b/fs/ufs/inode.c
> index debc282c1bb4..ac628de69601 100644
> --- a/fs/ufs/inode.c
> +++ b/fs/ufs/inode.c
> @@ -526,6 +526,7 @@ static sector_t ufs_bmap(struct address_space *mapping, sector_t block)
> }
>
> const struct address_space_operations ufs_aops = {
> + .set_page_dirty = __set_page_dirty_buffers,
> .readpage = ufs_readpage,
> .writepage = ufs_writepage,
> .write_begin = ufs_write_begin,
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 0062d5c57d41..081fa02236c2 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -32,7 +32,6 @@
> #include <linux/sysctl.h>
> #include <linux/cpu.h>
> #include <linux/syscalls.h>
> -#include <linux/buffer_head.h> /* __set_page_dirty_buffers */
> #include <linux/pagevec.h>
> #include <linux/timer.h>
> #include <linux/sched/rt.h>
> @@ -2546,13 +2545,9 @@ EXPORT_SYMBOL(redirty_page_for_writepage);
> /*
> * Dirty a page.
> *
> - * For pages with a mapping this should be done under the page lock
> - * for the benefit of asynchronous memory errors who prefer a consistent
> - * dirty state. This rule can be broken in some special cases,
> - * but should be better not to.
> - *
> - * If the mapping doesn't provide a set_page_dirty a_op, then
> - * just fall through and assume that it wants buffer_heads.
> + * For pages with a mapping this should be done under the page lock for the
> + * benefit of asynchronous memory errors who prefer a consistent dirty state.
> + * This rule can be broken in some special cases, but should be better not to.
> */
> int set_page_dirty(struct page *page)
> {
> @@ -2560,7 +2555,6 @@ int set_page_dirty(struct page *page)
>
> page = compound_head(page);
> if (likely(mapping)) {
> - int (*spd)(struct page *) = mapping->a_ops->set_page_dirty;
> /*
> * readahead/lru_deactivate_page could remain
> * PG_readahead/PG_reclaim due to race with end_page_writeback
> @@ -2573,11 +2567,7 @@ int set_page_dirty(struct page *page)
> */
> if (PageReclaim(page))
> ClearPageReclaim(page);
> -#ifdef CONFIG_BLOCK
> - if (!spd)
> - spd = __set_page_dirty_buffers;
> -#endif
> - return (*spd)(page);
> + return mapping->a_ops->set_page_dirty(page);
> }
> if (!PageDirty(page)) {
> if (!TestSetPageDirty(page))
> --
> 2.30.2
>
--
Jan Kara <[email protected]>
SUSE Labs, CR

2021-06-14 10:06:59

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 3/3] mm: require ->set_page_dirty to be explicitly wire up

On Mon, Jun 14, 2021 at 11:51:07AM +0200, Jan Kara wrote:
> On Mon 14-06-21 08:15:12, Christoph Hellwig wrote:
> > Remove the CONFIG_BLOCK default to __set_page_dirty_buffers and just
> > wire that method up for the missing instances.
> >
> > Signed-off-by: Christoph Hellwig <[email protected]>
>
> Make sense. Did you somehow autogenerate this? If this patch would race
> with addition of new aops struct, we'd get null-ptr-defer out of that so
> maybe providing the script would be better. But other than that the changes
> look good to me. You can add:

No, this was done manually as I audited all instances for actually being
able to dirty pages.

2021-06-14 12:28:06

by Matthew Wilcox

[permalink] [raw]
Subject: Re: remove the implicit .set_page_dirty default

On Mon, Jun 14, 2021 at 08:15:09AM +0200, Christoph Hellwig wrote:
> Hi all,
>
> this series cleans up a few lose ends around ->set_page_dirty, most
> importantly removes the default to the buffer head based on if no
> method is wired up.

i have a somewhat similar series in the works ...

https://git.infradead.org/users/willy/pagecache.git/commitdiff/1e7e8c2d82666b55690705d5bbe908e31d437edb
https://git.infradead.org/users/willy/pagecache.git/commitdiff/bf767a4969c0bc6735275ff7457a8082eef4c3fd

... the other patches rather depend on the folio work.

2021-06-14 12:34:43

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH 1/3] fs: unexport __set_page_dirty

On Mon, Jun 14, 2021 at 08:15:10AM +0200, Christoph Hellwig wrote:
> __set_page_dirty is only used by built-in code.
>
> Signed-off-by: Christoph Hellwig <[email protected]>

Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>

You might also want to do the equivalent of this:

https://git.infradead.org/users/willy/pagecache.git/commitdiff/19b3bf0d1a51f41ce5450fdd863969c3d32dfe12

2021-06-14 15:54:13

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 1/3] fs: unexport __set_page_dirty

On Mon, Jun 14, 2021 at 01:32:42PM +0100, Matthew Wilcox wrote:
> On Mon, Jun 14, 2021 at 08:15:10AM +0200, Christoph Hellwig wrote:
> > __set_page_dirty is only used by built-in code.
> >
> > Signed-off-by: Christoph Hellwig <[email protected]>
>
> Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>
>
> You might also want to do the equivalent of this:
>
> https://git.infradead.org/users/willy/pagecache.git/commitdiff/19b3bf0d1a51f41ce5450fdd863969c3d32dfe12

This looks nice, but not really directly related. I think you
should just send it to Andrew.

2021-06-14 15:57:19

by Christoph Hellwig

[permalink] [raw]
Subject: Re: remove the implicit .set_page_dirty default

On Mon, Jun 14, 2021 at 01:23:22PM +0100, Matthew Wilcox wrote:
> i have a somewhat similar series in the works ...
>
> https://git.infradead.org/users/willy/pagecache.git/commitdiff/1e7e8c2d82666b55690705d5bbe908e31d437edb
> https://git.infradead.org/users/willy/pagecache.git/commitdiff/bf767a4969c0bc6735275ff7457a8082eef4c3fd
>
> ... the other patches rather depend on the folio work.

Yes, these looks useful to me as well.

2021-06-14 15:57:19

by Christoph Hellwig

[permalink] [raw]
Subject: Re: remove the implicit .set_page_dirty default

On Mon, Jun 14, 2021 at 05:53:33PM +0200, Christoph Hellwig wrote:
> On Mon, Jun 14, 2021 at 01:23:22PM +0100, Matthew Wilcox wrote:
> > i have a somewhat similar series in the works ...
> >
> > https://git.infradead.org/users/willy/pagecache.git/commitdiff/1e7e8c2d82666b55690705d5bbe908e31d437edb
> > https://git.infradead.org/users/willy/pagecache.git/commitdiff/bf767a4969c0bc6735275ff7457a8082eef4c3fd
> >
> > ... the other patches rather depend on the folio work.
>
> Yes, these looks useful to me as well.


And in fact I suspect the code in __set_page_dirty_no_writeback should
really be the default if no ->set_page_dirty is set up. It is the
same code as the no-mapping case and really makes sense as the default
case..

2021-06-14 16:19:24

by Matthew Wilcox

[permalink] [raw]
Subject: Re: remove the implicit .set_page_dirty default

On Mon, Jun 14, 2021 at 05:55:30PM +0200, Christoph Hellwig wrote:
> On Mon, Jun 14, 2021 at 05:53:33PM +0200, Christoph Hellwig wrote:
> > On Mon, Jun 14, 2021 at 01:23:22PM +0100, Matthew Wilcox wrote:
> > > i have a somewhat similar series in the works ...
> > >
> > > https://git.infradead.org/users/willy/pagecache.git/commitdiff/1e7e8c2d82666b55690705d5bbe908e31d437edb
> > > https://git.infradead.org/users/willy/pagecache.git/commitdiff/bf767a4969c0bc6735275ff7457a8082eef4c3fd
> > >
> > > ... the other patches rather depend on the folio work.
> >
> > Yes, these looks useful to me as well.
>
>
> And in fact I suspect the code in __set_page_dirty_no_writeback should
> really be the default if no ->set_page_dirty is set up. It is the
> same code as the no-mapping case and really makes sense as the default
> case..

yes; i was even wondering if that should be conditional on one of the
"no writeback" bits in mapping ...

2021-06-19 03:30:20

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [PATCH 3/3] mm: require ->set_page_dirty to be explicitly wire up

14.06.2021 09:15, Christoph Hellwig пишет:
> Remove the CONFIG_BLOCK default to __set_page_dirty_buffers and just
> wire that method up for the missing instances.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> fs/adfs/inode.c | 1 +
> fs/affs/file.c | 2 ++
> fs/bfs/file.c | 1 +
> fs/block_dev.c | 1 +
> fs/exfat/inode.c | 1 +
> fs/ext2/inode.c | 2 ++
> fs/fat/inode.c | 1 +
> fs/gfs2/meta_io.c | 2 ++
> fs/hfs/inode.c | 2 ++
> fs/hfsplus/inode.c | 2 ++
> fs/hpfs/file.c | 1 +
> fs/jfs/inode.c | 1 +
> fs/minix/inode.c | 1 +
> fs/nilfs2/mdt.c | 1 +
> fs/ocfs2/aops.c | 1 +
> fs/omfs/file.c | 1 +
> fs/sysv/itree.c | 1 +
> fs/udf/file.c | 1 +
> fs/udf/inode.c | 1 +
> fs/ufs/inode.c | 1 +
> mm/page-writeback.c | 18 ++++--------------
> 21 files changed, 29 insertions(+), 14 deletions(-)

The ecryptfs is now crashing with NULL deference, please fix.

2021-06-21 05:27:36

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 3/3] mm: require ->set_page_dirty to be explicitly wire up

On Fri, Jun 18, 2021 at 10:23:55PM +0300, Dmitry Osipenko wrote:
> > 21 files changed, 29 insertions(+), 14 deletions(-)
>
> The ecryptfs is now crashing with NULL deference, please fix.

Which means it crashed the same before on configs without CONFIG_BLOCK.

Tyler, can you look at what ecryptfs should do for ->set_page_dirty?
Currently it implicitly gets __set_page_dirty_buffers for kernels
with COFIG_BLOCK set, but given that this function looks at buffer_heads
that ecryptfs doesn't use it can't really be the right choice.
__set_page_dirty_nobuffers will probably work, but I'd love to see an
audit of the page dirtying and writeback for ecryptfs while we're at it.