Re: [GIT PULL] logfs: bug fixes

Re: [GIT PULL] logfs: bug fixes

[Linus Torvalds]

On Tue, Jan 31, 2012 at 9:02 AM, Prasad Joshi
<prasadjoshi.linux@gmail.com> wrote:
>
> I have pushed the signed tag on the master branch as suggested by you

Ok, this works, but please don't put auto-generated content with no
semantics in the tag message. The shortlog and diffstat don't actually
tell me anything new - I can see those from the git details directly.

I want to see them in the *email* - so that I know what I'm pulling
beforehand, and so that I know that you are aware of what you are
pushing me - but there's no point in putting them in the signed tag.

So please put your own explanations for what is going on (if there is
anything half-way interesting) in the signed tag message, but not
anything that can be seen automatically from git itself. You signed
it, that signature covers the contents, but explanations for humans
are interesting.

Anyway, it does have a conflict. And I can (and did) easily resolve
the conflict that I get, even if I can't really test my end result.

HOWEVER: the conflict has a major semantic component to it that I want
people to discuss and be aware of my resolution.

So the conflict is between

 - commit f2933e86ad93: "Logfs: Allow NULL block_isbad() methods"

 - commits 7086c19d0742: "mtd: introduce mtd_block_isbad interface"
and d58b27ed58a3: "logfs: do not use 'mtd->block_isbad' directly"

both of which change how mtd->block_isbad' is used (the first
semantically by logfs, the others change the interface)

Now, my reaction and resolution to the conflict is to actually take
the *semantics* from commit f2933e86ad93, and just make
mtd_block_isbad() return zero (false) if the 'block_isbad' function is
NULL. But that also means that now "mtd_can_have_bb()" always returns
0.

At the same time, "mtd_block_markbad()" will obviously return an error
if the low-level driver doesn't support bad blocks, so this is
somewhat non-symmetric, but it actually makes sense if a NULL
"block_isbad" function is considered to mean "I assume that all my
blocks are always good".

Anyway, it is possible that the MTD people react very negatively to
this semantic change, but quite frankly, 99% of all uses of the new
mtd_block_isbad() function treated it as a boolean, so returning an
error from it didn't actually *work* correctly - and also made that
whole mtd_can_have_bb() function kind of pointless.

Essentially nobody checked it anyway with a *very* few exceptions. And
the mtd_can_have_bb() function was imnsho clearly misdesigned for the
whole concept.

So I think my resolution is reasonable, but I also think that you guys
should look at

 - getting rid of mtd_can_have_bb() entirely as being stupid
(mtd_block_isbad() does the right thing regardless - if the MTD
supports bad blocks it will do the right thing, and if it doesn't, it
will do the best thing it can)

 - instead introduce a "mtd_can_mark_blockbad()" and use that as a
"does this MTD device support *explicit* bad blocks" thing.

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.

Anyway, I think you guys should have discussed this conflict when you
saw it in -next (or when the MTD people changed the fs/logfs code - I
see that Jörn was cc'd).

As it is, I just did what I think was the "RightThing(tm)". Please
discuss, and feel free to send me patches/pull requests for whatever
resolution you get to.

                        Linus

Re: [GIT PULL] logfs: bug fixes

[Brian Norris]

On Tue, Jan 31, 2012 at 9:46 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Anyway, it does have a conflict. And I can (and did) easily resolve
> the conflict that I get, even if I can't really test my end result.
...
> Now, my reaction and resolution to the conflict is to actually take
> the *semantics* from commit f2933e86ad93, and just make
> mtd_block_isbad() return zero (false) if the 'block_isbad' function is
> NULL. But that also means that now "mtd_can_have_bb()" always returns
> 0.

For your (hopefully temporary) resolution, mtd_can_have_bb() should
return 1, not 0. Or better yet, do not change mtd_can_have_bb() right
now. Always returning 0 is interpreted as "the MTD cannot have bad
blocks," which is dangerous for MTD's that *do* support bad blocks.

> At the same time, "mtd_block_markbad()" will obviously return an error
> if the low-level driver doesn't support bad blocks, so this is
> somewhat non-symmetric, but it actually makes sense if a NULL
> "block_isbad" function is considered to mean "I assume that all my
> blocks are always good".
>
> Anyway, it is possible that the MTD people react very negatively to
> this semantic change, but quite frankly, 99% of all uses of the new
> mtd_block_isbad() function treated it as a boolean, so returning an
> error from it didn't actually *work* correctly - and also made that
> whole mtd_can_have_bb() function kind of pointless.
>
> Essentially nobody checked it anyway with a *very* few exceptions. And
> the mtd_can_have_bb() function was imnsho clearly misdesigned for the
> whole concept.
>
> So I think my resolution is reasonable

I dunno. You bring up some reasonable points, but your resolution
doesn't solve everything correctly.

> but I also think that you guys should look at
>
>  - getting rid of mtd_can_have_bb() entirely as being stupid
> (mtd_block_isbad() does the right thing regardless - if the MTD
> supports bad blocks it will do the right thing, and if it doesn't, it
> will do the best thing it can)

For most usages of mtd_can_have_bb(), we can remove it now, I think,
but the remaining situations require some knowledge about bad block
support, not just a blanket statement that "we can have bad blocks."

>  - instead introduce a "mtd_can_mark_blockbad()" and use that as a
> "does this MTD device support *explicit* bad blocks" thing.
>
> Some uses may care about that "does it support bad blocks explicitly",
> but nobody should ever care about "can_have_bb()".

There is at least one optimization where we don't even scan for bad
blocks when we know "mtd_can_have_bbt()" is false. But I think it's
only for the mtd_{read,speed,stress,torture}test modules, so small
performance gains aren't really significant.

> 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.

I think this is what UBI uses (currently via mtd_can_have_bbt()). I
believe NOR flash don't record bad blocks at all, since it has such
high reliability. On such devices, UBI just chooses to switch into a
failsafe read-only mode when it encounters errors.

> Anyway, I think you guys should have discussed this conflict when you
> saw it in -next (or when the MTD people changed the fs/logfs code - I
> see that Jörn was cc'd).

Does linux-next have automatic e-mailing setup for merge conflicts? A
notice to the mailing list could have cleared this up already. If it
was sent to specific developers...well they aren't always responsive
:(

Anyway, solving the conflict alone is very simple, but the additional
semantic change isn't quite so easy.

> As it is, I just did what I think was the "RightThing(tm)". Please
> discuss, and feel free to send me patches/pull requests for whatever
> resolution you get to.

I recommend *not* leaving Linus' merge resolution as is. I like the
change to mtd_block_isbad(), but the change to mtd_can_have_bb() will
produce some breakage, I think.

When I get a chance to sit down and write/test code, I may be able to
provide better suggestions. But most of the changes (including Linus'
honorable attempt at fixing mtd_can_have_bb() behavior) should
probably wait until the next release IMO, as there is some subtlety.

Another note: I think your merge incorrectly discards the intended fix
from commit f2933e8 by (re)introducing these lines in
fs/logfs/dev_mtd.c:

    if (!mtd_can_have_bb(mtd))
            return NULL;

Brian

Re: [GIT PULL] logfs: bug fixes

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

On Tue, Jan 31, 2012 at 7:07 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
>
> For your (hopefully temporary) resolution, mtd_can_have_bb() should
> return 1, not 0. Or better yet, do not change mtd_can_have_bb() right
> now. Always returning 0 is interpreted as "the MTD cannot have bad
> blocks," which is dangerous for MTD's that *do* support bad blocks.

Right you are. That was just a thinko on my part.

So together with the point that logfs should have dropped the use of
mtd_can_have_bb(), the fix to that merge should be the attached patch
- at least then things should hopefully work.

That still leaves the question of whether the MTD people want to
change the semantics in other ways. Sorry for the mis-merge,

                       Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1278 bytes --]

 fs/logfs/dev_mtd.c      |    6 ------
 include/linux/mtd/mtd.h |    2 +-
 2 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/fs/logfs/dev_mtd.c b/fs/logfs/dev_mtd.c
index e97404d611e0..9c501449450d 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 221295208fd0..266e90dcee35 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 1;
 }
 
 	/* Kernel-side ioctl definitions */

Re: [GIT PULL] logfs: bug fixes

[Artem Bityutskiy]

[-- Attachment #1.1: Type: text/plain, Size: 3601 bytes --]

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

[-- Attachment #1.2: mtd-patch-v2.diff --]
[-- Type: text/x-patch, Size: 1187 bytes --]

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 */

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [GIT PULL] logfs: bug fixes

[Artem Bityutskiy]

[-- Attachment #1.1: Type: text/plain, Size: 2699 bytes --]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Date: Wed, 1 Feb 2012 11:50:07 +0200
Subject: [PATCH] mtd: fix merge conflict resolution breakage

This patch fixes merge conflict resolution breakage introduced by merge
commit 'd3712b9dfcf44ca145cf87e7f4096fa2d923471a'.

The commit changed 'mtd_can_have_bb()' function and made it always return
zero, which is incorrect. Instead, we need it to return whether the
underlying flash device can have bad eraseblocks or not. UBI needs this
information because it affects how it handles the underlying flash.
E.g., if the underlying flash is NOR, it cannot have bad blocks and any
write or erase error is fatal, and all we can do is to switch to R/O
mode. We do not need to reserve a pool of good eraseblocks for bad
eraseblocks handling, and so on.

This patch also removes 'mtd_can_have_bb()' invocations from Logfs
to ensure correct Logfs behavior.

I've tested that with this patch UBI works on top of NOR and NAND
flashes emulated by mtdram and nandsim correspondingly.

This patch is based on patch from Linus Torvalds.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---

Linus, here is the emergency fix - basically your patch tested and
and slightly modified. You should probably put yourself as the author.

Also attached the patch for your convenience.

 fs/logfs/dev_mtd.c      |    6 ------
 include/linux/mtd/mtd.h |    2 +-
 2 files changed, 1 insertions(+), 7 deletions(-)

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 c09020a..d43dc25 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -487,7 +487,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 */
-- 
1.7.9

[-- Attachment #1.2: 0001-mtd-fix-mtd_can_have_bb.patch --]
[-- Type: text/x-patch, Size: 2580 bytes --]

From 191b90d59cb97b83d5f979d46940d0bab2b02b14 Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Date: Wed, 1 Feb 2012 11:50:07 +0200
Subject: [PATCH] mtd: fix merge conflict resolution breakage

This patch fixes merge conflict resolution breakage introduced by merge
commit 'd3712b9dfcf44ca145cf87e7f4096fa2d923471a'.

The commit changed 'mtd_can_have_bb()' function and made it always return
zero, which is incorrect. Instead, we need it to return whether the
underlying flash device can have bad eraseblocks or not. UBI needs this
information because it affects how it handles the underlying flash.
E.g., if the underlying flash is NOR, it cannot have bad blocks and any
write or erase error is fatal, and all we can do is to switch to R/O
mode. We do not need to reserve a pool of good eraseblocks for bad
eraseblocks handling, and so on.

This patch also removes 'mtd_can_have_bb()' invocations from Logfs
to ensure correct Logfs behavior.

I've tested that with this patch UBI works on top of NOR and NAND
flashes emulated by mtdram and nandsim correspondingly.

This patch is based on patch from Linus Torvalds.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 fs/logfs/dev_mtd.c      |    6 ------
 include/linux/mtd/mtd.h |    2 +-
 2 files changed, 1 insertions(+), 7 deletions(-)

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 c09020a..d43dc25 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -487,7 +487,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 */
-- 
1.7.9


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [GIT PULL] logfs: bug fixes

[Jörn Engel]

On Wed, 1 February 2012 14:54:27 +0200, Artem Bityutskiy wrote:
> 
> This patch fixes merge conflict resolution breakage introduced by merge
> commit 'd3712b9dfcf44ca145cf87e7f4096fa2d923471a'.
> 
> The commit changed 'mtd_can_have_bb()' function and made it always return
> zero, which is incorrect. Instead, we need it to return whether the
> underlying flash device can have bad eraseblocks or not. UBI needs this
> information because it affects how it handles the underlying flash.
> E.g., if the underlying flash is NOR, it cannot have bad blocks and any
> write or erase error is fatal, and all we can do is to switch to R/O
> mode. We do not need to reserve a pool of good eraseblocks for bad
> eraseblocks handling, and so on.
> 
> This patch also removes 'mtd_can_have_bb()' invocations from Logfs
> to ensure correct Logfs behavior.
> 
> I've tested that with this patch UBI works on top of NOR and NAND
> flashes emulated by mtdram and nandsim correspondingly.
> 
> This patch is based on patch from Linus Torvalds.
> 
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Looks good to me.
Acked-by: Joern Engel <joern@logfs.org>

Jörn

Re: [GIT PULL] logfs: bug fixes

[Prasad Joshi]

On Wed, Feb 1, 2012 at 6:24 PM, Artem Bityutskiy
<artem.bityutskiy@linux.intel.com> wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> Date: Wed, 1 Feb 2012 11:50:07 +0200
> Subject: [PATCH] mtd: fix merge conflict resolution breakage
>
> This patch fixes merge conflict resolution breakage introduced by merge
> commit 'd3712b9dfcf44ca145cf87e7f4096fa2d923471a'.
>
> The commit changed 'mtd_can_have_bb()' function and made it always return
> zero, which is incorrect. Instead, we need it to return whether the
> underlying flash device can have bad eraseblocks or not. UBI needs this
> information because it affects how it handles the underlying flash.
> E.g., if the underlying flash is NOR, it cannot have bad blocks and any
> write or erase error is fatal, and all we can do is to switch to R/O
> mode. We do not need to reserve a pool of good eraseblocks for bad
> eraseblocks handling, and so on.
>
> This patch also removes 'mtd_can_have_bb()' invocations from Logfs
> to ensure correct Logfs behavior.
>
> I've tested that with this patch UBI works on top of NOR and NAND
> flashes emulated by mtdram and nandsim correspondingly.
>
> This patch is based on patch from Linus Torvalds.
>
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>


Acked-by: Prasad Joshi <prasadjoshi.linux@gmail.com>

> ---
>
> Linus, here is the emergency fix - basically your patch tested and
> and slightly modified. You should probably put yourself as the author.
>
> Also attached the patch for your convenience.
>
>  fs/logfs/dev_mtd.c      |    6 ------
>  include/linux/mtd/mtd.h |    2 +-
>  2 files changed, 1 insertions(+), 7 deletions(-)
>
> 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 c09020a..d43dc25 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -487,7 +487,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 */
> --
> 1.7.9

Re: [GIT PULL] logfs: bug fixes

[Brian Norris]

On Wed, Feb 1, 2012 at 4:54 AM, Artem Bityutskiy
<artem.bityutskiy@linux.intel.com> wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> Date: Wed, 1 Feb 2012 11:50:07 +0200
> Subject: [PATCH] mtd: fix merge conflict resolution breakage
>
> This patch fixes merge conflict resolution breakage introduced by merge
> commit 'd3712b9dfcf44ca145cf87e7f4096fa2d923471a'.
>
> The commit changed 'mtd_can_have_bb()' function and made it always return
> zero, which is incorrect. Instead, we need it to return whether the
> underlying flash device can have bad eraseblocks or not. UBI needs this
> information because it affects how it handles the underlying flash.
> E.g., if the underlying flash is NOR, it cannot have bad blocks and any
> write or erase error is fatal, and all we can do is to switch to R/O
> mode. We do not need to reserve a pool of good eraseblocks for bad
> eraseblocks handling, and so on.
>
> This patch also removes 'mtd_can_have_bb()' invocations from Logfs
> to ensure correct Logfs behavior.
>
> I've tested that with this patch UBI works on top of NOR and NAND
> flashes emulated by mtdram and nandsim correspondingly.
>
> This patch is based on patch from Linus Torvalds.
>
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

This agrees with my suggestions.

Acked-by: Brian Norris <computersforpeace@gmail.com>