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

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

[Dominik Csapak]

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.

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

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

[Fabian Grünbichler]

On May 9, 2022 12:40 pm, 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.
> 
> 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 {}",
> +                    digest_str,
> +                    encoded_size,
> +                    new_len
> +                );
> +            }

I think I'd even go one step further here:

A chunk file exists but is empty => overwrite with new content
B chunk file exists but length doesn't match => bail out
C chunk file exists and length matches => treat as duplicate

A is to handle the 'crash/.., rename went through but writing chunk data 
didn't' case - we know an empty chunk file is never valid, so we can 
treat it as garbage and overwrite.

C is like before - we assume a file that is there and has the correct 
length can be treated as duplicate.

but B is IMHO worthwhile to differentiate from A - we don't have a way 
where this should be possible unless
- the file was truncated somehow, which seems unlikely unless done 
  manually (or maybe there is a way this could happen?)
- the uploaded chunk got logically truncated/.. somehow (bug 
  client-side, on-wire truncation would obviously break in other 
  fashions earlier)
- someone is actively trying to upload a broken (encrypted) chunk to 
  overwrite an existing valid one
- someone tries to fix the previous two instances (current on-disk chunk 
  was maliciously/buggily placed there)

in any case we can't tell for sure whether the incoming or the on-disk 
copy is "more correct" unless we do a full verify (only possible for 
plain-chunks, encrypted are out of luck since the CRC is meaningless to 
verify actual contents in cases 3/4).

so it seems to me it's safer to say "let's abort the backup and tell the 
user the chunk store is corrupted", because overwriting might break 
backups if the current client is bad and the on-disk chunk is correct, 
and not overwriting has the same problem in the inverse scenario. 
alternatively we could of course say existing on-disk chunk is corrupt, 
mark accordingly, and treat the incoming copy as correct since it's more 
recent (allows the backup to proceed according to the client's / user's 
wishes, even though it might be technically wrong).

>          }
>  
>          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 {} - {}",
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 

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

[Thomas Lamprecht]

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

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

[Dominik Csapak]

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