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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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. 

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. 

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.

> +                    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 {} - {}",