Fixes two bugs for rafs v5 #1275

jiangliu · 2023-05-13T17:01:31Z

Fixes two bugs for rafs v5:

record correct inode number for v5
avoid a possible panic for v5

anolis-bot · 2023-05-13T17:03:28Z

@jiangliu , a new test job has been submitted. Please wait in patience. The test job url: https://tone.openanolis.cn/ws/nrh4nnio/test_result/72439

codecov · 2023-05-13T17:05:04Z

Codecov Report

Merging #1275 (47afaac) into master (db0cc41) will increase coverage by 0.00%.
The diff coverage is 72.72%.

@@           Coverage Diff           @@
##           master    #1275   +/-   ##
=======================================
  Coverage   45.06%   45.07%           
=======================================
  Files         126      126           
  Lines       37395    37403    +8     
  Branches    37395    37403    +8     
=======================================
+ Hits        16852    16858    +6     
- Misses      19655    19656    +1     
- Partials      888      889    +1

Impacted Files	Coverage Δ
rafs/src/metadata/md_v5.rs	`15.38% <0.00%> (ø)`
rafs/src/builder/core/bootstrap.rs	`53.45% <100.00%> (+1.78%)`	⬆️

... and 3 files with indirect coverage changes

anolis-bot · 2023-05-13T17:22:59Z

@jiangliu , The CI test is completed, please check result:

Test Case		Test Result
build rust golang image		✅ SUCCESS
compile nydusd		✅ SUCCESS
compile ctr remote		✅ SUCCESS
compile nydus snapshotter		✅ SUCCESS
run container with rafs		✅ SUCCESS
run container with zran		✅ SUCCESS
run container with rafs and compile linux		✅ SUCCESS

Congratulations, your test job passed!

imeoer · 2023-05-15T06:58:49Z

rafs/src/builder/core/v5.rs

@@ -187,7 +187,7 @@ impl Bootstrap {

        // Set super block
        let mut super_block = RafsV5SuperBlock::new();
-        let inodes_count = bootstrap_ctx.inode_map.len() as u64;
+        let inodes_count = bootstrap_ctx.inode_map.len() as u64 + 1;


Have this plus one before?

No, I found the inodes count is incorrect for v5.

Could you explain more about what this is trying to fix? Why must the inode count be one more than the actual recorded inode number on the map? Or is it because we forgot to insert the root inode in the inode_map?

Yes, the root inode is missing here:

root@903f8fcca5c8:/nydus# target/debug/nydus-image inspect images/0de6bc4212c22aa2bcc8026697cbb93db330421c7a3a09f3ae2cfa9b90c4332c [2023-05-15 09:35:34.626189 +00:00] INFO RAFS v5 super block features: HASH_BLAKE3 | EXPLICIT_UID_GID | COMPRESSION_ZSTD Inspecting RAFS :> stats Version: 5 Inodes Count: 1 Chunk Size: 1024KB Root Inode: 1 Flags: HASH_BLAKE3 | EXPLICIT_UID_GID | COMPRESSION_ZSTD Blob table offset: 0x2008 Blob table size: 0x0 Prefetch table offset: 0x2008 Prefetch table entries: 0x0 Chunk table offset: 0x0 Chunk table size: 0x0 Inspecting RAFS :> ls - 2 "text.txt" Inspecting RAFS :>

Earlier on, we had

tree.node.index = RAFS_ROOT_INODE; tree.node.inode.set_ino(RAFS_ROOT_INODE); // Filesystem walking skips root inode within subsequent while loop, however, we allow // user to pass the source root as prefetch hint. Check it here. ctx.prefetch.insert_if_need(&tree.node); bootstrap_ctx.inode_map.insert( (tree.node.layer_idx, tree.node.src_ino, tree.node.src_dev), vec![tree.node.index], );

Should we add it back instead?

The inode_map is used to detect hardlinks for regular files, so it doesn't make sense to add directories inode (including /) into the inode_map. How about adding a comments for the `+`` instead?

But we have all the other directory inodes (except for the root inode) in the inode_map, no?

fair enough:)

bergwolf · 2023-05-15T07:56:35Z

rafs/src/builder/core/v5.rs

@@ -187,7 +187,7 @@ impl Bootstrap {

        // Set super block
        let mut super_block = RafsV5SuperBlock::new();
-        let inodes_count = bootstrap_ctx.inode_map.len() as u64;
+        let inodes_count = bootstrap_ctx.inode_map.len() as u64 + 1;


Could you explain more about what this is trying to fix? Why must the inode count be one more than the actual recorded inode number on the map? Or is it because we forgot to insert the root inode in the inode_map?

bergwolf · 2023-05-15T07:58:32Z

rafs/src/metadata/md_v5.rs

-                    let next_size = if next_size < window_size {
+                    let next_size = if next_size == 0 {
+                        continue;
+                    } else if next_size < window_size {


Which debug_assert is triggered by this? Could you paste the error message?

Refined the commit message.

Cannot find it. Did you forget to push the new code? :)

anolis-bot · 2023-05-15T13:47:27Z

@jiangliu , the code has been updated, so a new test job has been submitted. Please wait in patience. The test job url: https://tone.openanolis.cn/ws/nrh4nnio/test_result/72770

In function RafsSuper::amplify_io(), is the next inode `ni` is zero-sized, the debug assertion in function calculate_bio_chunk_index() (rafs/src/metadata/layout/v5.rs) will get triggered. So zero-sized file should be skipped by amplify_io(). Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>

anolis-bot · 2023-05-15T13:48:23Z

@jiangliu , the code has been updated, so a new test job has been submitted. Please wait in patience. The test job url: https://tone.openanolis.cn/ws/nrh4nnio/test_result/72771

anolis-bot · 2023-05-15T14:06:42Z

@jiangliu , The CI test is completed, please check result:

Test Case		Test Result
build rust golang image		✅ SUCCESS
compile nydusd		✅ SUCCESS
compile ctr remote		✅ SUCCESS
compile nydus snapshotter		✅ SUCCESS
run container with rafs		✅ SUCCESS
run container with zran		✅ SUCCESS
run container with rafs and compile linux		✅ SUCCESS

Congratulations, your test job passed!

anolis-bot · 2023-05-15T14:24:54Z

@jiangliu , The CI test is completed, please check result:

Test Case		Test Result
build rust golang image		✅ SUCCESS
compile nydusd		✅ SUCCESS
compile ctr remote		✅ SUCCESS
compile nydus snapshotter		✅ SUCCESS
run container with rafs		✅ SUCCESS
run container with zran		✅ SUCCESS
run container with rafs and compile linux		❌ FAIL

Sorry, your test job failed. Please get the details in the link.

Add root inode into inode map when building RAFS filesystem, so RAFS v5 gets correct inode number counts. Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>

anolis-bot · 2023-05-16T07:35:26Z

@jiangliu , the code has been updated, so a new test job has been submitted. Please wait in patience. The test job url: https://tone.openanolis.cn/ws/nrh4nnio/test_result/72973

anolis-bot · 2023-05-16T07:54:27Z

@jiangliu , The CI test is completed, please check result:

Test Case		Test Result
build rust golang image		✅ SUCCESS
compile nydusd		✅ SUCCESS
compile ctr remote		✅ SUCCESS
compile nydus snapshotter		✅ SUCCESS
run container with rafs		✅ SUCCESS
run container with zran		✅ SUCCESS
run container with rafs and compile linux		✅ SUCCESS

Congratulations, your test job passed!

bergwolf

lgtm! Thanks!

jiangliu requested a review from a team as a code owner May 13, 2023 17:01

jiangliu requested review from liubin, liubogithub and luodw and removed request for a team May 13, 2023 17:01

jiangliu added backport/v2.1 backport/v2.2 labels May 13, 2023

anolis-bot added the anolis_testing label May 13, 2023

jiangliu mentioned this pull request May 13, 2023

Optimize RAFS filesystem builder #1257

Merged

anolis-bot added anolis_test_pass and removed anolis_testing labels May 13, 2023

imeoer reviewed May 15, 2023

View reviewed changes

bergwolf reviewed May 15, 2023

View reviewed changes

jiangliu force-pushed the rafsv5 branch from adcfe0e to d15e17b Compare May 15, 2023 13:47

anolis-bot added anolis_testing and removed anolis_test_pass labels May 15, 2023

jiangliu force-pushed the rafsv5 branch from d15e17b to 4638241 Compare May 15, 2023 13:48

anolis-bot added anolis_test_pass and removed anolis_testing labels May 15, 2023

anolis-bot added anolis_test_fail and removed anolis_test_pass labels May 15, 2023

rafs: add root inode into inode map when building RAFS

47afaac

Add root inode into inode map when building RAFS filesystem, so RAFS v5 gets correct inode number counts. Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>

jiangliu force-pushed the rafsv5 branch from 4638241 to 47afaac Compare May 16, 2023 07:35

anolis-bot added anolis_testing and removed anolis_test_fail labels May 16, 2023

anolis-bot added anolis_test_pass and removed anolis_testing labels May 16, 2023

bergwolf approved these changes May 16, 2023

View reviewed changes

bergwolf merged commit 293b032 into dragonflyoss:master May 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes two bugs for rafs v5 #1275

Fixes two bugs for rafs v5 #1275

jiangliu commented May 13, 2023

anolis-bot commented May 13, 2023

codecov bot commented May 13, 2023 •

edited

Loading

anolis-bot commented May 13, 2023

imeoer May 15, 2023

jiangliu May 15, 2023

bergwolf May 15, 2023

jiangliu May 15, 2023

bergwolf May 15, 2023

jiangliu May 15, 2023

bergwolf May 16, 2023

jiangliu May 16, 2023

bergwolf May 15, 2023

bergwolf May 15, 2023

jiangliu May 15, 2023

bergwolf May 15, 2023

jiangliu May 16, 2023

anolis-bot commented May 15, 2023

anolis-bot commented May 15, 2023

anolis-bot commented May 15, 2023

anolis-bot commented May 15, 2023

anolis-bot commented May 16, 2023

anolis-bot commented May 16, 2023

bergwolf left a comment

Fixes two bugs for rafs v5 #1275

Fixes two bugs for rafs v5 #1275

Conversation

jiangliu commented May 13, 2023

anolis-bot commented May 13, 2023

codecov bot commented May 13, 2023 • edited Loading

Codecov Report

anolis-bot commented May 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anolis-bot commented May 15, 2023

anolis-bot commented May 15, 2023

anolis-bot commented May 15, 2023

anolis-bot commented May 15, 2023

anolis-bot commented May 16, 2023

anolis-bot commented May 16, 2023

bergwolf left a comment

Choose a reason for hiding this comment

codecov bot commented May 13, 2023 •

edited

Loading