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

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: [email protected]
=====================================================================

Hi,

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

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

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

From: Artem Bityutskiy <[email protected]>
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 <[email protected]> there, probably in form of a "Tested-by:" tag.

---
 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(-)

index 9301464..7b7f992 100644
@@ -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;
 

index 6f720cc..a50348b 100644
@@ -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);

index e393213..8d8cd34 100644
@@ -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;
 

index a5d31ba..06b489a 100644
@@ -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 (Артём Битюцкий)

Hi Artem,

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

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: [email protected]
=====================================================================

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 (Артём Битюцкий)

Artem:

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

Best,

Grant