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

[Fabian Grünbichler]

On March 6, 2025 12:30 pm, Christian Ebner wrote:
> On 3/6/25 12:02, Fabian Grünbichler wrote:
>> On March 5, 2025 4:14 pm, Christian Ebner wrote:
>>> +            pre_existing,
>>> +        ) {
>>> +            (true, false) => info!("Access time safety check successful (inserted chunk for test)."),
>>> +            (true, true) => {
>>> +                info!("Access time safety check successful (used pre-existing chunk for test).")
>>> +            }
>>> +            (false, false) => bail!(
>>> +                "Access time safety check failed (inserted chunk for test), is atime support \
>>> +                enabled on datastore backing filesystem?"
>>> +            ),
>>> +            (false, true) => bail!(
>>> +                "Access time safety check failed (used pre-existing chunk for test), is atime \
>>> +                support enabled on datastore backing filesystem?"
>>> +            ),
>>> +        }
>> 
>> this is a bit excessive while at the same time not including the
>> interesting bits..
>> 
>> how about the following:
>> 
>> if metadata_before.accessed()? >= metadata_now.accessed()? {
>>      let chunk_info_str = if pre_existing { "pre-existing" } else { "newly inserted" };
>>      log::warn!("Chunk metadata was not correctly updated during atime test:");
>>      info!("Metadata before update: {metadata_before:?}");
>>      info!("Metadata after update: {metadata_now:?}");
>>      bail!("atime safety check using {chunk_info_str} chunk failed, aborting GC!");
>> }
>> 
>> info!("atime safety check successful, proceeding with GC.");
>> 
>> 
>> would look like this in the task log:
>> 
>> Chunk metadata was not correctly updated during atime test:
>> Metadata before update: Metadata { file_type: FileType { is_file: true, is_dir: false, is_symlink: false, .. }, permissions: Permissions(FilePermissions { mode: 0o100500 (-r-x------) }), len: 158, modified: SystemTime { tv_sec: 1673516596, tv_nsec: 65483144 }, accessed: SystemTime { tv_sec: 1741256689, tv_nsec: 18294604 }, created: SystemTime { tv_sec: 1673516596, tv_nsec: 65483144 }, .. }
>> Metadata after update: Metadata { file_type: FileType { is_file: true, is_dir: false, is_symlink: false, .. }, permissions: Permissions(FilePermissions { mode: 0o100500 (-r-x------) }), len: 158, modified: SystemTime { tv_sec: 1673516596, tv_nsec: 65483144 }, accessed: SystemTime { tv_sec: 1741256689, tv_nsec: 18294604 }, created: SystemTime { tv_sec: 1673516596, tv_nsec: 65483144 }, .. }
>> TASK ERROR: atime safety check using pre-existing chunk failed, aborting GC!
>> 
>> alternatively we could of course do our own pretty printing, or add `#`
>> to the format string to get:
>> 
>> Metadata before update:
>> Metadata {
>>      file_type: FileType {
>>          is_file: true,
>>          is_dir: false,
>>          is_symlink: false,
>>          ..
>>      },
>>      permissions: Permissions(
>>          FilePermissions {
>>              mode: 0o100500 (-r-x------),
>>          },
>>      ),
>>      len: 158,
>>      modified: SystemTime {
>>          tv_sec: 1673516596,
>>          tv_nsec: 65483144,
>>      },
>>      accessed: SystemTime {
>>          tv_sec: 1741256877,
>>          tv_nsec: 225020600,
>>      },
>>      created: SystemTime {
>>          tv_sec: 1673516596,
>>          tv_nsec: 65483144,
>>      },
>>      ..
>> }
>> 
>> I think it is important to get the logging right here because if
>> somebody runs into a failing check, they should report the relevant data
>> to us without the need to jump through hoops.
> 
> Agreed on this and above comments regarding error context, would however 
> opt for a custom output, e.g. something along the line of what is used 
> to output pxar entries via `format_single_line_entry`...
> 
> Do we really need information like file type, size and permissions here? 
> That should already be covered by the chunk insert above?

that's a bit hard to tell a priori ;)

file type is probably not *that* relevant since we assume it is a
regular file, the permissions might very well be relevant, but probably
the actual attributes (like append-only or immutable) would be even more
interesting, in case a filesystem is broken and ignores the update
instead of failing in such cases.

so yeah, maybe just pretty printing the timestamps already is enough
here, it might need to be followed-up with lsattr and questions about
their storage setup in many cases anyway.