[PATCH] JFFS2: Retry Medium Scan Buffer Allocations

[PATCH] JFFS2: Retry Medium Scan Buffer Allocations

[Grant Erickson]

When handling a JFFS2 medium scan request exponentially back off on
the size of the requested scan buffer until it succeeds or until the
requested scan buffer size falls below a page. This helps ensure the
allocation and subsequent scan operation can succeed under low-memory,
highly-fragmented situations albeit somewhat more slowly.

Signed-off-by: Grant Erickson <marathon96@gmail.com>
---
 fs/jffs2/scan.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/jffs2/scan.c b/fs/jffs2/scan.c
index b632ddd..4d8746d 100644
--- a/fs/jffs2/scan.c
+++ b/fs/jffs2/scan.c
@@ -118,11 +118,14 @@ int jffs2_scan_medium(struct jffs2_sb_info *c)
 			buf_size = PAGE_SIZE;
 
 		/* Respect kmalloc limitations */
-		if (buf_size > 128*1024)
-			buf_size = 128*1024;
+		buf_size = min_t(uint32_t, buf_size, 128*1024);
+
+		do {
+			D1(printk(KERN_DEBUG "Trying to allocate readbuf of %d "
+					  "bytes\n", buf_size));
+			flashbuf = kmalloc(buf_size, GFP_KERNEL);
+		} while (!flashbuf && ((buf_size >>= 1) >= PAGE_SIZE));
 
-		D1(printk(KERN_DEBUG "Allocating readbuf of %d bytes\n", buf_size));
-		flashbuf = kmalloc(buf_size, GFP_KERNEL);
 		if (!flashbuf)
 			return -ENOMEM;
 	}
-- 
1.7.4.2

[PATCH v2] JFFS2: Retry Medium Scan Buffer Allocations

[Grant Erickson]

When handling a JFFS2 medium scan request exponentially back off on
the size of the requested scan buffer until it succeeds or until the
requested scan buffer size falls below a page. This helps ensure the
allocation and subsequent scan operation can succeed under low-memory,
highly-fragmented situations albeit somewhat more slowly.

  v2: Added __GFP_NOWARN flag as recommended by Artem.

Signed-off-by: Grant Erickson <marathon96@gmail.com>
---
 fs/jffs2/scan.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/jffs2/scan.c b/fs/jffs2/scan.c
index b632ddd..4d8746d 100644
--- a/fs/jffs2/scan.c
+++ b/fs/jffs2/scan.c
@@ -118,11 +118,14 @@ int jffs2_scan_medium(struct jffs2_sb_info *c)
 			buf_size = PAGE_SIZE;
 
 		/* Respect kmalloc limitations */
-		if (buf_size > 128*1024)
-			buf_size = 128*1024;
+		buf_size = min_t(uint32_t, buf_size, 128*1024);
+
+		do {
+			D1(printk(KERN_DEBUG "Trying to allocate readbuf of %d "
+					  "bytes\n", buf_size));
+			flashbuf = kmalloc(buf_size, GFP_KERNEL | __GFP_NOWARN);
+		} while (!flashbuf && ((buf_size >>= 1) >= PAGE_SIZE));
 
-		D1(printk(KERN_DEBUG "Allocating readbuf of %d bytes\n", buf_size));
-		flashbuf = kmalloc(buf_size, GFP_KERNEL);
 		if (!flashbuf)
 			return -ENOMEM;
 	}
-- 
1.7.4.2

RE: [PATCH] MTD: Retry Read/Write Transfer Buffer Allocations

[Grant Erickson]

As promised, this is the alternate, work-in-progress implementation of
this patch using the more complex get_user_pages and iovecs to perform
a user-requested read or write operation, reusing the buffer passed
from user-space.

As implemented below, reads work 100% of the time for the few test
cases I've thrown at it (fw_printenv and nanddump). However, writes
will not work in this implementation unless the size and offset meet
the alignment requirements stipulated by nand_do_write_ops. Addressing
this will require deblocking the head and tail of the transfer.

I see this as a follow on, interative approach to the simpler,
short-term patch submitted by Jarkko and myself. Hopefully, we can
upstream that patch now and follow up with this later when complete.

Comments and tweaks welcomed.

Best,

Grant

---
 drivers/mtd/mtdchar.c |  342 ++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 265 insertions(+), 77 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 145b3d0d..cd33128 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -51,6 +51,18 @@ struct mtd_file_info {
 	enum mtd_file_modes mode;
 };
 
+/*
+ * Data structure to handle scatter/gather transfer pages and
+ * page extents for user-space read and write requests.
+ */
+struct mtd_xio {
+	bool write;
+	struct page **pages;
+	unsigned long npages;
+	struct iovec *iovecs;
+	unsigned long niovecs;
+};
+
 static loff_t mtd_lseek (struct file *file, loff_t offset, int orig)
 {
 	struct mtd_file_info *mfi = file->private_data;
@@ -166,60 +178,170 @@ static int mtd_close(struct inode *inode, struct file *file)
 	return 0;
 } /* mtd_close */
 
-/* FIXME: This _really_ needs to die. In 2.5, we should lock the
-   userspace buffer down and use it directly with readv/writev.
-*/
-#define MAX_KMALLOC_SIZE 0x20000
+static inline bool pages_are_adjacent(struct page *first, struct page *second)
+{
+    return ((page_address(first) + PAGE_SIZE) == page_address(second));
+}
 
-static ssize_t mtd_read(struct file *file, char __user *buf, size_t count,loff_t *ppos)
+static void pages_to_iovecs(size_t len, unsigned long off, struct page **pages, unsigned long npages, struct iovec *iovecs, unsigned long *niovecsp)
 {
-	struct mtd_file_info *mfi = file->private_data;
-	struct mtd_info *mtd = mfi->mtd;
-	size_t retlen=0;
-	size_t total_retlen=0;
-	int ret=0;
-	int len;
-	char *kbuf;
+	unsigned long iovec_len = 0;
+	size_t head_len, tail_len;
+	struct page *last_page, *curr_page;
+	void *address;
+	unsigned long p, v;
 
-	DEBUG(MTD_DEBUG_LEVEL0,"MTD_read\n");
+	/* Handle the head. */
 
-	if (*ppos + count > mtd->size)
-		count = mtd->size - *ppos;
+	head_len = min_t(size_t, len, PAGE_SIZE - off);
+	tail_len = (len - head_len) % PAGE_SIZE;
 
-	if (!count)
-		return 0;
+	iovecs[0].iov_base = page_address(pages[0]) + off;
+	iovecs[0].iov_len  = head_len;
 
-	/* FIXME: Use kiovec in 2.5 to lock down the user's buffers
-	   and pass them directly to the MTD functions */
+	/* Handle the body + tail. */
 
-	if (count > MAX_KMALLOC_SIZE)
-		kbuf=kmalloc(MAX_KMALLOC_SIZE, GFP_KERNEL);
-	else
-		kbuf=kmalloc(count, GFP_KERNEL);
+	for (v = 0, p = 1; p < npages; p++) {
+		last_page = pages[p - 1];
+		curr_page = pages[p];
+		address = page_address(curr_page);
 
-	if (!kbuf)
-		return -ENOMEM;
+		if (!pages_are_adjacent(last_page, curr_page)) {
+		    iovecs[++v].iov_base = address;
+		}
+
+		iovecs[v].iov_len += PAGE_SIZE;
+	}
+
+	/* Fix-up the tail length. */
+
+	iovecs[v].iov_len -= (PAGE_SIZE - tail_len);
+
+	*niovecsp = v + 1;
+
+	for (v = 0; v < *niovecsp; v++) {
+		iovec_len += iovecs[v].iov_len;
+	}
+}
+
+static void put_pages(struct page **pages, unsigned long npages, bool write)
+{
+	unsigned long p;
+	struct page *page;
+
+	for (p = 0; p < npages; p++) {
+		page = pages[p];
+		if (write)
+			set_page_dirty_lock(page);
+		put_page(page);
+	}
+}
+
+static int mtd_get_user_pages(struct mtd_xio *mxio, void __user *base, size_t len, bool write)
+{
+	unsigned long ustart, off, npages, uend, niovecs;
+	struct page **pages;
+	struct iovec *iovecs;
+	int status;
+
+	if (unlikely(!access_ok(write ? VERIFY_WRITE : VERIFY_READ, base, len))) {
+		status = -EFAULT;
+		goto done;
+	}
+
+	ustart = (unsigned long)base & PAGE_MASK;
+	off = (unsigned long)base & ~PAGE_MASK;
+	npages = (off + len + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	uend = (unsigned long)(base + len);
+
+	niovecs = npages;
+
+	pages = kmalloc(npages * sizeof (struct page *), GFP_KERNEL);
+	if (!pages) {
+		status = -ENOMEM;
+		goto done;
+	}
+
+	iovecs = kzalloc(niovecs * sizeof (struct iovec), GFP_KERNEL);
+	if (!iovecs) {
+		status = -ENOMEM;
+		goto done_free_pages;
+	}
+
+	status = get_user_pages_fast(ustart, npages, write, pages);
+	if (status < 0)
+		goto done_free_iovecs;
+
+	if (status != npages) {
+		npages = status;
+		status = -EFAULT;
+		goto done_pages;
+	}
+
+	pages_to_iovecs(len, off, pages, npages, iovecs, &niovecs);
+
+	mxio->write	= write;
+	mxio->pages	= pages;
+	mxio->npages	= npages;
+	mxio->iovecs	= iovecs;
+	mxio->niovecs	= niovecs;
+
+	return status;
+
+ done_pages:
+	put_pages(pages, npages, write);
+
+ done_free_iovecs:
+	kfree(iovecs);
+
+ done_free_pages:
+	kfree(pages);
+
+ done:
+	return status;
+}
+
+static void mtd_put_user_pages(struct mtd_xio *mxio)
+{
+	if (!mxio)
+		return;
+
+	if (mxio->npages && mxio->pages) {
+		put_pages(mxio->pages, mxio->npages, mxio->write);
+		kfree(mxio->pages);
+	}
+
+	if (mxio->niovecs && mxio->iovecs) {
+		kfree(mxio->iovecs);
+	}
+}
+
+static ssize_t mtd_do_read(struct file *file, char __kernel *buf, size_t count, loff_t *ppos)
+{
+	struct mtd_file_info *mfi = file->private_data;
+	struct mtd_info *mtd = mfi->mtd;
+	size_t retlen=0;
+	size_t total_retlen=0;
+	int ret=0;
+	int len;
 
 	while (count) {
 
-		if (count > MAX_KMALLOC_SIZE)
-			len = MAX_KMALLOC_SIZE;
-		else
-			len = count;
+		len = count;
 
 		switch (mfi->mode) {
 		case MTD_MODE_OTP_FACTORY:
-			ret = mtd->read_fact_prot_reg(mtd, *ppos, len, &retlen, kbuf);
+			ret = mtd->read_fact_prot_reg(mtd, *ppos, len, &retlen, buf);
 			break;
 		case MTD_MODE_OTP_USER:
-			ret = mtd->read_user_prot_reg(mtd, *ppos, len, &retlen, kbuf);
+			ret = mtd->read_user_prot_reg(mtd, *ppos, len, &retlen, buf);
 			break;
 		case MTD_MODE_RAW:
 		{
 			struct mtd_oob_ops ops;
 
 			ops.mode = MTD_OOB_RAW;
-			ops.datbuf = kbuf;
+			ops.datbuf = buf;
 			ops.oobbuf = NULL;
 			ops.len = len;
 
@@ -228,7 +350,7 @@ static ssize_t mtd_read(struct file *file, char __user *buf, size_t count,loff_t
 			break;
 		}
 		default:
-			ret = mtd->read(mtd, *ppos, len, &retlen, kbuf);
+			ret = mtd->read(mtd, *ppos, len, &retlen, buf);
 		}
 		/* Nand returns -EBADMSG on ecc errors, but it returns
 		 * the data. For our userspace tools it is important
@@ -241,12 +363,7 @@ static ssize_t mtd_read(struct file *file, char __user *buf, size_t count,loff_t
 		 */
 		if (!ret || (ret == -EUCLEAN) || (ret == -EBADMSG)) {
 			*ppos += retlen;
-			if (copy_to_user(buf, kbuf, retlen)) {
-				kfree(kbuf);
-				return -EFAULT;
-			}
-			else
-				total_retlen += retlen;
+			total_retlen += retlen;
 
 			count -= retlen;
 			buf += retlen;
@@ -254,56 +371,26 @@ static ssize_t mtd_read(struct file *file, char __user *buf, size_t count,loff_t
 				count = 0;
 		}
 		else {
-			kfree(kbuf);
 			return ret;
 		}
 
 	}
 
-	kfree(kbuf);
 	return total_retlen;
-} /* mtd_read */
+}
 
-static ssize_t mtd_write(struct file *file, const char __user *buf, size_t count,loff_t *ppos)
+static ssize_t mtd_do_write(struct file *file, const char __kernel *buf, size_t count, loff_t *ppos)
 {
 	struct mtd_file_info *mfi = file->private_data;
 	struct mtd_info *mtd = mfi->mtd;
-	char *kbuf;
 	size_t retlen;
 	size_t total_retlen=0;
 	int ret=0;
 	int len;
 
-	DEBUG(MTD_DEBUG_LEVEL0,"MTD_write\n");
-
-	if (*ppos == mtd->size)
-		return -ENOSPC;
-
-	if (*ppos + count > mtd->size)
-		count = mtd->size - *ppos;
-
-	if (!count)
-		return 0;
-
-	if (count > MAX_KMALLOC_SIZE)
-		kbuf=kmalloc(MAX_KMALLOC_SIZE, GFP_KERNEL);
-	else
-		kbuf=kmalloc(count, GFP_KERNEL);
-
-	if (!kbuf)
-		return -ENOMEM;
-
 	while (count) {
 
-		if (count > MAX_KMALLOC_SIZE)
-			len = MAX_KMALLOC_SIZE;
-		else
-			len = count;
-
-		if (copy_from_user(kbuf, buf, len)) {
-			kfree(kbuf);
-			return -EFAULT;
-		}
+	    	len = count;
 
 		switch (mfi->mode) {
 		case MTD_MODE_OTP_FACTORY:
@@ -314,7 +401,7 @@ static ssize_t mtd_write(struct file *file, const char __user *buf, size_t count
 				ret = -EOPNOTSUPP;
 				break;
 			}
-			ret = mtd->write_user_prot_reg(mtd, *ppos, len, &retlen, kbuf);
+			ret = mtd->write_user_prot_reg(mtd, *ppos, len, &retlen, (char __kernel *)buf);
 			break;
 
 		case MTD_MODE_RAW:
@@ -322,7 +409,7 @@ static ssize_t mtd_write(struct file *file, const char __user *buf, size_t count
 			struct mtd_oob_ops ops;
 
 			ops.mode = MTD_OOB_RAW;
-			ops.datbuf = kbuf;
+			ops.datbuf = (char __kernel *)buf;
 			ops.oobbuf = NULL;
 			ops.len = len;
 
@@ -332,7 +419,7 @@ static ssize_t mtd_write(struct file *file, const char __user *buf, size_t count
 		}
 
 		default:
-			ret = (*(mtd->write))(mtd, *ppos, len, &retlen, kbuf);
+			ret = (*(mtd->write))(mtd, *ppos, len, &retlen, buf);
 		}
 		if (!ret) {
 			*ppos += retlen;
@@ -341,13 +428,114 @@ static ssize_t mtd_write(struct file *file, const char __user *buf, size_t count
 			buf += retlen;
 		}
 		else {
-			kfree(kbuf);
 			return ret;
 		}
 	}
 
-	kfree(kbuf);
 	return total_retlen;
+}
+
+typedef ssize_t (*mtd_io_fn_t)(struct file *, char *, size_t, loff_t *);
+
+static ssize_t mtd_do_loop_readv_writev(struct file *file, const struct iovec *iov, unsigned long iovcnt, loff_t *ppos, mtd_io_fn_t fn)
+{
+	const struct iovec *vector = iov;
+	ssize_t ret = 0;
+
+	while (iovcnt > 0) {
+		void *base;
+		size_t len;
+		ssize_t nr;
+
+		base = vector->iov_base;
+		len = vector->iov_len;
+		vector++;
+		iovcnt--;
+
+		nr = fn(file, base, len, ppos);
+
+		if (nr < 0) {
+			if (!ret)
+				ret = nr;
+			break;
+		}
+		ret += nr;
+		if (nr != len)
+			break;
+	}
+
+	return ret;
+}
+
+static ssize_t mtd_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
+{
+	struct mtd_file_info *mfi = file->private_data;
+	struct mtd_info *mtd = mfi->mtd;
+	ssize_t ret;
+	size_t total_retlen=0;
+	struct mtd_xio mxio;
+
+	DEBUG(MTD_DEBUG_LEVEL0,"MTD_read\n");
+
+	if (*ppos + count > mtd->size)
+		count = mtd->size - *ppos;
+
+	if (!count)
+		return 0;
+
+	ret = mtd_get_user_pages(&mxio, buf, count, false);
+	if (ret < 0)
+	    goto done_put_pages;
+
+	total_retlen = mtd_do_loop_readv_writev(file,
+						mxio.iovecs,
+						mxio.niovecs,
+						ppos,
+						mtd_do_read);
+
+	ret = total_retlen;
+
+ done_put_pages:
+	mtd_put_user_pages(&mxio);
+
+	return ret;
+} /* mtd_read */
+
+static ssize_t mtd_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
+{
+	struct mtd_file_info *mfi = file->private_data;
+	struct mtd_info *mtd = mfi->mtd;
+	ssize_t ret;
+	size_t total_retlen=0;
+	struct mtd_xio mxio;
+
+	DEBUG(MTD_DEBUG_LEVEL0,"MTD_write\n");
+
+	if (*ppos == mtd->size)
+		return -ENOSPC;
+
+	if (*ppos + count > mtd->size)
+		count = mtd->size - *ppos;
+
+	if (!count)
+		return 0;
+
+	ret = mtd_get_user_pages(&mxio, (void __user *)buf, count, true);
+	if (ret < 0)
+	    goto done_put_pages;
+
+	total_retlen = mtd_do_loop_readv_writev(file,
+						mxio.iovecs,
+						mxio.niovecs,
+						ppos,
+						(mtd_io_fn_t)mtd_do_write);
+
+	ret = total_retlen;
+
+ done_put_pages:
+	mtd_put_user_pages(&mxio);
+
+	return ret;
 } /* mtd_write */
 
 /*======================================================================

[PATCH] MTD: Retry Read/Write Transfer Buffer Allocations

[Grant Erickson]

When handling user space read or write requests via mtd_{read,write},
exponentially back off on the size of the requested kernel transfer
buffer until it succeeds or until the requested transfer buffer size
falls below the page size.

This helps ensure the operation can succeed under low-memory,
highly-fragmented situations albeit somewhat more slowly.

Signed-off-by: Grant Erickson <marathon96@gmail.com>
---
 drivers/mtd/mtdchar.c |   30 ++++++++++++++----------------
 1 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 145b3d0d..d887b91 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -179,6 +179,7 @@ static ssize_t mtd_read(struct file *file, char __user *buf, size_t count,loff_t
 	size_t total_retlen=0;
 	int ret=0;
 	int len;
+	size_t size;
 	char *kbuf;
 
 	DEBUG(MTD_DEBUG_LEVEL0,"MTD_read\n");
@@ -192,20 +193,18 @@ static ssize_t mtd_read(struct file *file, char __user *buf, size_t count,loff_t
 	/* FIXME: Use kiovec in 2.5 to lock down the user's buffers
 	   and pass them directly to the MTD functions */
 
-	if (count > MAX_KMALLOC_SIZE)
-		kbuf=kmalloc(MAX_KMALLOC_SIZE, GFP_KERNEL);
-	else
-		kbuf=kmalloc(count, GFP_KERNEL);
+	size = min_t(size_t, count, MAX_KMALLOC_SIZE);
+
+	do {
+		kbuf=kmalloc(size, GFP_KERNEL);
+	} while (!kbuf && ((size >>= 1) >= PAGE_SIZE));
 
 	if (!kbuf)
 		return -ENOMEM;
 
 	while (count) {
 
-		if (count > MAX_KMALLOC_SIZE)
-			len = MAX_KMALLOC_SIZE;
-		else
-			len = count;
+		len = min_t(size_t, count, size);
 
 		switch (mfi->mode) {
 		case MTD_MODE_OTP_FACTORY:
@@ -268,6 +267,7 @@ static ssize_t mtd_write(struct file *file, const char __user *buf, size_t count
 {
 	struct mtd_file_info *mfi = file->private_data;
 	struct mtd_info *mtd = mfi->mtd;
+	size_t size;
 	char *kbuf;
 	size_t retlen;
 	size_t total_retlen=0;
@@ -285,20 +285,18 @@ static ssize_t mtd_write(struct file *file, const char __user *buf, size_t count
 	if (!count)
 		return 0;
 
-	if (count > MAX_KMALLOC_SIZE)
-		kbuf=kmalloc(MAX_KMALLOC_SIZE, GFP_KERNEL);
-	else
-		kbuf=kmalloc(count, GFP_KERNEL);
+	size = min_t(size_t, count, MAX_KMALLOC_SIZE);
+
+	do {
+		kbuf=kmalloc(size, GFP_KERNEL);
+	} while (!kbuf && ((size >>= 1) >= PAGE_SIZE));
 
 	if (!kbuf)
 		return -ENOMEM;
 
 	while (count) {
 
-		if (count > MAX_KMALLOC_SIZE)
-			len = MAX_KMALLOC_SIZE;
-		else
-			len = count;
+		len = min_t(size_t, count, size);
 
 		if (copy_from_user(kbuf, buf, len)) {
 			kfree(kbuf);
-- 
1.7.4.2

Re: [PATCH] MTD: Retry Read/Write Transfer Buffer Allocations

[Artem Bityutskiy]

[CCing LKML in a hope to get good suggestions]
[The patch: http://lists.infradead.org/pipermail/linux-mtd/2011-April/034645.html]

Hi Grant,

Just in case, Jarkko was trying to address the same issue recently:

http://lists.infradead.org/pipermail/linux-mtd/2011-March/034416.html
On Fri, 2011-04-01 at 17:44 -0700, Grant Erickson wrote:
> When handling user space read or write requests via mtd_{read,write},
> exponentially back off on the size of the requested kernel transfer
> buffer until it succeeds or until the requested transfer buffer size
> falls below the page size.
> 
> This helps ensure the operation can succeed under low-memory,
> highly-fragmented situations albeit somewhat more slowly.
> 
> Signed-off-by: Grant Erickson <marathon96@gmail.com>
> ---
>  drivers/mtd/mtdchar.c |   30 ++++++++++++++----------------
>  1 files changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 145b3d0d..d887b91 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -179,6 +179,7 @@ static ssize_t mtd_read(struct file *file, char __user *buf, size_t count,loff_t
>  	size_t total_retlen=0;
>  	int ret=0;
>  	int len;
> +	size_t size;
>  	char *kbuf;
>  
>  	DEBUG(MTD_DEBUG_LEVEL0,"MTD_read\n");
> @@ -192,20 +193,18 @@ static ssize_t mtd_read(struct file *file, char __user *buf, size_t count,loff_t
>  	/* FIXME: Use kiovec in 2.5 to lock down the user's buffers
>  	   and pass them directly to the MTD functions */
>  
> -	if (count > MAX_KMALLOC_SIZE)
> -		kbuf=kmalloc(MAX_KMALLOC_SIZE, GFP_KERNEL);
> -	else
> -		kbuf=kmalloc(count, GFP_KERNEL);
> +	size = min_t(size_t, count, MAX_KMALLOC_SIZE);
> +
> +	do {
> +		kbuf=kmalloc(size, GFP_KERNEL);
> +	} while (!kbuf && ((size >>= 1) >= PAGE_SIZE));

This should be a bit more complex I think. First of all, I think it is
better to make this a separate function. Second, you should make sure
the system does not print scary warnings when the allocation fails - use
__GFP_NOWARN flag, just like Jarkko did.

An third, as I wrote in my answer to Jarkko, allocating large contiguous
buffers is bad for performance: if the system memory is fragmented and
there is no such large contiguous areas, the kernel will start writing
back dirty FS data, killing FS caches, shrinking caches and buggers,
probably even swapping out applications. We do not want MTD to cause
this at all.

Probably we can mitigate this with kmalloc flags. Now, I'm not sure what
flags are the optimal, but I'd do:

__GFP_NOWARN | __GFP_WAIT | __GFP_NORETRY

May be even __GFP_WAIT flag could be kicked out.

>  
>  	if (!kbuf)
>  		return -ENOMEM;
>  
>  	while (count) {
>  
> -		if (count > MAX_KMALLOC_SIZE)
> -			len = MAX_KMALLOC_SIZE;
> -		else
> -			len = count;
> +		len = min_t(size_t, count, size);
>  
>  		switch (mfi->mode) {
>  		case MTD_MODE_OTP_FACTORY:
> @@ -268,6 +267,7 @@ static ssize_t mtd_write(struct file *file, const char __user *buf, size_t count
>  {
>  	struct mtd_file_info *mfi = file->private_data;
>  	struct mtd_info *mtd = mfi->mtd;
> +	size_t size;
>  	char *kbuf;
>  	size_t retlen;
>  	size_t total_retlen=0;
> @@ -285,20 +285,18 @@ static ssize_t mtd_write(struct file *file, const char __user *buf, size_t count
>  	if (!count)
>  		return 0;
>  
> -	if (count > MAX_KMALLOC_SIZE)
> -		kbuf=kmalloc(MAX_KMALLOC_SIZE, GFP_KERNEL);
> -	else
> -		kbuf=kmalloc(count, GFP_KERNEL);
> +	size = min_t(size_t, count, MAX_KMALLOC_SIZE);
> +
> +	do {
> +		kbuf=kmalloc(size, GFP_KERNEL);
> +	} while (!kbuf && ((size >>= 1) >= PAGE_SIZE));
>  
>  	if (!kbuf)
>  		return -ENOMEM;
>  
>  	while (count) {
>  
> -		if (count > MAX_KMALLOC_SIZE)
> -			len = MAX_KMALLOC_SIZE;
> -		else
> -			len = count;
> +		len = min_t(size_t, count, size);
>  
>  		if (copy_from_user(kbuf, buf, len)) {
>  			kfree(kbuf);


-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH] MTD: Retry Read/Write Transfer Buffer Allocations

[Artem Bityutskiy]

On Mon, 2011-04-04 at 10:27 +0300, Artem Bityutskiy wrote:
> An third, as I wrote in my answer to Jarkko, allocating large contiguous
> buffers is bad for performance: if the system memory is fragmented and
> there is no such large contiguous areas, the kernel will start writing
> back dirty FS data, killing FS caches, shrinking caches and buggers,
> probably even swapping out applications. We do not want MTD to cause
> this at all.

s/buggers/buffers/

> Probably we can mitigate this with kmalloc flags. Now, I'm not sure what
> flags are the optimal, but I'd do:
> 
> __GFP_NOWARN | __GFP_WAIT | __GFP_NORETRY

Of course I meant you should use special flags as long as you are
allocating more than 1 contiguous page. But the last PAGE_SIZE
allocation should be done with standard GFP_KERNEL flag.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH] MTD: Retry Read/Write Transfer Buffer Allocations

[Grant Erickson]

On 4/4/11 12:27 AM, Artem Bityutskiy wrote:
> [CCing LKML in a hope to get good suggestions]
> [The patch: 
> http://lists.infradead.org/pipermail/linux-mtd/2011-April/034645.html]
> 
> Hi Grant,
> 
> Just in case, Jarkko was trying to address the same issue recently:
> 
> http://lists.infradead.org/pipermail/linux-mtd/2011-March/034416.html
> 
> This should be a bit more complex I think. First of all, I think it is
> better to make this a separate function. Second, you should make sure
> the system does not print scary warnings when the allocation fails - use
> __GFP_NOWARN flag, just like Jarkko did.
> 
> An third, as I wrote in my answer to Jarkko, allocating large contiguous
> buffers is bad for performance: if the system memory is fragmented and
> there is no such large contiguous areas, the kernel will start writing
> back dirty FS data, killing FS caches, shrinking caches and buggers,
> probably even swapping out applications. We do not want MTD to cause
> this at all.
> 
> Probably we can mitigate this with kmalloc flags. Now, I'm not sure what
> flags are the optimal, but I'd do:
> 
> __GFP_NOWARN | __GFP_WAIT | __GFP_NORETRY
> 
> May be even __GFP_WAIT flag could be kicked out.

Artem:

Thanks for the feedback and the link to Jarkko's very similar patch. Your
suggestions will be incorporated into a subsequent patch.

For reference, I pursued a second uses-less-memory-but-is-more-complex
approach that does get_user_pages, builds up a series of iovecs for the page
extents. This worked well for all read cases I could test; however, for the
write case, the approach required yet more refinement and overhead since the
head and tail of the transfer need to be deblocked with read-modify-write
due to the NOTALIGNED checks in nand_base.c:nand_do_write_ops. I am happy to
share the work-in-progress with the list if anyone is interested.

I propose a two-stage approach. This issue has been in the kernel for about
six years. 

Can we take a modified version of Jarkko's or my simpler fixes for the first
pass and then iterate toward the get_user_pages scatter/gather approach
later?

Best,

Grant

Re: [PATCH] MTD: Retry Read/Write Transfer Buffer Allocations

[Artem Bityutskiy]

On Mon, 2011-04-04 at 09:05 -0700, Grant Erickson wrote:
> I propose a two-stage approach. This issue has been in the kernel for about
> six years. 
> 
> Can we take a modified version of Jarkko's or my simpler fixes for the first
> pass and then iterate toward the get_user_pages scatter/gather approach

Sure, I've just commented on your patch.

-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)