[PATCH 0/2] mtd: rawnand: nandsim: stop pre-populating the cache file's page cache

[PATCH 0/2] mtd: rawnand: nandsim: stop pre-populating the cache file's page cache

[Tal Zussman]

nandsim runs its cache file I/O under PF_MEMALLOC and pre-populates
the page cache before each access, both to get NOFS allocation out of
kernel_read() and kernel_write(). Use memalloc_nofs_save() for that
instead and remove the pre-population. The pre-population is the
driver's only struct page usage and one of the last users of
find_get_page() and find_or_create_page().

Tested on KVM using the MTD test modules, nandtest, and fsx/fsstress on
UBIFS over a nandsim cache file, with no errors and a 6-13% speedup in
mtd_speedtest.

---
Tal Zussman (2):
      mtd: rawnand: nandsim: use a NOFS scope for cache file I/O
      mtd: rawnand: nandsim: stop pre-populating the cache file's page cache

 drivers/mtd/nand/raw/nandsim.c | 85 +++++++-----------------------------------
 1 file changed, 14 insertions(+), 71 deletions(-)
---
base-commit: 30daaf3e7897b4a8d9b72e5b0c86dd6e2a4d7d85
change-id: 20261012-nandsim-nofs-8f3ae982181e

Best regards,
--  
Tal Zussman <tz2294@columbia.edu>


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/2] mtd: rawnand: nandsim: use a NOFS scope for cache file I/O

[Tal Zussman]

nandsim reads and writes its cache file from inside MTD operations,
which the filesystem on top of it can issue from writeback, so an
allocation in that path must not recurse into filesystem reclaim. The
driver ensures that by running kernel_read() and kernel_write() under
PF_MEMALLOC, which is more than it needs. Use memalloc_nofs_save()
instead.

Signed-off-by: Tal Zussman <tz2294@columbia.edu>
---
 drivers/mtd/nand/raw/nandsim.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/raw/nandsim.c b/drivers/mtd/nand/raw/nandsim.c
index fe968037f75a..4559df2cfa3c 100644
--- a/drivers/mtd/nand/raw/nandsim.c
+++ b/drivers/mtd/nand/raw/nandsim.c
@@ -1342,14 +1342,14 @@ static ssize_t ns_read_file(struct nandsim *ns, struct file *file, void *buf,
 {
 	ssize_t tx;
 	int err;
-	unsigned int noreclaim_flag;
+	unsigned int nofs_flag;
 
 	err = ns_get_pages(ns, file, count, pos);
 	if (err)
 		return err;
-	noreclaim_flag = memalloc_noreclaim_save();
+	nofs_flag = memalloc_nofs_save();
 	tx = kernel_read(file, buf, count, &pos);
-	memalloc_noreclaim_restore(noreclaim_flag);
+	memalloc_nofs_restore(nofs_flag);
 	ns_put_pages(ns);
 	return tx;
 }
@@ -1359,14 +1359,14 @@ static ssize_t ns_write_file(struct nandsim *ns, struct file *file, void *buf,
 {
 	ssize_t tx;
 	int err;
-	unsigned int noreclaim_flag;
+	unsigned int nofs_flag;
 
 	err = ns_get_pages(ns, file, count, pos);
 	if (err)
 		return err;
-	noreclaim_flag = memalloc_noreclaim_save();
+	nofs_flag = memalloc_nofs_save();
 	tx = kernel_write(file, buf, count, &pos);
-	memalloc_noreclaim_restore(noreclaim_flag);
+	memalloc_nofs_restore(nofs_flag);
 	ns_put_pages(ns);
 	return tx;
 }

-- 
2.39.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/2] mtd: rawnand: nandsim: stop pre-populating the cache file's page cache

[Tal Zussman]

ns_get_pages() grabs the page cache pages for a cache file access
ahead of the read or write so that kernel_read() and kernel_write()
find them and don't allocate on their own. Now that the I/O runs under
memalloc_nofs_save(), the pre-population no longer buys anything.

Remove it, and the nandsim parameter of the two helpers that only it
needed. This removes the last struct page usage in the driver, and its
last callers of find_get_page() and find_or_create_page().

Signed-off-by: Tal Zussman <tz2294@columbia.edu>
---
 drivers/mtd/nand/raw/nandsim.c | 73 +++++-------------------------------------
 1 file changed, 8 insertions(+), 65 deletions(-)

diff --git a/drivers/mtd/nand/raw/nandsim.c b/drivers/mtd/nand/raw/nandsim.c
index 4559df2cfa3c..7de2f584795e 100644
--- a/drivers/mtd/nand/raw/nandsim.c
+++ b/drivers/mtd/nand/raw/nandsim.c
@@ -30,7 +30,6 @@
 #include <linux/sched.h>
 #include <linux/sched/mm.h>
 #include <linux/fs.h>
-#include <linux/pagemap.h>
 #include <linux/seq_file.h>
 #include <linux/debugfs.h>
 
@@ -272,9 +271,6 @@ MODULE_PARM_DESC(bch,		 "Enable BCH ecc and set how many bits should "
  */
 #define NS_MAX_PREVSTATES 1
 
-/* Maximum page cache pages needed to read or write a NAND page to the cache_file */
-#define NS_MAX_HELD_PAGES 16
-
 /*
  * A union to represent flash memory contents and flash buffer.
  */
@@ -353,8 +349,6 @@ struct nandsim {
 	struct file *cfile; /* Open file */
 	unsigned long *pages_written; /* Which pages have been written */
 	void *file_buf;
-	struct page *held_pages[NS_MAX_HELD_PAGES];
-	int held_cnt;
 
 	/* debugfs entry */
 	struct dentry *dent;
@@ -1297,77 +1291,27 @@ static int ns_find_operation(struct nandsim *ns, uint32_t flag)
 	return -1;
 }
 
-static void ns_put_pages(struct nandsim *ns)
-{
-	int i;
-
-	for (i = 0; i < ns->held_cnt; i++)
-		put_page(ns->held_pages[i]);
-}
-
-/* Get page cache pages in advance to provide NOFS memory allocation */
-static int ns_get_pages(struct nandsim *ns, struct file *file, size_t count,
-			loff_t pos)
-{
-	pgoff_t index, start_index, end_index;
-	struct page *page;
-	struct address_space *mapping = file->f_mapping;
-
-	start_index = pos >> PAGE_SHIFT;
-	end_index = (pos + count - 1) >> PAGE_SHIFT;
-	if (end_index - start_index + 1 > NS_MAX_HELD_PAGES)
-		return -EINVAL;
-	ns->held_cnt = 0;
-	for (index = start_index; index <= end_index; index++) {
-		page = find_get_page(mapping, index);
-		if (page == NULL) {
-			page = find_or_create_page(mapping, index, GFP_NOFS);
-			if (page == NULL) {
-				write_inode_now(mapping->host, 1);
-				page = find_or_create_page(mapping, index, GFP_NOFS);
-			}
-			if (page == NULL) {
-				ns_put_pages(ns);
-				return -ENOMEM;
-			}
-			unlock_page(page);
-		}
-		ns->held_pages[ns->held_cnt++] = page;
-	}
-	return 0;
-}
-
-static ssize_t ns_read_file(struct nandsim *ns, struct file *file, void *buf,
-			    size_t count, loff_t pos)
+static ssize_t ns_read_file(struct file *file, void *buf, size_t count,
+			    loff_t pos)
 {
 	ssize_t tx;
-	int err;
 	unsigned int nofs_flag;
 
-	err = ns_get_pages(ns, file, count, pos);
-	if (err)
-		return err;
 	nofs_flag = memalloc_nofs_save();
 	tx = kernel_read(file, buf, count, &pos);
 	memalloc_nofs_restore(nofs_flag);
-	ns_put_pages(ns);
 	return tx;
 }
 
-static ssize_t ns_write_file(struct nandsim *ns, struct file *file, void *buf,
-			     size_t count, loff_t pos)
+static ssize_t ns_write_file(struct file *file, void *buf, size_t count,
+			     loff_t pos)
 {
 	ssize_t tx;
-	int err;
 	unsigned int nofs_flag;
 
-	err = ns_get_pages(ns, file, count, pos);
-	if (err)
-		return err;
 	nofs_flag = memalloc_nofs_save();
 	tx = kernel_write(file, buf, count, &pos);
 	memalloc_nofs_restore(nofs_flag);
-	ns_put_pages(ns);
 	return tx;
 }
 
@@ -1436,8 +1380,7 @@ static void ns_read_page(struct nandsim *ns, int num)
 			if (ns_do_read_error(ns, num))
 				return;
 			pos = (loff_t)NS_RAW_OFFSET(ns) + ns->regs.off;
-			tx = ns_read_file(ns, ns->cfile, ns->buf.byte, num,
-					  pos);
+			tx = ns_read_file(ns->cfile, ns->buf.byte, num, pos);
 			if (tx != num) {
 				NS_ERR("read_page: read error for page %d ret %ld\n", ns->regs.row, (long)tx);
 				return;
@@ -1511,7 +1454,7 @@ static int ns_prog_page(struct nandsim *ns, int num)
 			memset(ns->file_buf, 0xff, ns->geom.pgszoob);
 		} else {
 			all = 0;
-			tx = ns_read_file(ns, ns->cfile, pg_off, num, off);
+			tx = ns_read_file(ns->cfile, pg_off, num, off);
 			if (tx != num) {
 				NS_ERR("prog_page: read error for page %d ret %ld\n", ns->regs.row, (long)tx);
 				return -1;
@@ -1521,7 +1464,7 @@ static int ns_prog_page(struct nandsim *ns, int num)
 			pg_off[i] &= ns->buf.byte[i];
 		if (all) {
 			loff_t pos = (loff_t)ns->regs.row * ns->geom.pgszoob;
-			tx = ns_write_file(ns, ns->cfile, ns->file_buf,
+			tx = ns_write_file(ns->cfile, ns->file_buf,
 					   ns->geom.pgszoob, pos);
 			if (tx != ns->geom.pgszoob) {
 				NS_ERR("prog_page: write error for page %d ret %ld\n", ns->regs.row, (long)tx);
@@ -1529,7 +1472,7 @@ static int ns_prog_page(struct nandsim *ns, int num)
 			}
 			__set_bit(ns->regs.row, ns->pages_written);
 		} else {
-			tx = ns_write_file(ns, ns->cfile, pg_off, num, off);
+			tx = ns_write_file(ns->cfile, pg_off, num, off);
 			if (tx != num) {
 				NS_ERR("prog_page: write error for page %d ret %ld\n", ns->regs.row, (long)tx);
 				return -1;

-- 
2.39.5


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: rawnand: nandsim: use a NOFS scope for cache file I/O

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mtd: nandsim: IO reclaim self-deadlock due to memalloc_nofs_save()
--

commit 6c4530163aa059b5f98e17ba82a9c2b06c3f48bb
Author: Tal Zussman <tz2294@columbia.edu>

mtd: rawnand: nandsim: use a NOFS scope for cache file I/O

This commit replaces memalloc_noreclaim_save() with memalloc_nofs_save() to
prevent allocations during nandsim cache file operations from recursing into
filesystem reclaim.

> diff --git a/drivers/mtd/nand/raw/nandsim.c b/drivers/mtd/nand/raw/nandsim.c
> index fe968037f75a2..4559df2cfa3cc 100644
> --- a/drivers/mtd/nand/raw/nandsim.c
> +++ b/drivers/mtd/nand/raw/nandsim.c
[ ... ]
> @@ -1342,14 +1342,14 @@ static ssize_t ns_read_file(struct nandsim *ns, struct file *file, void *buf,
>  {
>  	ssize_t tx;
>  	int err;
> -	unsigned int noreclaim_flag;
> +	unsigned int nofs_flag;
>  
>  	err = ns_get_pages(ns, file, count, pos);
>  	if (err)
>  		return err;
> -	noreclaim_flag = memalloc_noreclaim_save();
> +	nofs_flag = memalloc_nofs_save();
>  	tx = kernel_read(file, buf, count, &pos);
> -	memalloc_noreclaim_restore(noreclaim_flag);

[Severity: High]
Can this cause an IO reclaim self-deadlock?

Nandsim executes MTD operations while holding the NAND subsystem device
lock (chip->lock). While memalloc_nofs_save() strips the filesystem reclaim
flags, it leaves direct IO reclaim enabled.

If memory allocations within kernel_read() trigger direct reclaim, the
shrinker might attempt to write out dirty pages belonging to this same
device (for example, if used as a swap device or via mtdblock).

Writing out these pages would route through the block layer back to the MTD
layer, which will attempt to acquire chip->lock again, resulting in a
self-deadlock.

Should this use memalloc_noio_save() instead to safely prevent reclaim
from issuing any block IO while the MTD lock is held?

>  	ns_put_pages(ns);
>  	return tx;
>  }
> @@ -1359,14 +1359,14 @@ static ssize_t ns_write_file(struct nandsim *ns, struct file *file, void *buf,
>  {
>  	ssize_t tx;
>  	int err;
> -	unsigned int noreclaim_flag;
> +	unsigned int nofs_flag;
>  
>  	err = ns_get_pages(ns, file, count, pos);
>  	if (err)
>  		return err;
> -	noreclaim_flag = memalloc_noreclaim_save();
> +	nofs_flag = memalloc_nofs_save();
>  	tx = kernel_write(file, buf, count, &pos);
> -	memalloc_noreclaim_restore(noreclaim_flag);

[Severity: High]
Does kernel_write() face the same IO reclaim self-deadlock risk here?

Allocations within kernel_write() could also trigger direct IO reclaim
while the chip lock is held. Using memalloc_noio_save() would protect this
path as well.

>  	ns_put_pages(ns);
>  	return tx;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-nandsim-nofs-v1-0-47e49f37e387@columbia.edu?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/