ubiformat: libmt error (side effect from last Large Buffer Allocations patch)

ubiformat: libmt error (side effect from last Large Buffer Allocations patch)

[Stefano Babic]

Hi,

I have seen a side effect introduced with Grant's patch. I have applied
the patchset and this fix the allocation problem in kernel.

However, on my system (davinci with only 32 MB RAM), ubiformat does not
work and reports the following error:

libmtd: error!: cannot write 129024 bytes to mtd3 (eraseblock 13, offset 0)
        error 22 (Invalid argument)
ubiformat: error!: cannot write eraseblock 13
           error 22 (Invalid argument)


I have found that an issue is raised when ubiformat tries to optimize
and skipping 0xFF, calling its internal function drop_ffs(). When a file
image is flashed, it is checked if the last part of the buffer is 0xFF,
and the length is reduced to contain the last byte in buffer not equal
to 0xFF. The resulting length is then aligned with the minimum flash I/O
size.

However, the result lenght can be an odd multiple of the minimum I/O
size, and when there is not enough memory available, the introduced
mtd_malloc_up_to_size() tries to allocate half the amount of requested
size, getting a buffer not aligned with the minimum I/O size. The result
is that the test in nand_base.c for the alignment fails:

if (NOTALIGNED(to) || NOTALIGNED(ops->len)) {
                printk(KERN_NOTICE "%s: Attempt to write not "


I traced the values of ops_len in nand_do_write_ops and the requested
size in ubiformat (I traced the new computed length after drop_ffs), and
I got that drop_ffs changed the length from 131072 (sector size) to
129024 (this is still aligned). This is 63 times the minimum I/O size on
my NAND (page size, 2048 bytes).

If enough memory is available, it works. If kmalloc() fails in
mtd_malloc_up_to_size(), a buffer of half length is allocated --> 64512
bytes. However, this buffer is *NOT* aligned with the minimum I/O size,
and then mtd_write fails:

ubiformat: flashing eraseblock 28 -- 17 % complete
	  CHANGED len newlen: 129024 len: 131072 min I/O size: 2048
           ^- Traced after drop_ffs:

subpagesize 2048 129024

^---Traced in kernel, nand_do_write_ops: subpage size, ops->len

.....

ubiformat: flashing eraseblock 31 -- 19 % complete  CHANGED len 129024
131072 2048
subpagesize 2048 64512 64512
                   ^
                   |-- kmalloc fails, 129024/2=64512 is allocated
			but this is not aligned

nand_do_write_ops: Attempt to write not page aligned data 1be0000 0
64512 2048
libmtd: error!: cannot write 129024 bytes to mtd3 (eraseblock 31, offset 0)
        error 22 (Invalid argument)
ubiformat: error!: cannot write eraseblock 31
           error 22 (Invalid argument)


If I remove the optimization in ubiformat, it works, because sector size
is a n-multiple of minimum I/O size. When kmalloc fails, a
131072/2=65536 buffer is allocated.

IMHO mtd_malloc_up_to_size() must allocate a buffer aligned with the
minimum I/O size. What about adding the I/O size as parameter to
mtd_malloc_up_to_size, so the function will return always an aligned
buffer ?

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de
=====================================================================

Re: ubiformat: libmt error (side effect from last Large Buffer Allocations patch)

[Artem Bityutskiy]

Hi,

On Tue, 2011-04-26 at 08:56 +0200, Stefano Babic wrote:
> Hi,
> 
> I have seen a side effect introduced with Grant's patch. I have applied
> the patchset and this fix the allocation problem in kernel.

I think it would be great to actually CC grant :-) So, this e-mail is
about this patch:

http://lists.infradead.org/pipermail/linux-mtd/2011-April/034818.html

Grant, the original e-mail from Stefano:
http://lists.infradead.org/pipermail/linux-mtd/2011-April/035190.html

> However, the result lenght can be an odd multiple of the minimum I/O
> size, and when there is not enough memory available, the introduced
> mtd_malloc_up_to_size() tries to allocate half the amount of requested
> size, getting a buffer not aligned with the minimum I/O size. The result
> is that the test in nand_base.c for the alignment fails:

Good catch! Shame on me to not foresee this issue. Thanks for reporting!

> IMHO mtd_malloc_up_to_size() must allocate a buffer aligned with the
> minimum I/O size. What about adding the I/O size as parameter to
> mtd_malloc_up_to_size, so the function will return always an aligned
> buffer ?

Yes, I think this is the right solution. I've cooked the following
patch, compile-tested only - please review and test.


>From 18d93dac176025defd5711b429acd91ef8563a9a Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Date: Tue, 26 Apr 2011 11:42:10 +0300
Subject: [PATCH] mtd: a fix candidate for mtd_kmalloc_up_to

This is a fix candidate for the problem reported here:

http://lists.infradead.org/pipermail/linux-mtd/2011-April/035190.html

If it is OK, we'll need to fold it to Grant's patches and add
credits to Stefano Babic <sbabic@denx.de> there, probably in form of a
"Tested-by:" tag.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 drivers/mtd/mtdchar.c   |    4 ++--
 drivers/mtd/mtdcore.c   |   28 ++++++++++++++++++----------
 fs/jffs2/scan.c         |    2 +-
 include/linux/mtd/mtd.h |    2 +-
 4 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 9301464..7b7f992 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -203,7 +203,7 @@ static ssize_t mtd_read(struct file *file, char __user *buf, size_t count,loff_t
 	if (!count)
 		return 0;
 
-	kbuf = mtd_kmalloc_up_to(&size);
+	kbuf = mtd_kmalloc_up_to(mtd, &size);
 	if (!kbuf)
 		return -ENOMEM;
 
@@ -289,7 +289,7 @@ static ssize_t mtd_write(struct file *file, const char __user *buf, size_t count
 	if (!count)
 		return 0;
 
-	kbuf = mtd_kmalloc_up_to(&size);
+	kbuf = mtd_kmalloc_up_to(mtd, &size);
 	if (!kbuf)
 		return -ENOMEM;
 
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 6f720cc..a50348b 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -651,6 +651,9 @@ int default_mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
  * ask the memory allocator to avoid re-trying, swapping, writing back
  * or performing I/O.
  *
+ * Note, this function also makes sure that the allocated buffer is aligned to
+ * the MTD device's min. I/O unit, i.e. the "mtd->writesize" value.
+ *
  * This is called, for example by mtd_{read,write} and jffs2_scan_medium,
  * to handle smaller (i.e. degraded) buffer allocations under low- or
  * fragmented-memory situations where such reduced allocations, from a
@@ -658,24 +661,29 @@ int default_mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
  *
  * Returns a pointer to the allocated buffer on success; otherwise, NULL.
  */
-void *mtd_kmalloc_up_to(size_t *size)
+void *mtd_kmalloc_up_to(const struct mtd_info *mtd, size_t *size)
 {
 	gfp_t flags = __GFP_NOWARN | __GFP_WAIT |
 		       __GFP_NORETRY | __GFP_NO_KSWAPD;
-	size_t try;
+	size_t min_alloc = max_t(size_t, mtd->writesize, PAGE_SIZE);
 	void *kbuf;
 
-	try = min_t(size_t, *size, KMALLOC_MAX_SIZE);
+	*size = min_t(size_t, *size, KMALLOC_MAX_SIZE);
 
-	do {
-		if (try <= PAGE_SIZE)
-			flags = GFP_KERNEL;
+	while (*size > min_alloc) {
+		kbuf = kmalloc(*size, flags);
+		if (kbuf)
+			return kbuf;
 
-		kbuf = kmalloc(try, flags);
-	} while (!kbuf && ((try >>= 1) >= PAGE_SIZE));
+		*size >>= 1;
+		*size = ALIGN(*size, mtd->writesize);
+	}
 
-	*size = try;
-	return kbuf;
+	/*
+	 * For the last resort allocation allow 'kmalloc()' to do all sorts of
+	 * things (write-back, dropping caches, etc) by using GFP_KERNEL.
+	 */
+	return kmalloc(*size, GFP_KERNEL);
 }
 
 EXPORT_SYMBOL_GPL(add_mtd_device);
diff --git a/fs/jffs2/scan.c b/fs/jffs2/scan.c
index e393213..8d8cd34 100644
--- a/fs/jffs2/scan.c
+++ b/fs/jffs2/scan.c
@@ -120,7 +120,7 @@ int jffs2_scan_medium(struct jffs2_sb_info *c)
 		D1(printk(KERN_DEBUG "Trying to allocate readbuf of %zu "
 			"bytes\n", try_size));
 
-		flashbuf = mtd_kmalloc_up_to(&try_size);
+		flashbuf = mtd_kmalloc_up_to(c->mtd, &try_size);
 		if (!flashbuf)
 			return -ENOMEM;
 
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index a5d31ba..06b489a 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -348,7 +348,7 @@ int default_mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
 int default_mtd_readv(struct mtd_info *mtd, struct kvec *vecs,
 		      unsigned long count, loff_t from, size_t *retlen);
 
-void *mtd_kmalloc_up_to(size_t *size);
+void *mtd_kmalloc_up_to(const struct mtd_info *mtd, size_t *size);
 
 #ifdef CONFIG_MTD_PARTITIONS
 void mtd_erase_callback(struct erase_info *instr);
-- 
1.7.2.3



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

Re: ubiformat: libmt error (side effect from last Large Buffer Allocations patch)

[Stefano Babic]

On 04/26/2011 10:37 AM, Artem Bityutskiy wrote:
> Hi,

Hi Artem,

> Yes, I think this is the right solution. I've cooked the following
> patch, compile-tested only - please review and test.

I have applied and tested your patch and the problem is solved.

Tested-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de
=====================================================================

Re: ubiformat: libmt error (side effect from last Large Buffer Allocations patch)

[Grant Erickson]

On 4/26/11 1:37 AM, Artem Bityutskiy wrote:
> On Tue, 2011-04-26 at 08:56 +0200, Stefano Babic wrote:
>> I have seen a side effect introduced with Grant's patch. I have applied
>> the patchset and this fix the allocation problem in kernel.
> 
> I think it would be great to actually CC grant :-) So, this e-mail is
> about this patch:
> 
> http://lists.infradead.org/pipermail/linux-mtd/2011-April/034818.html
> 
> Grant, the original e-mail from Stefano:
> http://lists.infradead.org/pipermail/linux-mtd/2011-April/035190.html
> 
>> However, the result lenght can be an odd multiple of the minimum I/O
>> size, and when there is not enough memory available, the introduced
>> mtd_malloc_up_to_size() tries to allocate half the amount of requested
>> size, getting a buffer not aligned with the minimum I/O size. The result
>> is that the test in nand_base.c for the alignment fails:
> 
> Good catch! Shame on me to not foresee this issue. Thanks for reporting!
> 
>> IMHO mtd_malloc_up_to_size() must allocate a buffer aligned with the
>> minimum I/O size. What about adding the I/O size as parameter to
>> mtd_malloc_up_to_size, so the function will return always an aligned
>> buffer ?
> 
> Yes, I think this is the right solution. I've cooked the following
> patch, compile-tested only - please review and test.
> 
> 
> From 18d93dac176025defd5711b429acd91ef8563a9a Mon Sep 17 00:00:00 2001
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> Date: Tue, 26 Apr 2011 11:42:10 +0300
> Subject: [PATCH] mtd: a fix candidate for mtd_kmalloc_up_to

Artem:

Thanks for adding me to the thread. Agree that the proposed patch seems like
a good solution.

Best,

Grant

Re: ubiformat: libmt error (side effect from last Large Buffer Allocations patch)

[Artem Bityutskiy]

On Tue, 2011-04-26 at 12:45 +0200, Stefano Babic wrote:
> Tested-by: Stefano Babic <sbabic@denx.de>

Thanks, I've tweaked Grant's patches and folded my patch into them, and
added your Tested-by tag, and pushed to l2-mtd-2.6.git.

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