public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Christian Ebner <c.ebner@proxmox.com>,
	Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox-backup] fix #5967: datastore lookup: check for disappeared datastores
Date: Wed, 11 Dec 2024 10:32:40 +0100	[thread overview]
Message-ID: <1733909481.8vepx620lu.astroid@yuna.none> (raw)
In-Reply-To: <0d0fc0cf-dfaf-4137-9fa4-c6ce28855a4c@proxmox.com>

On December 6, 2024 3:03 pm, Christian Ebner wrote:
> Had a look at this, one comment inline.
> 
> On 12/5/24 10:12, Fabian Grünbichler wrote:
>> some code paths will only read the datastore base - if the datastore
>> disappeared (e.g., a lazy unmount, or some network storage disappearing without
>> blocking) then we should at least detect the issue at lookup time before
>> re-using an already opened datastore.
>> 
>> any code path that tried to access the chunks themselves would already have
>> failed without this additional safeguard, but in particular queries listing
>> namespaces or groups in the root namespace don't, which might cause a remote
>> client such as pull sync to mistakenly think that the datastore is empty.
>> 
>> limit the additional check to `Read` or `Write` lookups, since any code path
>> doing those will most likely want to do I/O on the datastore afterwards, as
>> opposed to a `Lookup` lookup or one without an operation, which might not
>> require the datastore to be fully functional.
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>> there is of course still a gap beween the lookup and the readdir after it.. we
>> could track down all API endpoints that access the datastore but don't touch
>> the chunk dir, and before returning, check a second time that the datastore
>> hasn't vanished, but I am not sure that is worth the effort?
>> 
>>   pbs-datastore/src/chunk_store.rs | 2 +-
>>   pbs-datastore/src/datastore.rs   | 6 ++++++
>>   2 files changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
>> index 29d5874a1..189c4ebcd 100644
>> --- a/pbs-datastore/src/chunk_store.rs
>> +++ b/pbs-datastore/src/chunk_store.rs
>> @@ -158,7 +158,7 @@ impl ChunkStore {
>>   
>>       /// Check if the chunkstore path is absolute and that we can
>>       /// access it. Returns the absolute '.chunks' path on success.
>> -    fn chunk_dir_accessible(base: &Path) -> Result<PathBuf, Error> {
>> +    pub(crate) fn chunk_dir_accessible(base: &Path) -> Result<PathBuf, Error> {
> 
> `chunk_dir_accessible` tries to get the metadata for the chunk directory 
> in order to check if the chunk store is available.
> 
> So this could be used to not only check if there is a chunk directory, 
> but rather that the chunk directory has the same device ID and i-node 
> number. For that check, one would only need to get and store the chunk 
> directory metadata on `ChunkStore::open`. That would be a rather non 
> invasive check to also detect if the datastore has been over-mounted, 
> not just if it vanished.
> 
> What do you think? Is this feasible? Not fully sure about the 
> implications for removable datastores, but that should be fine as well, 
> as the device id and i-node should not change as long as the datastore 
> remains online.

detecting a changed FS and reloading the chunk store might be nice as
well, yes. would need to carefully check that this doesn't affect either
removable datastores or the ProcessLocker semantics..

> 
>>           if !base.is_absolute() {
>>               bail!("expected absolute path - got {:?}", base);
>>           }
>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
>> index 0801b4bf6..bb9b4471f 100644
>> --- a/pbs-datastore/src/datastore.rs
>> +++ b/pbs-datastore/src/datastore.rs
>> @@ -232,6 +232,12 @@ impl DataStore {
>>           // reuse chunk store so that we keep using the same process locker instance!
>>           let chunk_store = if let Some(datastore) = &entry {
>>               let last_digest = datastore.last_digest.as_ref();
>> +            match operation {
>> +                Some(Operation::Read) | Some(Operation::Write) => {
>> +                    ChunkStore::chunk_dir_accessible(&datastore.chunk_store.base_path())?;
>> +                }
>> +                Some(Operation::Lookup) | None => {}
>> +            }
>>               if let Some(true) = last_digest.map(|last_digest| last_digest == &digest) {
>>                   if let Some(operation) = operation {
>>                       update_active_operations(name, operation, 1)?;
> 
> 


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

      reply	other threads:[~2024-12-11  9:32 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-12-05  9:12 Fabian Grünbichler
2024-12-06 14:03 ` Christian Ebner
2024-12-11  9:32   ` Fabian Grünbichler [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1733909481.8vepx620lu.astroid@yuna.none \
    --to=f.gruenbichler@proxmox.com \
    --cc=c.ebner@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal