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 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.

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