Re: [pbs-devel] [PATCH proxmox-backup] chunk_store: insert_chunk: write chunk again if sizes don't match

[Dominik Csapak <d.csapak@proxmox.com>]

On 5/9/22 13:34, Thomas Lamprecht wrote:
> On 09/05/2022 12:40, Dominik Csapak wrote:
>> if the on-disk size of a chunk is not correct, write it again when
>> inserting and log a warning.
>>
>> This is currently possible if PBS crashes, but the rename of the chunk
>> was flushed to disk, when the actual data was not.
> 
> could be also interesting to note here that we basically got all data
> required to do that already anyway.

what exactly do you mean here? just adding

'since we already have the complete chunk data, we are able to overwrite it'
(or similar?)

> 
> And I'd think that a verify would catch this too and rename it to .bad, albeit
> that can naturally be to late if one is unlucky, so still good to do.

yes a verify will trigger that, but as you said, that can be too late ;)

also any input on @fabians suggestion to bail out when the old_size != 0 but
!= new_size?

> 
> small nit inline (can be probably just fixed up on apply)
> 
>>
>> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   pbs-datastore/src/chunk_store.rs | 23 ++++++++++++++++-------
>>   1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
>> index 8d7df513..93f56e8b 100644
>> --- a/pbs-datastore/src/chunk_store.rs
>> +++ b/pbs-datastore/src/chunk_store.rs
>> @@ -458,17 +458,29 @@ impl ChunkStore {
>>   
>>           let lock = self.mutex.lock();
>>   
>> +        let raw_data = chunk.raw_data();
>> +        let encoded_size = raw_data.len() as u64
>> +
>>           if let Ok(metadata) = std::fs::metadata(&chunk_path) {
>> -            if metadata.is_file() {
>> -                self.touch_chunk(digest)?;
>> -                return Ok((true, metadata.len()));
>> -            } else {
>> +            if !metadata.is_file() {
>>                   bail!(
>>                       "Got unexpected file type on store '{}' for chunk {}",
>>                       self.name,
>>                       digest_str
>>                   );
>>               }
>> +            let new_len = metadata.len();
>> +            if encoded_size == new_len {
>> +                self.touch_chunk(digest)?;
>> +                return Ok((true, new_len));
>> +            } else {
>> +                log::warn!(
>> +                    "chunk size mismatch on insert for {}: old {} - new {}",
> 
> fyi: you can now use variable names directly in format strings:
> 
> "chunk size mismatch on insert for {digest_str}: old {encoded_size} - new {new_len}",
> 
> nit: why is one named a "size" and one a "len", if they're the same thing it'd be
> nice to have it consistent.
> 

argh... 'new_len' is actually the old one. so it'll be
'encoded_size' and 'old_size'..

thanks for making me look again ^^

>> +                    digest_str,
>> +                    encoded_size,
>> +                    new_len
>> +                );
>> +            }
>>           }
>>   
>>           let mut tmp_path = chunk_path.clone();
>> @@ -483,9 +495,6 @@ impl ChunkStore {
>>               )
>>           })?;
>>   
>> -        let raw_data = chunk.raw_data();
>> -        let encoded_size = raw_data.len() as u64;
>> -
>>           file.write_all(raw_data).map_err(|err| {
>>               format_err!(
>>                   "writing temporary chunk on store '{}' failed for {} - {}",
>