[GIT PULL] logfs: bug fixes

[Artem Bityutskiy]

On Tue, 2012-01-31 at 09:46 -0800, Linus Torvalds wrote:
> Some uses may care about that "does it support bad blocks explicitly",
> but nobody should ever care about "can_have_bb()". Possibly somebody
> could care about the "inverse", in the sense that some filesystem
> might want to only support devices that are defined to be perfect, but
> that doesn't sound much like a MTD filesystem.

Hi Linus,

apologies for the hassle, this should have been dealt with in
linux-next, I agree.

I agree with your 'mtd_block_isbad()' change which now returns 0 if the
underlying flash does not support bad blocks. This is cleaner and
imposes less job on the MTD API users.

This semantic change needs some additional clean-up work which is not
critical though and will be dealt with normally during the v3.4 merge
window. This whole API rework is going to continue and there will be
many changes - only the first step is merged now. Just an example of the
clean-up we need after your conflict resolution.

--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -257,8 +257,7 @@ static void find_next_position(struct mtdoops_context *cxt)
        size_t retlen;
 
        for (page = 0; page < cxt->oops_pages; page++) {
-               if (mtd_can_have_bb(mtd) &&
-                   mtd_block_isbad(mtd, page * record_size))
+               if (mtd_block_isbad(mtd, page * record_size))
                        continue;
                /* Assume the page is used */
                mark_page_used(cxt, page);


However, as Brian pointed, the 'can_have_bb()' change is incorrect and
breaks for example UBI. We need an interface which tells us whether the
underlying flash can have bad eraseblocks and they should be dealt with
by software (NAND) or it cannot have bad eraseblocks and I/O errors mean
that flash is worn-out and should be physically changed (NOR). For
example, UBI behaves differently depending on this information.

Thus, returning hard-coded 0 or 1 from 'can_have_bb()' is incorrect.

The patch you attached looks good except that we should have this change
instead:

 static inline int mtd_can_have_bb(const struct mtd_info *mtd)
 {
-       return 0;
+       return !!mtd->block_isbad;
 }

I've inlined and attached the modified version of your patch. If no one
objects, let's merge it as a quick fix.

diff --git a/fs/logfs/dev_mtd.c b/fs/logfs/dev_mtd.c
index e97404d..9c50144 100644
--- a/fs/logfs/dev_mtd.c
+++ b/fs/logfs/dev_mtd.c
@@ -152,9 +152,6 @@ static struct page *logfs_mtd_find_first_sb(struct super_block *sb, u64 *ofs)
 	filler_t *filler = logfs_mtd_readpage;
 	struct mtd_info *mtd = super->s_mtd;
 
-	if (!mtd_can_have_bb(mtd))
-		return NULL;
-
 	*ofs = 0;
 	while (mtd_block_isbad(mtd, *ofs)) {
 		*ofs += mtd->erasesize;
@@ -172,9 +169,6 @@ static struct page *logfs_mtd_find_last_sb(struct super_block *sb, u64 *ofs)
 	filler_t *filler = logfs_mtd_readpage;
 	struct mtd_info *mtd = super->s_mtd;
 
-	if (!mtd_can_have_bb(mtd))
-		return NULL;
-
 	*ofs = mtd->size - mtd->erasesize;
 	while (mtd_block_isbad(mtd, *ofs)) {
 		*ofs -= mtd->erasesize;
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 2212952..887ebe3 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -489,7 +489,7 @@ static inline int mtd_has_oob(const struct mtd_info *mtd)
 
 static inline int mtd_can_have_bb(const struct mtd_info *mtd)
 {
-	return 0;
+	return !!mtd->block_isbad;
 }
 
 	/* Kernel-side ioctl definitions */

-- 
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mtd-patch-v2.diff
Type: text/x-patch
Size: 1187 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-mtd/attachments/20120201/42c34945/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-mtd/attachments/20120201/42c34945/attachment.sig>