From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pbs-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id C564B1FF172
for <inbox@lore.proxmox.com>; Wed, 16 Apr 2025 12:00:30 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 6D601325BD;
Wed, 16 Apr 2025 12:00:29 +0200 (CEST)
Date: Wed, 16 Apr 2025 12:00:22 +0200
From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= <f.gruenbichler@proxmox.com>
To: Proxmox Backup Server development discussion <pbs-devel@lists.proxmox.com>
References: <20250416091718.182071-1-c.ebner@proxmox.com>
<20250416091718.182071-2-c.ebner@proxmox.com>
In-Reply-To: <20250416091718.182071-2-c.ebner@proxmox.com>
MIME-Version: 1.0
User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid)
Message-Id: <1744796504.hxwuuqt9qk.astroid@yuna.none>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.046 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
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
Subject: Re: [pbs-devel] [PATCH v4 proxmox-backup 1/2] garbage collection:
distinguish variants for failed open index reader
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pbs-devel-bounces@lists.proxmox.com
Sender: "pbs-devel" <pbs-devel-bounces@lists.proxmox.com>
On April 16, 2025 11:17 am, Christian Ebner wrote:
> In preparation for adapting the garbage collection to be able to
> distinguish cases for the index file not being accessible because it
> is no longer present or because it is a blob and therefore should be
> ignored.
this seems very involved..
>
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> pbs-datastore/src/datastore.rs | 33 ++++++++++++++++++++++-----------
> 1 file changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index aa38e2ac1..4630b1b39 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1033,11 +1033,14 @@ impl DataStore {
>
> // Similar to open index, but ignore index files with blob or unknown archive type.
> // Further, do not fail if file vanished.
> - fn open_index_reader(&self, absolute_path: &Path) -> Result<Option<Box<dyn IndexFile>>, Error> {
> + fn open_index_reader(
> + &self,
> + absolute_path: &Path,
> + ) -> Result<IndexReaderOption<Box<dyn IndexFile>>, Error> {
> let archive_type = match ArchiveType::from_path(absolute_path) {
> - Ok(archive_type) => archive_type,
> // ignore archives with unknown archive type
> - Err(_) => return Ok(None),
> + Ok(ArchiveType::Blob) | Err(_) => return Ok(IndexReaderOption::NoneWrongType),
or filter the snapshot.files by archive type first to only call this
with index paths? see below
the unprocessed_index_list never contains blobs anyway..
> + Ok(archive_type) => archive_type,
> };
>
> if absolute_path.is_relative() {
> @@ -1047,7 +1050,9 @@ impl DataStore {
> let file = match std::fs::File::open(absolute_path) {
> Ok(file) => file,
> // ignore vanished files
> - Err(err) if err.kind() == io::ErrorKind::NotFound => return Ok(None),
> + Err(err) if err.kind() == io::ErrorKind::NotFound => {
> + return Ok(IndexReaderOption::NoneNotFound)
> + }
> Err(err) => {
> return Err(Error::from(err).context(format!("can't open file '{absolute_path:?}'")))
> }
> @@ -1057,14 +1062,14 @@ impl DataStore {
> ArchiveType::FixedIndex => {
> let reader = FixedIndexReader::new(file)
> .with_context(|| format!("can't open fixed index '{absolute_path:?}'"))?;
> - Ok(Some(Box::new(reader)))
> + Ok(IndexReaderOption::Some(Box::new(reader)))
> }
> ArchiveType::DynamicIndex => {
> let reader = DynamicIndexReader::new(file)
> .with_context(|| format!("can't open dynamic index '{absolute_path:?}'"))?;
> - Ok(Some(Box::new(reader)))
> + Ok(IndexReaderOption::Some(Box::new(reader)))
> }
> - ArchiveType::Blob => Ok(None),
> + ArchiveType::Blob => Ok(IndexReaderOption::NoneWrongType),
> }
> }
>
> @@ -1155,8 +1160,8 @@ impl DataStore {
> path.push(file);
>
> let index = match self.open_index_reader(&path)? {
> - Some(index) => index,
> - None => {
> + IndexReaderOption::Some(index) => index,
> + IndexReaderOption::NoneWrongType | IndexReaderOption::NoneNotFound => {
> unprocessed_index_list.remove(&path);
> continue;
> }
> @@ -1191,8 +1196,8 @@ impl DataStore {
> let mut strange_paths_count = unprocessed_index_list.len();
> for path in unprocessed_index_list {
> let index = match self.open_index_reader(&path)? {
> - Some(index) => index,
> - None => {
> + IndexReaderOption::Some(index) => index,
> + IndexReaderOption::NoneWrongType | IndexReaderOption::NoneNotFound => {
> // do not count vanished (pruned) backup snapshots as strange paths.
> strange_paths_count -= 1;
> continue;
> @@ -1695,3 +1700,9 @@ impl DataStore {
> *OLD_LOCKING
> }
> }
> +
> +enum IndexReaderOption<T> {
> + Some(T),
> + NoneWrongType,
> + NoneNotFound,
> +}
this is a NACK from me anyway, either this is an enum, then it should be
Index(T) / Blob / Vanished, or it is a custom error, then the Option
should be dropped and we have
Result<T, GCIndexError>
where GCIndexError has WrongType / NotFound / Other or something like
that, and the call site can choose to ignore NotFound and/or WrongType.
but I am not sure that is worth it for such an internal helper in any
case, we can just make blobs a hard error in the helper, and filter the
blobs out in the single code path where they might be included..
> --
> 2.39.5
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel