Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add zffs #11026

Closed
wants to merge 12 commits into from
Closed

Add zffs #11026

wants to merge 12 commits into from

Conversation

findlayfeng
Copy link
Contributor

@findlayfeng findlayfeng commented Nov 2, 2018

ZFFS is supposed to be used instead of NFFS.

Reason for creatin another FS is that there are so many serious bugs in NFFS.
While trying to fix them, I found that I was almost going to refactored it.
Why not rewrite one?

It has been roughly completed.

This PR contain the flash_map external extension PR
Since I used the modified flash_map api in #8837, I merged #8837 into it.

Unfinished work.

  • Garbage collection
  • Document
  • Tests

I think I need some help.

@codecov-io
Copy link

codecov-io commented Nov 2, 2018

Codecov Report

Merging #11026 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #11026   +/-   ##
=======================================
  Coverage   48.37%   48.37%           
=======================================
  Files         265      265           
  Lines       42188    42188           
  Branches    10137    10137           
=======================================
  Hits        20408    20408           
  Misses      17703    17703           
  Partials     4077     4077

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 817f4b3...6d81b40. Read the comment docs.

@hongshui3000
Copy link
Contributor

is it appropriate to put it in the ext folder?

subsys/fs/Kconfig Outdated Show resolved Hide resolved
@findlayfeng
Copy link
Contributor Author

@hongshui3000 lib folder ?

@findlayfeng findlayfeng force-pushed the add_znffs branch 2 times, most recently from 3260877 to 9064081 Compare November 3, 2018 06:40
@hongshui3000
Copy link
Contributor

hongshui3000 commented Nov 3, 2018

There are many bugs in nffs. What are the bugs in the nffs? Compared with nffs, what are the improvements of znffs? @findlayfeng

@nvlsianpu
Copy link
Collaborator

nvlsianpu commented Nov 3, 2018

There are many bugs in nffs.

That is known - I have in my backlog to fix them all.

Trying to fix it, I found that I almost refactored it.

I agree that there is a lot of effort required to fix the bugs, but not really refactoring 100%. Rather something like 20%. Two solution proposals were discuses here: apache/mynewt-nffs#10

Why not rewrite one?

We know that NFFS have two serious issue. many other bugs were squashed so far. Another FS will have similar childhood diseases. I prefer to fix NFFS than introduce another FS - even if this mean to refactor a lot (conclusion from discussion mentioned above was that we expected to refactor a lot).
I expected a lot of bug squashing for znffs as well - this is just by my experience. I saw similar situation many times - the last was NVS. Even tough someone is a excellent developer - nobody is perfect.

But - adding a FS might have sense...
I can see sense in introduction of another FS if it give us serious capabilities which are not provided by any other FS we have. Adding a new module mean to maintain it as well. (This was the case when we accepted NVS introduction a few months ago)

I'm not against this path - but I need serious reasons, even for reviewing this.

@carlescufi ^^

@findlayfeng
Copy link
Contributor Author

嗯,采用nffs的一个理由是较少的ram占用。这个在小容量设备非常致命。
我是用的是 nrf51822_qfac(32kb ram)。
我仔细的阅读过nffs的代码。
对于内存的使用没有很好的控制,或者说不能以牺牲速度的前提下,节约内存的使用。
另外遇到的问题还有,以回收节点为目标的垃圾回收过程不是有效的,会造成一直不间断的copy
当几乎满节点的情况下,重建过程中遇到删除的节点不能正确处理,因为会超出系统的内存最大值。
节点数太容易达到上限(几乎所有操作都会增加节点,意味着更多的内存占用,为了合并更多的节点,垃圾回收需要更多的内存,非常的矛盾)
还有一个不够严谨的部分,crc,我认为存储结构设计者并不是非常的了解这种算法的特性
From Google Translate
Well, one reason to adopt nffs is that less ram is used. This is very deadly in small capacity devices.
I am using nrf51822_qfac (32kb ram).
I have read the code of nffs carefully.
There is no good control over the use of memory, or the use of memory can be saved without sacrificing speed.
Another problem encountered is that the garbage collection process targeting the recycling node is not effective and will result in uninterrupted copying.
In the case of almost full nodes, nodes that encounter deletion during the rebuild process cannot be processed correctly because the system's memory maximum is exceeded.
The number of nodes is too easy to reach the upper limit (almost all operations will increase the number of nodes, meaning more memory usage, in order to merge more nodes, garbage collection requires more memory, very contradictory)
There is also a less rigorous part, crc, I think storage architecture designers are not very aware of the characteristics of this algorithm.

@findlayfeng
Copy link
Contributor Author

@nvlsianpu nffs在内存中维护一个哈希表,所有的节点都会被添加到哈希表,这是一个在小内存的情况下限制nffs性能的因素。
我使用一个b-tree(增量式写入硬盘+缓存方式)的实现代替了它。
最后导致几乎需要重构,干脆完全的重构了。
From google translate
Nffs maintains a hash table in memory, and all nodes are added to the hash table, which is a factor limiting the performance of nffs in the case of small memory.
I replaced it with an implementation of a b-tree (incremental write to hard disk + cache).
In the end, it almost requires refactoring, and it is completely refactored.

@findlayfeng
Copy link
Contributor Author

@nvlsianpu I started to pay attention to the problem of nffs. The reason is what you discussed.
At first I thought it was wrong to call the delete, but also tried to patch, after a few tests and found that this is not correct, began to read the nffs code. More and more problems are starting to appear, and there are many fixes for problems in my local repository.
So you saw the prototype of znffs.
Zephyr api is used, so ‘z’ is added before the name

@carlescufi
Copy link
Member

@findlayfeng thanks for this submission.
The first thing to do in this PR is that if you are going to replace NFFS with a fork (znffs), then you should do the following things in this PR:

  • Rename znffs to zffs
  • Move the code from ext/fs/znffs to subsys/fs/zffs
  • Make both filesystem available for a while, leaving ext/fs/nffs and subsys/fs/nffs_fs.c untouched

That way we will be able to compare and have both side-by-side until we decide to take one direction.

@carlescufi
Copy link
Member

@findlayfeng also please address the outstanding issues in #8837 so we can merge it, then this PR will not have any dependencies.

@nvlsianpu
Copy link
Collaborator

I think what @carlescufi proposed (have nffs and znffs side by side as transient steep) have sense (discussed with him off- channel).
@findlayfeng Your proposition of implements algorithms differently also make sense (thanks for elaboration). Looks like making zffs will be better idea than fixing nffs (we saw that it was had to have support for it very recently)

I look roughly trough the code. At the first sight I saw that you should use zephyr primitives instead of own implementations - misc/dlist.h, misc/slist.h.

@findlayfeng
Copy link
Contributor Author

I think what @carlescufi proposed (have nffs and znffs side by side as transient steep) have sense (discussed with him off- channel).

Thanks for your proposed, I will adjust the name

@findlayfeng Your proposition of implements algorithms differently also make sense (thanks for elaboration). Looks like making zffs will be better idea than fixing nffs (we saw that it was had to have support for it very recently)

I look roughly trough the code. At the first sight I saw that you should use zephyr primitives instead of own implementations - misc/dlist.h, misc/slist.h.

Thanks for your advice.
The slist and dlist here are implemented on disk and not memory.
I think misc/list_gen.h is probably used, unlike the peek function of misc/dlist.h, misc/slist.h.

subsys/settings/Kconfig Outdated Show resolved Hide resolved
subsys/settings/Kconfig Outdated Show resolved Hide resolved
@carlescufi carlescufi changed the title Add znffs Add zffs Nov 13, 2018
subsys/fs/CMakeLists.txt Outdated Show resolved Hide resolved
subsys/settings/Kconfig Outdated Show resolved Hide resolved
subsys/settings/Kconfig Outdated Show resolved Hide resolved
Copy link
Collaborator

@tbursztyka tbursztyka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verify the code style.

Also, in some places there are so many if/for/do/while/switch imbricated (4 or more) that it makes the code hard to read. Usually this can be improved a lot by reversing and exiting from some test, or moving parts of code in dedicated functions, or even using carefully goto.

Different flash devices will rely on different buses,
so use different .yaml files.

Signed-off-by: Findlay Feng <[email protected]>
Qspi with nrf52840 enabled and Added driver code(block only).

Signed-off-by: Findlay Feng <[email protected]>
Updated the dts configuration of pca10056.
Added an example of qspi flash controller.

Signed-off-by: Findlay Feng <[email protected]>
Add zephyr,flash_map to choose, accept values or lists,
default is zephyr,flash
zephyr,flash_map is used to generate flash_drivers_map

Signed-off-by: Findlay Feng <[email protected]>
generate default_flash_map

Signed-off-by: Findlay Feng <[email protected]>
It introduces flash_map subsystem to operate on flash
footprint instead of flash_driver.
Use flash_area ID index image, not offset.
Select FLASH_MAP.
Update tests.

Signed-off-by: Findlay Feng <[email protected]>
Now flash img uses the flash area id to determine which flash partition
is being operated.
Update tests.

Signed-off-by: Findlay Feng <[email protected]>
update flash img api
update mcuboot api

Signed-off-by: Findlay Feng <[email protected]>
update flash img api
update mcuboot api

Signed-off-by: Findlay Feng <[email protected]>
Added flash_map example

Signed-off-by: Findlay Feng <[email protected]>
flash_area_get_sector_info_by_offs(),
flash_area_get_sector_info_by_idx() Get sector information
flash_area_get_sector_count() Get sector information

Signed-off-by: Findlay Feng <[email protected]>
todo

Signed-off-by: Findlay Feng <[email protected]>
@nashif nashif removed the TSC Topics that need TSC discussion label Feb 6, 2019
@hongshui3000
Copy link
Contributor

What is the status of this pull request?

@nvlsianpu
Copy link
Collaborator

looks like orphaned.

@nvlsianpu
Copy link
Collaborator

I'm closing this PR as:

  • PR is orphaned by the contributor, no others interested in finishing it
  • litlefs was introduced as another substitute to NFFS. It is much better to use it.
  • the code is far from being ready to merge (even without conflicts)

@nvlsianpu nvlsianpu closed this Oct 8, 2019
@findlayfeng findlayfeng deleted the add_znffs branch April 19, 2023 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants