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 53FC461041 for ; Fri, 4 Sep 2020 14:20:50 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 41C3A208DF for ; Fri, 4 Sep 2020 14:20:20 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (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 02994208D2 for ; Fri, 4 Sep 2020 14:20:19 +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 C7FF844A1A for ; Fri, 4 Sep 2020 14:20:18 +0200 (CEST) To: Proxmox Backup Server development discussion , Stefan Reiter References: <20200903141705.6344-1-s.reiter@proxmox.com> <20200903141705.6344-4-s.reiter@proxmox.com> From: Thomas Lamprecht Message-ID: <2708e6fa-c5b5-aa6c-e1a4-0a210a51f5b2@proxmox.com> Date: Fri, 4 Sep 2020 14:20:17 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:81.0) Gecko/20100101 Thunderbird/81.0 MIME-Version: 1.0 In-Reply-To: <20200903141705.6344-4-s.reiter@proxmox.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 1.072 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -2.403 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup 3/4] gc: remove .bad files on garbage collect 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: Fri, 04 Sep 2020 12:20:50 -0000 On 03.09.20 16:17, Stefan Reiter wrote: > diff --git a/src/backup/chunk_store.rs b/src/backup/chunk_store.rs > index e1da5a8a..5c2fb29d 100644 > --- a/src/backup/chunk_store.rs > +++ b/src/backup/chunk_store.rs > @@ -187,7 +187,7 @@ impl ChunkStore { > pub fn get_chunk_iterator( > &self, > ) -> Result< > - impl Iterator= , usize)> + std::iter::FusedIterator, > + impl Iterator= , usize, bool)> + std::iter::FusedIterator, > Error > > { > use nix::dir::Dir; > @@ -218,20 +218,26 @@ impl ChunkStore { > match inner.next() { > Some(Ok(entry)) =3D> { > // skip files if they're not a hash > - let bytes =3D entry.file_name().to_bytes()= ; > - if bytes.len() !=3D 64 { > - continue; > + let hash =3D { > + let bytes =3D entry.file_name().to_byt= es(); > + bytes.len() =3D=3D 64 && bytes.iter().= all(u8::is_ascii_hexdigit) why change the check, we do not care for deleting a non hex named file, this is a private controlled directory after all. I'd avoid doing extra checks, their costs, even if small, are amplified a lot. > + }; > + > + if hash { > + return Some((Ok(entry), percentage, fa= lse)); > + } else if let Ok(name) =3D entry.file_name= ().to_str() { > + if name.ends_with(".bad") { > + return Some((Ok(entry), percentage= , true)); > + } we only want to remove bad chunks if a good exists again, else we may loose information - e.g., if the detected corruption was caused by bad memory (RAM) not the chunk itself. > [...] > @@ -321,7 +327,20 @@ impl ChunkStore { > let lock =3D self.mutex.lock(); > =20 > if let Ok(stat) =3D fstatat(dirfd, filename, nix::fcntl::A= tFlags::AT_SYMLINK_NOFOLLOW) { > - if stat.st_atime < min_atime { > + if bad { > + let res =3D unsafe { libc::unlinkat(dirfd, filenam= e.as_ptr(), 0) }; don't we have something for this already? Else I'd move this out to a clo= sure or local function > + if res !=3D 0 { > + let err =3D nix::Error::last(); > + worker.warn(format!( > + "unlink .bad file {:?} failed on store '{}= ' - {}", "unlinking corrupt chunk ... > + filename, > + self.name, > + err, > + )); > + } > + status.removed_bad +=3D 1; > + status.removed_bytes +=3D stat.st_size as u64; you count this up even if the unlinkat failed? > + } else if stat.st_atime < min_atime { > //let age =3D now - stat.st_atime; > //println!("UNLINK {} {:?}", age/(3600*24), filen= ame); > let res =3D unsafe { libc::unlinkat(dirfd, filenam= e.as_ptr(), 0) };