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

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

On 10/30/25 12:01 PM, Fabian Grünbichler wrote:
> 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?

Yeah, was pondering for a bit about that as well, but then did not 
mention it as like this it limits the creation of the index file to be 
within a valid datastore location. Which should protect against ever 
creating strange paths for index files.


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