[pbs-devel] [PATCH proxmox-backup 2/2] fix #5982: garbage collection: check atime updates are honored

[Christian Ebner]

On 2/18/25 14:31, Thomas Lamprecht wrote:
> Am 18.02.25 um 13:39 schrieb Christian Ebner:
>> On 2/18/25 12:53, Thomas Lamprecht wrote:
>>> +1; one (additional) option _might_  be to trigger suck a check on
>>> datastore creation, e.g. create the all-zero chunk and then do that
>>> test. As of now that probably would not win us much, but if we make
>>> the 24h-wait opt-in then users would be warned early enough, or we
>>> could even auto-set that option in such a case.
>>
>> Only checking the atime update check on datastore creation is not enough
>> I think, as the backing filesystem might get remounted with changed
>> mount parameters? Or do you mean to *also* check on datastore creation
>> already to early on detect issues? Although, in my testing even with
>> `noatime` the atime update seems to be honored by the way the garbage
>> collection performs the time updates (further details see below).
> 
> yes, I meant doing that additionally to checking on GC.
> 
>> Anyways, creating the all-zero chunk and use that for the check sounds
>> like a good optimization to me, as that allows to avoid conditional
>> checking in the phase 1 of garbage collection. However, at the cost of
>> having to make sure that it is never cleaned up by phase 2...
> 
> I saw your second reply already but even without that in mind it would
> be IMO fine to only use the all-zero chunk for the on-create check, as
> I would not see it as a big problem if it then gets pruned during
> GC, if the latter would use an actually existing chunk. But no hard
> feelings here at all either way.

I think using a 4M fixed size chunk for both cases makes it even more 
elegant, as one can then use the same logic for both cases, the check on 
datastore create and the check on garbage collection. And to be 
backwards compatible, this simply creates the zero chunk for both cases 
if it does not exist, covering existing datastores already.

>> Regarding the 24 hour waiting period, as mentioned above I noted that
>> atime updates are honored even if I set the `noatime` for an ext4 or
>> `atime=off` on zfs.
>> Seems like the utimensat() bypasses this directly, as it calls into
>> vfs_utimes() [0], which sets this to be an explicit time update,
>> followed by the notify_change() [1], which then calls the setattr() of
>> the corresponding filesystem [2] via the given inode.
>> This seems to bypass the atime_needs_update() [3], only called by
>> touch_atime(). The atime_needs_update() also checks the
>> relatime_needs_update() [4].
>>
>> Although not conclusive (yet).
>>
> 
> Yeah, that would support making this opt-in. FWIW, we could maybe "sell"
> this as sort of feature by not just transforming it into a boolean
> "24h-wait period <true|false>" option but rather a more generic
> "wait-period <X hours>" option that defaults to 0 hours (or maybe a
> few minutes if we want to support minute granularity). Not sure if there
> are enough (real world) use cases to warrant this, so mostly mentioned
> for the sake of completeness.

Yes, that sounds good to me!