Re: [pbs-devel] [RFC proxmox-backup] index writers: remove dead code

[Christian Ebner <c.ebner@proxmox.com>]

On 10/30/25 11:25 AM, Fabian Grünbichler wrote:
> On October 29, 2025 9:04 am, Christian Ebner wrote:
>> Thanks for the cleanup, two questions inline.
>>
>> On 10/27/25 2:55 PM, Fabian Grünbichler wrote:
>>> the current code base doesn't use the index writers for inserting chunks into a
>>> chunk store, and it probably shouldn't given the intricate requirements around
>>> interacting with S3.
>>>
>>> all of thise code seems to be dead code, remove it to make reasoning about
>>> chunk insertion code paths easier.
>>>
>>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>>> ---
>>> stumbled upon this while thinking about S3 synchronization issues..
>>> AFAICT this is leftover from the very initial phase of PBS development
>>>
>>>    pbs-datastore/src/dynamic_index.rs | 142 -----------------------------
>>>    pbs-datastore/src/fixed_index.rs   |  38 --------
>>>    2 files changed, 180 deletions(-)
>>>
>>> diff --git a/pbs-datastore/src/dynamic_index.rs b/pbs-datastore/src/dynamic_index.rs
>>> index ff6c36782..624df0119 100644
>>> --- a/pbs-datastore/src/dynamic_index.rs
>>> +++ b/pbs-datastore/src/dynamic_index.rs
>>> @@ -16,13 +16,10 @@ use pxar::accessor::{MaybeReady, ReadAt, ReadAtOperation};
>>>    
>>>    use pbs_tools::lru_cache::LruCache;
>>>    
>>> -use crate::chunk_stat::ChunkStat;
>>>    use crate::chunk_store::ChunkStore;
>>> -use crate::data_blob::{DataBlob, DataChunkBuilder};
>>>    use crate::file_formats;
>>>    use crate::index::{ChunkReadInfo, IndexFile};
>>>    use crate::read_chunk::ReadChunk;
>>> -use crate::{Chunker, ChunkerImpl};
>>>    
>>>    /// Header format definition for dynamic index files (`.dixd`)
>>>    #[repr(C)]
>>> @@ -275,7 +272,6 @@ impl IndexFile for DynamicIndexReader {
>>>    
>>>    /// Create dynamic index files (`.dixd`)
>>>    pub struct DynamicIndexWriter {
>>> -    store: Arc<ChunkStore>,
>>>        writer: BufWriter<File>,
>>>        closed: bool,
>>>        filename: PathBuf,
>>> @@ -321,7 +317,6 @@ impl DynamicIndexWriter {
>>>            let csum = Some(openssl::sha::Sha256::new());
>>>    
>>>            Ok(Self {
>>> -            store,
>>
>> question: not sure if it is fine to drop the chunk store here? The chunk
>> store holds the process locker, which should outlive the writer ...
> 
> technically true, but
> 
> the writer can already outlive the lock, this is only protected by how
> we are calling things, no matter whether a reference to the chunk store
> is stored inside the writer or not. the actual lock guard (which
> releases the lock when dropped) is what counts, and that is stored in
> the backup writer env, not in the index writer here.. (the env also
> contains a reference to the chunk store via the reference to the
> datastore).
> 
> or am I missing something?

No, this was exactly my question here... Thanks for clarification, with 
that information this now looks good to me.

Consider:

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel