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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On October 30, 2025 11:46 am, Christian Ebner wrote:
> 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>

I think we could consider moving the path handling outside of the writer
to get rid of all the chunk store references here, if we wanted to?


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