Page MenuHomeFreeBSD

tmpfs: restore atime updates for reads from page cache.
ClosedPublic

Authored by kib on Sep 16 2020, 12:54 PM.
Tags
None
Referenced Files
F103058247: D26451.diff
Wed, Nov 20, 9:28 AM
F102991961: D26451.id77094.diff
Tue, Nov 19, 2:01 PM
Unknown Object (File)
Mon, Oct 28, 6:38 AM
Unknown Object (File)
Oct 20 2024, 1:15 PM
Unknown Object (File)
Oct 19 2024, 9:54 AM
Unknown Object (File)
Oct 16 2024, 10:44 AM
Unknown Object (File)
Oct 16 2024, 9:14 AM
Unknown Object (File)
Oct 8 2024, 10:02 AM
Subscribers

Details

Summary

Split TMPFS_NODE_ACCCESSED bit into dedicated byte that can be updated atomically without locks or (locked) atomics.

tn_update_getattr() change also contains unrelated bug fix.

Reported by: lwhsu
PR: 249362

(Not tested).

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 33598

Event Timeline

kib requested review of this revision.Sep 16 2020, 12:54 PM

The tmpfs_set_accessed call in pgread has to move up prior to vfs_smr_exit. I can't make an inline comment for some reason.

On a more general note perhaps this can finally move to just getnanotime() on access and updating the stored value only if it goes forward. While this would incur some loss of concurrency it may be small enough to be more than tolerable, all while finally providing reasonable timestamp accuracy. Note as it is atime is updated who-knows-when. Same goes for other counters.

In D26451#588463, @mjg wrote:

The tmpfs_set_accessed call in pgread has to move up prior to vfs_smr_exit. I can't make an inline comment for some reason.

Done.

On a more general note perhaps this can finally move to just getnanotime() on access and updating the stored value only if it goes forward. While this would incur some loss of concurrency it may be small enough to be more than tolerable, all while finally providing reasonable timestamp accuracy. Note as it is atime is updated who-knows-when. Same goes for other counters.

At least we do not write torn values to Xtime fields. I do not want to take node lock in tmpfs_pgcache_read(), after all efforts I put to remove object and node referencing.

Move write to tn_accesses before vfs_smr_exit().

markj added inline comments.
sys/fs/tmpfs/tmpfs.h
175–179

I would add a sentence explaining why the field is split.

sys/fs/tmpfs/tmpfs_subr.c
1870

atomic_store_8()?

This revision is now accepted and ready to land.Sep 16 2020, 4:19 PM
In D26451#588469, @kib wrote:
In D26451#588463, @mjg wrote:

On a more general note perhaps this can finally move to just getnanotime() on access and updating the stored value only if it goes forward. While this would incur some loss of concurrency it may be small enough to be more than tolerable, all while finally providing reasonable timestamp accuracy. Note as it is atime is updated who-knows-when. Same goes for other counters.

At least we do not write torn values to Xtime fields. I do not want to take node lock in tmpfs_pgcache_read(), after all efforts I put to remove object and node referencing.

But we don't have to have torn writes or any locking. The most aggressive take on it would just store the 4 byte ticks and make sure something eventually takes care of it before a potential wrap around. The general problem is that atime updates from reads on all filesystems is drastically inaccurate - timestamp is updated many seconds later. That said, fixing even if only for tmpfs may be beyond the scope of this patch.

sys/fs/tmpfs/tmpfs_subr.c
1868

Can we skip tm_ronly checks? Instead it can be asserted that tn_status never gets TMPFS_NODE_ACCESSED nor TMPFS_NODE_MODIFIED bits set and the tn_accessed field can be ignored in tmpfs_itimes.

kib marked 3 inline comments as done.Sep 16 2020, 6:09 PM
In D26451#588524, @mjg wrote:
In D26451#588469, @kib wrote:
In D26451#588463, @mjg wrote:

On a more general note perhaps this can finally move to just getnanotime() on access and updating the stored value only if it goes forward. While this would incur some loss of concurrency it may be small enough to be more than tolerable, all while finally providing reasonable timestamp accuracy. Note as it is atime is updated who-knows-when. Same goes for other counters.

At least we do not write torn values to Xtime fields. I do not want to take node lock in tmpfs_pgcache_read(), after all efforts I put to remove object and node referencing.

But we don't have to have torn writes or any locking. The most aggressive take on it would just store the 4 byte ticks and make sure something eventually takes care of it before a potential wrap around. The general problem is that atime updates from reads on all filesystems is drastically inaccurate - timestamp is updated many seconds later. That said, fixing even if only for tmpfs may be beyond the scope of this patch.

Storing timecounter raw value means that we need to do active scan each second.

sys/fs/tmpfs/tmpfs_subr.c
1868

It is not that simple, right now code is organized so that we must process all times even after tm_ronly and MNT_RDONLY are set.

I might look at restructuring rw->ro remount, then it could be possible with some adjustments.

kib marked an inline comment as done.

Add comment about tn_accessed.
Use atomic_store_8.
Unrelated style changes will be committed separately.

This revision now requires review to proceed.Sep 16 2020, 6:11 PM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 16 2020, 9:28 PM
This revision was automatically updated to reflect the committed changes.