Re: [pbs-devel] [PATCH v8 proxmox proxmox-backup 0/9] fix #5982: check atime update is honored

["Max Carrara" <m.carrara@proxmox.com>]

On Fri Apr 4, 2025 at 5:41 PM CEST, Max Carrara wrote:
> On Fri Mar 21, 2025 at 9:02 AM CET, Christian Ebner wrote:
> > These patches add a check to phase 1 of garbage collection and datastore
> > creation in order to detect when the filesystem backing the chunk store
> > does not honor atime updates. This avoids possible data loss for situations
> > where garbage collection could otherwise delete chunks still referenced by
> > a backup snaphost's index file.
> >
> > The check is performed on a fixed size 4 MiB unencrypted and compressed
> > chunk of all-zeros, inserted if not present yet.
> > The Linux kernel timestamp granularity is taken into account by sleeping
> > for 1 second to avoid discarded atime update attempts by utimensat calls.
> > The test is enabled by default, but an opt-out option can be set via the
> > datastore tuning parameters for backwards compatibility.
> >
> > Further, add a datastore tuning parameter to reduce the wait period for
> > chunk removal in phase 2 of garbage collection.
>
> Gave this a quite thorough look and did a bunch of testing.

One thing I forgot to mention: The patches for `proxmox-backup` do
apply, but you need `git am -3`.

>
> To address the most important things first...
>
> Testing
> =======
>
> tl;dr The check works but there's a permission issue -- the block you're
> using for the check isn't owned by the `backup` user when created.
>
> See my analysis in my response to patch 04 for *all* of the details.
> There's some additional info also.
>
> There's also a minor UI thing; see my response to patch 05.
>
> After changing the ownership of the block used for the check, I did some
> more testing on ext4; running GC, making backups, etc.
>
> - The atime check seems to *run* for ext4 mounted with `rw,relatime` and
>   also ext4 with `rw,noatime,nodiratime`. Turns out that
>   `noatime,nodiratime` just means that the atime isn't updated
>   automatically when accessing a file, but when you e.g. `touch -a` some
>   file, the atime can still be updated. You just gotta do it by hand.
>
> - I'm assuming the same applies for other filesystems; I did a quick
>   test with ZFS (`zfs set atime=off $POOLNAME`) and didn't notice
>   anything.
>
> - I did some research and couldn't find a filesystem that doesn't
>   support changing the atime at all (or is broken in some way), so if
>   you got any pointers for that, I'll gladly do more testing.
>
>
> Code Review
> ===========
>
> This one was otherwise quite quick; as always, the code is easy to
> follow and doesn't try anything funky. Neither `cargo fmt --all` and
> `cargo clippy --all-features --all-targets --workspace` showed anything
> wrong with your series in either `proxmox/` or `proxmox-backup/`.
>
> Also, you get bonus points for including a docstring that explains what
> `check_fs_atime_updates` actually does. That little description goes a
> long way in terms of maintainability.
>
> So, in summary, apart from the two things I encountered, this LGTM. 🦀
>
> Please ping me once you refresh this series; I'll happily test it all
> again; should be much faster now that I've got it all set up, too.
> Then I'll also add my R-b & T-b trailers.
>
> >
> > Most notable changes sice version 7:
> > - rebased on current master
> > - removed unused import
> >
> > Most notable changes sice version 6 (thanks Wolfgang for feedback):
> > - render option as error instead of converting to unix time
> > - propagate anyhow error context up to caller logging or printing it
> >
> > Most notable changes sice version 5 (thanks Thomas for feedback):
> > - Move atime update check on datastore creation from chunk store to data
> >   store creation.
> > - Check atime updates also when reusing an existing datastore
> > - Shorten field labels and add more info in qtip.
> > - Add additional context to errors.
> > - Improve readability of constants by writing them as their factors.
> >
> > Note: I the end adapting the cutoff to HumanTime and render it
> > accordingly in the UI turned out to be to much of a hussle as expected,
> > therefore sticked to the integer schema.
> >
> > Most notable changes sice version 4 (thanks Fabian for feedback):
> > - Decouple gc-atime-safety-check from gc-atime-cutoff
> > - Improve logging and error handling
> > - fixed typo in docs (thanks Maximilliano for noticing).
> >
> > Most notable changes sice version 3 (thanks Fabian for feedback):
> > - Drop check for relatime like behaviour, as this is not supported and
> >   does not show up in any of the tests performed on btrfs, cephfs, ext4,
> >   NFS3, NFS4, ntfs, SMB3_11, xfs or ZFS.
> > - Additionally check chunk inode to detect possible but very unlikely
> >   file changes, perform check once again in that case.
> > - Move atime cutoff selection and min_atime calculation to the same
> >   location, as they are logically related.
> >
> > Most notable changes sice version 2 (thanks Fabian and Thomas for
> > comments and suggestions):
> > - Take into account Linux timestamp granularity, do not set timestamp
> >   to the past, as that introduces other error paths such as lack of
> >   permissions or fs limitations.
> > - Check relatime behavior, if atime behaviour is not honored. Fallback
> >   to original cutoff in that case.
> > - Adapt tuning parameter names.
> >
> > Most notable changes sice version 1 (thanks Fabian and Thomas for
> > comments and suggestions):
> > - Optimize check by using the all zero chunk
> > - Enable the check by default and fail GC job if not honored, but allow
> >   to opt-out
> > - Add GC wait period tuning option
> >
> > Link to the issue in the bugtracker:
> > https://bugzilla.proxmox.com/show_bug.cgi?id=5982
> >
> > proxmox:
> >
> > Christian Ebner (2):
> >   pbs api types: add garbage collection atime safety check flag
> >   pbs api types: add option to set GC chunk cleanup atime cutoff
> >
> >  pbs-api-types/src/datastore.rs | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >
> > proxmox-backup:
> >
> > Christian Ebner (7):
> >   garbage collection: format error including anyhow error context
> >   fix #5982: garbage collection: check atime updates are honored
> >   ui: expose GC atime safety check flag in datastore tuning options
> >   docs: mention GC atime update check for tuning options
> >   datastore: use custom GC atime cutoff if set
> >   ui: expose GC atime cutoff in datastore tuning option
> >   docs: mention gc-atime-cutoff as datastore tuning option
> >
> >  docs/storage.rst                 | 16 ++++++-
> >  pbs-datastore/src/chunk_store.rs | 82 +++++++++++++++++++++++++++-----
> >  pbs-datastore/src/datastore.rs   | 41 +++++++++++++++-
> >  src/api2/admin/datastore.rs      |  6 +--
> >  src/api2/config/datastore.rs     | 28 ++++++++---
> >  src/bin/proxmox-backup-proxy.rs  |  2 +-
> >  www/Utils.js                     |  9 ++++
> >  www/datastore/OptionView.js      | 23 +++++++++
> >  8 files changed, 179 insertions(+), 28 deletions(-)
>
>
>
> _______________________________________________
> 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