Restrain rafs v6 mount if its configuration has digest_validate #576
Conversation
Rafs v6 does not take chunk digest, so storage subsystem can't validate chunk data. thread 'nydus_storage_worker_0' panicked at 'BlobMetaChunk doesn't support `chunk_id()`', storage/src/meta/mod.rs:740:9 stack backtrace: thread 'nydus_storage_worker_3' panicked at 'BlobMetaChunk doesn't support `chunk_id()`', storage/src/meta/mod.rs:740:9 thread 'nydus_storage_worker_1' panicked at 'BlobMetaChunk doesn't support `chunk_id()`', storage/src/meta/mod.rs:740:9 0: std::panicking::begin_panic 1: <nydus_storage::meta::BlobMetaChunk as nydus_storage::device::BlobChunkInfo>::chunk_id note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. stack backtrace: Signed-off-by: Changewei Ge <gechangwei@bytedance.com>
|
@changweige , a new test job has been submitted. Please wait in patience. |
|
@changweige , The CI test is completed, please check result:
Congratulations, your test job passed! |
||||||||||||||||||||||||
| @@ -719,6 +719,9 @@ impl BlobMetaState { | |||
| } | |||
|
|
|||
| /// A fake `BlobChunkInfo` object created from blob metadata. | |||
| /// It represents a chunk within memory mapped chunk maps, which | |||
There was a problem hiding this comment.
Please help to add a blank line above, so the generated doc looks better.
| @@ -402,10 +402,18 @@ pub trait BlobChunkInfo: Any + Sync + Send { | |||
| } | |||
|
|
|||
| /// An enumeration to encapsulate different [BlobChunkInfo] implementations for [BlobIoDesc]. | |||
| /// This helps to feed unified IO description to storage subsystem from both rafs v6 and v5 since | |||
There was a problem hiding this comment.
Please help to add a blank line above.
| Address(u32, u32), | ||
| // TODO: `Address` is is always converted to `Base` for v6, it is a little confusing. |
There was a problem hiding this comment.
For rafs v6, the chunk info may be stored in the data blob instead of bootstrap blob. So only the blob reader from the storage subsystem could convert Address into BlobChunkInfo.
There was a problem hiding this comment.
Yes. I am thinking about removing Base(dyn BlobChunkinfo) out from BlobIoChunk. We can define BlobIoChunk as a medium type defining how this IO requesting backend storage data varies on v5 and v6(without decompression control info). Then trait BlobChunkinfo is the core structure in storage sub-system related to a single backend chunk IO. Then we don't have nested type conversion any more. storage subsystem should be capable to generate dyn BlobChunkinfo from BlobIoChunk
| } | ||
|
|
||
| fn blob_index(&self) -> u32 { | ||
| self.as_base().blob_index() | ||
| } | ||
|
|
||
| fn compress_offset(&self) -> u64 { | ||
| self.as_base().compress_offset() | ||
| match self { | ||
| Self::Address(_, _) => 0, |
There was a problem hiding this comment.
Which code could path trigger the panic?
There was a problem hiding this comment.
Aha, got it. Debug may trigger the panic.
There was a problem hiding this comment.
Could you please help to add a comments for it? Otherwise the code smells badly:)
`BlobIoDesc` implements `Debug`, but the method called from fmt() is calling its methods which panic. Signed-off-by: Changewei Ge <gechangwei@bytedance.com>
|
@changweige , your pull request has been updated. A new test job will be submitted. Please wait in patience. |
|
@changweige , your test job has passed, and no need to test again. |
| self.as_base().id() | ||
| // BlobIoChunk::Address is a medium type to pass chunk IO description | ||
| // for rafs v6. It can't implement BlobChunkInfo and calling `as_base` | ||
| // causes panic. So this is a workaround to avoid panic. |
There was a problem hiding this comment.
Wouldn't it panic too in all the other methods that also calls self.as_base()?
There was a problem hiding this comment.
Yes. other methods are also calling as_base(), however, they are not required and called by BlobIoDesc(). So they will not cause program panic now. My thought is to refactor the code a bit in the future in Rafs layer and to solve the panic problem for once and all.
Rafs v6 chunk has no digest recorded yet, so it can't perform data validation and necessary method used for validation causes panic. So we enforce rafs v6 mount the rule that no digest_validate is configured.
In addition, we have to avoid call
as_basefor rafs v6 chunk info, otherwise it panics