[PATCH v1 0/2] jffs2: make cleanmarker support option

[PATCH v1 0/2] jffs2: make cleanmarker support option

[Martin Kurbanov]

This patchset support for disable cleanmarker option. This is useful on
some NAND devices which entire OOB area is protected by ECC. Problem
fires when JFFS2 driver writes cleanmarker to some page and later it
tries to write to this page - write will be done successfully, but after
that such page becomes unreadable due to invalid ECC codes. This occurs
because the second write necessitates an update to ECC, but it is
impossible to do it correctly without block erase.

Martin Kurbanov (2):
  jffs2: introduce jffs2_nandflash()
  jffs2: make cleanmarker support option

 fs/jffs2/Kconfig    | 10 ++++++++++
 fs/jffs2/erase.c    |  2 +-
 fs/jffs2/fs.c       |  4 ++--
 fs/jffs2/os-linux.h |  7 ++++++-
 fs/jffs2/scan.c     |  2 +-
 5 files changed, 20 insertions(+), 5 deletions(-)

-- 
2.40.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v1 1/2] jffs2: introduce jffs2_nandflash()

[Martin Kurbanov]

Introduce jffs2_nandflash() instead of jffs2_cleanmarker_oob().
The jffs2_nandflash() is used to determine the type of flash.
And the jffs2_cleanmarker_oob() determines whether cleanmarkers
are needed.

The function name is chosen by analogy with jffs2_dataflash() and
jffs2_ubivol().

This is a preparation patch making further changes a bit cleaner
(no functional change).

Signed-off-by: Martin Kurbanov <mmkurbanov@salutedevices.com>
---
 fs/jffs2/erase.c    | 2 +-
 fs/jffs2/fs.c       | 4 ++--
 fs/jffs2/os-linux.h | 3 +++
 fs/jffs2/scan.c     | 2 +-
 4 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
index acd32f05b519..4475de5206c0 100644
--- a/fs/jffs2/erase.c
+++ b/fs/jffs2/erase.c
@@ -173,7 +173,7 @@ static void jffs2_erase_failed(struct jffs2_sb_info *c, struct jffs2_eraseblock
 {
 	/* For NAND, if the failure did not occur at the device level for a
 	   specific physical page, don't bother updating the bad block table. */
-	if (jffs2_cleanmarker_oob(c) && (bad_offset != (uint32_t)MTD_FAIL_ADDR_UNKNOWN)) {
+	if (jffs2_nandflash(c) && (bad_offset != (uint32_t)MTD_FAIL_ADDR_UNKNOWN)) {
 		/* We had a device-level failure to erase.  Let's see if we've
 		   failed too many times. */
 		if (!jffs2_write_nand_badblock(c, jeb, bad_offset)) {
diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index 038516bee1ab..6de91c00eb73 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -687,7 +687,7 @@ struct jffs2_inode_info *jffs2_gc_fetch_inode(struct jffs2_sb_info *c,
 static int jffs2_flash_setup(struct jffs2_sb_info *c) {
 	int ret = 0;
 
-	if (jffs2_cleanmarker_oob(c)) {
+	if (jffs2_nandflash(c)) {
 		/* NAND flash... do setup accordingly */
 		ret = jffs2_nand_flash_setup(c);
 		if (ret)
@@ -720,7 +720,7 @@ static int jffs2_flash_setup(struct jffs2_sb_info *c) {
 
 void jffs2_flash_cleanup(struct jffs2_sb_info *c) {
 
-	if (jffs2_cleanmarker_oob(c)) {
+	if (jffs2_nandflash(c)) {
 		jffs2_nand_flash_cleanup(c);
 	}
 
diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
index 8da19766c101..c604f639a00f 100644
--- a/fs/jffs2/os-linux.h
+++ b/fs/jffs2/os-linux.h
@@ -81,6 +81,7 @@ static inline void jffs2_init_inode_info(struct jffs2_inode_info *f)
 #define jffs2_flush_wbuf_pad(c) ({ do{} while(0); (void)(c), 0; })
 #define jffs2_flush_wbuf_gc(c, i) ({ do{} while(0); (void)(c), (void) i, 0; })
 #define jffs2_write_nand_badblock(c,jeb,bad_offset) (1)
+#define jffs2_nandflash(c) (0)
 #define jffs2_nand_flash_setup(c) (0)
 #define jffs2_nand_flash_cleanup(c) do {} while(0)
 #define jffs2_wbuf_dirty(c) (0)
@@ -124,6 +125,8 @@ void jffs2_wbuf_timeout(unsigned long data);
 void jffs2_wbuf_process(void *data);
 int jffs2_flush_wbuf_gc(struct jffs2_sb_info *c, uint32_t ino);
 int jffs2_flush_wbuf_pad(struct jffs2_sb_info *c);
+
+#define jffs2_nandflash(c) ((c)->mtd->type == MTD_NANDFLASH)
 int jffs2_nand_flash_setup(struct jffs2_sb_info *c);
 void jffs2_nand_flash_cleanup(struct jffs2_sb_info *c);
 
diff --git a/fs/jffs2/scan.c b/fs/jffs2/scan.c
index 29671e33a171..005d0af950ea 100644
--- a/fs/jffs2/scan.c
+++ b/fs/jffs2/scan.c
@@ -113,7 +113,7 @@ int jffs2_scan_medium(struct jffs2_sb_info *c)
 	if (!flashbuf) {
 		/* For NAND it's quicker to read a whole eraseblock at a time,
 		   apparently */
-		if (jffs2_cleanmarker_oob(c))
+		if (jffs2_nandflash(c))
 			try_size = c->sector_size;
 		else
 			try_size = PAGE_SIZE;
-- 
2.40.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v1 2/2] jffs2: make cleanmarker support option

[Martin Kurbanov]

This patch support for disable cleanmarker option. This is useful on
some NAND devices which entire OOB area is protected by ECC. Problem
fires when JFFS2 driver writes cleanmarker to some page and later it
tries to write to this page - write will be done successfully, but after
that such page becomes unreadable due to invalid ECC codes. This occurs
because the second write necessitates an update to ECC, but it is
impossible to do it correctly without block erase.

Signed-off-by: Martin Kurbanov <mmkurbanov@salutedevices.com>
---
 fs/jffs2/Kconfig    | 10 ++++++++++
 fs/jffs2/os-linux.h |  4 +++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/jffs2/Kconfig b/fs/jffs2/Kconfig
index 7c96bc107218..8a66941d1e93 100644
--- a/fs/jffs2/Kconfig
+++ b/fs/jffs2/Kconfig
@@ -29,6 +29,16 @@ config JFFS2_FS_DEBUG
 	  If reporting bugs, please try to have available a full dump of the
 	  messages at debug level 1 while the misbehaviour was occurring.
 
+config JFFS2_FS_NOCLEANMARKER
+	bool "Disable cleanmarkers JFFS2 feature"
+	depends on JFFS2_FS_WRITEBUFFER
+	depends on MTD_NAND || MTD_SPI_NAND
+	default n
+	help
+	  Do not write 'CLEANMARKER' nodes to the beginning of each erase block.
+	  This option can be useful on NAND flash where there is no free
+	  space in the OOB area or the entire OOB area is protected by ECC.
+
 config JFFS2_FS_WRITEBUFFER
 	bool "JFFS2 write-buffering support"
 	depends on JFFS2_FS
diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
index c604f639a00f..ea42964d8118 100644
--- a/fs/jffs2/os-linux.h
+++ b/fs/jffs2/os-linux.h
@@ -109,7 +109,9 @@ static inline void jffs2_init_inode_info(struct jffs2_inode_info *f)
 #define jffs2_can_mark_obsolete(c) (c->mtd->flags & (MTD_BIT_WRITEABLE))
 #endif
 
-#define jffs2_cleanmarker_oob(c) (c->mtd->type == MTD_NANDFLASH)
+#define jffs2_cleanmarker_oob(c)			\
+	(!IS_ENABLED(CONFIG_JFFS2_FS_NOCLEANMARKER) &&	\
+	((c)->mtd->type == MTD_NANDFLASH))
 
 #define jffs2_wbuf_dirty(c) (!!(c)->wbuf_len)
 
-- 
2.40.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v1 2/2] jffs2: make cleanmarker support option

[Richard Weinberger]

Martin,

----- Ursprüngliche Mail -----
> Von: "Martin Kurbanov" <mmkurbanov@salutedevices.com>
> This patch support for disable cleanmarker option. This is useful on
> some NAND devices which entire OOB area is protected by ECC. Problem
> fires when JFFS2 driver writes cleanmarker to some page and later it
> tries to write to this page - write will be done successfully, but after
> that such page becomes unreadable due to invalid ECC codes. This occurs
> because the second write necessitates an update to ECC, but it is
> impossible to do it correctly without block erase.

Hmm, I miss an explanation why this change is correct and safe.
You explain why the OOB area can't be used, okay. But you need to
add more details on why you change is safe in terms of filesystem
consistency.

Beside of that, I don't think this should be kernel config option.
Why not a mount option?

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v1 2/2] jffs2: make cleanmarker support option

[Martin Kurbanov]

Hello Richard,

On 19.10.2023 11:12, Richard Weinberger wrote:
>> This patch support for disable cleanmarker option. This is useful on
>> some NAND devices which entire OOB area is protected by ECC. Problem
>> fires when JFFS2 driver writes cleanmarker to some page and later it
>> tries to write to this page - write will be done successfully, but after
>> that such page becomes unreadable due to invalid ECC codes. This occurs
>> because the second write necessitates an update to ECC, but it is
>> impossible to do it correctly without block erase.
> Hmm, I miss an explanation why this change is correct and safe.
> You explain why the OOB area can't be used, okay. But you need to
> add more details on why you change is safe in terms of filesystem
> consistency.
 
If you disable the cleanmarker, the found clean block (filled with 0xff)
will be erased again (see fs/jffs2/scan.c#L162).
In my opinion, it is better to perform the block erasure again than to
not work with such a nand flash at all.


> Beside of that, I don't think this should be kernel config option.
> Why not a mount option?

Agreed

-- 
Best Regards,
Martin Kurbanov

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v1 2/2] jffs2: make cleanmarker support option

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Martin Kurbanov" <mmkurbanov@salutedevices.com>
> If you disable the cleanmarker, the found clean block (filled with 0xff)
> will be erased again (see fs/jffs2/scan.c#L162).
> In my opinion, it is better to perform the block erasure again than to
> not work with such a nand flash at all.

Doesn't this case many re-erases at each mount time?

BTW: I tried your patch in nandsim, jffs2 was unhappy.
[   56.147361] jffs2: notice: (440) jffs2_build_xattr_subsystem: complete building xattr subsystem, 0 of xdatum (0 unchecked, 0 orphan) and 0 of xref (0 dead, 0 orphan) found.
[   56.200438] nand: nand_do_write_ops: attempt to write non page aligned data
[   56.201090] jffs2: Write clean marker to block at 0x001f8000 failed: -22

Do you have an idea?

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v1 2/2] jffs2: make cleanmarker support option

[Martin Kurbanov]

On 23.10.2023 20:44, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
>> Von: "Martin Kurbanov" <mmkurbanov@salutedevices.com>
>> If you disable the cleanmarker, the found clean block (filled with 0xff)
>> will be erased again (see fs/jffs2/scan.c#L162).
>> In my opinion, it is better to perform the block erasure again than to
>> not work with such a nand flash at all.
> 
> Doesn't this case many re-erases at each mount time?

You are right. David proposed the good solution.

> BTW: I tried your patch in nandsim, jffs2 was unhappy.
> [   56.147361] jffs2: notice: (440) jffs2_build_xattr_subsystem: complete building xattr subsystem, 0 of xdatum (0 unchecked, 0 orphan) and 0 of xref (0 dead, 0 orphan) found.
> [   56.200438] nand: nand_do_write_ops: attempt to write non page aligned data
> [   56.201090] jffs2: Write clean marker to block at 0x001f8000 failed: -22
> 
> Do you have an idea?

According to this code from the function jffs2_mark_erased_block():

```
if (jffs2_cleanmarker_oob(c) || c->cleanmarker_size == 0) {

    if (jffs2_cleanmarker_oob(c)) {
        if (jffs2_write_nand_cleanmarker(c, jeb))
            goto filebad;
    }
} else {

    struct kvec vecs[1];
    struct jffs2_unknown_node marker = {
        .magic = cpu_to_je16(JFFS2_MAGIC_BITMASK),
        .nodetype = cpu_to_je16(JFFS2_NODETYPE_CLEANMARKER),
        .totlen = cpu_to_je32(c->cleanmarker_size)
    };
```

the "if" branch should be executed because "cleanmarker_size" is set to
0 for NAND flash:

```
int jffs2_nand_flash_setup(struct jffs2_sb_info *c)
{
    if (!c->mtd->oobsize)
        return 0;

    /* Cleanmarker is out-of-band, so inline size zero */
    c->cleanmarker_size = 0;
```

In your case, the "else" branch was executed. I assume that "oobsize" is
equal to 0. In this scenario, JFFS2 will not mount without
applying my patch.

-- 
Best Regards,
Martin Kurbanov

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v1 2/2] jffs2: make cleanmarker support option

[David Woodhouse]

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

On Mon, 2023-10-23 at 17:54 +0300, Martin Kurbanov wrote:
> Hello Richard,
> 
> On 19.10.2023 11:12, Richard Weinberger wrote:
> > > This patch support for disable cleanmarker option. This is useful on
> > > some NAND devices which entire OOB area is protected by ECC. Problem
> > > fires when JFFS2 driver writes cleanmarker to some page and later it
> > > tries to write to this page - write will be done successfully, but after
> > > that such page becomes unreadable due to invalid ECC codes. This occurs
> > > because the second write necessitates an update to ECC, but it is
> > > impossible to do it correctly without block erase.
> > Hmm, I miss an explanation why this change is correct and safe.
> > You explain why the OOB area can't be used, okay. But you need to
> > add more details on why you change is safe in terms of filesystem
> > consistency.
>  
> If you disable the cleanmarker, the found clean block (filled with 0xff)
> will be erased again (see fs/jffs2/scan.c#L162).
> In my opinion, it is better to perform the block erasure again than to
> not work with such a nand flash at all.

Erasing all unused blocks over and over again on every reboot/remount
is going to destroy your flash quite quickly, surely?

I think you need to come up with a way to log the clean blocks (or
erase requests) in the JFFS2 log itself.

Perhaps a 'block erase log' node type, which just contains a version#
and a list of blocks which are currently being erased. You write it out
before doing any erase operation. And then at *some* point after the
erase completes (it doesn't need to be immediate) you write out a new
one (which may be empty, or may list new blocks which are about to be
erased).

On mount, we just need to re-erase any blocks which are indicated as
being erased in the latest erase log node.

> > Beside of that, I don't think this should be kernel config option.
> > Why not a mount option?
> 
> Agreed

Why even a mount option? Shouldn't it be automatic, depending on the
type of flash chip?



[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v1 2/2] jffs2: make cleanmarker support option

[Martin Kurbanov]

Hi David, Thank you for reply.

On 23.10.2023 20:55, David Woodhouse wrote:
> On Mon, 2023-10-23 at 17:54 +0300, Martin Kurbanov wrote:
>> Hello Richard,
>>
>> On 19.10.2023 11:12, Richard Weinberger wrote:
>>>> This patch support for disable cleanmarker option. This is useful on
>>>> some NAND devices which entire OOB area is protected by ECC. Problem
>>>> fires when JFFS2 driver writes cleanmarker to some page and later it
>>>> tries to write to this page - write will be done successfully, but after
>>>> that such page becomes unreadable due to invalid ECC codes. This occurs
>>>> because the second write necessitates an update to ECC, but it is
>>>> impossible to do it correctly without block erase.
>>> Hmm, I miss an explanation why this change is correct and safe.
>>> You explain why the OOB area can't be used, okay. But you need to
>>> add more details on why you change is safe in terms of filesystem
>>> consistency.
>>  
>> If you disable the cleanmarker, the found clean block (filled with 0xff)
>> will be erased again (see fs/jffs2/scan.c#L162).
>> In my opinion, it is better to perform the block erasure again than to
>> not work with such a nand flash at all.
> 
> Erasing all unused blocks over and over again on every reboot/remount
> is going to destroy your flash quite quickly, surely?
> 
> I think you need to come up with a way to log the clean blocks (or
> erase requests) in the JFFS2 log itself.
> 
> Perhaps a 'block erase log' node type, which just contains a version#
> and a list of blocks which are currently being erased. You write it out
> before doing any erase operation. And then at *some* point after the
> erase completes (it doesn't need to be immediate) you write out a new
> one (which may be empty, or may list new blocks which are about to be
> erased).
> 
> On mount, we just need to re-erase any blocks which are indicated as
> being erased in the latest erase log node.

What if we don't erase the free blocks during mounting, but instead
erase them when a clean block is needed (before writing)?

-- 
Best Regards,
Martin Kurbanov

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/