all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: Gabriel Goller <g.goller@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
	Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox-backup] fix #4823: datastore: ignore vanished files when walking directory
Date: Fri, 8 Sep 2023 14:56:17 +0200	[thread overview]
Message-ID: <318f786b-a715-d99b-7819-1b25899bb6ac@proxmox.com> (raw)
In-Reply-To: <4111ea6e-b478-488e-b954-d6d9f3add1d4@proxmox.com>


On 9/8/23 11:36, Thomas Lamprecht wrote:
> On 08/09/2023 09:41, Gabriel Goller wrote:
>>> that's some deep indentation level.. not a must, but maybe you find
>>> some good/simple way to refactor some of this to make it a bit less
>>> crowded here (if, then in a separate patch please)
>> I could check if `err` is an `io:Error`, thus returning early. Then calling `.unwrap()`
>> to get the actual `io::Error` later on.
> I re-checked the whole code and pushed a clean-up that reduces the
> indentation level and also code lines. Can you please re-check on that
> if to ensure I did not missed something by mistake?
>
> Oh, and while checking this out I noticed something odd, i.e., that we
> silence any error that isn't a io::Error one,  as of Walkdir::Error
> rustdocs [0] that can only happen if there's a (symlink) loop, but
> still. So I checked the history and it seems that I caused this way
> back in 2020 by the original addition of the "ignore lost+found" EPERM
> commit [1].
> But just making that an error again now might be a breaking change,
> depending on what effect symlink loops currently actually have on GC.
> Can you test this please? If it does makes GC endlessly loop then we
> can just introduce the error again, as it's now broken already anyway.
> But if walkdir breaks the loop, or the like, then we might just make
> that an log with warning level. As we do not enable WalkDir's opt-in
> follow_symlinks it might not matter after all as the Loop error
> shouldn't be triggered if symlinks aren't followed, but still good
> to test and be certain.
>
> [0]: https://rust.velas.com/walkdir/struct.Error.html#method.io_error
> [1]: c3b090ac ("backup: list images: handle walkdir error, catch "lost+found"")

We don't follow symbolic links when traversing files.
`walkdir` has a builder function `follow_links()` with which you can
select if you want to follow symbolic links or not, the default is no [0].

[0]: 
https://docs.rs/walkdir/latest/walkdir/struct.WalkDir.html#method.follow_links 

>>>> +                            // only allow to skip ext4 fsck directory, avoid GC if, for example,
>>>> +                            // a user got file permissions wrong on datastore rsync to new server
>>>> +                            if err.depth() > 1 || !path.ends_with("lost+found") {
>>>> +                                bail!("cannot continue garbage-collection safely, permission denied on: {:?}", path)
>>>> +                            }
>>>> +                        }
>>>> +                        io::ErrorKind::NotFound => {
>>>> +                            // ignore vanished file
>>> would be still good to log that here, at least at debug level
>>> if it can be noisy; but as there wasn't many that run into this
>>> in the four years of PBS existing I'd guess a always visible
>>> level is fine as long as the log message doesn't sounds scary.
>> How about a "ignoring/skipping vanished file: {path}" on the info log level?
> Yeah, something like that, but I'd use just one of "ignoring" or
> "skipping"
>
> Oh, and I would also mention in the commit message that backups that
> are just being created (where a atomic rename from e.g. a tmpdir to
> the actual one might also cause a ENOENT) are not problematic here,
> as GC is handling those explicitly anyway.
Submitted a new patch.




      reply	other threads:[~2023-09-08 12:56 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-05  9:37 Gabriel Goller
2023-09-07 15:05 ` Thomas Lamprecht
2023-09-08  7:41   ` Gabriel Goller
2023-09-08  9:36     ` Thomas Lamprecht
2023-09-08 12:56       ` Gabriel Goller [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=318f786b-a715-d99b-7819-1b25899bb6ac@proxmox.com \
    --to=g.goller@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    --cc=t.lamprecht@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal