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 [IPv6:2a01:7e0:0:424::9])
	by lore.proxmox.com (Postfix) with ESMTPS id 1C5E420EC88
	for <inbox@lore.proxmox.com>; Thu, 25 Apr 2024 10:42:20 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
	by firstgate.proxmox.com (Proxmox) with ESMTP id AFC261D5CD;
	Thu, 25 Apr 2024 10:42:25 +0200 (CEST)
Date: Thu, 25 Apr 2024 10:41:46 +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: <20240425083211.60986-1-h.laimer@proxmox.com>
In-Reply-To: <20240425083211.60986-1-h.laimer@proxmox.com>
MIME-Version: 1.0
User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid)
Message-Id: <1714034383.t1rdpl0dov.astroid@yuna.none>
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.055 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] datastore: fix problem with operations
 counting
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 25, 2024 10:32 am, Hannes Laimer wrote:
> ... if `.chunks/` is not available(deleted/moved) ChunkStore::open
> fails, but that would happen after updating the active operations on the
> datastore, so no reference that could be dropped is returned. Leading to
> the operations counter to always increase. This only updates the counter
> when a reference is returned, not before.
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  pbs-datastore/src/datastore.rs | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index f95da761..2f8952ae 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -158,10 +158,6 @@ impl DataStore {
>              }
>          }
>  
> -        if let Some(operation) = operation {
> -            update_active_operations(name, operation, 1)?;
> -        }
> -
>          // Our operation is registered, unlock the config.
>          drop(config_lock);

this comment here, and the bigger one just out of context at the start
of this fn:

        // Avoid TOCTOU between checking maintenance mode and updating active operation counter, as
        // we use it to decide whether it is okay to delete the datastore.
        let config_lock = pbs_config::datastore::lock_config()?;

make me think that this commit at least lacks a rationale why this
change is safe, but possibly means that we would need to hold the config
lock longer here?

>  
> @@ -172,6 +168,9 @@ impl DataStore {
>          let chunk_store = if let Some(datastore) = &entry {
>              let last_digest = datastore.last_digest.as_ref();
>              if let Some(true) = last_digest.map(|last_digest| last_digest == &digest) {
> +                if let Some(operation) = operation {
> +                    update_active_operations(name, operation, 1)?;
> +                }
>                  return Ok(Arc::new(Self {
>                      inner: Arc::clone(datastore),
>                      operation,
> @@ -195,6 +194,10 @@ impl DataStore {
>          let datastore = Arc::new(datastore);
>          datastore_cache.insert(name.to_string(), datastore.clone());
>  
> +        if let Some(operation) = operation {
> +            update_active_operations(name, operation, 1)?;
> +        }
> +
>          Ok(Arc::new(Self {
>              inner: datastore,
>              operation,
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> 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