From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id DDE0470B85 for ; Mon, 9 May 2022 13:34:20 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D34C72C336 for ; Mon, 9 May 2022 13:34:20 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id D3B922C304 for ; Mon, 9 May 2022 13:34:18 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id A9EEB4323E for ; Mon, 9 May 2022 13:34:18 +0200 (CEST) Date: Mon, 09 May 2022 13:34:11 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20220509104030.1943794-1-d.csapak@proxmox.com> In-Reply-To: <20220509104030.1943794-1-d.csapak@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1652095455.qd8d3f2zyo.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.173 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [PATCH proxmox-backup] chunk_store: insert_chunk: write chunk again if sizes don't match X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 May 2022 11:34:20 -0000 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. >=20 > This is currently possible if PBS crashes, but the rename of the chunk > was flushed to disk, when the actual data was not. >=20 > Suggested-by: Fabian Gr=C3=BCnbichler > Signed-off-by: Dominik Csapak > --- > pbs-datastore/src/chunk_store.rs | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) >=20 > diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_s= tore.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 { > =20 > let lock =3D self.mutex.lock(); > =20 > + let raw_data =3D chunk.raw_data(); > + let encoded_size =3D raw_data.len() as u64; > + > if let Ok(metadata) =3D 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 =3D metadata.len(); > + if encoded_size =3D=3D 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 =3D> overwrite with new content B chunk file exists but length doesn't match =3D> bail out C chunk file exists and length matches =3D> treat as duplicate A is to handle the 'crash/.., rename went through but writing chunk data=20 didn't' case - we know an empty chunk file is never valid, so we can=20 treat it as garbage and overwrite. C is like before - we assume a file that is there and has the correct=20 length can be treated as duplicate. but B is IMHO worthwhile to differentiate from A - we don't have a way=20 where this should be possible unless - the file was truncated somehow, which seems unlikely unless done=20 manually (or maybe there is a way this could happen?) - the uploaded chunk got logically truncated/.. somehow (bug=20 client-side, on-wire truncation would obviously break in other=20 fashions earlier) - someone is actively trying to upload a broken (encrypted) chunk to=20 overwrite an existing valid one - someone tries to fix the previous two instances (current on-disk chunk=20 was maliciously/buggily placed there) in any case we can't tell for sure whether the incoming or the on-disk=20 copy is "more correct" unless we do a full verify (only possible for=20 plain-chunks, encrypted are out of luck since the CRC is meaningless to=20 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=20 user the chunk store is corrupted", because overwriting might break=20 backups if the current client is bad and the on-disk chunk is correct,=20 and not overwriting has the same problem in the inverse scenario.=20 alternatively we could of course say existing on-disk chunk is corrupt,=20 mark accordingly, and treat the incoming copy as correct since it's more=20 recent (allows the backup to proceed according to the client's / user's=20 wishes, even though it might be technically wrong). > } > =20 > let mut tmp_path =3D chunk_path.clone(); > @@ -483,9 +495,6 @@ impl ChunkStore { > ) > })?; > =20 > - let raw_data =3D chunk.raw_data(); > - let encoded_size =3D raw_data.len() as u64; > - > file.write_all(raw_data).map_err(|err| { > format_err!( > "writing temporary chunk on store '{}' failed for {} - {= }", > --=20 > 2.30.2 >=20 >=20 >=20 > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel >=20