jffs2: Excess summary entries

jffs2: Excess summary entries

[Thomas.Betker]

We ran into a problem with jffs2 where the filesystem became unusable 
after some specific MTD failures; summary was enabled, write buffering was 
disabled:

1. For some reason, mtd_writev() returned -EINTR and *retlen == 0. No 
INODE data was written to flash, but jffs2_flash_direct_writev() still 
added a summary entry, by jffs2_sum_add_kvec(). Actually, this happened 
twice in a row:

        jffs2: Write of 4164 bytes at 0x00ec5b88 failed. returned -4, 
retlen 0
        jffs2: Not marking the space at 0x00ec5b88 as dirty because the 
flash driver returned retlen zero
        jffs2: Write of 4164 bytes at 0x00ec5b88 failed. returned -4, 
retlen 0
        jffs2: Not marking the space at 0x00ec5b88 as dirty because the 
flash driver returned retlen zero

2. When rebooting after the summary data was written to flash (including 
the two excess entries), we got the following messages:

        jffs2: error: (79) jffs2_link_node_ref: Adding new ref c3048d18 at 
(0x00ec5b88-0x00ec6bcc) not immediately after previous 
(0x00ec5b88-0x00ec5b88)
        jffs2: error: (79) jffs2_link_node_ref: Adding new ref c3048d20 at 
(0x00ec5b88-0x00ec6000) not immediately after previous 
(0x00ec5b88-0x00ec5b88)
        ...
        jffs2: warning: (79) jffs2_sum_scan_sumnode: Free size 0xffffdf78 
bytes in eraseblock @0x00ec0000 with summary?

        jffs2: Checked all inodes but still 0x2088 bytes of unchecked 
space?
        jffs2: No space for garbage collection. Aborting GC thread

The excess entries added up to "unchecked space", so that 
jffs2_garbage_collect_pass() returned -ENOSPC.

3. Since garbage collection was off, the flash filled up over time until 
the filesystem could no longer be modified (can't add or write to files, 
can't even delete files -- "No space left on device"). Basically, we were 
bricked.

Here are the solutions I have considered so far:

The minimal solution would be to check *retlen == 0 in 
jffs2_flash_direct_writev() and jffs2_flash_direct_write() before running 
jffs2_sum_add_kvec().

        if (jffs2_sum_active() && *retlen) {
                ...
                res = jffs2_sum_add_kvec(...)
                ...
        }

The general failure case, though, is (ret != 0 || *retlen != len), where 
'ret' is the return code of mtd_writev(), and 'len' is the data size to be 
written. When write buffering is enabled, jffs2_flash_writev() in wbuf.c 
skips the summary entry in this case; perhaps we should do this in 
writev.c as well?

        if (jffs2_sum_active() && !ret && *retlen == len) {
                ...
                res = jffs2_sum_add_kvec(...)
                ...
        }

I ran some quick tests, simulating write failures, and it seems that 
adding the summary entry doesn't harm when *retlen != 0 [so the minimal 
solution would suffice]. This is because the calling function will reserve 
the node space, marking it as dirty, and there is no confusion about 
unchecked space.

On the other hand, running the same quick tests _without_ adding the 
summary entry didn't seem to harm either [so the general solution would 
work as well]. It is entirely possible that I have overlooked something, 
though.

Any opinions on that? When in doubt, I would provide a patch for the 
minimal solution, changing as little as possible. However, it may make 
sense to go for the general solution to be consistent with write 
buffering.

Best regards,
Thomas Betker

Re: jffs2: Excess summary entries

[Wei Fang]

Hi Thomas,

On 2015/11/6 2:36, Thomas.Betker@rohde-schwarz.com wrote:
> We ran into a problem with jffs2 where the filesystem became unusable 
> after some specific MTD failures; summary was enabled, write buffering was 
> disabled:

[snip...]

> Here are the solutions I have considered so far:
> 
> The minimal solution would be to check *retlen == 0 in 
> jffs2_flash_direct_writev() and jffs2_flash_direct_write() before running 
> jffs2_sum_add_kvec().
> 
>         if (jffs2_sum_active() && *retlen) {
>                 ...
>                 res = jffs2_sum_add_kvec(...)
>                 ...
>         }
> 
> The general failure case, though, is (ret != 0 || *retlen != len), where 
> 'ret' is the return code of mtd_writev(), and 'len' is the data size to be 
> written. When write buffering is enabled, jffs2_flash_writev() in wbuf.c 
> skips the summary entry in this case; perhaps we should do this in 
> writev.c as well?
> 
>         if (jffs2_sum_active() && !ret && *retlen == len) {
>                 ...
>                 res = jffs2_sum_add_kvec(...)
>                 ...
>         }

I prefer:
	if (jffs2_sum_active() && *retlen == len) {
		...
		res = jffs2_sum_add_kvec(...)
		...
	}

In the case that part of node has been written to flash, this whole node
will be marked as dirty node, only in memory, not marked without
JFFS2_NODE_ACCURATE on flash.

If the summary is stored when *retlen != len, there are two cases:

* In most case, another write with the same node info performs
  successfully later, the node written partially before will be marked
  as obsolete node when scan, and we won't read from it
* This node is the newest node about this region, it will be treated
  as normal node when scan, and we may read the data already corrupted.
  Yet it won't break any rules of JFFS2 and lead to a muddle

The node written partially will be treated as normal node in a full scan
routine too, so I think we should mark this node as dirty on flash in
the case that *retlen != len.

Thanks,
Wei

> I ran some quick tests, simulating write failures, and it seems that 
> adding the summary entry doesn't harm when *retlen != 0 [so the minimal 
> solution would suffice]. This is because the calling function will reserve 
> the node space, marking it as dirty, and there is no confusion about 
> unchecked space.
>
> On the other hand, running the same quick tests _without_ adding the 
> summary entry didn't seem to harm either [so the general solution would 
> work as well]. It is entirely possible that I have overlooked something, 
> though.
> 
> Any opinions on that? When in doubt, I would provide a patch for the 
> minimal solution, changing as little as possible. However, it may make 
> sense to go for the general solution to be consistent with write 
> buffering.
> 
> Best regards,
> Thomas Betker
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
> 

Re: jffs2: Excess summary entries

[Wei Fang]

Hi Thomas,

On 2015/11/9 15:19, Wei Fang wrote:
> Hi Thomas,
> 
> On 2015/11/6 2:36, Thomas.Betker@rohde-schwarz.com wrote:
>> We ran into a problem with jffs2 where the filesystem became unusable 
>> after some specific MTD failures; summary was enabled, write buffering was 
>> disabled:
> 
> [snip...]
> 
>> Here are the solutions I have considered so far:
>>
>> The minimal solution would be to check *retlen == 0 in 
>> jffs2_flash_direct_writev() and jffs2_flash_direct_write() before running 
>> jffs2_sum_add_kvec().
>>
>>         if (jffs2_sum_active() && *retlen) {
>>                 ...
>>                 res = jffs2_sum_add_kvec(...)
>>                 ...
>>         }
>>
>> The general failure case, though, is (ret != 0 || *retlen != len), where 
>> 'ret' is the return code of mtd_writev(), and 'len' is the data size to be 
>> written. When write buffering is enabled, jffs2_flash_writev() in wbuf.c 
>> skips the summary entry in this case; perhaps we should do this in 
>> writev.c as well?
>>
>>         if (jffs2_sum_active() && !ret && *retlen == len) {
>>                 ...
>>                 res = jffs2_sum_add_kvec(...)
>>                 ...
>>         }
> 
> I prefer:
> 	if (jffs2_sum_active() && *retlen == len) {
> 		...
> 		res = jffs2_sum_add_kvec(...)
> 		...
> 	}
> 
> In the case that part of node has been written to flash, this whole node
> will be marked as dirty node, only in memory, not marked without
> JFFS2_NODE_ACCURATE on flash.
> 
> If the summary is stored when *retlen != len, there are two cases:
> 
> * In most case, another write with the same node info performs
>   successfully later, the node written partially before will be marked
>   as obsolete node when scan, and we won't read from it
> * This node is the newest node about this region, it will be treated
>   as normal node when scan, and we may read the data already corrupted.
>   Yet it won't break any rules of JFFS2 and lead to a muddle

I forgot there's crc check about the data, so in this case, this node
couldn't pass the crc check and will be marked as obsolete too :)

Thanks,
Wei

> The node written partially will be treated as normal node in a full scan
> routine too, so I think we should mark this node as dirty on flash in
> the case that *retlen != len.
> 
> Thanks,
> Wei
> 
>> I ran some quick tests, simulating write failures, and it seems that 
>> adding the summary entry doesn't harm when *retlen != 0 [so the minimal 
>> solution would suffice]. This is because the calling function will reserve 
>> the node space, marking it as dirty, and there is no confusion about 
>> unchecked space.
>>
>> On the other hand, running the same quick tests _without_ adding the 
>> summary entry didn't seem to harm either [so the general solution would 
>> work as well]. It is entirely possible that I have overlooked something, 
>> though.
>>
>> Any opinions on that? When in doubt, I would provide a patch for the 
>> minimal solution, changing as little as possible. However, it may make 
>> sense to go for the general solution to be consistent with write 
>> buffering.
>>
>> Best regards,
>> Thomas Betker
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>
>>

Re: jffs2: Excess summary entries

[Thomas.Betker]

Hello Wei:

> > I prefer:
> >    if (jffs2_sum_active() && *retlen == len) {
> >       ...
> >       res = jffs2_sum_add_kvec(...)
> >       ...
> >    }
> > 
> > In the case that part of node has been written to flash, this whole 
node
> > will be marked as dirty node, only in memory, not marked without
> > JFFS2_NODE_ACCURATE on flash.
> > 
> > If the summary is stored when *retlen != len, there are two cases:
> > 
> > * In most case, another write with the same node info performs
> >   successfully later, the node written partially before will be marked
> >   as obsolete node when scan, and we won't read from it
> > * This node is the newest node about this region, it will be treated
> >   as normal node when scan, and we may read the data already 
corrupted.
> >   Yet it won't break any rules of JFFS2 and lead to a muddle
> 
> I forgot there's crc check about the data, so in this case, this node
> couldn't pass the crc check and will be marked as obsolete too :)
> 
> > The node written partially will be treated as normal node in a full 
scan
> > routine too, so I think we should mark this node as dirty on flash in
> > the case that *retlen != len.

Yes, this is basically what I have been seeing in my tests: After the 
first write has failed, the node is written again, and only the second 
node is used upon remount. So I can guess we can go either way: dropping 
the summary entry, or keeping it.

By now, I have come to the conclusion that it's probably cleaner to drop 
the summary entry, just as it is done when write buffering is enabled. 
This way, we can be sure that the node data is ignored on reboot the same 
way as it is ignored at the moment when the MTD write fails. And while 
extra summary entries won't usually cause any problems, there is no 
advantage in wrting them out if they are ignored anyway.

Best regards,
Thomas Betker