subsys: fs/nvs: Improvements

[Laczen]

NVS needed improvements to provide better support for systems with write block size > 1 byte. Also the mechanism to protect NVS against incomplete writes was not good enough. This PR provides a rewrite of NVS to handle both problems.

The way data is stored on flash, has been modified: data is stored in length-id-data-length and this complete set is aligned. The length at the end is used as a verification at start (nvs_init). An advantage of this change is also that the metadata (length and id) takes up less space so more data can be stored.

A CRC is not added to the data, but (if required) this can be done by the user application easily.

After each write a read is performed to check if the data was written OK.

When an error is detected (either during write/verification/initialization) NVS goes into a locked state. This allows the recovery of data before the error. A call to reinit() when in locked state erases flash storage and enables write acces.

[Laczen]

Hi @aurel32, I have updated NVS to provide better support on systems with write block size > 1. If you have some time could you please test/review.

[dbkinder]

remove the double back ticks here. The double colon at the end of the previous line says the following indented text is a code-block, so as written the back ticks will show up in the generated doc output.

[dbkinder]

Clean up some capitalization issues, and combine the sentences a bit for clarity:

This is a simple application demonstrating use of the NVS
module for non-volatile (flash) storage.  In this application,
a counter is incremented on every reboot and stored in flash,
the application reboots, and the reboot counter data is retrieved.

[dbkinder]

Missing the build and run commands (and sample output).

[dbkinder]

remove the double back ticks here. The double colon at the end of the previous line says the following indented text is a code-block, so as written the back ticks will show up in the generated doc output.

[dbkinder]

Again, the two "length" metadata fields are confusing?

[dbkinder]

best to add a space around arithmetic operators: 254 * 20,000` minutes

[aurel32]

Hi @Laczen, thanks for this new version. I'll try to have a look at it and try it tonight, if not tomorrow night. So far I have only seen it is based on an old version of the master branch which is in conflict with the current one. Any chance you can rebase it? Anyway that should not prevent me to test it.

[nvlsianpu]

thanks for the patch - will review it soon.

[Laczen]

Hi @dbkinder, I have updated the documentation as requested.

[codecov-io]

Codecov Report

Merging #8141 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8141      +/-   ##
==========================================
+ Coverage   52.47%   52.48%   +<.01%     
==========================================
  Files         198      198              
  Lines       24977    24975       -2     
  Branches     5210     5210              
==========================================
  Hits        13107    13107              
+ Misses       9761     9759       -2     
  Partials     2109     2109

Impacted Files	Coverage Δ
include/logging/log_core.h	80% <0%> (+13.33%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72f75e5...362581c. Read the comment docs.

[nashif]

please fix those, looks like a conflict that was not resolved.

[Laczen]

@nashif, corrected

[aurel32]

Hi @Laczen. Thanks for the rebase, it makes the testing a lot more easy, as my application depends on changes that have been done recently.

I have tested your patch on my board, and it works as expected. I haven't been able to reproduce the issue with the garbage collector on power failure. I'll review the code itself tomorrow.

As said in our previous discussion, mixing the metadata and the data doesn't allow to implement a secure delete function (at least when the write block size is > 1), that said i believe we can do the same by triggering the garbage collector on the sector after writing the new/empty value.

[nvlsianpu]

will put much more commend on Monday

[nvlsianpu]

f. name shoud describes what the function does, maybe: _nvs_flash_len_align or _nvs_flash_get/calc/_aligned_len

[Laczen]

I will change this to: _get_wbs_aligned_len()

[nvlsianpu]

previous name was better, maybe _nvs_flash_get/calc/_entry_len

[Laczen]

I will change this to get_wbs_aligned_entry_len()

[nvlsianpu]

better: _nvs_decode_lenght

[nvlsianpu]

better: _nvs_encode_lenght

[nvlsianpu]

I thing a too big size error should be generated in case the length exceeds 0x3fff. For example this function can return 0xFFFF in this case, and this value can be recognized as the error signal once it was caled by the _nvs_flash_wrt_entry function.

[nvlsianpu]

What about changing encoding - It would increase the range by one byte:

short length <0, 0x7f>:

---------------
| B0          |
|-------------|
|b7 | b0 - b6 |
|-------------|
| 0 | length  |
---------------

long length <0, 0x7fff):

------------------------------------
| B0               | B1            |
|------------------|---------------|
|b7 | b0 - b6      |   b0 - b7     |
|------------------|---------------|
| 1 | length >> 8  | length & 0xFF |
------------------------------------

[nvlsianpu]

What about changing encoding - It would increase the range by one bit:

short length <0, 0x7f>:

---------------
| B0          |
|-------------|
|b7 | b0 - b6 |
|-------------|
| 0 | length  |
---------------

long length <0, 0x7fff):

------------------------------------
| B0               | B1            |
|------------------|---------------|
|b7 | b0 - b6      |   b0 - b7     |
|------------------|---------------|
| 1 | length >> 8  | length & 0xFF |
------------------------------------

[Laczen]

There should still be a possibility to detect 0xff and 0xff. The range of 16383 bytes should be more then sufficient.

[Laczen]

After rethinking, it is easily changed and easier to understand than the present. I will change it to the proposal, except length is max 0x7ffe.

[nvlsianpu]

better: _nvs_decode_lenght

[Laczen]

Yes.

[nvlsianpu]

better: _nvs_encode_lenght

[Laczen]

Yes.

[aurel32]

I have finally reviewed your changes, sorry for the delay. Overall the new flash format and the architecture of the code looks fine to me. I think it still needs some improvements in a few places, but nothing really big.

[aurel32]

You should define block_size as size_t, as it's the type returned by sizeof and also the type of len, so that the comparison is done on the same type.

[Laczen]

Correct block_size should be size_t, will change

[aurel32]

I think the code can be simplified by using a while (len > 0), and then computing bytes to compare as min(block_size, len). Actually it's the strategy chosen a few lines before in _nvs_flash_block_move.

[Laczen]

Correct, will change (and also change block_size to size_t)

[aurel32]

I know that ver means "verify", but "version" first came to my mind. I think it would make it more understandable to call it _nvs_flash_write_cmp, and would be consistent with _nvs_flash_block_cmp.

[Laczen]

Yes, _cmp is more consistent, will change

[aurel32]

All the cases in this function looks like error prone. For example if write_block_size is 2 bytes and the data length to be written below 128 bytes, byte_cnt will be 3. This mean the first call to _nvs_flash_write_ver will try to write 3 bytes, which is the wrong number for a 2-byte write block size.

[Laczen]

This is an error, I will review this function once again to make it less error prone.

[aurel32]

It looks simpler to test for len_size != 0xffff and use the value of len_size directly instead of testing both cases.

[aurel32]

There you probably want to use min(len, len_flash), to make sure the data from the next entry, or even from outside the NVS area is read. This could be a security issue when combined with missing input validation from the caller. I actually wonder if _nvs_flash_read should also limit the access to the NVS area only.

[aurel32]

I think all those private functions that are not part of the API should just be marked as static in nvs.c, so that the compiler can do more aggressive optimizations. I just report that here, but that applies to many functions in this file.

[aurel32]

I don't know zephyr enough to know about the API policy, but removing this function is breaking the API. I know there is also a file system format change, so that might be fine after all.

[Laczen]

I will reintroduce nvs_clear, if the user would like to clear the flash area for some other purpose, nvs_clear should be there.

[nvlsianpu]

I thing a too big size error should be generated in case the length exceeds 0x3fff. For example this function can return 0xFFFF in this case, and this value can be recognized as the error signal once it was caled by the _nvs_flash_wrt_entry function.

[Laczen]

Yes, I have introduced a define NVS_MAX_LEN (0x7ffd), 0xffff is returned if len> NVS_MAX_LEN.

[nvlsianpu]

add spaces around - operator.

[nvlsianpu]

spaces around '+'

[nvlsianpu]

why the second length is swapped?

[Laczen]

Hi @nvlsianpu, thanks for all comments, the second length needs to be swapped because you will always read 2 bytes, but this can be data as well, so to make sure you get the correct length you are only sure about the last byte (this is definitely part of the length), so the last byte should be byte0 of the length.

[nvlsianpu]

thank for elaboration.

[nvlsianpu]

why not _nvs_addr_advance(fs, &addr, len_size + sizeof(u16_t)); instead of this 'if'?

[Laczen]

Correct, changed

[nvlsianpu]

I think it would be worth to serve the case when this is about the last entry which was not entirely written to the flash (caused for instance by power down).

[Laczen]

Hi @nvlsianpu, I am changing this routine a bit, fs->write_location is now updated everytime the length comparison is ok. So in the case of an entry that was not completely written the initialization routine will give a fs that is read-only but does not include the latest (invalid) entry. It is then possible to retrieve all information that is valid.

[nvlsianpu]

What you are changing sound fine to me. Otherwise what I was asking might be added in a future PR as well. I think this is very likely to have the last entry corrupted for some devices. I would be great If nvs would can fix this in place by performing customized sector rotation.

[nvlsianpu]

I understood that this fill procedure is introduced due the backward serach feature you had added?

[Laczen]

Hi andrzej, also the first version of NVS has filling at the end of a sector. The length was even set to jump over the next sector header. However only the length was written to flash. Now ff's are written. The procedure has changed because of the variable length format, making the length parameter variable in size makes it impossible to reach certain sizes (e.g. on a system with byte write size a length of 127 bytes generates a fill block of 133 bytes; a length of 128 bytes generates a length of 136 bytes, so it is impossible to generate a block of size 134 and 135). This is of course more complicated for different write sizes, so I decided to avoid this difficulty by cutting it down to smaller blocks.

[nvlsianpu]

note that "larger dataset" means those which are using the long format of length fields.

[Laczen]

Yes

[nvlsianpu]

@Laczen I put all comments I had.

[Laczen]

Hi @aurel32 and @nvlsianpu, I have integrated your comments in my latest rework.

[lemrey]

Good job guys! I'd suggest to make write verification optional, since some drivers already implement that.
In general, are there plans to make the flash program unit available at compile time? That'd simplify things a bit, I believe.

[nvlsianpu]

I think we still need an automatic mechanism for restore FS automatically after the power down while writing issue - it is something we really need for any device in the field. this comment is continuation of the #8141 (comment) thread
cc @lemrey You opinion?

[lemrey]

It is definitely desirable, the scenario is not uncommon.
I will comment on #8367.

[Laczen]

Hi @lemrey, thanks for the comment. I think it's a good idea to make the verification optional. I will add this.

I don't quite get what you mean by making the flash program unit available at compile time. Could you explain this a bit more ?

[lemrey]

Sure @Laczen ,
I wasn't referring to nvs in particular with that, but more generally about zephyr's flash subsystem.
I believe that flash libraries could benefit from knowing how large is the smallest unit of memory that can independently programmed (written). I think that is not possible at the moment, but it would spare some of the logic necessary for padding perhaps? :)

[aurel32]

I believe that flash libraries could benefit from knowing how large is the smallest unit of memory that can independently programmed (written).

Even if you know it, it doesn't mean it is fixed. You can have a flash internal to the SoC with a write block size of 4 bytes, and an SPI flash with a write block size of 1.

[lemrey]

You are right @aurel32 :/
I guess we'll have to live with that then!

[lemrey]

Is there any reason why we a reading one byte at a time?
I would suggest to read a multiple, like a word, to reduce the number of calls.

[lemrey]

This is basically the maximum length that will be passed to the driver API in a single call, right?
I can definitely see a use for this, and I would be great to let the user see it menuconfig.

[nvlsianpu]

@Laczen Better to keep this within this PR as we have discussion story here.

[Laczen]

@nvlsianpu, @lemrey, @aurel32, to improve robustness against incomplete writes I have rewritten NVS with a completely different layout on flash. It now stores the data at the beginning of a sector and the metadata (id, length and a offset to the data) at the end of a sector. This way NVS should always be readable in case of a incomplete write (either data or metadata).
@dbkinder, I still have to update the documentation to reflect the changes.

[nvlsianpu]

spaces around + operator

[nvlsianpu]

/* flash ate read */

[nvlsianpu]

static

[nvlsianpu]

static

[nvlsianpu]

static

[nvlsianpu]

static

[nvlsianpu]

static

[nvlsianpu]

static

[nvlsianpu]

static

[nvlsianpu]

Is it ensured that read operations are aligned to block-size? It is not expected form a flash driver that it support byte alignment while reading.

[nvlsianpu]

static

[nvlsianpu]

static

[nvlsianpu]

Can we avoid magic values - It looks great now for code review as it helps understanding code while I'm analyzing it, but in few months it will become an dark secret for me and other developers.
This applies to address's mask as well.

Of course using 0xff and 0x00 for memory fills are OK everywhere.

[Laczen]

Hi @nvlsianpu, I am not sure what you mean by this comment. The address definition that is used in nvs is: high 2 bytes are the sector number, low 2 bytes are the offset in the sector. The calculation is mapping the address to an offset in flash. All address masks used are either to find the sector (addr >> 16) or the offset (addr & 0xffff).

[nvlsianpu]

That I understood well. What I mens to use predefined mask for extracting these numbers/addresses, something like that:

#define  _SECTOR_NUM_MASK 0xFFFF0000
#define  _SECTOR_NUM_SHIFT 16
#define  _OFF_MASK 0x0000FFFF

[nvlsianpu]

exveption -> exception

[nvlsianpu]

rc = _nvs_flash_cmp_const(...

[nvlsianpu]

this renew assignment from line538

[Laczen]

addr has been changed by _nvs_prev_ate()

[nvlsianpu]

ok

[nvlsianpu]

shouldn't nvs iterates more for seek ate_wra? this assume that ate_wra referencing the second ate in a sector.

[Laczen]

no ate_wra is found by _nvs_prev_ate(), more iterations are not needed. ate_wra is not referencing the second ate in a sector.

[nvlsianpu]

ok

[nvlsianpu]

I reviewed re-implementation complacently.

[nvlsianpu]

spaces around *

[Laczen]

I think leaving spaces around * makes it confusing, I am using spaces for +/- but not for *.

[nvlsianpu]

I see your point.

[nvlsianpu]

revert free space value before exit as write didn't happen - a smaller entry might fit.

[Laczen]

correct, wil change this

[nvlsianpu]

In API documentation pleas rephrase Any other value, indicates an error. -> Any negative value, indicates an error.
This is applicable to the nvs_read() as well.

[nvlsianpu]

This is fixed.

[nvlsianpu]

I don't understand ho we come to this case as fs->ate_wra refer to place before newest ate metadata.

[Laczen]

This happens when we have completely gone through nvs in reverse direction.

[Laczen]

Hi @nvlsianpu, I think all your remarks have been addressed in the latest commit.

[nvlsianpu]

@Laczen I'm happy witch changes. Can you fix shippable, merge conflict and clean git history (squash some commit, especial these which align code to review request.

Appart form that the code is in expected shape to be approved.

[Laczen]

@nvlsianpu, fixed shippable and merge conflict (was result of removing no longer valid configuration option NVS_PROTECT_FLASH, I will remove this option in a later PR).

[nvlsianpu]

@nashif Ready to be merged IMO.

[nvlsianpu]

@nashif, @aurel32 - have you still any pending comments on the code - It is ready since few weeks.
@lemrey - has not as far I know.
@galak, @carlescufi - can we merge this or we should wait for @nashif

[aurel32]

Hi @Laczen. Sorry to not come back earlier about this, I have been busy with real life.

I gave a look to your new implementation, and I like the new design of not mixing the data and the metadata. Thanks also for keeping a CRC. Overall it looks good to me. I have however found a few issues and small things that can be improved. The main one is that it doesn't work when block size != 1. I will send a pull request to fix it. I will also prepare a pull request for the small improvements.

I have still a few things that I don't fully understand, I hope you don't mind I ask them there:

• There are 3 places with this code:
  block_size = NVS_BLOCK_SIZE & ~(fs->write_block_size - 1);
  I am not fully sure what it is supposed to do. Moreover it will return 0 if write_block_size is bigger than NVS_BLOCK_SIZE and will end-up in an endless loop.

• I am not fully sure about the free space accounting. In my case with 2x2048-byte sectors, the free space is always 7c8 aka 1992 bytes at start, independently of the data already present. In addition I think there is an issue in the following part of _nvs_flash_wrt:

        rc = _nvs_flash_data_wrt(fs, data, len);
        if (rc) {
                return rc;
        }
        rc = _nvs_flash_ate_wrt(fs, &entry);
        if (rc) {
                return rc;
        }

        if (len != 0) {
                fs->free_space -= _nvs_al_size(fs, len);
                fs->free_space -= ate_size;
        }

When len is 0, the ate is still written, so the free space is reduced. I am also not sure it's a good idea to call _nvs_flash_data_wrt. Even it if doesn't write anything, it still unsets the flash protection and sets it back.

• nvs_clear calls _nvs_flash_erase_sector on all sectors which errors out if the sector is already erased. It means that all sectors might not been erased.

[Laczen]

Hi @aurel32, thanks for the fix when block size !=1, I don't understand how I missed this. I have also been busy doing other things. Regarding the calculation of block_size, this is only to protect for silly definitions of NVS_BLOCK_SIZE, e.g. setting it to 12 on a system with write_block_size 8. You are correct that there is a problem when NVS_BLOCK_SIZE is smaller than write_block_size, but I don't know how to fix that. Since NVS can be used on external flash, internal flash, ... there is no way of figuring out in advance what a good setting of NVS_BLOCK_SIZE is. I set it to 8 because I expect that for the moment that are no systems with write_block_size bigger than 8. Regrading the free space calculation: the free space calculation should return how much data can be written to the system (possibly after garbage collection), it is not an indication of how much flash is used at the moment. Deleted items will not be copied when doing gc, so their data and also ate should not be counted. On your system I would expect it to report 0x7d8, as NVS will always need two extra ate's for pointing to the last entry and a sector closing ate. I need to go through the code again to see why there is space reserved for two extra ate's (possibly to always allow a delete). Regarding the call of _nvs_flash_data_wrt(), the catch of len=0 would be good to handle in the aligned write, than the flash protection unset and set can be avoided. Regarding nvs_clear I need to check the code again. Op do 23 aug. 2018 om 23:37 schreef aurel32 <[email protected]>:

…

Hi @Laczen <https://github.com/Laczen>. Sorry to not come back earlier about this, I have been busy with real life. I gave a look to your new implementation, and I like the new design of not mixing the data and the metadata. Thanks also for keeping a CRC. Overall it looks good to me. I have however found a few issues and small things that can be improved. The main one is that it doesn't work when block size != 1. I will send a pull request to fix it. I will also prepare a pull request for the small improvements. I have still a few things that I don't fully understand, I hope you don't mind I ask them there: - There are 3 places with this code: block_size = NVS_BLOCK_SIZE & ~(fs->write_block_size - 1); I am not fully sure what it is supposed to do. Moreover it will return 0 if write_block_size is bigger than NVS_BLOCK_SIZE and will end-up in an endless loop. - I am not fully sure about the free space accounting. In my case with 2x2048-byte sectors, the free space is always 7c8 aka 1992 bytes at start, independently of the data already present. In addition I think there is an issue in the following part of _nvs_flash_wrt: rc = _nvs_flash_data_wrt(fs, data, len); if (rc) { return rc; } rc = _nvs_flash_ate_wrt(fs, &entry); if (rc) { return rc; } if (len != 0) { fs->free_space -= _nvs_al_size(fs, len); fs->free_space -= ate_size; } When len is 0, the ate is still written, so the free space is reduced. I am also not sure it's a good idea to call _nvs_flash_data_wrt. Even it if doesn't write anything, it still unsets the flash protection and sets it back. - nvs_clear calls _nvs_flash_erase_sector on all sectors which errors out if the sector is already erased. It means that all sectors might not been erased. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8141 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjVvNmiB0x5tY17ZInmPDvH_p-McaMSks5uTyCbgaJpZM4UYs0O> .

[aurel32]

Hi @aurel32, thanks for the fix when block size !=1, I don't understand how I missed this. I have also been busy doing other things.

No problem, this kind of things happen.

Regarding the calculation of block_size, this is only to protect for silly definitions of NVS_BLOCK_SIZE, e.g. setting it to 12 on a system with write_block_size 8. You are correct that there is a problem when NVS_BLOCK_SIZE is smaller than write_block_size, but I don't know how to fix that.

Ok, now that I understand the use case, I'll try to think about a solution. As you said very few systems should have a write block size bigger than 8 bytes.

Regrading the free space calculation: the free space calculation should return how much data can be written to the system (possibly after garbage collection), it is not an indication of how much flash is used at the moment.

Thanks for the explanation, I'll look at that part again with that in mind.

Regarding the call of _nvs_flash_data_wrt(), the catch of len=0 would be good to handle in the aligned write, than the flash protection unset and set can be avoided.

Ok, I'll try to do that and include that in PR #9601.

[Laczen]

Hi @aurel32, I have had some time to look into the problem of the free space calculation as well as nvs_clear.

I have not been able to reproduce your numbers for the free space calculation. When nvs is initialized (with a sector of 2048 byte) it reports a free space of 2040 bytes, and the alloc wra is 0x7f8.

For nvs_clear() I also do not see it error out on already erased sectors, the compare with 0xff returns a zero and _nvs_flash_erase_sector() returns a 0. It just does not do a erase (because it is not needed).

[aurel32]

Hi @Laczen, thanks for checking my points on nvs_clear and free space calculation. I have tried to look at both of them in details, and then both looks fine. I probably test them wrongly last time, sorry about that.