statx, inode: document the new STATX_INO_VERSION field

I'm proposing to expose the inode change attribute via statx [1]. Document what this value means and what an observer can infer from it changing.

Signed-off-by Jeff Layton <[email protected]>

[1]: https://lore.kernel.org/linux-nfs/[email protected]/T/#t

---
 man2/statx.2 |  8 ++++++++
 man7/inode.7 | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

v4: add paragraph pointing out the lack of atomicity wrt other changes

I think these patches are racing with another change to add DIO alignment info to statx. I imagine this will go in after that, so this will probably need to be respun to account for contextual differences.

What I'm mostly interested in here is getting the sematics and description of the i_version counter nailed down.

man2/statx.2 +8 -0

index 0d1b4591f74c..d98d5148a442 100644
@@ -62,6 +62,7 @@ struct statx {
     __u32 stx_dev_major;   /* Major ID */
     __u32 stx_dev_minor;   /* Minor ID */
     __u64 stx_mnt_id;      /* Mount ID */
+    __u64 stx_ino_version; /* Inode change attribute */
 };
 .EE
 .in

@@ -247,6 +248,7 @@ STATX_BTIME	Want stx_btime
 STATX_ALL	The same as STATX_BASIC_STATS | STATX_BTIME.
 	It is deprecated and should not be used.
 STATX_MNT_ID	Want stx_mnt_id (since Linux 5.8)
+STATX_INO_VERSION	Want stx_ino_version (DRAFT)
 .TE
 .in
 .PP

@@ -407,10 +409,16 @@ This is the same number reported by
 .BR name_to_handle_at (2)
 and corresponds to the number in the first field in one of the records in
 .IR /proc/self/mountinfo .
+.TP
+.I stx_ino_version
+The inode version, also known as the inode change attribute. See
+.BR inode (7)
+for details.
 .PP
 For further information on the above fields, see
 .BR inode (7).
 .\"
+.TP
 .SS File attributes
 The
 .I stx_attributes

man7/inode.7 +14 -0

index 9b255a890720..8e83836594d8 100644
@@ -184,6 +184,12 @@ Last status change timestamp (ctime)
 This is the file's last status change timestamp.
 It is changed by writing or by setting inode information
 (i.e., owner, group, link count, mode, etc.).
+.TP
+Inode version (i_version)
+(not returned in the \fIstat\fP structure); \fIstatx.stx_ino_version\fP
+.IP
+This is the inode change counter. See the discussion of
+\fBthe inode version counter\fP, below.
 .PP
 The timestamp fields report time measured with a zero point at the
 .IR Epoch ,

@@ -424,6 +430,39 @@ on a directory means that a file
 in that directory can be renamed or deleted only by the owner
 of the file, by the owner of the directory, and by a privileged
 process.
+.SS The inode version counter
+.PP
+The
+.I statx.stx_ino_version
+field is the inode change counter. Any operation that would result in a
+change to \fIstatx.stx_ctime\fP must result in an increase to this value.
+The value must increase even in the case where the ctime change is not
+evident due to coarse timestamp granularity.

man7/inode.7 on a directory means that a file · line 450

Quoted 4 lines · Jeff Layton

man7/inode.7 +4 -0

+The change to \fIstatx.stx_ino_version\fP is not atomic with respect to the
+other changes in the inode. On a write, for instance, the i_version it usually
+incremented before the data is copied into the pagecache. Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless? Surely the change number must change no sooner than the change itself is visible, otherwise stale data could be cached indefinitely.

If currently implementations behave this way, surely they are broken.

NeilBrown

Quoted 9 lines · NeilBrown

man7/inode.7 on a directory means that a file · line 450

Quoted 4 lines · Jeff Layton

man7/inode.7 +4 -0

+The change to \fIstatx.stx_ino_version\fP is not atomic with respect to the
+other changes in the inode. On a write, for instance, the i_version it usually
+incremented before the data is copied into the pagecache. Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless? Surely the change number must change no sooner than the change itself is visible, otherwise stale data could be cached indefinitely.

For the purposes of NFS close-to-open, I guess all we need is for the change attribute increment to happen sometime between the open and the close.

But, yes, it'd seem a lot more useful if it was guaranteed to happen after. (Or before and after both--extraneous increments aren't a big problem here.)

Quoted 3 lines

If currently implementations behave this way, surely they are broken.

Quoted 90 lines · Jeff Layton

I'm proposing to expose the inode change attribute via statx [1]. Document what this value means and what an observer can infer from it changing.

Signed-off-by Jeff Layton <[email protected]>

[1]: https://lore.kernel.org/linux-nfs/[email protected]/T/#t

---
 man2/statx.2 |  8 ++++++++
 man7/inode.7 | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

v4: add paragraph pointing out the lack of atomicity wrt other changes

I think these patches are racing with another change to add DIO alignment info to statx. I imagine this will go in after that, so this will probably need to be respun to account for contextual differences.

What I'm mostly interested in here is getting the sematics and description of the i_version counter nailed down.

man2/statx.2 +8 -0

index 0d1b4591f74c..d98d5148a442 100644
@@ -62,6 +62,7 @@ struct statx {
     __u32 stx_dev_major;   /* Major ID */
     __u32 stx_dev_minor;   /* Minor ID */
     __u64 stx_mnt_id;      /* Mount ID */
+    __u64 stx_ino_version; /* Inode change attribute */
 };
 .EE
 .in

@@ -247,6 +248,7 @@ STATX_BTIME	Want stx_btime
 STATX_ALL	The same as STATX_BASIC_STATS | STATX_BTIME.
 	It is deprecated and should not be used.
 STATX_MNT_ID	Want stx_mnt_id (since Linux 5.8)
+STATX_INO_VERSION	Want stx_ino_version (DRAFT)
 .TE
 .in
 .PP

@@ -407,10 +409,16 @@ This is the same number reported by
 .BR name_to_handle_at (2)
 and corresponds to the number in the first field in one of the records in
 .IR /proc/self/mountinfo .
+.TP
+.I stx_ino_version
+The inode version, also known as the inode change attribute. See
+.BR inode (7)
+for details.
 .PP
 For further information on the above fields, see
 .BR inode (7).
 .\"
+.TP
 .SS File attributes
 The
 .I stx_attributes

man7/inode.7 +18 -0

index 9b255a890720..8e83836594d8 100644
@@ -184,6 +184,12 @@ Last status change timestamp (ctime)
 This is the file's last status change timestamp.
 It is changed by writing or by setting inode information
 (i.e., owner, group, link count, mode, etc.).
+.TP
+Inode version (i_version)
+(not returned in the \fIstat\fP structure); \fIstatx.stx_ino_version\fP
+.IP
+This is the inode change counter. See the discussion of
+\fBthe inode version counter\fP, below.
 .PP
 The timestamp fields report time measured with a zero point at the
 .IR Epoch ,

@@ -424,6 +430,39 @@ on a directory means that a file
 in that directory can be renamed or deleted only by the owner
 of the file, by the owner of the directory, and by a privileged
 process.
+.SS The inode version counter
+.PP
+The
+.I statx.stx_ino_version
+field is the inode change counter. Any operation that would result in a
+change to \fIstatx.stx_ctime\fP must result in an increase to this value.
+The value must increase even in the case where the ctime change is not
+evident due to coarse timestamp granularity.
+.PP
+An observer cannot infer anything from amount of increase about the
+nature or magnitude of the change. If the returned value is different
+from the last time it was checked, then something has made an explicit

man7/inode.7 on a directory means that a file · line 450

Quoted 6 lines

man7/inode.7 +6 -0

+data and/or metadata change to the inode.
+.PP
+The change to \fIstatx.stx_ino_version\fP is not atomic with respect to the
+other changes in the inode. On a write, for instance, the i_version it usually
+incremented before the data is copied into the pagecache. Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Surely the change number must
change no sooner than the change itself is visible, otherwise stale data could be cached indefinitely.

If currently implementations behave this way, surely they are broken.

Quoted 8 lines · NeilBrown

man7/inode.7 on a directory means that a file · line 450

Quoted 4 lines · Jeff Layton

man7/inode.7 +4 -0

+The change to \fIstatx.stx_ino_version\fP is not atomic with respect to the
+other changes in the inode. On a write, for instance, the i_version it usually
+incremented before the data is copied into the pagecache. Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

No, I don't think so. It's only really useful for comparing to an older sample anyway. If you do "statx; read; statx" and the value hasn't changed, then you know that things are stable.

Quoted 6 lines

Surely the change number must
change no sooner than the change itself is visible, otherwise stale data could be cached indefinitely.

If currently implementations behave this way, surely they are broken.

It's certainly not ideal but we've never been able to offer truly atomic behavior here given that Linux is a general-purpose OS. The behavior is a little inconsistent too:

The c/mtime update and i_version bump on directories (mostly) occur after the operation. c/mtime updates for files however are mostly driven by calls to file_update_time, which happens before data is copied to the pagecache.

It's not clear to me why it's done this way. Maybe to ensure that the metadata is up to date in the event that a statx comes in? Improving this would be nice, but I don't see a way to do that without regressing performance.

Signature · 1 line

Jeff Layton <[email protected]>

Quoted 10 lines · Jeff Layton

Quoted 8 lines · NeilBrown

man7/inode.7 on a directory means that a file · line 450

Quoted 4 lines · Jeff Layton

man7/inode.7 +4 -0

+The change to \fIstatx.stx_ino_version\fP is not atomic with respect to the
+other changes in the inode. On a write, for instance, the i_version it usually
+incremented before the data is copied into the pagecache. Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 3 lines

No, I don't think so. It's only really useful for comparing to an older sample anyway. If you do "statx; read; statx" and the value hasn't changed, then you know that things are stable.

I don't see how that helps. It's still possible to get:

		reader		writer
		------		------
				i_version++
		statx
		read
		statx
				update page cache

right?

Quoted 22 lines

Quoted 6 lines

Surely the change number must
change no sooner than the change itself is visible, otherwise stale data could be cached indefinitely.

If currently implementations behave this way, surely they are broken.

It's certainly not ideal but we've never been able to offer truly atomic behavior here given that Linux is a general-purpose OS. The behavior is a little inconsistent too:

The c/mtime update and i_version bump on directories (mostly) occur after the operation. c/mtime updates for files however are mostly driven by calls to file_update_time, which happens before data is copied to the pagecache.

It's not clear to me why it's done this way. Maybe to ensure that the metadata is up to date in the event that a statx comes in? Improving this would be nice, but I don't see a way to do that without regressing performance.

Signature · 1 line

Jeff Layton <[email protected]>

Quoted 31 lines · J. Bruce Fields

Quoted 14 lines · Jeff Layton

Quoted 12 lines · NeilBrown

Quoted 8 lines · Jeff Layton

+The change to \fIstatx.stx_ino_version\fP is not atomic with respect to the
+other changes in the inode. On a write, for instance, the i_version it usually
+incremented before the data is copied into the pagecache. Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 5 lines

No, I don't think so. It's only really useful for comparing to an older
sample anyway. If you do "statx; read; statx" and the value hasn't
changed, then you know that things are stable.

I don't see how that helps.  It's still possible to get:

                reader          writer
                ------          ------
                                i_version++
                statx
                read
                statx
                                update page cache

Quoted 2 lines

right?

Yeah, I suppose so -- the statx wouldn't necessitate any locking. In that case, maybe this is useless then other than for testing purposes and userland NFS servers.

Would it be better to not consume a statx field with this if so? What could we use as an alternate interface? ioctl? Some sort of global virtual xattr? It does need to be something per-inode.

Quoted 35 lines · Jeff Layton

Quoted 31 lines · J. Bruce Fields

Quoted 14 lines · Jeff Layton

Quoted 12 lines · NeilBrown

Quoted 8 lines · Jeff Layton

+The change to \fIstatx.stx_ino_version\fP is not atomic with respect to the
+other changes in the inode. On a write, for instance, the i_version it usually
+incremented before the data is copied into the pagecache. Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 5 lines

No, I don't think so. It's only really useful for comparing to an older
sample anyway. If you do "statx; read; statx" and the value hasn't
changed, then you know that things are stable.

I don't see how that helps.  It's still possible to get:

                reader          writer
                ------          ------
                                i_version++
                statx
                read
                statx
                                update page cache

Quoted 2 lines

right?

Quoted 7 lines

Yeah, I suppose so -- the statx wouldn't necessitate any locking. In that case, maybe this is useless then other than for testing purposes and userland NFS servers.

Would it be better to not consume a statx field with this if so? What could we use as an alternate interface? ioctl? Some sort of global virtual xattr? It does need to be something per-inode.

I don't see how a non-atomic change attribute is remotely useful even for NFS.

The main problem is not so much the above (although NFS clients are vulnerable to that too) but the behaviour w.r.t. directory changes.

If the server can't guarantee that file/directory/... creation and unlink are atomically recorded with change attribute updates, then the client has to always assume that the server is lying, and that it has to revalidate all its caches anyway. Cue endless readdir/lookup/getattr requests after each and every directory modification in order to check that some other client didn't also sneak in a change of their own.

Quoted 59 lines · Trond Myklebust

Quoted 45 lines · Jeff Layton

Quoted 35 lines · J. Bruce Fields

Quoted 17 lines · Jeff Layton

Quoted 15 lines · NeilBrown

Quoted 11 lines · Jeff Layton

+The change to \fIstatx.stx_ino_version\fP is not atomic with
respect to the
+other changes in the inode. On a write, for instance, the
i_version it usually
+incremented before the data is copied into the pagecache.
Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 6 lines

No, I don't think so. It's only really useful for comparing to an
older
sample anyway. If you do "statx; read; statx" and the value hasn't
changed, then you know that things are stable.

I don't see how that helps.  It's still possible to get:

                reader          writer
                ------          ------
                                i_version++
                statx
                read
                statx
                                update page cache

Quoted 2 lines

right?

Yeah, I suppose so -- the statx wouldn't necessitate any locking. In
that case, maybe this is useless then other than for testing purposes
and userland NFS servers.

Quoted 5 lines

Would it be better to not consume a statx field with this if so? What
could we use as an alternate interface? ioctl? Some sort of global
virtual xattr? It does need to be something per-inode.

I don't see how a non-atomic change attribute is remotely useful even
for NFS.

The main problem is not so much the above (although NFS clients are vulnerable to that too) but the behaviour w.r.t. directory changes.

Quoted 11 lines

If the server can't guarantee that file/directory/... creation and unlink are atomically recorded with change attribute updates, then the
client has to always assume that the server is lying, and that it has
to revalidate all its caches anyway. Cue endless readdir/lookup/getattr
requests after each and every directory modification in order to check
that some other client didn't also sneak in a change of their own.

We generally hold the parent dir's inode->i_rwsem exclusively over most
important directory changes, and the times/i_version are also updated while holding it. What we don't do is serialize reads of this value vs.
the i_rwsem, so you could see new directory contents alongside an old i_version. Maybe we should be taking it for read when we query it on a
directory?

Achieving atomicity with file writes though is another matter entirely.
I'm not sure that's even doable or how to approach it if so. Suggestions?

Quoted 72 lines · Jeff Layton

Quoted 59 lines · Trond Myklebust

Quoted 45 lines · Jeff Layton

Quoted 35 lines · J. Bruce Fields

Quoted 17 lines · Jeff Layton

Quoted 15 lines · NeilBrown

Quoted 11 lines · Jeff Layton

+The change to \fIstatx.stx_ino_version\fP is not atomic with
respect to the
+other changes in the inode. On a write, for instance, the
i_version it usually
+incremented before the data is copied into the pagecache.
Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 6 lines

No, I don't think so. It's only really useful for comparing to an
older
sample anyway. If you do "statx; read; statx" and the value hasn't
changed, then you know that things are stable.

I don't see how that helps.  It's still possible to get:

                reader          writer
                ------          ------
                                i_version++
                statx
                read
                statx
                                update page cache

Quoted 2 lines

right?

Yeah, I suppose so -- the statx wouldn't necessitate any locking. In
that case, maybe this is useless then other than for testing purposes
and userland NFS servers.

Quoted 5 lines

Would it be better to not consume a statx field with this if so? What
could we use as an alternate interface? ioctl? Some sort of global
virtual xattr? It does need to be something per-inode.

I don't see how a non-atomic change attribute is remotely useful even
for NFS.

The main problem is not so much the above (although NFS clients are vulnerable to that too) but the behaviour w.r.t. directory changes.

Quoted 11 lines

If the server can't guarantee that file/directory/... creation and unlink are atomically recorded with change attribute updates, then the
client has to always assume that the server is lying, and that it has
to revalidate all its caches anyway. Cue endless readdir/lookup/getattr
requests after each and every directory modification in order to check
that some other client didn't also sneak in a change of their own.

Quoted 9 lines

We generally hold the parent dir's inode->i_rwsem exclusively over most
important directory changes, and the times/i_version are also updated while holding it. What we don't do is serialize reads of this value vs.
the i_rwsem, so you could see new directory contents alongside an old i_version. Maybe we should be taking it for read when we query it on a
directory?

Serialising reads is not the problem. The problem is ensuring that knfsd is able to provide an atomic change_info4 structure when the client modifies the directory.
i.e. the requirement is that if the directory changed, then that modification is atomically accompanied by an update of the change attribute that can be retrieved by knfsd and placed in the reply to the client.

Quoted 4 lines

Achieving atomicity with file writes though is another matter entirely.
I'm not sure that's even doable or how to approach it if so. Suggestions?

The problem outlined by Bruce above isn't a big deal. Just check the I_VERSION_QUERIED flag after the 'update_page_cache' bit, and bump the i_version if that's the case. The real problem is what happens if you then crash during writeback...

Quoted 50 lines · Trond Myklebust

Quoted 35 lines · Jeff Layton

Quoted 31 lines · J. Bruce Fields

Quoted 14 lines · Jeff Layton

Quoted 12 lines · NeilBrown

Quoted 8 lines · Jeff Layton

+The change to \fIstatx.stx_ino_version\fP is not atomic with respect to the
+other changes in the inode. On a write, for instance, the i_version it usually
+incremented before the data is copied into the pagecache. Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 5 lines

No, I don't think so. It's only really useful for comparing to an older
sample anyway. If you do "statx; read; statx" and the value hasn't
changed, then you know that things are stable.

I don't see how that helps.  It's still possible to get:

                reader          writer
                ------          ------
                                i_version++
                statx
                read
                statx
                                update page cache

Quoted 2 lines

right?

Quoted 7 lines

Yeah, I suppose so -- the statx wouldn't necessitate any locking. In that case, maybe this is useless then other than for testing purposes and userland NFS servers.

Would it be better to not consume a statx field with this if so? What could we use as an alternate interface? ioctl? Some sort of global virtual xattr? It does need to be something per-inode.

I don't see how a non-atomic change attribute is remotely useful even for NFS.

The main problem is not so much the above (although NFS clients are vulnerable to that too) but the behaviour w.r.t. directory changes.

Quoted 7 lines

If the server can't guarantee that file/directory/... creation and unlink are atomically recorded with change attribute updates, then the client has to always assume that the server is lying, and that it has to revalidate all its caches anyway. Cue endless readdir/lookup/getattr requests after each and every directory modification in order to check that some other client didn't also sneak in a change of their own.

We generally hold the parent dir's inode->i_rwsem exclusively over most important directory changes, and the times/i_version are also updated while holding it. What we don't do is serialize reads of this value vs. the i_rwsem, so you could see new directory contents alongside an old i_version. Maybe we should be taking it for read when we query it on a directory?

Achieving atomicity with file writes though is another matter entirely. I'm not sure that's even doable or how to approach it if so. Suggestions?

Quoted 59 lines · Jeff Layton

Quoted 50 lines · Trond Myklebust

Quoted 35 lines · Jeff Layton

Quoted 31 lines · J. Bruce Fields

Quoted 14 lines · Jeff Layton

Quoted 12 lines · NeilBrown

Quoted 8 lines · Jeff Layton

+The change to \fIstatx.stx_ino_version\fP is not atomic with respect to the
+other changes in the inode. On a write, for instance, the i_version it usually
+incremented before the data is copied into the pagecache. Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 5 lines

No, I don't think so. It's only really useful for comparing to an older
sample anyway. If you do "statx; read; statx" and the value hasn't
changed, then you know that things are stable.

I don't see how that helps.  It's still possible to get:

                reader          writer
                ------          ------
                                i_version++
                statx
                read
                statx
                                update page cache

Quoted 2 lines

right?

Quoted 7 lines

Yeah, I suppose so -- the statx wouldn't necessitate any locking. In that case, maybe this is useless then other than for testing purposes and userland NFS servers.

Would it be better to not consume a statx field with this if so? What could we use as an alternate interface? ioctl? Some sort of global virtual xattr? It does need to be something per-inode.

I don't see how a non-atomic change attribute is remotely useful even for NFS.

The main problem is not so much the above (although NFS clients are vulnerable to that too) but the behaviour w.r.t. directory changes.

Quoted 7 lines

If the server can't guarantee that file/directory/... creation and unlink are atomically recorded with change attribute updates, then the client has to always assume that the server is lying, and that it has to revalidate all its caches anyway. Cue endless readdir/lookup/getattr requests after each and every directory modification in order to check that some other client didn't also sneak in a change of their own.

Quoted 6 lines

We generally hold the parent dir's inode->i_rwsem exclusively over most important directory changes, and the times/i_version are also updated while holding it. What we don't do is serialize reads of this value vs. the i_rwsem, so you could see new directory contents alongside an old i_version. Maybe we should be taking it for read when we query it on a directory?

We do hold i_rwsem today. I'm working on changing that. Preserving atomic directory changeinfo will be a challenge. The only mechanism I can think if is to pass a "u64*" to all the directory modification ops, and they fill in the version number at the point where it is incremented (inode_maybe_inc_iversion_return()). The (nfsd) caller assumes that "before" was one less than "after". If you don't want to internally require single increments, then you would need to pass a 'u64 [2]' to get two iversions back.

Quoted 4 lines

Achieving atomicity with file writes though is another matter entirely. I'm not sure that's even doable or how to approach it if so. Suggestions?

Call inode_maybe_inc_version(page->host) in __folio_mark_dirty() ??

Quoted 67 lines · NeilBrown

Quoted 59 lines · Jeff Layton

Quoted 50 lines · Trond Myklebust

Quoted 35 lines · Jeff Layton

Quoted 31 lines · J. Bruce Fields

Quoted 14 lines · Jeff Layton

Quoted 12 lines · NeilBrown

Quoted 8 lines · Jeff Layton

+The change to \fIstatx.stx_ino_version\fP is not atomic with respect to the
+other changes in the inode. On a write, for instance, the i_version it usually
+incremented before the data is copied into the pagecache. Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 5 lines

No, I don't think so. It's only really useful for comparing to an older
sample anyway. If you do "statx; read; statx" and the value hasn't
changed, then you know that things are stable.

I don't see how that helps.  It's still possible to get:

                reader          writer
                ------          ------
                                i_version++
                statx
                read
                statx
                                update page cache

Quoted 2 lines

right?

Quoted 7 lines

Yeah, I suppose so -- the statx wouldn't necessitate any locking. In that case, maybe this is useless then other than for testing purposes and userland NFS servers.

Would it be better to not consume a statx field with this if so? What could we use as an alternate interface? ioctl? Some sort of global virtual xattr? It does need to be something per-inode.

I don't see how a non-atomic change attribute is remotely useful even for NFS.

The main problem is not so much the above (although NFS clients are vulnerable to that too) but the behaviour w.r.t. directory changes.

Quoted 7 lines

If the server can't guarantee that file/directory/... creation and unlink are atomically recorded with change attribute updates, then the client has to always assume that the server is lying, and that it has to revalidate all its caches anyway. Cue endless readdir/lookup/getattr requests after each and every directory modification in order to check that some other client didn't also sneak in a change of their own.

Quoted 6 lines

We generally hold the parent dir's inode->i_rwsem exclusively over most important directory changes, and the times/i_version are also updated while holding it. What we don't do is serialize reads of this value vs. the i_rwsem, so you could see new directory contents alongside an old i_version. Maybe we should be taking it for read when we query it on a directory?

Quoted 9 lines

We do hold i_rwsem today. I'm working on changing that. Preserving atomic directory changeinfo will be a challenge. The only mechanism I can think if is to pass a "u64*" to all the directory modification ops, and they fill in the version number at the point where it is incremented (inode_maybe_inc_iversion_return()). The (nfsd) caller assumes that "before" was one less than "after". If you don't want to internally require single increments, then you would need to pass a 'u64 [2]' to get two iversions back.

That's a major redesign of what the i_version counter is today. It may very well end up being needed, but that's going to touch a lot of stuff in the VFS. Are you planning to do that as a part of your locking changes?

Quoted 7 lines

Quoted 4 lines

Achieving atomicity with file writes though is another matter entirely. I'm not sure that's even doable or how to approach it if so. Suggestions?

Call inode_maybe_inc_version(page->host) in __folio_mark_dirty() ??

Writes can cover multiple folios so we'd be doing several increments per write. Maybe that's ok? Should we also be updating the ctime at that point as well?

Fetching the i_version under the i_rwsem is probably sufficient to fix this though. Most of the write_iter ops already bump the i_version while holding that lock, so this wouldn't add any extra locking to the write codepaths.

Signature · 2 lines

-- 
Jeff Layton <[email protected]>

Quoted 78 lines · Jeff Layton

Quoted 67 lines · NeilBrown

Quoted 59 lines · Jeff Layton

Quoted 50 lines · Trond Myklebust

Quoted 35 lines · Jeff Layton

Quoted 31 lines · J. Bruce Fields

Quoted 14 lines · Jeff Layton

Quoted 12 lines · NeilBrown

Quoted 8 lines · Jeff Layton

+The change to \fIstatx.stx_ino_version\fP is not atomic with respect to the
+other changes in the inode. On a write, for instance, the i_version it usually
+incremented before the data is copied into the pagecache. Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 5 lines

No, I don't think so. It's only really useful for comparing to an older
sample anyway. If you do "statx; read; statx" and the value hasn't
changed, then you know that things are stable.

I don't see how that helps.  It's still possible to get:

                reader          writer
                ------          ------
                                i_version++
                statx
                read
                statx
                                update page cache

Quoted 2 lines

right?

Quoted 7 lines

Yeah, I suppose so -- the statx wouldn't necessitate any locking. In that case, maybe this is useless then other than for testing purposes and userland NFS servers.

Would it be better to not consume a statx field with this if so? What could we use as an alternate interface? ioctl? Some sort of global virtual xattr? It does need to be something per-inode.

I don't see how a non-atomic change attribute is remotely useful even for NFS.

The main problem is not so much the above (although NFS clients are vulnerable to that too) but the behaviour w.r.t. directory changes.

Quoted 7 lines

If the server can't guarantee that file/directory/... creation and unlink are atomically recorded with change attribute updates, then the client has to always assume that the server is lying, and that it has to revalidate all its caches anyway. Cue endless readdir/lookup/getattr requests after each and every directory modification in order to check that some other client didn't also sneak in a change of their own.

Quoted 6 lines

We generally hold the parent dir's inode->i_rwsem exclusively over most important directory changes, and the times/i_version are also updated while holding it. What we don't do is serialize reads of this value vs. the i_rwsem, so you could see new directory contents alongside an old i_version. Maybe we should be taking it for read when we query it on a directory?

Quoted 9 lines

We do hold i_rwsem today. I'm working on changing that. Preserving atomic directory changeinfo will be a challenge. The only mechanism I can think if is to pass a "u64*" to all the directory modification ops, and they fill in the version number at the point where it is incremented (inode_maybe_inc_iversion_return()). The (nfsd) caller assumes that "before" was one less than "after". If you don't want to internally require single increments, then you would need to pass a 'u64 [2]' to get two iversions back.

Quoted 5 lines

That's a major redesign of what the i_version counter is today. It may very well end up being needed, but that's going to touch a lot of stuff in the VFS. Are you planning to do that as a part of your locking changes?

"A major design"? How? The "one less than" might be, but allowing a directory morphing op to fill in a "u64 [2]" is just a new interface to existing data. One that allows fine grained atomicity.

This would actually be really good for NFS. nfs_mkdir (for example) could easily have access to the atomic pre/post changedid provided by the server, and so could easily provide them to nfsd.

I'm not planning to do this as part of my locking changes. In the first instance only NFS changes behaviour, and it doesn't provide atomic changeids, so there is no loss of functionality.

When some other filesystem wants to opt-in to shared-locking on directories - that would be the time to push through a better interface.

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

Achieving atomicity with file writes though is another matter entirely. I'm not sure that's even doable or how to approach it if so. Suggestions?

Call inode_maybe_inc_version(page->host) in __folio_mark_dirty() ??

Quoted 3 lines

Writes can cover multiple folios so we'd be doing several increments per write. Maybe that's ok? Should we also be updating the ctime at that point as well?

You would only do several increments if something was reading the value concurrently, and then you really should to several increments for correctness.

Quoted 5 lines

Fetching the i_version under the i_rwsem is probably sufficient to fix this though. Most of the write_iter ops already bump the i_version while holding that lock, so this wouldn't add any extra locking to the write codepaths.

Adding new locking doesn't seem like a good idea. It's bound to have performance implications. It may well end up serialising the directory op that I'm currently trying to make parallelisable.

Quoted 93 lines · NeilBrown

Quoted 78 lines · Jeff Layton

Quoted 67 lines · NeilBrown

Quoted 59 lines · Jeff Layton

Quoted 50 lines · Trond Myklebust

Quoted 35 lines · Jeff Layton

Quoted 31 lines · J. Bruce Fields

Quoted 14 lines · Jeff Layton

Quoted 12 lines · NeilBrown

Quoted 8 lines · Jeff Layton

+The change to \fIstatx.stx_ino_version\fP is not atomic with respect to the
+other changes in the inode. On a write, for instance, the i_version it usually
+incremented before the data is copied into the pagecache. Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 5 lines

No, I don't think so. It's only really useful for comparing to an older
sample anyway. If you do "statx; read; statx" and the value hasn't
changed, then you know that things are stable.

I don't see how that helps.  It's still possible to get:

                reader          writer
                ------          ------
                                i_version++
                statx
                read
                statx
                                update page cache

Quoted 2 lines

right?

Quoted 7 lines

Yeah, I suppose so -- the statx wouldn't necessitate any locking. In that case, maybe this is useless then other than for testing purposes and userland NFS servers.

Would it be better to not consume a statx field with this if so? What could we use as an alternate interface? ioctl? Some sort of global virtual xattr? It does need to be something per-inode.

I don't see how a non-atomic change attribute is remotely useful even for NFS.

The main problem is not so much the above (although NFS clients are vulnerable to that too) but the behaviour w.r.t. directory changes.

Quoted 7 lines

If the server can't guarantee that file/directory/... creation and unlink are atomically recorded with change attribute updates, then the client has to always assume that the server is lying, and that it has to revalidate all its caches anyway. Cue endless readdir/lookup/getattr requests after each and every directory modification in order to check that some other client didn't also sneak in a change of their own.

Quoted 6 lines

We generally hold the parent dir's inode->i_rwsem exclusively over most important directory changes, and the times/i_version are also updated while holding it. What we don't do is serialize reads of this value vs. the i_rwsem, so you could see new directory contents alongside an old i_version. Maybe we should be taking it for read when we query it on a directory?

Quoted 9 lines

We do hold i_rwsem today. I'm working on changing that. Preserving atomic directory changeinfo will be a challenge. The only mechanism I can think if is to pass a "u64*" to all the directory modification ops, and they fill in the version number at the point where it is incremented (inode_maybe_inc_iversion_return()). The (nfsd) caller assumes that "before" was one less than "after". If you don't want to internally require single increments, then you would need to pass a 'u64 [2]' to get two iversions back.

Quoted 5 lines

That's a major redesign of what the i_version counter is today. It may very well end up being needed, but that's going to touch a lot of stuff in the VFS. Are you planning to do that as a part of your locking changes?

"A major design"? How? The "one less than" might be, but allowing a directory morphing op to fill in a "u64 [2]" is just a new interface to existing data. One that allows fine grained atomicity.

This would actually be really good for NFS. nfs_mkdir (for example) could easily have access to the atomic pre/post changedid provided by the server, and so could easily provide them to nfsd.

Quoted 7 lines

I'm not planning to do this as part of my locking changes. In the first instance only NFS changes behaviour, and it doesn't provide atomic changeids, so there is no loss of functionality.

When some other filesystem wants to opt-in to shared-locking on directories - that would be the time to push through a better interface.

I think nfsd does provide atomic changeids for directory operations currently. AFAICT, any operation where we're changing directory contents is done while holding the i_rwsem exclusively, and we hold that lock over the pre and post i_version fetch for the change_info4.

If you change nfsd to allow parallel directory morphing operations without addressing this, then I think that would be a regression.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

Achieving atomicity with file writes though is another matter entirely. I'm not sure that's even doable or how to approach it if so. Suggestions?

Call inode_maybe_inc_version(page->host) in __folio_mark_dirty() ??

Quoted 8 lines

Quoted 3 lines

Writes can cover multiple folios so we'd be doing several increments per write. Maybe that's ok? Should we also be updating the ctime at that point as well?

You would only do several increments if something was reading the value concurrently, and then you really should to several increments for correctness.

Agreed.

Quoted 10 lines

Quoted 5 lines

Fetching the i_version under the i_rwsem is probably sufficient to fix this though. Most of the write_iter ops already bump the i_version while holding that lock, so this wouldn't add any extra locking to the write codepaths.

Adding new locking doesn't seem like a good idea. It's bound to have performance implications. It may well end up serialising the directory op that I'm currently trying to make parallelisable.

The new locking would only be in the NFSv4 GETATTR codepath:

    https://lore.kernel.org/linux-nfs/[email protected]/T/#u

Maybe we'd still better off taking a hit in the write codepath instead of doing this, but with this, most of the penalty would be paid by nfsd which I would think would be preferred here.

The problem of mmap writes is another matter though. Not sure what we can do about that without making i_version bumps a lot more expensive.

Signature · 1 line

Jeff Layton <[email protected]>

We do hold i_rwsem today. I'm working on changing that. Preserving atomic directory changeinfo will be a challenge. The only mechanism I can think if is to pass a "u64*" to all the directory modification ops, and they fill in the version number at the point where it is incremented (inode_maybe_inc_iversion_return()). The (nfsd) caller assumes that "before" was one less than "after". If you don't want to internally require single increments, then you would need to pass a 'u64 [2]' to get two iversions back.

Quoted 8 lines · NeilBrown

We do hold i_rwsem today. I'm working on changing that. Preserving atomic directory changeinfo will be a challenge. The only mechanism I can think if is to pass a "u64*" to all the directory modification ops, and they fill in the version number at the point where it is incremented (inode_maybe_inc_iversion_return()). The (nfsd) caller assumes that "before" was one less than "after". If you don't want to internally require single increments, then you would need to pass a 'u64 [2]' to get two iversions back.

Are you serious? What kind of boilerplate would that inflict on the filesystems not, er, opting in for that... scalability improvement experiment?

Quoted 35 lines · Jeff Layton

Quoted 31 lines · J. Bruce Fields

Quoted 14 lines · Jeff Layton

Quoted 12 lines · NeilBrown

Quoted 8 lines · Jeff Layton

+The change to \fIstatx.stx_ino_version\fP is not atomic with respect to the
+other changes in the inode. On a write, for instance, the i_version it usually
+incremented before the data is copied into the pagecache. Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 5 lines

No, I don't think so. It's only really useful for comparing to an older
sample anyway. If you do "statx; read; statx" and the value hasn't
changed, then you know that things are stable.

I don't see how that helps.  It's still possible to get:

                reader          writer
                ------          ------
                                i_version++
                statx
                read
                statx
                                update page cache

Quoted 2 lines

right?

Quoted 7 lines

Yeah, I suppose so -- the statx wouldn't necessitate any locking. In that case, maybe this is useless then other than for testing purposes and userland NFS servers.

Would it be better to not consume a statx field with this if so? What could we use as an alternate interface? ioctl? Some sort of global virtual xattr? It does need to be something per-inode.

I don't see how a non-atomic change attribute is remotely useful even for NFS.

The main problem is not so much the above (although NFS clients are vulnerable to that too) but the behaviour w.r.t. directory changes.

If the server can't guarantee that file/directory/... creation and unlink are atomically recorded with change attribute updates, then the client has to always assume that the server is lying, and that it has to revalidate all its caches anyway. Cue endless readdir/lookup/getattr requests after each and every directory modification in order to check that some other client didn't also sneak in a change of their own.

Quoted 59 lines · Trond Myklebust

Quoted 45 lines · Jeff Layton

Quoted 35 lines · J. Bruce Fields

Quoted 17 lines · Jeff Layton

Quoted 15 lines · NeilBrown

Quoted 11 lines · Jeff Layton

+The change to \fIstatx.stx_ino_version\fP is not atomic with
respect to the
+other changes in the inode. On a write, for instance, the
i_version it usually
+incremented before the data is copied into the pagecache.
Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 6 lines

No, I don't think so. It's only really useful for comparing to an
older
sample anyway. If you do "statx; read; statx" and the value hasn't
changed, then you know that things are stable.

I don't see how that helps.  It's still possible to get:

                reader          writer
                ------          ------
                                i_version++
                statx
                read
                statx
                                update page cache

Quoted 2 lines

right?

Yeah, I suppose so -- the statx wouldn't necessitate any locking. In
that case, maybe this is useless then other than for testing purposes
and userland NFS servers.

Quoted 5 lines

Would it be better to not consume a statx field with this if so? What
could we use as an alternate interface? ioctl? Some sort of global
virtual xattr? It does need to be something per-inode.

I don't see how a non-atomic change attribute is remotely useful even
for NFS.

The main problem is not so much the above (although NFS clients are vulnerable to that too) but the behaviour w.r.t. directory changes.

Quoted 10 lines

If the server can't guarantee that file/directory/... creation and unlink are atomically recorded with change attribute updates, then the
client has to always assume that the server is lying, and that it has
to revalidate all its caches anyway. Cue endless readdir/lookup/getattr
requests after each and every directory modification in order to check
that some other client didn't also sneak in a change of their own.

NFS re-export doesn't support atomic change attributes on directories.
Do we see the endless revalidate requests after directory modification
in that situation?  Just curious.

Quoted 71 lines · NeilBrown

Quoted 59 lines · Trond Myklebust

Quoted 45 lines · Jeff Layton

Quoted 35 lines · J. Bruce Fields

Quoted 17 lines · Jeff Layton

Quoted 15 lines · NeilBrown

Quoted 11 lines · Jeff Layton

+The change to \fIstatx.stx_ino_version\fP is not atomic with
respect to the
+other changes in the inode. On a write, for instance, the
i_version it usually
+incremented before the data is copied into the pagecache.
Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 6 lines

No, I don't think so. It's only really useful for comparing to an
older
sample anyway. If you do "statx; read; statx" and the value hasn't
changed, then you know that things are stable.

I don't see how that helps.  It's still possible to get:

                reader          writer
                ------          ------
                                i_version++
                statx
                read
                statx
                                update page cache

Quoted 2 lines

right?

Yeah, I suppose so -- the statx wouldn't necessitate any locking. In
that case, maybe this is useless then other than for testing purposes
and userland NFS servers.

Quoted 5 lines

Would it be better to not consume a statx field with this if so? What
could we use as an alternate interface? ioctl? Some sort of global
virtual xattr? It does need to be something per-inode.

I don't see how a non-atomic change attribute is remotely useful even
for NFS.

The main problem is not so much the above (although NFS clients are vulnerable to that too) but the behaviour w.r.t. directory changes.

Quoted 10 lines

If the server can't guarantee that file/directory/... creation and unlink are atomically recorded with change attribute updates, then the
client has to always assume that the server is lying, and that it has
to revalidate all its caches anyway. Cue endless readdir/lookup/getattr
requests after each and every directory modification in order to check
that some other client didn't also sneak in a change of their own.

Quoted 5 lines

NFS re-export doesn't support atomic change attributes on directories.
Do we see the endless revalidate requests after directory modification
in that situation?  Just curious.

Why wouldn't NFS re-export be capable of supporting atomic change attributes in those cases, provided that the server does? It seems to me that is just a question of providing the correct information w.r.t. atomicity to knfsd.

...but yes, a quick glance at nfs4_update_changeattr_locked(), and what happens when !cinfo->atomic should tell you all you need to know.

My impression is that pre/post attributes in NFSv3 have not turned out to be as useful as their inventors predicted.

Signature · 4 lines

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]

Quoted 71 lines · NeilBrown

Quoted 59 lines · Trond Myklebust

Quoted 45 lines · Jeff Layton

Quoted 35 lines · J. Bruce Fields

Quoted 17 lines · Jeff Layton

Quoted 15 lines · NeilBrown

Quoted 11 lines · Jeff Layton

+The change to \fIstatx.stx_ino_version\fP is not atomic with
respect to the
+other changes in the inode. On a write, for instance, the
i_version it usually
+incremented before the data is copied into the pagecache.
Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 6 lines

No, I don't think so. It's only really useful for comparing to an
older
sample anyway. If you do "statx; read; statx" and the value hasn't
changed, then you know that things are stable.

I don't see how that helps.  It's still possible to get:

                reader          writer
                ------          ------
                                i_version++
                statx
                read
                statx
                                update page cache

Quoted 2 lines

right?

Yeah, I suppose so -- the statx wouldn't necessitate any locking. In
that case, maybe this is useless then other than for testing purposes
and userland NFS servers.

Quoted 5 lines

Would it be better to not consume a statx field with this if so? What
could we use as an alternate interface? ioctl? Some sort of global
virtual xattr? It does need to be something per-inode.

I don't see how a non-atomic change attribute is remotely useful even
for NFS.

The main problem is not so much the above (although NFS clients are vulnerable to that too) but the behaviour w.r.t. directory changes.

Quoted 10 lines

If the server can't guarantee that file/directory/... creation and unlink are atomically recorded with change attribute updates, then the
client has to always assume that the server is lying, and that it has
to revalidate all its caches anyway. Cue endless readdir/lookup/getattr
requests after each and every directory modification in order to check
that some other client didn't also sneak in a change of their own.

Quoted 5 lines

NFS re-export doesn't support atomic change attributes on directories.
Do we see the endless revalidate requests after directory modification
in that situation?  Just curious.

Why wouldn't NFS re-export be capable of supporting atomic change attributes in those cases, provided that the server does? It seems to me that is just a question of providing the correct information w.r.t. atomicity to knfsd.

...but yes, a quick glance at nfs4_update_changeattr_locked(), and what happens when !cinfo->atomic should tell you all you need to know.

Quoted 101 lines · Trond Myklebust

Quoted 87 lines · NeilBrown

Quoted 70 lines · Trond Myklebust

Quoted 52 lines · Jeff Layton

Quoted 41 lines · J. Bruce Fields

Quoted 20 lines · Jeff Layton

Quoted 18 lines · NeilBrown

Quoted 6 lines · Jeff Layton

+The change to \fIstatx.stx_ino_version\fP is not
atomic
with
respect to the
+other changes in the inode. On a write, for
instance,

Quoted 8 lines

the
i_version it usually
+incremented before the data is copied into the pagecache.
Therefore it is
+possible to see a new i_version value while a read still
shows the old data.

Doesn't that make the value useless?

Quoted 8 lines

No, I don't think so. It's only really useful for comparing
to an
older
sample anyway. If you do "statx; read; statx" and the value
hasn't
changed, then you know that things are stable.

I don't see how that helps.  It's still possible to get:

                reader          writer
                ------          ------
                                i_version++
                statx
                read
                statx
                                update page cache

Quoted 2 lines

right?

Yeah, I suppose so -- the statx wouldn't necessitate any locking.
In
that case, maybe this is useless then other than for testing purposes
and userland NFS servers.

Quoted 6 lines

Would it be better to not consume a statx field with this if so?
What
could we use as an alternate interface? ioctl? Some sort of global
virtual xattr? It does need to be something per-inode.

I don't see how a non-atomic change attribute is remotely useful
even
for NFS.

The main problem is not so much the above (although NFS clients are
vulnerable to that too) but the behaviour w.r.t. directory changes.

Quoted 15 lines

If the server can't guarantee that file/directory/... creation and
unlink are atomically recorded with change attribute updates, then
the
client has to always assume that the server is lying, and that it
has
to revalidate all its caches anyway. Cue endless readdir/lookup/getattr
requests after each and every directory modification in order to
check
that some other client didn't also sneak in a change of their own.

Quoted 5 lines

NFS re-export doesn't support atomic change attributes on directories.
Do we see the endless revalidate requests after directory modification
in that situation?  Just curious.

Why wouldn't NFS re-export be capable of supporting atomic change attributes in those cases, provided that the server does? It seems to
me that is just a question of providing the correct information w.r.t.
atomicity to knfsd.

Quoted 3 lines

...but yes, a quick glance at nfs4_update_changeattr_locked(), and what
happens when !cinfo->atomic should tell you all you need to know.

The main reason we disabled atomic change attribute updates was that getattr calls on NFS can be pretty expensive. By setting the NOWCC flag,
we can avoid those for WCC info, but at the expense of the client having
to do more revalidation on its own.

Quoted 25 lines · J. Bruce Fields

Quoted 10 lines · Jeff Layton

Quoted 8 lines · NeilBrown

man7/inode.7 on a directory means that a file · line 450

Quoted 4 lines · Jeff Layton

man7/inode.7 +4 -0

+The change to \fIstatx.stx_ino_version\fP is not atomic with respect to the
+other changes in the inode. On a write, for instance, the i_version it usually
+incremented before the data is copied into the pagecache. Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 3 lines

No, I don't think so. It's only really useful for comparing to an older sample anyway. If you do "statx; read; statx" and the value hasn't changed, then you know that things are stable.

I don't see how that helps. It's still possible to get:

		reader		writer
		------		------
				i_version++
		statx
		read
		statx
				update page cache

Quoted 2 lines

right?

Yeah, I suppose so -- the statx wouldn't necessitate any locking. In that case, maybe this is useless then other than for testing purposes and userland NFS servers.

Would it be better to not consume a statx field with this if so? What could we use as an alternate interface? ioctl? Some sort of global virtual xattr? It does need to be something per-inode.

Quoted 29 lines · Jeff Layton

Quoted 25 lines · J. Bruce Fields

Quoted 10 lines · Jeff Layton

Quoted 8 lines · NeilBrown

man7/inode.7 on a directory means that a file · line 450

Quoted 4 lines · Jeff Layton

man7/inode.7 +4 -0

+The change to \fIstatx.stx_ino_version\fP is not atomic with respect to the
+other changes in the inode. On a write, for instance, the i_version it usually
+incremented before the data is copied into the pagecache. Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 3 lines

No, I don't think so. It's only really useful for comparing to an older sample anyway. If you do "statx; read; statx" and the value hasn't changed, then you know that things are stable.

I don't see how that helps. It's still possible to get:

		reader		writer
		------		------
				i_version++
		statx
		read
		statx
				update page cache

Quoted 2 lines

right?

Quoted 7 lines

Yeah, I suppose so -- the statx wouldn't necessitate any locking. In that case, maybe this is useless then other than for testing purposes and userland NFS servers.

Would it be better to not consume a statx field with this if so? What could we use as an alternate interface? ioctl? Some sort of global virtual xattr? It does need to be something per-inode.

I was thinking how hard would it be to increment i_version after updating data but it will be rather hairy. In particular because of stuff like IOCB_NOWAIT support which needs to bail if i_version update is needed. So yeah, I don't think there's an easy way how to provide useful i_version for general purpose use.

Quoted 29 lines · Jeff Layton

Quoted 25 lines · J. Bruce Fields

Quoted 10 lines · Jeff Layton

Quoted 8 lines · NeilBrown

man7/inode.7 on a directory means that a file · line 450

Quoted 4 lines · Jeff Layton

man7/inode.7 +4 -0

+The change to \fIstatx.stx_ino_version\fP is not atomic with respect to the
+other changes in the inode. On a write, for instance, the i_version it usually
+incremented before the data is copied into the pagecache. Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 3 lines

No, I don't think so. It's only really useful for comparing to an older sample anyway. If you do "statx; read; statx" and the value hasn't changed, then you know that things are stable.

I don't see how that helps. It's still possible to get:

		reader		writer
		------		------
				i_version++
		statx
		read
		statx
				update page cache

Quoted 2 lines

right?

Quoted 7 lines

Yeah, I suppose so -- the statx wouldn't necessitate any locking. In that case, maybe this is useless then other than for testing purposes and userland NFS servers.

Would it be better to not consume a statx field with this if so? What could we use as an alternate interface? ioctl? Some sort of global virtual xattr? It does need to be something per-inode.

I was thinking how hard would it be to increment i_version after updating data but it will be rather hairy. In particular because of stuff like IOCB_NOWAIT support which needs to bail if i_version update is needed. So yeah, I don't think there's an easy way how to provide useful i_version for general purpose use.

Quoted 38 lines · Jan Kara

Quoted 29 lines · Jeff Layton

Quoted 25 lines · J. Bruce Fields

Quoted 10 lines · Jeff Layton

Quoted 8 lines · NeilBrown

Review

Quoted 4 lines · Jeff Layton

(quoted diff) +4 -0

+The change to \fIstatx.stx_ino_version\fP is not atomic with respect to the
+other changes in the inode. On a write, for instance, the i_version it usually
+incremented before the data is copied into the pagecache. Therefore it is
+possible to see a new i_version value while a read still shows the old data.

Doesn't that make the value useless?

Quoted 3 lines

No, I don't think so. It's only really useful for comparing to an older sample anyway. If you do "statx; read; statx" and the value hasn't changed, then you know that things are stable.

I don't see how that helps. It's still possible to get:

		reader		writer
		------		------
				i_version++
		statx
		read
		statx
				update page cache

Quoted 2 lines

right?

Quoted 7 lines

Yeah, I suppose so -- the statx wouldn't necessitate any locking. In that case, maybe this is useless then other than for testing purposes and userland NFS servers.

Would it be better to not consume a statx field with this if so? What could we use as an alternate interface? ioctl? Some sort of global virtual xattr? It does need to be something per-inode.

Quoted 6 lines

I was thinking how hard would it be to increment i_version after updating data but it will be rather hairy. In particular because of stuff like IOCB_NOWAIT support which needs to bail if i_version update is needed. So yeah, I don't think there's an easy way how to provide useful i_version for general purpose use.

Why cannot IOCB_NOWAIT update i_version? Do we not want to wait on the cmp_xchg loop in inode_maybe_inc_iversion(), or do we not want to trigger an inode update?

The first seems unlikely, but the second seems unreasonable. We already acknowledge that after a crash iversion might go backwards and/or miss changes.

It boils down to the fact that we don't want to call mark_inode_dirty() from IOCB_NOWAIT path because for lots of filesystems that means journal operation and there are high chances that may block.

Presumably we could treat inode dirtying after i_version change similarly to how we handle timestamp updates with lazytime mount option (i.e., not dirty the inode immediately but only with a delay) but then the time window for i_version inconsistencies due to a crash would be much larger.

Quoted 8 lines · Jan Kara

It boils down to the fact that we don't want to call mark_inode_dirty() from IOCB_NOWAIT path because for lots of filesystems that means journal operation and there are high chances that may block.

Presumably we could treat inode dirtying after i_version change similarly to how we handle timestamp updates with lazytime mount option (i.e., not dirty the inode immediately but only with a delay) but then the time window for i_version inconsistencies due to a crash would be much larger.

Perhaps this is a radical suggestion, but there seems to be a lot of the problems which are due to the concern "what if the file system crashes" (and so we need to worry about making sure that any increments to i_version MUST be persisted after it is incremented).

Well, if we assume that unclean shutdowns are rare, then perhaps we shouldn't be optimizing for that case. So.... what if a file system had a counter which got incremented each time its journal is replayed representing an unclean shutdown. That shouldn't happen often, but if it does, there might be any number of i_version updates that may have gotten lost. So in that case, the NFS client should invalidate all of its caches.

If the i_version field was large enough, we could just prefix the "unclean shutdown counter" with the existing i_version number when it is sent over the NFS protocol to the client. But if that field is too small,

and if (as I understand things) NFS just needs to know when i_version is different, we could just simply hash the "unclean shtudown counter" with the inode's "i_version counter", and let that be the version which is sent from the NFS client to the server.

If we could do that, then it doesn't become critical that every single i_version bump has to be persisted to disk, and we could treat it like a lazytime update; it's guaranteed to updated when we do an clean unmount of the file system (and when the file system is frozen), but on a crash, there is no guaranteee that all i_version bumps will be persisted, but we do have this "unclean shutdown" counter to deal with that case.

Would this make life easier for folks?

Quoted 8 lines · Jan Kara

It boils down to the fact that we don't want to call mark_inode_dirty() from IOCB_NOWAIT path because for lots of filesystems that means journal operation and there are high chances that may block.

Presumably we could treat inode dirtying after i_version change similarly to how we handle timestamp updates with lazytime mount option (i.e., not dirty the inode immediately but only with a delay) but then the time window for i_version inconsistencies due to a crash would be much larger.

Perhaps this is a radical suggestion, but there seems to be a lot of the problems which are due to the concern "what if the file system crashes" (and so we need to worry about making sure that any increments to i_version MUST be persisted after it is incremented).

Well, if we assume that unclean shutdowns are rare, then perhaps we shouldn't be optimizing for that case. So.... what if a file system had a counter which got incremented each time its journal is replayed representing an unclean shutdown. That shouldn't happen often, but if it does, there might be any number of i_version updates that may have gotten lost. So in that case, the NFS client should invalidate all of its caches.

If the i_version field was large enough, we could just prefix the "unclean shutdown counter" with the existing i_version number when it is sent over the NFS protocol to the client. But if that field is too small, and if (as I understand things) NFS just needs to know when i_version is different, we could just simply hash the "unclean shtudown counter" with the inode's "i_version counter", and let that be the version which is sent from the NFS client to the server.

If we could do that, then it doesn't become critical that every single i_version bump has to be persisted to disk, and we could treat it like a lazytime update; it's guaranteed to updated when we do an clean unmount of the file system (and when the file system is frozen), but on a crash, there is no guaranteee that all i_version bumps will be persisted, but we do have this "unclean shutdown" counter to deal with that case.

Would this make life easier for folks?

- Ted

Quoted 39 lines · Theodore Ts'o

Quoted 8 lines · Jan Kara

It boils down to the fact that we don't want to call mark_inode_dirty() from IOCB_NOWAIT path because for lots of filesystems that means journal operation and there are high chances that may block.

Presumably we could treat inode dirtying after i_version change similarly to how we handle timestamp updates with lazytime mount option (i.e., not dirty the inode immediately but only with a delay) but then the time window for i_version inconsistencies due to a crash would be much larger.

Perhaps this is a radical suggestion, but there seems to be a lot of the problems which are due to the concern "what if the file system crashes" (and so we need to worry about making sure that any increments to i_version MUST be persisted after it is incremented).

Well, if we assume that unclean shutdowns are rare, then perhaps we shouldn't be optimizing for that case. So.... what if a file system had a counter which got incremented each time its journal is replayed representing an unclean shutdown. That shouldn't happen often, but if it does, there might be any number of i_version updates that may have gotten lost. So in that case, the NFS client should invalidate all of its caches.

If the i_version field was large enough, we could just prefix the "unclean shutdown counter" with the existing i_version number when it is sent over the NFS protocol to the client. But if that field is too small, and if (as I understand things) NFS just needs to know when i_version is different, we could just simply hash the "unclean shtudown counter" with the inode's "i_version counter", and let that be the version which is sent from the NFS client to the server.

If we could do that, then it doesn't become critical that every single i_version bump has to be persisted to disk, and we could treat it like a lazytime update; it's guaranteed to updated when we do an clean unmount of the file system (and when the file system is frozen), but on a crash, there is no guaranteee that all i_version bumps will be persisted, but we do have this "unclean shutdown" counter to deal with that case.

Quoted 3 lines

Would this make life easier for folks?

- Ted

Thanks for chiming in, Ted. That's part of the problem, but we're actually not too worried about that case:

nfsd mixes the ctime in with i_version, so you'd have to crash+clock jump backward by juuuust enough to allow you to get the i_version and ctime into a state it was before the crash, but with different data. We're assuming that that is difficult to achieve in practice.

The issue with a reboot counter (or similar) is that on an unclean crash the NFS client would end up invalidating every inode in the cache, as all of the i_versions would change. That's probably excessive.

The bigger issue (at the moment) is atomicity: when we fetch an i_version, the natural inclination is to associate that with the state of the inode at some point in time, so we need this to be updated atomically with certain other attributes of the inode. That's the part I'm trying to sort through at the moment.

Quoted 44 lines · Jeff Layton

Quoted 39 lines · Theodore Ts'o

Quoted 8 lines · Jan Kara

It boils down to the fact that we don't want to call mark_inode_dirty() from IOCB_NOWAIT path because for lots of filesystems that means journal operation and there are high chances that may block.

Presumably we could treat inode dirtying after i_version change similarly to how we handle timestamp updates with lazytime mount option (i.e., not dirty the inode immediately but only with a delay) but then the time window for i_version inconsistencies due to a crash would be much larger.

Perhaps this is a radical suggestion, but there seems to be a lot of the problems which are due to the concern "what if the file system crashes" (and so we need to worry about making sure that any increments to i_version MUST be persisted after it is incremented).

Well, if we assume that unclean shutdowns are rare, then perhaps we shouldn't be optimizing for that case. So.... what if a file system had a counter which got incremented each time its journal is replayed representing an unclean shutdown. That shouldn't happen often, but if it does, there might be any number of i_version updates that may have gotten lost. So in that case, the NFS client should invalidate all of its caches.

If the i_version field was large enough, we could just prefix the "unclean shutdown counter" with the existing i_version number when it is sent over the NFS protocol to the client. But if that field is too small, and if (as I understand things) NFS just needs to know when i_version is different, we could just simply hash the "unclean shtudown counter" with the inode's "i_version counter", and let that be the version which is sent from the NFS client to the server.

If we could do that, then it doesn't become critical that every single i_version bump has to be persisted to disk, and we could treat it like a lazytime update; it's guaranteed to updated when we do an clean unmount of the file system (and when the file system is frozen), but on a crash, there is no guaranteee that all i_version bumps will be persisted, but we do have this "unclean shutdown" counter to deal with that case.

Quoted 3 lines

Would this make life easier for folks?

- Ted

Quoted 7 lines

Thanks for chiming in, Ted. That's part of the problem, but we're actually not too worried about that case:

nfsd mixes the ctime in with i_version, so you'd have to crash+clock jump backward by juuuust enough to allow you to get the i_version and ctime into a state it was before the crash, but with different data. We're assuming that that is difficult to achieve in practice.

But a change in the clock could still cause our returned change attribute to go backwards (even without a crash). Not sure how to evaluate the risk, but it was enough that Trond hasn't been comfortable with nfsd advertising NFS4_CHANGE_TYPE_IS_MONOTONIC.

Ted's idea would be sufficient to allow us to turn that flag on, which I think allows some client-side optimizations.

Quoted 3 lines

The issue with a reboot counter (or similar) is that on an unclean crash the NFS client would end up invalidating every inode in the cache, as all of the i_versions would change. That's probably excessive.

But if we use the crash counter on write instead of read, we don't invalidate caches unnecessarily. And I think the monotonicity would still be close enough for our purposes?

Quoted 5 lines

The bigger issue (at the moment) is atomicity: when we fetch an i_version, the natural inclination is to associate that with the state of the inode at some point in time, so we need this to be updated atomically with certain other attributes of the inode. That's the part I'm trying to sort through at the moment.

That may be, but I still suspect the crash counter would help.

Quoted 44 lines · Jeff Layton

Quoted 39 lines · Theodore Ts'o

Quoted 8 lines · Jan Kara

It boils down to the fact that we don't want to call mark_inode_dirty() from IOCB_NOWAIT path because for lots of filesystems that means journal operation and there are high chances that may block.

Presumably we could treat inode dirtying after i_version change similarly to how we handle timestamp updates with lazytime mount option (i.e., not dirty the inode immediately but only with a delay) but then the time window for i_version inconsistencies due to a crash would be much larger.

Perhaps this is a radical suggestion, but there seems to be a lot of the problems which are due to the concern "what if the file system crashes" (and so we need to worry about making sure that any increments to i_version MUST be persisted after it is incremented).

Well, if we assume that unclean shutdowns are rare, then perhaps we shouldn't be optimizing for that case. So.... what if a file system had a counter which got incremented each time its journal is replayed representing an unclean shutdown. That shouldn't happen often, but if it does, there might be any number of i_version updates that may have gotten lost. So in that case, the NFS client should invalidate all of its caches.

If the i_version field was large enough, we could just prefix the "unclean shutdown counter" with the existing i_version number when it is sent over the NFS protocol to the client. But if that field is too small, and if (as I understand things) NFS just needs to know when i_version is different, we could just simply hash the "unclean shtudown counter" with the inode's "i_version counter", and let that be the version which is sent from the NFS client to the server.

If we could do that, then it doesn't become critical that every single i_version bump has to be persisted to disk, and we could treat it like a lazytime update; it's guaranteed to updated when we do an clean unmount of the file system (and when the file system is frozen), but on a crash, there is no guaranteee that all i_version bumps will be persisted, but we do have this "unclean shutdown" counter to deal with that case.

Quoted 3 lines

Would this make life easier for folks?

- Ted

Quoted 7 lines

Thanks for chiming in, Ted. That's part of the problem, but we're actually not too worried about that case:

nfsd mixes the ctime in with i_version, so you'd have to crash+clock jump backward by juuuust enough to allow you to get the i_version and ctime into a state it was before the crash, but with different data. We're assuming that that is difficult to achieve in practice.

But a change in the clock could still cause our returned change attribute to go backwards (even without a crash). Not sure how to evaluate the risk, but it was enough that Trond hasn't been comfortable with nfsd advertising NFS4_CHANGE_TYPE_IS_MONOTONIC.

Ted's idea would be sufficient to allow us to turn that flag on, which I think allows some client-side optimizations.

Quoted 3 lines

The issue with a reboot counter (or similar) is that on an unclean crash the NFS client would end up invalidating every inode in the cache, as all of the i_versions would change. That's probably excessive.

But if we use the crash counter on write instead of read, we don't invalidate caches unnecessarily. And I think the monotonicity would still be close enough for our purposes?

Quoted 5 lines

The bigger issue (at the moment) is atomicity: when we fetch an i_version, the natural inclination is to associate that with the state of the inode at some point in time, so we need this to be updated atomically with certain other attributes of the inode. That's the part I'm trying to sort through at the moment.

That may be, but I still suspect the crash counter would help.

Yeah, ok. That does make some sense. So we would mix this into the i_version instead of the ctime when it was available. Preferably, we'd mix that in when we store the i_version rather than adding it afterward.

Ted, how would we access this? Maybe we could just add a new (generic) super_block field for this that ext4 (and other filesystems) could populate at mount time?

Quoted 4 lines · Jeff Layton

Yeah, ok. That does make some sense. So we would mix this into the i_version instead of the ctime when it was available. Preferably, we'd mix that in when we store the i_version rather than adding it afterward.

Quoted 3 lines

Ted, how would we access this? Maybe we could just add a new (generic) super_block field for this that ext4 (and other filesystems) could populate at mount time?

Couldn't the filesystem just return an ino_version that already includes it?

Quoted 5 lines · J. Bruce Fields

Quoted 4 lines · Jeff Layton

Yeah, ok. That does make some sense. So we would mix this into the i_version instead of the ctime when it was available. Preferably, we'd mix that in when we store the i_version rather than adding it afterward.

Quoted 7 lines

Quoted 3 lines

Ted, how would we access this? Maybe we could just add a new (generic) super_block field for this that ext4 (and other filesystems) could populate at mount time?

Couldn't the filesystem just return an ino_version that already includes it?

Yes. That's simple if we want to just fold it in during getattr. If we want to fold that into the values stored on disk, then I'm a little less clear on how that will work.

Maybe I need a concrete example of how that will work:

Suppose we have an i_version value X with the previous crash counter already factored in that makes it to disk. We hand out a newer version X+1 to a client, but that value never makes it to disk.

The machine crashes and comes back up, and we get a query for i_version and it comes back as X. Fine, it's an old version. Now there is a write. What do we do to ensure that the new value doesn't collide with X+1?

Signature · 1 line

Jeff Layton <[email protected]>

Quoted 18 lines · Jeff Layton

Quoted 5 lines · J. Bruce Fields

Quoted 4 lines · Jeff Layton

Yeah, ok. That does make some sense. So we would mix this into the i_version instead of the ctime when it was available. Preferably, we'd mix that in when we store the i_version rather than adding it afterward.

Quoted 7 lines

Quoted 3 lines

Ted, how would we access this? Maybe we could just add a new (generic) super_block field for this that ext4 (and other filesystems) could populate at mount time?

Couldn't the filesystem just return an ino_version that already includes it?

Yes. That's simple if we want to just fold it in during getattr. If we want to fold that into the values stored on disk, then I'm a little less clear on how that will work.

Quoted 5 lines

Maybe I need a concrete example of how that will work:

Suppose we have an i_version value X with the previous crash counter already factored in that makes it to disk. We hand out a newer version X+1 to a client, but that value never makes it to disk.

As I understand it, the crash counter would NEVER appear in the on-disk i_version.
The crash counter is stable while a filesystem is mounted so is the same when loading an inode from disk and when writing it back.

When loading, add crash counter to on-disk i_version to provide in-memory i_version.
when storing, subtract crash counter from in-memory i_version to provide on-disk i_version.

"add" and "subtract" could be any reversible hash, and its inverse. I would probably shift the crash counter up 16 and add/subtract.

Quoted 7 lines

The machine crashes and comes back up, and we get a query for i_version and it comes back as X. Fine, it's an old version. Now there is a write. What do we do to ensure that the new value doesn't collide with X+1?

Signature · 1 line

Jeff Layton <[email protected]>

Quoted 30 lines · NeilBrown

Quoted 18 lines · Jeff Layton

Quoted 5 lines · J. Bruce Fields

Quoted 4 lines · Jeff Layton

Yeah, ok. That does make some sense. So we would mix this into the i_version instead of the ctime when it was available. Preferably, we'd mix that in when we store the i_version rather than adding it afterward.

Quoted 7 lines

Quoted 3 lines

Ted, how would we access this? Maybe we could just add a new (generic) super_block field for this that ext4 (and other filesystems) could populate at mount time?

Couldn't the filesystem just return an ino_version that already includes it?

Yes. That's simple if we want to just fold it in during getattr. If we want to fold that into the values stored on disk, then I'm a little less clear on how that will work.

Quoted 5 lines

Maybe I need a concrete example of how that will work:

Suppose we have an i_version value X with the previous crash counter already factored in that makes it to disk. We hand out a newer version X+1 to a client, but that value never makes it to disk.

As I understand it, the crash counter would NEVER appear in the on-disk i_version.
The crash counter is stable while a filesystem is mounted so is the same when loading an inode from disk and when writing it back.

Quoted 9 lines

When loading, add crash counter to on-disk i_version to provide in-memory i_version.
when storing, subtract crash counter from in-memory i_version to provide on-disk i_version.

"add" and "subtract" could be any reversible hash, and its inverse. I would probably shift the crash counter up 16 and add/subtract.

If you store the value with the crash counter already factored-in, then not every inode would end up being invalidated after a crash. If we try to mix it in later, the client will end up invalidating the cache even for inodes that had no changes.

Quoted 8 lines

Quoted 8 lines

The machine crashes and comes back up, and we get a query for i_version and it comes back as X. Fine, it's an old version. Now there is a write. What do we do to ensure that the new value doesn't collide with X+1?

Signature · 1 line

Jeff Layton <[email protected]>

Signature · 1 line

Jeff Layton <[email protected]>

Quoted 5 lines · J. Bruce Fields

Quoted 4 lines · Jeff Layton

Yeah, ok. That does make some sense. So we would mix this into the i_version instead of the ctime when it was available. Preferably, we'd mix that in when we store the i_version rather than adding it afterward.

Quoted 7 lines

Quoted 3 lines

Ted, how would we access this? Maybe we could just add a new (generic) super_block field for this that ext4 (and other filesystems) could populate at mount time?

Couldn't the filesystem just return an ino_version that already includes it?

Yes. That's simple if we want to just fold it in during getattr. If we want to fold that into the values stored on disk, then I'm a little less clear on how that will work.

Maybe I need a concrete example of how that will work:

Suppose we have an i_version value X with the previous crash counter already factored in that makes it to disk. We hand out a newer version X+1 to a client, but that value never makes it to disk.

The machine crashes and comes back up, and we get a query for i_version and it comes back as X. Fine, it's an old version. Now there is a write. What do we do to ensure that the new value doesn't collide with X+1?

Quoted 20 lines · Jeff Layton

Quoted 5 lines · J. Bruce Fields

Quoted 4 lines · Jeff Layton

Yeah, ok. That does make some sense. So we would mix this into the i_version instead of the ctime when it was available. Preferably, we'd mix that in when we store the i_version rather than adding it afterward.

Quoted 7 lines

Quoted 3 lines

Ted, how would we access this? Maybe we could just add a new (generic) super_block field for this that ext4 (and other filesystems) could populate at mount time?

Couldn't the filesystem just return an ino_version that already includes it?

Yes. That's simple if we want to just fold it in during getattr. If we want to fold that into the values stored on disk, then I'm a little less clear on how that will work.

Maybe I need a concrete example of how that will work:

Quoted 7 lines

Suppose we have an i_version value X with the previous crash counter already factored in that makes it to disk. We hand out a newer version X+1 to a client, but that value never makes it to disk.

The machine crashes and comes back up, and we get a query for i_version and it comes back as X. Fine, it's an old version. Now there is a write. What do we do to ensure that the new value doesn't collide with X+1?

I was assuming we could partition i_version's 64 bits somehow: e.g., top 16 bits store the crash counter. You increment the i_version by: 1) replacing the top bits by the new crash counter, if it has changed, and
2) incrementing.

Do the numbers work out? 2^16 mounts after unclean shutdowns sounds like a lot for one filesystem, as does 2^48 changes to a single file, but people do weird things. Maybe there's a better partitioning, or some more flexible way of maintaining an i_version that still allows you to identify whether a given i_version preceded a crash.

Quoted 34 lines · J. Bruce Fields

Quoted 20 lines · Jeff Layton

Quoted 5 lines · J. Bruce Fields

Quoted 4 lines · Jeff Layton

Yeah, ok. That does make some sense. So we would mix this into the i_version instead of the ctime when it was available. Preferably, we'd mix that in when we store the i_version rather than adding it afterward.

Quoted 7 lines

Quoted 3 lines

Ted, how would we access this? Maybe we could just add a new (generic) super_block field for this that ext4 (and other filesystems) could populate at mount time?

Couldn't the filesystem just return an ino_version that already includes it?

Yes. That's simple if we want to just fold it in during getattr. If we want to fold that into the values stored on disk, then I'm a little less clear on how that will work.

Maybe I need a concrete example of how that will work:

Quoted 7 lines

Suppose we have an i_version value X with the previous crash counter already factored in that makes it to disk. We hand out a newer version X+1 to a client, but that value never makes it to disk.

The machine crashes and comes back up, and we get a query for i_version and it comes back as X. Fine, it's an old version. Now there is a write. What do we do to ensure that the new value doesn't collide with X+1?

I was assuming we could partition i_version's 64 bits somehow: e.g., top 16 bits store the crash counter. You increment the i_version by: 1) replacing the top bits by the new crash counter, if it has changed, and
2) incrementing.

Quoted 6 lines

Do the numbers work out? 2^16 mounts after unclean shutdowns sounds like a lot for one filesystem, as does 2^48 changes to a single file, but people do weird things. Maybe there's a better partitioning, or some more flexible way of maintaining an i_version that still allows you to identify whether a given i_version preceded a crash.

We consume one bit to keep track of the "seen" flag, so it would be a 16+47 split. I assume that we'd also reset the version counter to 0 when the crash counter changes? Maybe that doesn't matter as long as we don't overflow into the crash counter.

I'm not sure we can get away with 16 bits for the crash counter, as it'll leave us subject to the version counter wrapping after a long uptimes.

If you increment a counter every nanosecond, how long until that counter wraps? With 63 bits, that's 292 years (and change). With 16+47 bits, that's less than two days. An 8+55 split would give us ~416 days which seems a bit more reasonable?

Quoted 47 lines · Jeff Layton

Quoted 34 lines · J. Bruce Fields

Quoted 20 lines · Jeff Layton

Quoted 5 lines · J. Bruce Fields

Quoted 4 lines · Jeff Layton

Yeah, ok. That does make some sense. So we would mix this into the i_version instead of the ctime when it was available. Preferably, we'd mix that in when we store the i_version rather than adding it afterward.

Quoted 7 lines

Quoted 3 lines

Ted, how would we access this? Maybe we could just add a new (generic) super_block field for this that ext4 (and other filesystems) could populate at mount time?

Couldn't the filesystem just return an ino_version that already includes it?

Yes. That's simple if we want to just fold it in during getattr. If we want to fold that into the values stored on disk, then I'm a little less clear on how that will work.

Maybe I need a concrete example of how that will work:

Quoted 7 lines

Suppose we have an i_version value X with the previous crash counter already factored in that makes it to disk. We hand out a newer version X+1 to a client, but that value never makes it to disk.

The machine crashes and comes back up, and we get a query for i_version and it comes back as X. Fine, it's an old version. Now there is a write. What do we do to ensure that the new value doesn't collide with X+1?

I was assuming we could partition i_version's 64 bits somehow: e.g., top 16 bits store the crash counter. You increment the i_version by: 1) replacing the top bits by the new crash counter, if it has changed, and
2) incrementing.

Quoted 6 lines

Do the numbers work out? 2^16 mounts after unclean shutdowns sounds like a lot for one filesystem, as does 2^48 changes to a single file, but people do weird things. Maybe there's a better partitioning, or some more flexible way of maintaining an i_version that still allows you to identify whether a given i_version preceded a crash.

We consume one bit to keep track of the "seen" flag, so it would be a 16+47 split. I assume that we'd also reset the version counter to 0 when the crash counter changes? Maybe that doesn't matter as long as we don't overflow into the crash counter.

Quoted 8 lines

I'm not sure we can get away with 16 bits for the crash counter, as it'll leave us subject to the version counter wrapping after a long uptimes.

If you increment a counter every nanosecond, how long until that counter wraps? With 63 bits, that's 292 years (and change). With 16+47 bits, that's less than two days. An 8+55 split would give us ~416 days which seems a bit more reasonable?

Though now it's starting to seem a little limiting to allow only 2^8 mounts after unclean shutdowns.

Another way to think of it might be: multiply that 8-bit crash counter by 2^48, and think of it as a 64-bit value that we believe (based on practical limits on how many times you can modify a single file) is gauranteed to be larger than any i_version that we gave out before the most recent crash.

Our goal is to ensure that after a crash, any *new* i_versions that we give out or write to disk are larger than any that have previously been given out. We can do that by ensuring that they're equal to at least that old maximum.

So think of the 64-bit value we're storing in the superblock as a ceiling on i_version values across all the filesystem's inodes. Call it s_version_max or something. We also need to know what the maximum was before the most recent crash. Call that s_version_max_old.

Then we could get correct behavior if we generated i_versions with something like:

	i_version++;
	if (i_version < s_version_max_old)
		i_version = s_version_max_old;
	if (i_version > s_version_max)
		s_version_max = i_version + 1;

But that last step makes this ludicrously expensive, because for this to be safe across crashes we need to update that value on disk as well, and we need to do that frequently.

Fortunately, s_version_max doesn't have to be a tight bound at all. We can easily just initialize it to, say, 2^40, and only bump it by 2^40 at a time. And recognize when we're running up against it way ahead of time, so we only need to say "here's an updated value, could you please make sure it gets to disk sometime in the next twenty minutes"? (Numbers made up.)

Sorry, that was way too many words. But I think something like that could work, and make it very difficult to hit any hard limits, and actually not be too complicated?? Unless I missed something.

To do this we'd need 2 64-bit fields in the on-disk and in-memory superblocks for ext4, xfs and btrfs. On the first mount after a crash, the filesystem would need to bump s_version_max by the significant increment (2^40 bits or whatever). On a "clean" mount, it wouldn't need to do that.

Would there be a way to ensure that the new s_version_max value has made it to disk? Bumping it by a large value and hoping for the best might be ok for most cases, but there are always outliers, so it might be worthwhile to make an i_version increment wait on that if necessary.

* Jeff Layton:

Quoted 10 lines

To do this we'd need 2 64-bit fields in the on-disk and in-memory superblocks for ext4, xfs and btrfs. On the first mount after a crash, the filesystem would need to bump s_version_max by the significant increment (2^40 bits or whatever). On a "clean" mount, it wouldn't need to do that.

Would there be a way to ensure that the new s_version_max value has made it to disk? Bumping it by a large value and hoping for the best might be ok for most cases, but there are always outliers, so it might be worthwhile to make an i_version increment wait on that if necessary.

How common are unclean shutdowns in practice? Do ex64/XFS/btrfs keep counters in the superblocks for journal replays that can be read easily?

Several useful i_version applications could be negatively impacted by frequent i_version invalidation.

Quoted 16 lines

* Jeff Layton:

Quoted 10 lines

To do this we'd need 2 64-bit fields in the on-disk and in-memory superblocks for ext4, xfs and btrfs. On the first mount after a crash, the filesystem would need to bump s_version_max by the significant increment (2^40 bits or whatever). On a "clean" mount, it wouldn't need to do that.

Would there be a way to ensure that the new s_version_max value has made it to disk? Bumping it by a large value and hoping for the best might be ok for most cases, but there are always outliers, so it might be worthwhile to make an i_version increment wait on that if necessary.

How common are unclean shutdowns in practice? Do ex64/XFS/btrfs keep counters in the superblocks for journal replays that can be read easily?

Quoted 3 lines

Several useful i_version applications could be negatively impacted by frequent i_version invalidation.

One would hope "not very often", but Oopses _are_ something that happens occasionally, even in very stable environments, and it would be best if what we're building can cope with them.

* Jeff Layton:

Quoted 17 lines · Florian Weimer

Quoted 16 lines

* Jeff Layton:

Quoted 10 lines

To do this we'd need 2 64-bit fields in the on-disk and in-memory superblocks for ext4, xfs and btrfs. On the first mount after a crash, the filesystem would need to bump s_version_max by the significant increment (2^40 bits or whatever). On a "clean" mount, it wouldn't need to do that.

Would there be a way to ensure that the new s_version_max value has made it to disk? Bumping it by a large value and hoping for the best might be ok for most cases, but there are always outliers, so it might be worthwhile to make an i_version increment wait on that if necessary.

How common are unclean shutdowns in practice? Do ex64/XFS/btrfs keep counters in the superblocks for journal replays that can be read easily?

Quoted 7 lines

Quoted 3 lines

Several useful i_version applications could be negatively impacted by frequent i_version invalidation.

One would hope "not very often", but Oopses _are_ something that happens occasionally, even in very stable environments, and it would be best if what we're building can cope with them.

I was wondering if such unclean shutdown events are associated with SSD “unsafe shutdowns”, as identified by the SMART counter. I think those aren't necessarily restricted to oopses or various forms of powerless (maybe depending on file system/devicemapper configuration)?

I admit it's possible that the file system is shut down cleanly before the kernel requests the power-off state from the firmware, but the underlying SSD is not.

Because of the "seen" flag, we have a 63 bit counter to play with. Could we use a similar scheme to the one we use to handle when "jiffies" wraps? Assume that we'd never compare two values that were more than 2^62 apart? We could add i_version_before/i_version_after macros to make it simple to handle this.

Quoted 5 lines · Jeff Layton

Because of the "seen" flag, we have a 63 bit counter to play with. Could we use a similar scheme to the one we use to handle when "jiffies" wraps? Assume that we'd never compare two values that were more than 2^62 apart? We could add i_version_before/i_version_after macros to make it simple to handle this.

As far as I recall the protocol just assumes it can never wrap. I guess you could add a new change_attr_type that works the way you describe. But without some new protocol clients aren't going to know what to do with a change attribute that wraps.

I think this just needs to be designed so that wrapping is impossible in any realistic scenario. I feel like that's doable?

If we feel we have to catch that case, the only 100% correct behavior would probably be to make the filesystem readonly.

Quoted 7 lines · J. Bruce Fields

Quoted 5 lines · Jeff Layton

Because of the "seen" flag, we have a 63 bit counter to play with. Could we use a similar scheme to the one we use to handle when "jiffies" wraps? Assume that we'd never compare two values that were more than 2^62 apart? We could add i_version_before/i_version_after macros to make it simple to handle this.

Quoted 5 lines

As far as I recall the protocol just assumes it can never wrap. I guess you could add a new change_attr_type that works the way you describe. But without some new protocol clients aren't going to know what to do with a change attribute that wraps.

Right, I think that's the case now, and with contemporary hardware that shouldn't ever happen, but in 10 years when we're looking at femtosecond latencies, could this be different? I don't know.

Quoted 6 lines

I think this just needs to be designed so that wrapping is impossible in any realistic scenario. I feel like that's doable?

If we feel we have to catch that case, the only 100% correct behavior would probably be to make the filesystem readonly.

What would be the recourse at that point? Rebuild the fs from scratch, I guess?

Quoted 8 lines · Jeff Layton

Because of the "seen" flag, we have a 63 bit counter to play with. Could
we use a similar scheme to the one we use to handle when "jiffies" wraps? Assume that we'd never compare two values that were more than
2^62 apart? We could add i_version_before/i_version_after macros to make
it simple to handle this.

As far as I recall the protocol just assumes it can never wrap.  I guess
you could add a new change_attr_type that works the way you describe. But without some new protocol clients aren't going to know what to do with a change attribute that wraps.

I think this just needs to be designed so that wrapping is impossible in
any realistic scenario.  I feel like that's doable?

If we feel we have to catch that case, the only 100% correct behavior would probably be to make the filesystem readonly.

Quoted 16 lines · J. Bruce Fields

Quoted 8 lines · Jeff Layton

Because of the "seen" flag, we have a 63 bit counter to play with. Could
we use a similar scheme to the one we use to handle when "jiffies" wraps? Assume that we'd never compare two values that were more than
2^62 apart? We could add i_version_before/i_version_after macros to make
it simple to handle this.

As far as I recall the protocol just assumes it can never wrap.  I guess
you could add a new change_attr_type that works the way you describe. But without some new protocol clients aren't going to know what to do with a change attribute that wraps.

Quoted 7 lines

I think this just needs to be designed so that wrapping is impossible in
any realistic scenario.  I feel like that's doable?

If we feel we have to catch that case, the only 100% correct behavior would probably be to make the filesystem readonly.

Which protocol? If you're talking about basic NFSv4, it doesn't assume anything about the change attribute and wrapping.

The NFSv4.2 protocol did introduce the optional attribute 'change_attr_type' that tries to describe the change attribute behaviour to the client. It tells you if the behaviour is monotonically increasing, but doesn't say anything about the behaviour when the attribute value overflows.

That said, the Linux NFSv4.2 client, which uses that change_attr_type attribute does deal with overflow by assuming standard uint64_t wrap around rules. i.e. it assumes bit values > 63 are truncated, meaning that the value obtained by incrementing (2^64-1) is 0.

Quoted 44 lines · Trond Myklebust

Quoted 22 lines · J. Bruce Fields

Quoted 11 lines · Jeff Layton

Because of the "seen" flag, we have a 63 bit counter to play with.
Could
we use a similar scheme to the one we use to handle when "jiffies"
wraps? Assume that we'd never compare two values that were more than
2^62 apart? We could add i_version_before/i_version_after macros to
make
it simple to handle this.

As far as I recall the protocol just assumes it can never wrap. I
guess
you could add a new change_attr_type that works the way you describe.
But without some new protocol clients aren't going to know what to do
with a change attribute that wraps.

Quoted 9 lines

I think this just needs to be designed so that wrapping is impossible
in
any realistic scenario.  I feel like that's doable?

If we feel we have to catch that case, the only 100% correct behavior
would probably be to make the filesystem readonly.

Which protocol? If you're talking about basic NFSv4, it doesn't assume
anything about the change attribute and wrapping.

The NFSv4.2 protocol did introduce the optional attribute 'change_attr_type' that tries to describe the change attribute behaviour to the client. It tells you if the behaviour is monotonically
increasing, but doesn't say anything about the behaviour when the attribute value overflows.

Quoted 7 lines

That said, the Linux NFSv4.2 client, which uses that change_attr_type
attribute does deal with overflow by assuming standard uint64_t wrap
around rules. i.e. it assumes bit values > 63 are truncated, meaning
that the value obtained by incrementing (2^64-1) is 0.

Yeah, it was the MONOTONIC_INCRE case I was thinking of.  That's interesting, I didn't know the client did that.

Quoted 55 lines · J. Bruce Fields

Quoted 46 lines · Trond Myklebust

Quoted 24 lines · J. Bruce Fields

Quoted 4 lines · Jeff Layton

Because of the "seen" flag, we have a 63 bit counter to play with.
Could
we use a similar scheme to the one we use to handle when

Quoted 8 lines

"jiffies"
wraps? Assume that we'd never compare two values that were more
than
2^62 apart? We could add i_version_before/i_version_after macros to
make
it simple to handle this.

As far as I recall the protocol just assumes it can never wrap.
I
guess
you could add a new change_attr_type that works the way you describe.
But without some new protocol clients aren't going to know what to do
with a change attribute that wraps.

Quoted 9 lines

I think this just needs to be designed so that wrapping is impossible
in
any realistic scenario.  I feel like that's doable?

If we feel we have to catch that case, the only 100% correct behavior
would probably be to make the filesystem readonly.

Which protocol? If you're talking about basic NFSv4, it doesn't assume
anything about the change attribute and wrapping.

The NFSv4.2 protocol did introduce the optional attribute 'change_attr_type' that tries to describe the change attribute behaviour to the client. It tells you if the behaviour is monotonically
increasing, but doesn't say anything about the behaviour when the attribute value overflows.

Quoted 7 lines

That said, the Linux NFSv4.2 client, which uses that change_attr_type
attribute does deal with overflow by assuming standard uint64_t wrap
around rules. i.e. it assumes bit values > 63 are truncated, meaning
that the value obtained by incrementing (2^64-1) is 0.

Quoted 3 lines

Yeah, it was the MONOTONIC_INCRE case I was thinking of.  That's interesting, I didn't know the client did that.

If you look at where we compare version numbers, it is always some variant of the following:

static int nfs_inode_attrs_cmp_monotonic(const struct nfs_fattr *fattr,
                                         const struct inode *inode) {
        s64 diff = fattr->change_attr - inode_peek_iversion_raw(inode);
        if (diff > 0)
                return 1;
        return diff == 0 ? 0 : -1;
}

i.e. we do an unsigned 64-bit subtraction, and then cast it to the signed 64-bit equivalent in order to figure out which is the more recent value.

Quoted 74 lines · Trond Myklebust

Quoted 55 lines · J. Bruce Fields

Quoted 46 lines · Trond Myklebust

Quoted 24 lines · J. Bruce Fields

Quoted 4 lines · Jeff Layton

Because of the "seen" flag, we have a 63 bit counter to play with.
Could
we use a similar scheme to the one we use to handle when

Quoted 8 lines

"jiffies"
wraps? Assume that we'd never compare two values that were more
than
2^62 apart? We could add i_version_before/i_version_after macros to
make
it simple to handle this.

As far as I recall the protocol just assumes it can never wrap.
I
guess
you could add a new change_attr_type that works the way you describe.
But without some new protocol clients aren't going to know what to do
with a change attribute that wraps.

Quoted 9 lines

I think this just needs to be designed so that wrapping is impossible
in
any realistic scenario.  I feel like that's doable?

If we feel we have to catch that case, the only 100% correct behavior
would probably be to make the filesystem readonly.

Which protocol? If you're talking about basic NFSv4, it doesn't assume
anything about the change attribute and wrapping.

The NFSv4.2 protocol did introduce the optional attribute 'change_attr_type' that tries to describe the change attribute behaviour to the client. It tells you if the behaviour is monotonically
increasing, but doesn't say anything about the behaviour when the attribute value overflows.

Quoted 7 lines

That said, the Linux NFSv4.2 client, which uses that change_attr_type
attribute does deal with overflow by assuming standard uint64_t wrap
around rules. i.e. it assumes bit values > 63 are truncated, meaning
that the value obtained by incrementing (2^64-1) is 0.

Quoted 3 lines

Yeah, it was the MONOTONIC_INCRE case I was thinking of.  That's interesting, I didn't know the client did that.

If you look at where we compare version numbers, it is always some variant of the following:

static int nfs_inode_attrs_cmp_monotonic(const struct nfs_fattr *fattr,
                                         const struct inode *inode) {
        s64 diff = fattr->change_attr - inode_peek_iversion_raw(inode);
        if (diff > 0)
                return 1;
        return diff == 0 ? 0 : -1;
}

Quoted 4 lines

i.e. we do an unsigned 64-bit subtraction, and then cast it to the signed 64-bit equivalent in order to figure out which is the more recent value.

...and by the way, yes this does mean that if you suddenly add a value of 2^63 to the change attribute, then you are likely to cause the client to think that you just handed it an old value.

i.e. you're better off having the crash counter increment the change attribute by a relatively small value. One that is guaranteed to be larger than the values that may have been lost, but that is not excessively large.

A scheme like that could work. It might be hard to do it without a spinlock or something, but maybe that's ok. Thinking more about how we'd implement this in the underlying filesystems:

To do this we'd need 2 64-bit fields in the on-disk and in-memory superblocks for ext4, xfs and btrfs. On the first mount after a crash, the filesystem would need to bump s_version_max by the significant increment (2^40 bits or whatever). On a "clean" mount, it wouldn't need to do that.

Would there be a way to ensure that the new s_version_max value has made it to disk? Bumping it by a large value and hoping for the best might be ok for most cases, but there are always outliers, so it might be worthwhile to make an i_version increment wait on that if necessary.

Quoted 10 lines · Jeff Layton

A scheme like that could work. It might be hard to do it without a spinlock or something, but maybe that's ok. Thinking more about how we'd implement this in the underlying filesystems:

To do this we'd need 2 64-bit fields in the on-disk and in-memory superblocks for ext4, xfs and btrfs. On the first mount after a crash, the filesystem would need to bump s_version_max by the significant increment (2^40 bits or whatever). On a "clean" mount, it wouldn't need to do that.

Quoted 4 lines

Would there be a way to ensure that the new s_version_max value has made it to disk? Bumping it by a large value and hoping for the best might be ok for most cases, but there are always outliers, so it might be worthwhile to make an i_version increment wait on that if necessary.

I was imagining that when you recognize you're getting close, you kick off something which writes s_version_max+2^40 to disk, and then updates s_version_max to that new value on success of the write.

The code that increments i_version checks to make sure it wouldn't exceed s_version_max. If it would, something has gone wrong--a write has failed or taken a long time--so it waits or errors out or something, depending on desired filesystem behavior in that case.

No locking required in the normal case?

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Jeff" == Jeff Layton <[email protected]> writes:

Quoted 96 lines · J. Bruce Fields

Quoted 47 lines · Jeff Layton

Quoted 34 lines · J. Bruce Fields

Quoted 20 lines · Jeff Layton

Quoted 5 lines · J. Bruce Fields

Quoted 4 lines · Jeff Layton

Yeah, ok. That does make some sense. So we would mix this into the i_version instead of the ctime when it was available. Preferably, we'd mix that in when we store the i_version rather than adding it afterward.

Quoted 7 lines

Quoted 3 lines

Ted, how would we access this? Maybe we could just add a new (generic) super_block field for this that ext4 (and other filesystems) could populate at mount time?

Couldn't the filesystem just return an ino_version that already includes it?

Yes. That's simple if we want to just fold it in during getattr. If we want to fold that into the values stored on disk, then I'm a little less clear on how that will work.

Maybe I need a concrete example of how that will work:

Quoted 7 lines

Suppose we have an i_version value X with the previous crash counter already factored in that makes it to disk. We hand out a newer version X+1 to a client, but that value never makes it to disk.

The machine crashes and comes back up, and we get a query for i_version and it comes back as X. Fine, it's an old version. Now there is a write. What do we do to ensure that the new value doesn't collide with X+1?

I was assuming we could partition i_version's 64 bits somehow: e.g., top 16 bits store the crash counter. You increment the i_version by: 1) replacing the top bits by the new crash counter, if it has changed, and
2) incrementing.

Quoted 6 lines

Do the numbers work out? 2^16 mounts after unclean shutdowns sounds like a lot for one filesystem, as does 2^48 changes to a single file, but people do weird things. Maybe there's a better partitioning, or some more flexible way of maintaining an i_version that still allows you to identify whether a given i_version preceded a crash.

We consume one bit to keep track of the "seen" flag, so it would be a 16+47 split. I assume that we'd also reset the version counter to 0 when the crash counter changes? Maybe that doesn't matter as long as we don't overflow into the crash counter.

Quoted 8 lines

I'm not sure we can get away with 16 bits for the crash counter, as it'll leave us subject to the version counter wrapping after a long uptimes.

If you increment a counter every nanosecond, how long until that counter wraps? With 63 bits, that's 292 years (and change). With 16+47 bits, that's less than two days. An 8+55 split would give us ~416 days which seems a bit more reasonable?

Though now it's starting to seem a little limiting to allow only 2^8 mounts after unclean shutdowns.

Another way to think of it might be: multiply that 8-bit crash counter by 2^48, and think of it as a 64-bit value that we believe (based on practical limits on how many times you can modify a single file) is gauranteed to be larger than any i_version that we gave out before the most recent crash.

Our goal is to ensure that after a crash, any *new* i_versions that we give out or write to disk are larger than any that have previously been given out. We can do that by ensuring that they're equal to at least that old maximum.

So think of the 64-bit value we're storing in the superblock as a ceiling on i_version values across all the filesystem's inodes. Call it s_version_max or something. We also need to know what the maximum was before the most recent crash. Call that s_version_max_old.

Then we could get correct behavior if we generated i_versions with something like:

i_version++;
if (i_version < s_version_max_old)
i_version = s_version_max_old;
if (i_version > s_version_max)
s_version_max = i_version + 1;

But that last step makes this ludicrously expensive, because for this to be safe across crashes we need to update that value on disk as well, and we need to do that frequently.

Fortunately, s_version_max doesn't have to be a tight bound at all. We can easily just initialize it to, say, 2^40, and only bump it by 2^40 at a time. And recognize when we're running up against it way ahead of time, so we only need to say "here's an updated value, could you please make sure it gets to disk sometime in the next twenty minutes"? (Numbers made up.)

Quoted 4 lines

Sorry, that was way too many words. But I think something like that could work, and make it very difficult to hit any hard limits, and actually not be too complicated?? Unless I missed something.

That's not too many words -- I appreciate a good "for dummies" explanation!

A scheme like that could work. It might be hard to do it without a spinlock or something, but maybe that's ok. Thinking more about how we'd implement this in the underlying filesystems:

To do this we'd need 2 64-bit fields in the on-disk and in-memory superblocks for ext4, xfs and btrfs. On the first mount after a crash, the filesystem would need to bump s_version_max by the significant increment (2^40 bits or whatever). On a "clean" mount, it wouldn't need to do that.

Would there be a way to ensure that the new s_version_max value has made it to disk? Bumping it by a large value and hoping for the best might be ok for most cases, but there are always outliers, so it might be worthwhile to make an i_version increment wait on that if necessary.

Quoted 31 lines · J. Bruce Fields

On Fri, Sep 09, 2022 at 12:36:29PM -0400, Jeff Layton wrote:
Our goal is to ensure that after a crash, any *new* i_versions that we give out or write to disk are larger than any that have previously been given out. We can do that by ensuring that they're equal to at least that old maximum.

So think of the 64-bit value we're storing in the superblock as a ceiling on i_version values across all the filesystem's inodes. Call it s_version_max or something. We also need to know what the maximum was before the most recent crash. Call that s_version_max_old.

Then we could get correct behavior if we generated i_versions with something like:

	i_version++;
	if (i_version < s_version_max_old)
		i_version = s_version_max_old;
	if (i_version > s_version_max)
		s_version_max = i_version + 1;

But that last step makes this ludicrously expensive, because for this to be safe across crashes we need to update that value on disk as well, and we need to do that frequently.

Fortunately, s_version_max doesn't have to be a tight bound at all. We can easily just initialize it to, say, 2^40, and only bump it by 2^40 at a time. And recognize when we're running up against it way ahead of time, so we only need to say "here's an updated value, could you please make sure it gets to disk sometime in the next twenty minutes"? (Numbers made up.)

Quoted 4 lines

Sorry, that was way too many words. But I think something like that could work, and make it very difficult to hit any hard limits, and actually not be too complicated?? Unless I missed something.

That's not too many words -- I appreciate a good "for dummies" explanation!

A scheme like that could work. It might be hard to do it without a spinlock or something, but maybe that's ok. Thinking more about how we'd implement this in the underlying filesystems:

To do this we'd need 2 64-bit fields in the on-disk and in-memory superblocks for ext4, xfs and btrfs. On the first mount after a crash, the filesystem would need to bump s_version_max by the significant increment (2^40 bits or whatever). On a "clean" mount, it wouldn't need to do that.

Would there be a way to ensure that the new s_version_max value has made it to disk?

Bumping it by a large value and hoping for the best might be ok for most cases, but there are always outliers, so it might be worthwhile to make an i_version increment wait on that if necessary.

Quoted 44 lines · Jeff Layton

Quoted 31 lines · J. Bruce Fields

On Fri, Sep 09, 2022 at 12:36:29PM -0400, Jeff Layton wrote:
Our goal is to ensure that after a crash, any *new* i_versions that we give out or write to disk are larger than any that have previously been given out. We can do that by ensuring that they're equal to at least that old maximum.

So think of the 64-bit value we're storing in the superblock as a ceiling on i_version values across all the filesystem's inodes. Call it s_version_max or something. We also need to know what the maximum was before the most recent crash. Call that s_version_max_old.

Then we could get correct behavior if we generated i_versions with something like:

	i_version++;
	if (i_version < s_version_max_old)
		i_version = s_version_max_old;
	if (i_version > s_version_max)
		s_version_max = i_version + 1;

But that last step makes this ludicrously expensive, because for this to be safe across crashes we need to update that value on disk as well, and we need to do that frequently.

Fortunately, s_version_max doesn't have to be a tight bound at all. We can easily just initialize it to, say, 2^40, and only bump it by 2^40 at a time. And recognize when we're running up against it way ahead of time, so we only need to say "here's an updated value, could you please make sure it gets to disk sometime in the next twenty minutes"? (Numbers made up.)

Quoted 4 lines

Sorry, that was way too many words. But I think something like that could work, and make it very difficult to hit any hard limits, and actually not be too complicated?? Unless I missed something.

That's not too many words -- I appreciate a good "for dummies" explanation!

A scheme like that could work. It might be hard to do it without a spinlock or something, but maybe that's ok. Thinking more about how we'd implement this in the underlying filesystems:

Quoted 5 lines

To do this we'd need 2 64-bit fields in the on-disk and in-memory superblocks for ext4, xfs and btrfs. On the first mount after a crash, the filesystem would need to bump s_version_max by the significant increment (2^40 bits or whatever). On a "clean" mount, it wouldn't need to do that.

Why only increment on crash? If the filesystem has been unmounted, then any cached data is -stale- and must be discarded. e.g. unmount, run fsck which cleans up corrupt files but does not modify i_version, then mount. Remote caches are now invalid, but i_version may not have changed, so we still need the clean unmount-mount cycle to invalidate caches.

IOWs, what we want is a salted i_version value, with the filesystem providing the unique per-mount salt that gets added to the externally visible i_version values.

If that's the case, the salt doesn't need to be restricted to just modifying the upper bits - as long as the salt increments substantially and independently to the on-disk inode i_version then we just don't care what bits of the superblock salt change from mount to mount.

For XFS we already have a unique 64 bit salt we could use for every mount - clean or unclean - and guarantee it is larger for every mount. It also gets substantially bumped by fsck, too. It's called a Log Sequence Number and we use them to track and strictly order every modification we write into the log. This is exactly what is needed for a i_version salt, and it's already guaranteed to be persistent.

Quoted 3 lines

Would there be a way to ensure that the new s_version_max value has made it to disk?

Yes, but that's not really relevant to the definition of the salt:
we don't need to design the filesystem implementation of a persistent per-mount salt value. All we need is to define the behaviour of the salt (e.g. must always increase across a umount/mount cycle) and then you can let the filesystem developers worry about how to provide the required salt behaviour and it's persistence.

In the mean time, you can implement the salting and testing it by using the system time to seed the superblock salt - that's good enough for proof of concept, and as a fallback for filesystems that cannot provide the required per-mount salt persistence....

Quoted 3 lines

Bumping it by a large value and hoping for the best might be ok for most cases, but there are always outliers, so it might be worthwhile to make an i_version increment wait on that if necessary.

Nothing should be able to query i_version until the filesystem is fully recovered, mounted and the salt has been set. Hence no application (kernel or userspace) should ever see an unsalted i_version value....

-Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Quoted 51 lines · Dave Chinner

Quoted 44 lines · Jeff Layton

Quoted 31 lines · J. Bruce Fields

On Fri, Sep 09, 2022 at 12:36:29PM -0400, Jeff Layton wrote:
Our goal is to ensure that after a crash, any *new* i_versions that we give out or write to disk are larger than any that have previously been given out. We can do that by ensuring that they're equal to at least that old maximum.

So think of the 64-bit value we're storing in the superblock as a ceiling on i_version values across all the filesystem's inodes. Call it s_version_max or something. We also need to know what the maximum was before the most recent crash. Call that s_version_max_old.

Then we could get correct behavior if we generated i_versions with something like:

	i_version++;
	if (i_version < s_version_max_old)
		i_version = s_version_max_old;
	if (i_version > s_version_max)
		s_version_max = i_version + 1;

But that last step makes this ludicrously expensive, because for this to be safe across crashes we need to update that value on disk as well, and we need to do that frequently.

Fortunately, s_version_max doesn't have to be a tight bound at all. We can easily just initialize it to, say, 2^40, and only bump it by 2^40 at a time. And recognize when we're running up against it way ahead of time, so we only need to say "here's an updated value, could you please make sure it gets to disk sometime in the next twenty minutes"? (Numbers made up.)

Quoted 4 lines

Sorry, that was way too many words. But I think something like that could work, and make it very difficult to hit any hard limits, and actually not be too complicated?? Unless I missed something.

That's not too many words -- I appreciate a good "for dummies" explanation!

A scheme like that could work. It might be hard to do it without a spinlock or something, but maybe that's ok. Thinking more about how we'd implement this in the underlying filesystems:

Quoted 5 lines

To do this we'd need 2 64-bit fields in the on-disk and in-memory superblocks for ext4, xfs and btrfs. On the first mount after a crash, the filesystem would need to bump s_version_max by the significant increment (2^40 bits or whatever). On a "clean" mount, it wouldn't need to do that.

Quoted 6 lines

Why only increment on crash? If the filesystem has been unmounted, then any cached data is -stale- and must be discarded. e.g. unmount, run fsck which cleans up corrupt files but does not modify i_version, then mount. Remote caches are now invalid, but i_version may not have changed, so we still need the clean unmount-mount cycle to invalidate caches.

I disagree. We do need fsck to cause caches to be invalidated IF IT FOUND SOMETHING TO REPAIR, but not if the filesystem was truely clean.

Quoted 3 lines

IOWs, what we want is a salted i_version value, with the filesystem providing the unique per-mount salt that gets added to the externally visible i_version values.

I agree this is a simple approach. Possible the best.

Quoted 7 lines

If that's the case, the salt doesn't need to be restricted to just modifying the upper bits - as long as the salt increments substantially and independently to the on-disk inode i_version then we just don't care what bits of the superblock salt change from mount to mount.

Quoted 7 lines

For XFS we already have a unique 64 bit salt we could use for every mount - clean or unclean - and guarantee it is larger for every mount. It also gets substantially bumped by fsck, too. It's called a Log Sequence Number and we use them to track and strictly order every modification we write into the log. This is exactly what is needed for a i_version salt, and it's already guaranteed to be persistent.

Invalidating the client cache on EVERY unmount/mount could impose unnecessary cost. Imagine a client that caches a lot of data (several large files) from a server which is expected to fail-over from one cluster node to another from time to time. Adding extra delays to a fail-over is not likely to be well received.

Indeed, we know there are many systems out there that mount a filesystem, preallocate and map the blocks that are allocated to a large file, unmount the filesysetm, mmap the ranges of the block device and pass them to RDMA hardware, then have sensor arrays rdma data directly into the block device. Then when the measurement application is done they walk the ondisk metadata to remove the unwritten flags on the extents, mount the filesystem again and export the file data to a HPC cluster for post-processing.....

So how does the filesystem know whether data the storage contains for it's files has been modified while it is unmounted and so needs to change the salt?

The short answer is that it can't, and so we cannot make assumptions that a unmount/mount cycle has not changed the filesystem in any way....

Quoted 6 lines · Dave Chinner

Indeed, we know there are many systems out there that mount a filesystem, preallocate and map the blocks that are allocated to a large file, unmount the filesysetm, mmap the ranges of the block device and pass them to RDMA hardware, then have sensor arrays rdma data directly into the block device.....

And this tool doesn't update the i_version? Sounds like a bug.

Invalidating the client cache on EVERY unmount/mount could impose unnecessary cost. Imagine a client that caches a lot of data (several large files) from a server which is expected to fail-over from one cluster node to another from time to time. Adding extra delays to a fail-over is not likely to be well received.

I don't *know* this cost would be unacceptable, and I *would* like to leave it to the filesystem to decide how to manage its own i_version values. So maybe XFS can use the LSN for a salt. If people notice the extra cost, they can complain.

Quoted 10 lines · NeilBrown

Invalidating the client cache on EVERY unmount/mount could impose unnecessary cost. Imagine a client that caches a lot of data (several large files) from a server which is expected to fail-over from one cluster node to another from time to time. Adding extra delays to a fail-over is not likely to be well received.

I don't *know* this cost would be unacceptable, and I *would* like to leave it to the filesystem to decide how to manage its own i_version values. So maybe XFS can use the LSN for a salt. If people notice the extra cost, they can complain.

I'd expect complaints.

NFS is actually even worse than this: it allows clients to reacquire file locks across server restart and unmount/remount, even though obviously the kernel will do nothing to prevent someone else from locking (or modifying) the file in between.

Administrators are just supposed to know not to allow other applications access to the filesystem until nfsd's started. It's always been this way.

You can imagine all sorts of measures to prevent that, and if anyone wants to work on ways to prevent people from shooting themselves in the foot here, great.

Just taking away the ability to cache or lock across reboots wouldn't make people happy, though....

--b.

Quoted 14 lines · J. Bruce Fields

Quoted 10 lines · NeilBrown

Invalidating the client cache on EVERY unmount/mount could impose unnecessary cost. Imagine a client that caches a lot of data (several large files) from a server which is expected to fail-over from one cluster node to another from time to time. Adding extra delays to a fail-over is not likely to be well received.

I don't *know* this cost would be unacceptable, and I *would* like to leave it to the filesystem to decide how to manage its own i_version values. So maybe XFS can use the LSN for a salt. If people notice the extra cost, they can complain.

I'd expect complaints.

Quoted 4 lines

NFS is actually even worse than this: it allows clients to reacquire file locks across server restart and unmount/remount, even though obviously the kernel will do nothing to prevent someone else from locking (or modifying) the file in between.

I don't understand this comment. You seem to be implying that changing the i_version during a server restart would stop a client from reclaiming locks. Is that correct?

I would have thought that the client would largely ignore i_version while it has a lock or open or delegation, as these tend to imply some degree of exclusive access ("open" being least exclusive).

Thanks,
NeilBrown

Quoted 14 lines

Administrators are just supposed to know not to allow other applications access to the filesystem until nfsd's started. It's always been this way.

You can imagine all sorts of measures to prevent that, and if anyone wants to work on ways to prevent people from shooting themselves in the foot here, great.

Just taking away the ability to cache or lock across reboots wouldn't make people happy, though....

--b.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Jeff" == Jeff Layton <[email protected]> writes:

Quoted 9 lines · J. Bruce Fields

Quoted 7 lines · Jeff Layton

Yeah, ok. That does make some sense. So we would mix this into the i_version instead of the ctime when it was available. Preferably, we'd mix that in when we store the i_version rather than adding it afterward.

Ted, how would we access this? Maybe we could just add a new (generic) super_block field for this that ext4 (and other filesystems) could populate at mount time?

Quoted 3 lines

Couldn't the filesystem just return an ino_version that already includes it?

Yes. That's simple if we want to just fold it in during getattr. If we want to fold that into the values stored on disk, then I'm a little less clear on how that will work.

The machine crashes and comes back up, and we get a query for i_version and it comes back as X. Fine, it's an old version. Now there is a write. What do we do to ensure that the new value doesn't collide with X+1?

Quoted 4 lines · Jeff Layton

The machine crashes and comes back up, and we get a query for i_version and it comes back as X. Fine, it's an old version. Now there is a write. What do we do to ensure that the new value doesn't collide with X+1?

(I missed this bit in my earlier reply..)

How is it "Fine" to see an old version?
The file could have changed without the version changing. And I thought one of the goals of the crash-count was to be able to provide a monotonic change id.

Quoted 4 lines · Jeff Layton

The machine crashes and comes back up, and we get a query for i_version and it comes back as X. Fine, it's an old version. Now there is a write. What do we do to ensure that the new value doesn't collide with X+1?

(I missed this bit in my earlier reply..)

How is it "Fine" to see an old version?
The file could have changed without the version changing. And I thought one of the goals of the crash-count was to be able to provide a monotonic change id.

Quoted 6 lines · NeilBrown

Quoted 4 lines · Jeff Layton

The machine crashes and comes back up, and we get a query for i_version and it comes back as X. Fine, it's an old version. Now there is a write. What do we do to ensure that the new value doesn't collide with X+1?

Quoted 6 lines

(I missed this bit in my earlier reply..)

How is it "Fine" to see an old version?
The file could have changed without the version changing. And I thought one of the goals of the crash-count was to be able to provide a monotonic change id.

I was still mainly thinking about how to provide reliable close-to-open semantics between NFS clients. In the case the writer was an NFS client, it wasn't done writing (or it would have COMMITted), so those writes will come in and bump the change attribute soon, and as long as we avoid the small chance of reusing an old change attribute, we're OK, and I think it'd even still be OK to advertise CHANGE_TYPE_IS_MONOTONIC_INCR.

If we're trying to do better than that, I'm just not sure what's right.

Quoted 14 lines · J. Bruce Fields

Quoted 6 lines · NeilBrown

Quoted 4 lines · Jeff Layton

The machine crashes and comes back up, and we get a query for i_version and it comes back as X. Fine, it's an old version. Now there is a write. What do we do to ensure that the new value doesn't collide with X+1?

Quoted 6 lines

(I missed this bit in my earlier reply..)

How is it "Fine" to see an old version?
The file could have changed without the version changing. And I thought one of the goals of the crash-count was to be able to provide a monotonic change id.

Quoted 7 lines

I was still mainly thinking about how to provide reliable close-to-open semantics between NFS clients. In the case the writer was an NFS client, it wasn't done writing (or it would have COMMITted), so those writes will come in and bump the change attribute soon, and as long as we avoid the small chance of reusing an old change attribute, we're OK, and I think it'd even still be OK to advertise CHANGE_TYPE_IS_MONOTONIC_INCR.

You seem to be assuming that the client doesn't crash at the same time as the server (maybe they are both VMs on a host that lost power...)

If client A reads and caches, client B writes, the server crashes after writing some data (to already allocated space so no inode update needed) but before writing the new i_version, then client B crashes. When server comes back the i_version will be unchanged but the data has changed. Client A will cache old data indefinitely...

I think we need to require the filesystem to ensure that the i_version is seen to increase shortly after any change becomes visible in the file, and no later than the moment when the request that initiated the change is acknowledged as being complete. In the case of an unclean restart, any file that is not known to have been unchanged immediately before the crash must have i_version increased.

The simplest implementation is to have an unclean-restart counter and to always included this multiplied by some constant X in the reported i_version. The filesystem guarantees to record (e.g. to journal at least) the i_version if it comes close to X more than the previous record. The filesystem gets to choose X.

A more complex solution would be to record (similar to the way orphans are recorded) any file which is open for write, and to add X to the i_version for any "dirty" file still recorded during an unclean restart. This would avoid bumping the i_version for read-only files.

There may be other solutions, but we should leave that up to the filesystem. Each filesystem might choose something different.

Quoted 36 lines · NeilBrown

Quoted 18 lines · J. Bruce Fields

Quoted 9 lines · NeilBrown

Quoted 7 lines · Jeff Layton

The machine crashes and comes back up, and we get a query for i_version
and it comes back as X. Fine, it's an old version. Now there is a write.
What do we do to ensure that the new value doesn't collide with X+1?

Quoted 7 lines

(I missed this bit in my earlier reply..)

How is it "Fine" to see an old version?
The file could have changed without the version changing. And I thought one of the goals of the crash-count was to be able to
provide a monotonic change id.

Quoted 11 lines

I was still mainly thinking about how to provide reliable close- to-open
semantics between NFS clients.  In the case the writer was an NFS client, it wasn't done writing (or it would have COMMITted), so those
writes will come in and bump the change attribute soon, and as long as
we avoid the small chance of reusing an old change attribute, we're OK,
and I think it'd even still be OK to advertise CHANGE_TYPE_IS_MONOTONIC_INCR.

You seem to be assuming that the client doesn't crash at the same time
as the server (maybe they are both VMs on a host that lost power...)

Quoted 8 lines

If client A reads and caches, client B writes, the server crashes after
writing some data (to already allocated space so no inode update needed)
but before writing the new i_version, then client B crashes. When server comes back the i_version will be unchanged but the data has
changed.  Client A will cache old data indefinitely...

I guess I assume that if all we're promising is close-to-open, then a client isn't allowed to trust its cache in that situation.  Maybe that's
an overly draconian interpretation of close-to-open.

Also, I'm trying to think about how to improve things incrementally. Incorporating something like a crash count into the on-disk i_version fixes some cases without introducing any new ones or regressing performance after a crash.

If we subsequently wanted to close those remaining holes, I think we'd
need the change attribute increment to be seen as atomic with respect to
its associated change, both to clients and (separately) on disk. (That
would still allow the change attribute to go backwards after a crash, to
the value it held as of the on-disk state of the file.  I think clients
should be able to deal with that case.)

But, I don't know, maybe a bigger hammer would be OK:

Quoted 68 lines · J. Bruce Fields

Quoted 36 lines · NeilBrown

Quoted 18 lines · J. Bruce Fields

Quoted 9 lines · NeilBrown

Quoted 7 lines · Jeff Layton

The machine crashes and comes back up, and we get a query for i_version
and it comes back as X. Fine, it's an old version. Now there is a write.
What do we do to ensure that the new value doesn't collide with X+1?

Quoted 7 lines

(I missed this bit in my earlier reply..)

How is it "Fine" to see an old version?
The file could have changed without the version changing. And I thought one of the goals of the crash-count was to be able to
provide a monotonic change id.

Quoted 11 lines

I was still mainly thinking about how to provide reliable close- to-open
semantics between NFS clients.  In the case the writer was an NFS client, it wasn't done writing (or it would have COMMITted), so those
writes will come in and bump the change attribute soon, and as long as
we avoid the small chance of reusing an old change attribute, we're OK,
and I think it'd even still be OK to advertise CHANGE_TYPE_IS_MONOTONIC_INCR.

You seem to be assuming that the client doesn't crash at the same time
as the server (maybe they are both VMs on a host that lost power...)

Quoted 8 lines

If client A reads and caches, client B writes, the server crashes after
writing some data (to already allocated space so no inode update needed)
but before writing the new i_version, then client B crashes. When server comes back the i_version will be unchanged but the data has
changed.  Client A will cache old data indefinitely...

I guess I assume that if all we're promising is close-to-open, then a client isn't allowed to trust its cache in that situation.  Maybe that's
an overly draconian interpretation of close-to-open.

Also, I'm trying to think about how to improve things incrementally. Incorporating something like a crash count into the on-disk i_version fixes some cases without introducing any new ones or regressing performance after a crash.

If we subsequently wanted to close those remaining holes, I think we'd
need the change attribute increment to be seen as atomic with respect to
its associated change, both to clients and (separately) on disk. (That
would still allow the change attribute to go backwards after a crash, to
the value it held as of the on-disk state of the file.  I think clients
should be able to deal with that case.)

Quoted 2 lines

But, I don't know, maybe a bigger hammer would be OK:

If you're not going to meet the minimum bar of data integrity, then this whole exercise is just a massive waste of everyone's time. The answer then going forward is just to recommend never using Linux as an NFS server. Makes my life much easier, because I no longer have to debug any of the issues.

Quoted 88 lines · Trond Myklebust

Quoted 84 lines · J. Bruce Fields

Quoted 44 lines · NeilBrown

Quoted 21 lines · J. Bruce Fields

Quoted 12 lines · NeilBrown

Quoted 10 lines · Jeff Layton

The machine crashes and comes back up, and we get a query for
i_version
and it comes back as X. Fine, it's an old version. Now there
is a write.
What do we do to ensure that the new value doesn't collide
with X+1?

Quoted 7 lines

(I missed this bit in my earlier reply..)

How is it "Fine" to see an old version?
The file could have changed without the version changing. And I thought one of the goals of the crash-count was to be able to
provide a monotonic change id.

Quoted 15 lines

I was still mainly thinking about how to provide reliable close-
to-open
semantics between NFS clients.  In the case the writer was an NFS
client, it wasn't done writing (or it would have COMMITted), so
those
writes will come in and bump the change attribute soon, and as
long as
we avoid the small chance of reusing an old change attribute, we're OK,
and I think it'd even still be OK to advertise CHANGE_TYPE_IS_MONOTONIC_INCR.

You seem to be assuming that the client doesn't crash at the same
time
as the server (maybe they are both VMs on a host that lost power...)

Quoted 11 lines

If client A reads and caches, client B writes, the server crashes
after
writing some data (to already allocated space so no inode update
needed)
but before writing the new i_version, then client B crashes. When server comes back the i_version will be unchanged but the data
has
changed.  Client A will cache old data indefinitely...

I guess I assume that if all we're promising is close-to-open, then a
client isn't allowed to trust its cache in that situation.  Maybe that's
an overly draconian interpretation of close-to-open.

Also, I'm trying to think about how to improve things incrementally.
Incorporating something like a crash count into the on-disk i_version
fixes some cases without introducing any new ones or regressing performance after a crash.

If we subsequently wanted to close those remaining holes, I think we'd
need the change attribute increment to be seen as atomic with respect
to
its associated change, both to clients and (separately) on disk. (That
would still allow the change attribute to go backwards after a crash,
to
the value it held as of the on-disk state of the file.  I think clients
should be able to deal with that case.)

Quoted 2 lines

But, I don't know, maybe a bigger hammer would be OK:

Quoted 8 lines

If you're not going to meet the minimum bar of data integrity, then this whole exercise is just a massive waste of everyone's time. The answer then going forward is just to recommend never using Linux as an
NFS server. Makes my life much easier, because I no longer have to debug any of the issues.

To be clear, you believe any scheme that would allow the client to see
an old change attr after a crash is insufficient?

The only way I can see to fix that (at least with only a crash counter)
would be to factor it in at presentation time like Neil suggested. Basically we'd just mask off the top 16 bits and plop the crash counter
in there before presenting it.

In principle, I suppose we could do that at the nfsd level as well (and
that might be the simplest way to fix this). We probably wouldn't be able to advertise a change attr type of MONOTONIC with this scheme though.

Quoted 98 lines · Jeff Layton

Quoted 88 lines · Trond Myklebust

Quoted 84 lines · J. Bruce Fields

Quoted 44 lines · NeilBrown

Quoted 21 lines · J. Bruce Fields

Quoted 12 lines · NeilBrown

Quoted 10 lines · Jeff Layton

The machine crashes and comes back up, and we get a query for
i_version
and it comes back as X. Fine, it's an old version. Now there
is a write.
What do we do to ensure that the new value doesn't collide
with X+1?

Quoted 7 lines

(I missed this bit in my earlier reply..)

How is it "Fine" to see an old version?
The file could have changed without the version changing. And I thought one of the goals of the crash-count was to be able to
provide a monotonic change id.

Quoted 15 lines

I was still mainly thinking about how to provide reliable close-
to-open
semantics between NFS clients.  In the case the writer was an NFS
client, it wasn't done writing (or it would have COMMITted), so
those
writes will come in and bump the change attribute soon, and as
long as
we avoid the small chance of reusing an old change attribute, we're OK,
and I think it'd even still be OK to advertise CHANGE_TYPE_IS_MONOTONIC_INCR.

You seem to be assuming that the client doesn't crash at the same
time
as the server (maybe they are both VMs on a host that lost power...)

Quoted 11 lines

If client A reads and caches, client B writes, the server crashes
after
writing some data (to already allocated space so no inode update
needed)
but before writing the new i_version, then client B crashes. When server comes back the i_version will be unchanged but the data
has
changed.  Client A will cache old data indefinitely...

I guess I assume that if all we're promising is close-to-open, then a
client isn't allowed to trust its cache in that situation.  Maybe that's
an overly draconian interpretation of close-to-open.

Also, I'm trying to think about how to improve things incrementally.
Incorporating something like a crash count into the on-disk i_version
fixes some cases without introducing any new ones or regressing performance after a crash.

If we subsequently wanted to close those remaining holes, I think we'd
need the change attribute increment to be seen as atomic with respect
to
its associated change, both to clients and (separately) on disk. (That
would still allow the change attribute to go backwards after a crash,
to
the value it held as of the on-disk state of the file.  I think clients
should be able to deal with that case.)

Quoted 2 lines

But, I don't know, maybe a bigger hammer would be OK:

Quoted 8 lines

If you're not going to meet the minimum bar of data integrity, then this whole exercise is just a massive waste of everyone's time. The answer then going forward is just to recommend never using Linux as an
NFS server. Makes my life much easier, because I no longer have to debug any of the issues.

Quoted 4 lines

To be clear, you believe any scheme that would allow the client to see
an old change attr after a crash is insufficient?

Correct. If a NFSv4 client or userspace application cannot trust that it will always see a change to the change attribute value when the file data changes, then you will eventually see data corruption due to the cached data no longer matching the stored data.

A false positive update of the change attribute (i.e. a case where the change attribute changes despite the data/metadata staying the same) is not desirable because it causes performance issues, but false negatives are far worse because they mean your data backup, cache, etc... are not consistent. Applications that have strong consistency requirements will have no option but to revalidate by always reading the entire file data
+ metadata.

Quoted 7 lines

The only way I can see to fix that (at least with only a crash counter)
would be to factor it in at presentation time like Neil suggested. Basically we'd just mask off the top 16 bits and plop the crash counter
in there before presenting it.

Quoted 5 lines

In principle, I suppose we could do that at the nfsd level as well (and
that might be the simplest way to fix this). We probably wouldn't be able to advertise a change attr type of MONOTONIC with this scheme though.

Why would you want to limit the crash counter to 16 bits?

Quoted 167 lines · Trond Myklebust

Quoted 121 lines · Jeff Layton

Quoted 107 lines · Trond Myklebust

Quoted 103 lines · J. Bruce Fields

Quoted 54 lines · NeilBrown

Quoted 37 lines · J. Bruce Fields

Quoted 16 lines · NeilBrown

Quoted 12 lines · Jeff Layton

The machine crashes and comes back up, and we get a query
for
i_version
and it comes back as X. Fine, it's an old version. Now
there
is a write.
What do we do to ensure that the new value doesn't collide
with X+1?

(I missed this bit in my earlier reply..)

Quoted 7 lines

How is it "Fine" to see an old version?
The file could have changed without the version changing.
And I thought one of the goals of the crash-count was to be
able to
provide a monotonic change id.

I was still mainly thinking about how to provide reliable close-
to-open
semantics between NFS clients.  In the case the writer was an
NFS
client, it wasn't done writing (or it would have COMMITted),
so
those
writes will come in and bump the change attribute soon,

Quoted 8 lines

and
as
long as
we avoid the small chance of reusing an old change attribute,
we're OK,
and I think it'd even still be OK to advertise CHANGE_TYPE_IS_MONOTONIC_INCR.

You seem to be assuming that the client doesn't crash at the
same
time
as the server (maybe they are both VMs on a host that lost power...)

Quoted 13 lines

If client A reads and caches, client B writes, the server crashes
after
writing some data (to already allocated space so no inode update
needed)
but before writing the new i_version, then client B crashes.
When server comes back the i_version will be unchanged but the
data
has
changed.  Client A will cache old data indefinitely...

I guess I assume that if all we're promising is close-to- open,
then a
client isn't allowed to trust its cache in that situation. Maybe
that's
an overly draconian interpretation of close-to-open.

Also, I'm trying to think about how to improve things incrementally.
Incorporating something like a crash count into the on-disk i_version
fixes some cases without introducing any new ones or regressing
performance after a crash.

If we subsequently wanted to close those remaining holes, I think
we'd
need the change attribute increment to be seen as atomic with respect
to
its associated change, both to clients and (separately) on disk.
(That
would still allow the change attribute to go backwards after a
crash,
to
the value it held as of the on-disk state of the file.  I think
clients
should be able to deal with that case.)

Quoted 2 lines

But, I don't know, maybe a bigger hammer would be OK:

Quoted 12 lines

If you're not going to meet the minimum bar of data integrity, then
this whole exercise is just a massive waste of everyone's time. The
answer then going forward is just to recommend never using Linux as
an
NFS server. Makes my life much easier, because I no longer have to
debug any of the issues.

Quoted 5 lines

To be clear, you believe any scheme that would allow the client to
see
an old change attr after a crash is insufficient?

Correct. If a NFSv4 client or userspace application cannot trust that
it will always see a change to the change attribute value when the file
data changes, then you will eventually see data corruption due to the
cached data no longer matching the stored data.

A false positive update of the change attribute (i.e. a case where the
change attribute changes despite the data/metadata staying the same) is
not desirable because it causes performance issues, but false negatives
are far worse because they mean your data backup, cache, etc... are not
consistent. Applications that have strong consistency requirements will
have no option but to revalidate by always reading the entire file data
+ metadata.

Quoted 8 lines

The only way I can see to fix that (at least with only a crash counter)
would be to factor it in at presentation time like Neil suggested.
Basically we'd just mask off the top 16 bits and plop the crash counter
in there before presenting it.

Quoted 8 lines

In principle, I suppose we could do that at the nfsd level as well
(and
that might be the simplest way to fix this). We probably wouldn't be
able to advertise a change attr type of MONOTONIC with this scheme
though.

Quoted 2 lines

Why would you want to limit the crash counter to 16 bits?

To leave more room for the "real" counter. Otherwise, an inode that gets
frequent writes after a long period of no crashes could experience the
counter wrap.

IOW, we have 63 bits to play with. Whatever part we dedicate to the crash counter will not be available for the actual version counter.

I'm proposing a 16+47+1 split, but I'm happy to hear arguments for a different one.

i_version updates that could get lost by an inode in a crash).

Quoted 177 lines · Jeff Layton

Quoted 167 lines · Trond Myklebust

Quoted 121 lines · Jeff Layton

Quoted 107 lines · Trond Myklebust

Quoted 103 lines · J. Bruce Fields

Quoted 54 lines · NeilBrown

Quoted 37 lines · J. Bruce Fields

Quoted 16 lines · NeilBrown

Quoted 12 lines · Jeff Layton

The machine crashes and comes back up, and we get a query
for
i_version
and it comes back as X. Fine, it's an old version. Now
there
is a write.
What do we do to ensure that the new value doesn't collide
with X+1?

(I missed this bit in my earlier reply..)

Quoted 7 lines

How is it "Fine" to see an old version?
The file could have changed without the version changing.
And I thought one of the goals of the crash-count was to be
able to
provide a monotonic change id.

I was still mainly thinking about how to provide reliable close-
to-open
semantics between NFS clients.  In the case the writer was an
NFS
client, it wasn't done writing (or it would have COMMITted),
so
those
writes will come in and bump the change attribute soon,

Quoted 8 lines

and
as
long as
we avoid the small chance of reusing an old change attribute,
we're OK,
and I think it'd even still be OK to advertise CHANGE_TYPE_IS_MONOTONIC_INCR.

You seem to be assuming that the client doesn't crash at the
same
time
as the server (maybe they are both VMs on a host that lost power...)

Quoted 13 lines

If client A reads and caches, client B writes, the server crashes
after
writing some data (to already allocated space so no inode update
needed)
but before writing the new i_version, then client B crashes.
When server comes back the i_version will be unchanged but the
data
has
changed.  Client A will cache old data indefinitely...

I guess I assume that if all we're promising is close-to- open,
then a
client isn't allowed to trust its cache in that situation. Maybe
that's
an overly draconian interpretation of close-to-open.

Also, I'm trying to think about how to improve things incrementally.
Incorporating something like a crash count into the on-disk i_version
fixes some cases without introducing any new ones or regressing
performance after a crash.

If we subsequently wanted to close those remaining holes, I think
we'd
need the change attribute increment to be seen as atomic with respect
to
its associated change, both to clients and (separately) on disk.
(That
would still allow the change attribute to go backwards after a
crash,
to
the value it held as of the on-disk state of the file.  I think
clients
should be able to deal with that case.)

Quoted 2 lines

But, I don't know, maybe a bigger hammer would be OK:

Quoted 12 lines

If you're not going to meet the minimum bar of data integrity, then
this whole exercise is just a massive waste of everyone's time. The
answer then going forward is just to recommend never using Linux as
an
NFS server. Makes my life much easier, because I no longer have to
debug any of the issues.

Quoted 5 lines

To be clear, you believe any scheme that would allow the client to
see
an old change attr after a crash is insufficient?

Correct. If a NFSv4 client or userspace application cannot trust that
it will always see a change to the change attribute value when the file
data changes, then you will eventually see data corruption due to the
cached data no longer matching the stored data.

A false positive update of the change attribute (i.e. a case where the
change attribute changes despite the data/metadata staying the same) is
not desirable because it causes performance issues, but false negatives
are far worse because they mean your data backup, cache, etc... are not
consistent. Applications that have strong consistency requirements will
have no option but to revalidate by always reading the entire file data
+ metadata.

Quoted 8 lines

The only way I can see to fix that (at least with only a crash counter)
would be to factor it in at presentation time like Neil suggested.
Basically we'd just mask off the top 16 bits and plop the crash counter
in there before presenting it.

Quoted 8 lines

In principle, I suppose we could do that at the nfsd level as well
(and
that might be the simplest way to fix this). We probably wouldn't be
able to advertise a change attr type of MONOTONIC with this scheme
though.

Quoted 2 lines

Why would you want to limit the crash counter to 16 bits?

To leave more room for the "real" counter. Otherwise, an inode that gets
frequent writes after a long period of no crashes could experience the
counter wrap.

Quoted 5 lines

IOW, we have 63 bits to play with. Whatever part we dedicate to the crash counter will not be available for the actual version counter.

I'm proposing a 16+47+1 split, but I'm happy to hear arguments for a different one.

What is the expectation when you have an unclean shutdown or crash? Do all change attribute values get updated to reflect the new crash counter value, or only some?

If the answer is that 'all values change', then why store the crash counter in the inode at all? Why not just add it as an offset when you're generating the user-visible change attribute?

i.e. statx.change_attr = inode->i_version + (crash counter * offset)

(where offset is chosen to be larger than the max number of inode-

Quoted 1 line

i_version updates that could get lost by an inode in a crash).

Presumably that offset could be significantly smaller than 2^63...

Quoted 193 lines · Trond Myklebust

Quoted 177 lines · Jeff Layton

Quoted 167 lines · Trond Myklebust

Quoted 121 lines · Jeff Layton

Quoted 107 lines · Trond Myklebust

Quoted 103 lines · J. Bruce Fields

Quoted 54 lines · NeilBrown

Quoted 37 lines · J. Bruce Fields

Quoted 16 lines · NeilBrown

Quoted 12 lines · Jeff Layton

The machine crashes and comes back up, and we get a query
for
i_version
and it comes back as X. Fine, it's an old version. Now
there
is a write.
What do we do to ensure that the new value doesn't collide
with X+1?

(I missed this bit in my earlier reply..)

Quoted 7 lines

How is it "Fine" to see an old version?
The file could have changed without the version changing.
And I thought one of the goals of the crash-count was to be
able to
provide a monotonic change id.

I was still mainly thinking about how to provide reliable close-
to-open
semantics between NFS clients.  In the case the writer was an
NFS
client, it wasn't done writing (or it would have COMMITted),
so
those
writes will come in and bump the change attribute soon,

Quoted 8 lines

and
as
long as
we avoid the small chance of reusing an old change attribute,
we're OK,
and I think it'd even still be OK to advertise CHANGE_TYPE_IS_MONOTONIC_INCR.

You seem to be assuming that the client doesn't crash at the
same
time
as the server (maybe they are both VMs on a host that lost power...)

Quoted 13 lines

If client A reads and caches, client B writes, the server crashes
after
writing some data (to already allocated space so no inode update
needed)
but before writing the new i_version, then client B crashes.
When server comes back the i_version will be unchanged but the
data
has
changed.  Client A will cache old data indefinitely...

I guess I assume that if all we're promising is close-to- open,
then a
client isn't allowed to trust its cache in that situation. Maybe
that's
an overly draconian interpretation of close-to-open.

Also, I'm trying to think about how to improve things incrementally.
Incorporating something like a crash count into the on-disk i_version
fixes some cases without introducing any new ones or regressing
performance after a crash.

If we subsequently wanted to close those remaining holes, I think
we'd
need the change attribute increment to be seen as atomic with respect
to
its associated change, both to clients and (separately) on disk.
(That
would still allow the change attribute to go backwards after a
crash,
to
the value it held as of the on-disk state of the file.  I think
clients
should be able to deal with that case.)

Quoted 2 lines

But, I don't know, maybe a bigger hammer would be OK:

Quoted 12 lines

If you're not going to meet the minimum bar of data integrity, then
this whole exercise is just a massive waste of everyone's time. The
answer then going forward is just to recommend never using Linux as
an
NFS server. Makes my life much easier, because I no longer have to
debug any of the issues.

Quoted 5 lines

To be clear, you believe any scheme that would allow the client to
see
an old change attr after a crash is insufficient?

Correct. If a NFSv4 client or userspace application cannot trust that
it will always see a change to the change attribute value when the file
data changes, then you will eventually see data corruption due to the
cached data no longer matching the stored data.

A false positive update of the change attribute (i.e. a case where the
change attribute changes despite the data/metadata staying the same) is
not desirable because it causes performance issues, but false negatives
are far worse because they mean your data backup, cache, etc... are not
consistent. Applications that have strong consistency requirements will
have no option but to revalidate by always reading the entire file data
+ metadata.

Quoted 8 lines

The only way I can see to fix that (at least with only a crash counter)
would be to factor it in at presentation time like Neil suggested.
Basically we'd just mask off the top 16 bits and plop the crash counter
in there before presenting it.

Quoted 8 lines

In principle, I suppose we could do that at the nfsd level as well
(and
that might be the simplest way to fix this). We probably wouldn't be
able to advertise a change attr type of MONOTONIC with this scheme
though.

Quoted 2 lines

Why would you want to limit the crash counter to 16 bits?

To leave more room for the "real" counter. Otherwise, an inode that gets
frequent writes after a long period of no crashes could experience the
counter wrap.

Quoted 5 lines

IOW, we have 63 bits to play with. Whatever part we dedicate to the crash counter will not be available for the actual version counter.

I'm proposing a 16+47+1 split, but I'm happy to hear arguments for a different one.

What is the expectation when you have an unclean shutdown or crash? Do all change attribute values get updated to reflect the new crash counter value, or only some?

If the answer is that 'all values change', then why store the crash counter in the inode at all? Why not just add it as an offset when you're generating the user-visible change attribute?

Quoted 7 lines

i.e. statx.change_attr = inode->i_version + (crash counter * offset)

(where offset is chosen to be larger than the max number of inode-

Quoted 1 line

i_version updates that could get lost by an inode in a crash).

Presumably that offset could be significantly smaller than 2^63...

Yes, if we plan to ensure that all the change attrs change after a crash, we can do that.

So what would make sense for an offset? Maybe 2**12? One would hope that there wouldn't be more than 4k increments before one of them made it to disk. OTOH, maybe that can happen with teeny-tiny writes.

If we want to leave this up to the filesystem, I guess we could just add a new struct super_block.s_version_offset field and let the filesystem precompute that value and set it at mount time. Then we can just add that in after querying i_version.

Signature · 1 line

Jeff Layton <[email protected]>

Quoted 5 lines

Quoted 5 lines

If the answer is that 'all values change', then why store the crash counter in the inode at all? Why not just add it as an offset when you're generating the user-visible change attribute?

i.e. statx.change_attr = inode->i_version + (crash counter * offset)

Quoted 6 lines

Yes, if we plan to ensure that all the change attrs change after a crash, we can do that.

So what would make sense for an offset? Maybe 2**12? One would hope that there wouldn't be more than 4k increments before one of them made it to disk. OTOH, maybe that can happen with teeny-tiny writes.

Leave it up the to filesystem to decide. The VFS and/or NFSD should have not have part in calculating the i_version. It should be entirely in the filesystem - though support code could be provided if common patterns exist across filesystems.

Quoted 5 lines · NeilBrown

Quoted 5 lines

Quoted 5 lines

If the answer is that 'all values change', then why store the crash counter in the inode at all? Why not just add it as an offset when you're generating the user-visible change attribute?

i.e. statx.change_attr = inode->i_version + (crash counter * offset)

I had suggested just hashing the crash counter with the file system's on-disk i_version number, which is essentially what you are suggested.

Quoted 11 lines

Quoted 6 lines

Yes, if we plan to ensure that all the change attrs change after a crash, we can do that.

So what would make sense for an offset? Maybe 2**12? One would hope that there wouldn't be more than 4k increments before one of them made it to disk. OTOH, maybe that can happen with teeny-tiny writes.

Leave it up the to filesystem to decide. The VFS and/or NFSD should have not have part in calculating the i_version. It should be entirely in the filesystem - though support code could be provided if common patterns exist across filesystems.

Oh, *heck* no. This parameter is for the NFS implementation to decide, because it's NFS's caching algorithms which are at stake here.

As a the file system maintainer, I had offered to make an on-disk "crash counter" which would get updated when the journal had gotten replayed, in addition to the on-disk i_version number. This will be available for the Linux implementation of NFSD to use, but that's up to *you* to decide how you want to use them.

I was perfectly happy with hashing the crash counter and the i_version because I had assumed that not *that* much stuff was going to be cached, and so invalidating all of the caches in the unusual case where there was a crash was acceptable. After all it's a !@#?!@ cache. Caches sometimmes get invalidated. "That is the order of things." (as Ramata'Klan once said in "Rocks and Shoals")

But if people expect that multiple TB's of data is going to be stored; that cache invalidation is unacceptable; and that a itsy-weeny chance of false negative failures which might cause data corruption might be acceptable tradeoff, hey, that's for the system which is providing caching semantics to determine.

PLEASE don't put this tradeoff on the file system authors; I would much prefer to leave this tradeoff in the hands of the system which is trying to do the caching.

Quoted 44 lines · Theodore Ts'o

Quoted 5 lines · NeilBrown

Quoted 5 lines

Quoted 5 lines

If the answer is that 'all values change', then why store the crash counter in the inode at all? Why not just add it as an offset when you're generating the user-visible change attribute?

i.e. statx.change_attr = inode->i_version + (crash counter * offset)

I had suggested just hashing the crash counter with the file system's on-disk i_version number, which is essentially what you are suggested.

Quoted 11 lines

Quoted 6 lines

Yes, if we plan to ensure that all the change attrs change after a crash, we can do that.

So what would make sense for an offset? Maybe 2**12? One would hope that there wouldn't be more than 4k increments before one of them made it to disk. OTOH, maybe that can happen with teeny-tiny writes.

Leave it up the to filesystem to decide. The VFS and/or NFSD should have not have part in calculating the i_version. It should be entirely in the filesystem - though support code could be provided if common patterns exist across filesystems.

Oh, *heck* no. This parameter is for the NFS implementation to decide, because it's NFS's caching algorithms which are at stake here.

As a the file system maintainer, I had offered to make an on-disk "crash counter" which would get updated when the journal had gotten replayed, in addition to the on-disk i_version number. This will be available for the Linux implementation of NFSD to use, but that's up to *you* to decide how you want to use them.

I was perfectly happy with hashing the crash counter and the i_version because I had assumed that not *that* much stuff was going to be cached, and so invalidating all of the caches in the unusual case where there was a crash was acceptable. After all it's a !@#?!@ cache. Caches sometimmes get invalidated. "That is the order of things." (as Ramata'Klan once said in "Rocks and Shoals")

But if people expect that multiple TB's of data is going to be stored; that cache invalidation is unacceptable; and that a itsy-weeny chance of false negative failures which might cause data corruption might be acceptable tradeoff, hey, that's for the system which is providing caching semantics to determine.

Quoted 4 lines

PLEASE don't put this tradeoff on the file system authors; I would much prefer to leave this tradeoff in the hands of the system which is trying to do the caching.

Yeah, if we were designing this from scratch, I might agree with leaving more up to the filesystem, but the existing users all have pretty much the same needs. I'm going to plan to try to keep most of this in the common infrastructure defined in iversion.h.

Ted, for the ext4 crash counter, what wordsize were you thinking? I doubt we'll be able to use much more than 32 bits so a larger integer is probably not worthwhile. There are several holes in struct super_block (at least on x86_64), so adding this field to the generic structure needn't grow it.

Quoted 55 lines · Jeff Layton

Quoted 44 lines · Theodore Ts'o

Quoted 5 lines · NeilBrown

Quoted 5 lines

Quoted 5 lines

If the answer is that 'all values change', then why store the crash counter in the inode at all? Why not just add it as an offset when you're generating the user-visible change attribute?

i.e. statx.change_attr = inode->i_version + (crash counter * offset)

I had suggested just hashing the crash counter with the file system's on-disk i_version number, which is essentially what you are suggested.

Quoted 11 lines

Quoted 6 lines

Yes, if we plan to ensure that all the change attrs change after a crash, we can do that.

So what would make sense for an offset? Maybe 2**12? One would hope that there wouldn't be more than 4k increments before one of them made it to disk. OTOH, maybe that can happen with teeny-tiny writes.

Leave it up the to filesystem to decide. The VFS and/or NFSD should have not have part in calculating the i_version. It should be entirely in the filesystem - though support code could be provided if common patterns exist across filesystems.

Oh, *heck* no. This parameter is for the NFS implementation to decide, because it's NFS's caching algorithms which are at stake here.

As a the file system maintainer, I had offered to make an on-disk "crash counter" which would get updated when the journal had gotten replayed, in addition to the on-disk i_version number. This will be available for the Linux implementation of NFSD to use, but that's up to *you* to decide how you want to use them.

I was perfectly happy with hashing the crash counter and the i_version because I had assumed that not *that* much stuff was going to be cached, and so invalidating all of the caches in the unusual case where there was a crash was acceptable. After all it's a !@#?!@ cache. Caches sometimmes get invalidated. "That is the order of things." (as Ramata'Klan once said in "Rocks and Shoals")

But if people expect that multiple TB's of data is going to be stored; that cache invalidation is unacceptable; and that a itsy-weeny chance of false negative failures which might cause data corruption might be acceptable tradeoff, hey, that's for the system which is providing caching semantics to determine.

Quoted 4 lines

PLEASE don't put this tradeoff on the file system authors; I would much prefer to leave this tradeoff in the hands of the system which is trying to do the caching.

Yeah, if we were designing this from scratch, I might agree with leaving more up to the filesystem, but the existing users all have pretty much the same needs. I'm going to plan to try to keep most of this in the common infrastructure defined in iversion.h.

Quoted 5 lines

Ted, for the ext4 crash counter, what wordsize were you thinking? I doubt we'll be able to use much more than 32 bits so a larger integer is probably not worthwhile. There are several holes in struct super_block (at least on x86_64), so adding this field to the generic structure needn't grow it.

That said, now that I've taken a swipe at implementing this, I need more information than just the crash counter. We need to multiply the crash counter with a reasonable estimate of the maximum number of individual writes that could occur between an i_version being incremented and that value making it to the backing store.

IOW, given a write that bumps the i_version to X, how many more write calls could race in before X makes it to the platter? I took a SWAG and said 4k in an earlier email, but I don't really have a way to know, and that could vary wildly with different filesystems and storage.

What I'd like to see is this in struct super_block:

	u32		s_version_offset;

...and then individual filesystems can calculate:

crash_counter * max_number_of_writes

and put the correct value in there at mount time.

Quoted 73 lines · Jeff Layton

Quoted 55 lines · Jeff Layton

Quoted 44 lines · Theodore Ts'o

Quoted 5 lines · NeilBrown

Quoted 5 lines

Quoted 5 lines

If the answer is that 'all values change', then why store the crash counter in the inode at all? Why not just add it as an offset when you're generating the user-visible change attribute?

i.e. statx.change_attr = inode->i_version + (crash counter * offset)

I had suggested just hashing the crash counter with the file system's on-disk i_version number, which is essentially what you are suggested.

Quoted 11 lines

Quoted 6 lines

Yes, if we plan to ensure that all the change attrs change after a crash, we can do that.

So what would make sense for an offset? Maybe 2**12? One would hope that there wouldn't be more than 4k increments before one of them made it to disk. OTOH, maybe that can happen with teeny-tiny writes.

Leave it up the to filesystem to decide. The VFS and/or NFSD should have not have part in calculating the i_version. It should be entirely in the filesystem - though support code could be provided if common patterns exist across filesystems.

Oh, *heck* no. This parameter is for the NFS implementation to decide, because it's NFS's caching algorithms which are at stake here.

As a the file system maintainer, I had offered to make an on-disk "crash counter" which would get updated when the journal had gotten replayed, in addition to the on-disk i_version number. This will be available for the Linux implementation of NFSD to use, but that's up to *you* to decide how you want to use them.

I was perfectly happy with hashing the crash counter and the i_version because I had assumed that not *that* much stuff was going to be cached, and so invalidating all of the caches in the unusual case where there was a crash was acceptable. After all it's a !@#?!@ cache. Caches sometimmes get invalidated. "That is the order of things." (as Ramata'Klan once said in "Rocks and Shoals")

But if people expect that multiple TB's of data is going to be stored; that cache invalidation is unacceptable; and that a itsy-weeny chance of false negative failures which might cause data corruption might be acceptable tradeoff, hey, that's for the system which is providing caching semantics to determine.

Quoted 4 lines

PLEASE don't put this tradeoff on the file system authors; I would much prefer to leave this tradeoff in the hands of the system which is trying to do the caching.

Yeah, if we were designing this from scratch, I might agree with leaving more up to the filesystem, but the existing users all have pretty much the same needs. I'm going to plan to try to keep most of this in the common infrastructure defined in iversion.h.

Quoted 5 lines

Ted, for the ext4 crash counter, what wordsize were you thinking? I doubt we'll be able to use much more than 32 bits so a larger integer is probably not worthwhile. There are several holes in struct super_block (at least on x86_64), so adding this field to the generic structure needn't grow it.

That said, now that I've taken a swipe at implementing this, I need more information than just the crash counter. We need to multiply the crash counter with a reasonable estimate of the maximum number of individual writes that could occur between an i_version being incremented and that value making it to the backing store.

IOW, given a write that bumps the i_version to X, how many more write calls could race in before X makes it to the platter? I took a SWAG and said 4k in an earlier email, but I don't really have a way to know, and that could vary wildly with different filesystems and storage.

Quoted 3 lines

What I'd like to see is this in struct super_block:

	u32		s_version_offset;

	u64		s_version_salt;

Quoted 5 lines

...and then individual filesystems can calculate:

crash_counter * max_number_of_writes

and put the correct value in there at mount time.

Other filesystems might not have a crash counter but have other information that can be substituted, like a mount counter or a global change sequence number that is guaranteed to increment from one mount to the next.

Further, have you thought about what "max number of writes" might be in ten years time? e.g. what happens if a filesysetm as "max number of writes" being greater than 2^32? I mean, we already have machines out there running Linux with 64-128TB of physical RAM, so it's already practical to hold > 2^32 individual writes to a single inode that each bump i_version in memory....

So when we consider this sort of scale, the "crash counter * max writes" scheme largely falls apart because "max writes" is a really large number to begin with. We're going to be stuck with whatever algorithm is decided on for the foreseeable future, so we must recognise that _we've already overrun 32 bit counter schemes_ in terms of tracking "i_version changes in memory vs what we have on disk".

Hence I really think that we should be leaving the implementation of the salt value to the individual filesysetms as different filesytsems are aimed at different use cases and so may not necessarily have to all care about the same things (like 2^32 bit max write overruns). All the high level VFS code then needs to do is add the two together:

	statx.change_attr = inode->i_version + sb->s_version_salt;

Quoted 74 lines · Dave Chinner

Quoted 73 lines · Jeff Layton

Quoted 55 lines · Jeff Layton

Quoted 44 lines · Theodore Ts'o

Quoted 5 lines · NeilBrown

Quoted 5 lines

Quoted 5 lines

If the answer is that 'all values change', then why store the crash counter in the inode at all? Why not just add it as an offset when you're generating the user-visible change attribute?

i.e. statx.change_attr = inode->i_version + (crash counter * offset)

I had suggested just hashing the crash counter with the file system's on-disk i_version number, which is essentially what you are suggested.

Quoted 11 lines

Quoted 6 lines

Yes, if we plan to ensure that all the change attrs change after a crash, we can do that.

So what would make sense for an offset? Maybe 2**12? One would hope that there wouldn't be more than 4k increments before one of them made it to disk. OTOH, maybe that can happen with teeny-tiny writes.

Leave it up the to filesystem to decide. The VFS and/or NFSD should have not have part in calculating the i_version. It should be entirely in the filesystem - though support code could be provided if common patterns exist across filesystems.

Oh, *heck* no. This parameter is for the NFS implementation to decide, because it's NFS's caching algorithms which are at stake here.

As a the file system maintainer, I had offered to make an on-disk "crash counter" which would get updated when the journal had gotten replayed, in addition to the on-disk i_version number. This will be available for the Linux implementation of NFSD to use, but that's up to *you* to decide how you want to use them.

I was perfectly happy with hashing the crash counter and the i_version because I had assumed that not *that* much stuff was going to be cached, and so invalidating all of the caches in the unusual case where there was a crash was acceptable. After all it's a !@#?!@ cache. Caches sometimmes get invalidated. "That is the order of things." (as Ramata'Klan once said in "Rocks and Shoals")

But if people expect that multiple TB's of data is going to be stored; that cache invalidation is unacceptable; and that a itsy-weeny chance of false negative failures which might cause data corruption might be acceptable tradeoff, hey, that's for the system which is providing caching semantics to determine.

Quoted 4 lines

PLEASE don't put this tradeoff on the file system authors; I would much prefer to leave this tradeoff in the hands of the system which is trying to do the caching.

Yeah, if we were designing this from scratch, I might agree with leaving more up to the filesystem, but the existing users all have pretty much the same needs. I'm going to plan to try to keep most of this in the common infrastructure defined in iversion.h.

Quoted 5 lines

Ted, for the ext4 crash counter, what wordsize were you thinking? I doubt we'll be able to use much more than 32 bits so a larger integer is probably not worthwhile. There are several holes in struct super_block (at least on x86_64), so adding this field to the generic structure needn't grow it.

That said, now that I've taken a swipe at implementing this, I need more information than just the crash counter. We need to multiply the crash counter with a reasonable estimate of the maximum number of individual writes that could occur between an i_version being incremented and that value making it to the backing store.

IOW, given a write that bumps the i_version to X, how many more write calls could race in before X makes it to the platter? I took a SWAG and said 4k in an earlier email, but I don't really have a way to know, and that could vary wildly with different filesystems and storage.

Quoted 6 lines

Quoted 3 lines

What I'd like to see is this in struct super_block:

	u32		s_version_offset;

	u64		s_version_salt;

IDK...it _is_ an offset since we're folding it in with addition, and it has a real meaning. Filesystems do need to be cognizant of that fact, I think.

Also does anyone have a preference on doing this vs. a get_version_salt or get_version_offset sb operation? I figured the value should be mostly static so it'd be nice to avoid an operation for it.

Quoted 11 lines

Quoted 5 lines

...and then individual filesystems can calculate:

crash_counter * max_number_of_writes

and put the correct value in there at mount time.

Other filesystems might not have a crash counter but have other information that can be substituted, like a mount counter or a global change sequence number that is guaranteed to increment from one mount to the next.

The problem there is that you're going to cause the invalidation of all of the NFS client's cached regular files, even on clean server reboots. That's not a desirable outcome.

Quoted 102 lines · Jeff Layton

Quoted 74 lines · Dave Chinner

Quoted 73 lines · Jeff Layton

Quoted 55 lines · Jeff Layton

Quoted 44 lines · Theodore Ts'o

Quoted 5 lines · NeilBrown

Quoted 5 lines

Quoted 5 lines

If the answer is that 'all values change', then why store the crash counter in the inode at all? Why not just add it as an offset when you're generating the user-visible change attribute?

i.e. statx.change_attr = inode->i_version + (crash counter * offset)

I had suggested just hashing the crash counter with the file system's on-disk i_version number, which is essentially what you are suggested.

Quoted 11 lines

Quoted 6 lines

Yes, if we plan to ensure that all the change attrs change after a crash, we can do that.

So what would make sense for an offset? Maybe 2**12? One would hope that there wouldn't be more than 4k increments before one of them made it to disk. OTOH, maybe that can happen with teeny-tiny writes.

Leave it up the to filesystem to decide. The VFS and/or NFSD should have not have part in calculating the i_version. It should be entirely in the filesystem - though support code could be provided if common patterns exist across filesystems.

Oh, *heck* no. This parameter is for the NFS implementation to decide, because it's NFS's caching algorithms which are at stake here.

As a the file system maintainer, I had offered to make an on-disk "crash counter" which would get updated when the journal had gotten replayed, in addition to the on-disk i_version number. This will be available for the Linux implementation of NFSD to use, but that's up to *you* to decide how you want to use them.

I was perfectly happy with hashing the crash counter and the i_version because I had assumed that not *that* much stuff was going to be cached, and so invalidating all of the caches in the unusual case where there was a crash was acceptable. After all it's a !@#?!@ cache. Caches sometimmes get invalidated. "That is the order of things." (as Ramata'Klan once said in "Rocks and Shoals")

But if people expect that multiple TB's of data is going to be stored; that cache invalidation is unacceptable; and that a itsy-weeny chance of false negative failures which might cause data corruption might be acceptable tradeoff, hey, that's for the system which is providing caching semantics to determine.

Quoted 4 lines

PLEASE don't put this tradeoff on the file system authors; I would much prefer to leave this tradeoff in the hands of the system which is trying to do the caching.

Yeah, if we were designing this from scratch, I might agree with leaving more up to the filesystem, but the existing users all have pretty much the same needs. I'm going to plan to try to keep most of this in the common infrastructure defined in iversion.h.

Quoted 5 lines

Ted, for the ext4 crash counter, what wordsize were you thinking? I doubt we'll be able to use much more than 32 bits so a larger integer is probably not worthwhile. There are several holes in struct super_block (at least on x86_64), so adding this field to the generic structure needn't grow it.

That said, now that I've taken a swipe at implementing this, I need more information than just the crash counter. We need to multiply the crash counter with a reasonable estimate of the maximum number of individual writes that could occur between an i_version being incremented and that value making it to the backing store.

IOW, given a write that bumps the i_version to X, how many more write calls could race in before X makes it to the platter? I took a SWAG and said 4k in an earlier email, but I don't really have a way to know, and that could vary wildly with different filesystems and storage.

Quoted 6 lines

Quoted 3 lines

What I'd like to see is this in struct super_block:

	u32		s_version_offset;

	u64		s_version_salt;

IDK...it _is_ an offset since we're folding it in with addition, and it has a real meaning. Filesystems do need to be cognizant of that fact, I think.

Also does anyone have a preference on doing this vs. a get_version_salt or get_version_offset sb operation? I figured the value should be mostly static so it'd be nice to avoid an operation for it.

Quoted 11 lines

Quoted 5 lines

...and then individual filesystems can calculate:

crash_counter * max_number_of_writes

and put the correct value in there at mount time.

Other filesystems might not have a crash counter but have other information that can be substituted, like a mount counter or a global change sequence number that is guaranteed to increment from one mount to the next.

Quoted 3 lines

The problem there is that you're going to cause the invalidation of all of the NFS client's cached regular files, even on clean server reboots. That's not a desirable outcome.

Stop saying "anything less than perfect is unacceptible". I *know* that changing the salt on every mount might result in less than perfect results, but the fact is that a -false negative- is a data corruption event, whilst a false positive is not. False positives may not be desirable, but false negatives are *not acceptible at all*.

XFS can give you a guarantee of no false negatives right now with no on-disk format changes necessary, but it comes with the downside of false positives. That's not the end of the world, and it gives NFS the functionality it needs immediately and allows us time to add purpose-built on-disk functionality that gives NFS exactly what it wants. The reality is that this purpose-built on-disk change will take years to roll out to production systems, whilst using what we have now is just a kernel patch and upgrade away....

Changing on-disk metadata formats takes time, no matter how simple the change, and this timeframe is not something the NFS server actually controls.

But there is a way for the NFS server to define and control it's own on-disk persistent metadata: *extended attributes*.

How about we set a "crash" extended attribute on the root of an NFS export when the filesystem is exported, and then remove it when the filesystem is unexported.

This gives the NFS server it's own persistent attribute that tells it whether the filesystem was *unexported* cleanly. If the exportfs code calls syncfs() before the xattr is removed, then it guarantees that everything the NFS clients have written and modified will be exactly present the next time the filesystem is exported. If the "crash" xattr is present when the filesystem is exported, then it wasn't cleanly synced before it was taken out of service, and so something may have been lost and the "crash counter" needs to be bumped.

Yes, the "crash counter" is held in another xattr, so that it is persistent across crash and mount/unmount cycles. If the crash xattr is present, the NFSD reads, bumps and writes the crash counter xattr, and uses the new value for the life of that export. If the crash xattr is not present, then is just reads the counter xattr and uses it unchanged.

IOWs, the NFS server can define it's own on-disk persistent metadata using xattrs, and you don't need local filesystems to be modified at all. You can add the crash epoch into the change attr that is sent to NFS clients without having to change the VFS i_version implementation at all.

This whole problem is solvable entirely within the NFS server code, and we don't need to change local filesystems at all. NFS can control the persistence and format of the xattrs it uses, and it does not need new custom on-disk format changes from every filesystem to support this new application requirement.

At this point, NFS server developers don't need to care what the underlying filesystem format provides - the xattrs provide the crash detection and enumeration the NFS server functionality requires.

Quoted 12 lines · Dave Chinner

IOWs, the NFS server can define it's own on-disk persistent metadata using xattrs, and you don't need local filesystems to be modified at all. You can add the crash epoch into the change attr that is sent to NFS clients without having to change the VFS i_version implementation at all.

This whole problem is solvable entirely within the NFS server code, and we don't need to change local filesystems at all. NFS can control the persistence and format of the xattrs it uses, and it does not need new custom on-disk format changes from every filesystem to support this new application requirement.

Quoted 4 lines

At this point, NFS server developers don't need to care what the underlying filesystem format provides - the xattrs provide the crash detection and enumeration the NFS server functionality requires.

Doesn't the filesystem already detect when it's been mounted after an unclean shutdown?

I'm not sure what good we'll get out of bolting this scheme onto the NFS server, when the filesystem could just as easily give us this info.

In any case, the main problem at this point is not so much in detecting when there has been an unclean shutdown, but rather what to do when there is one. We need to to advance the presented change attributes beyond the largest possible one that may have been handed out prior to the crash.

How do we determine what that offset should be? Your last email suggested that there really is no limit to the number of i_version bumps that can happen in memory before one of them makes it to disk. What can we do to address that?

Quoted 13 lines · Jeff Layton

Quoted 12 lines · Dave Chinner

IOWs, the NFS server can define it's own on-disk persistent metadata using xattrs, and you don't need local filesystems to be modified at all. You can add the crash epoch into the change attr that is sent to NFS clients without having to change the VFS i_version implementation at all.

This whole problem is solvable entirely within the NFS server code, and we don't need to change local filesystems at all. NFS can control the persistence and format of the xattrs it uses, and it does not need new custom on-disk format changes from every filesystem to support this new application requirement.

Quoted 7 lines

Quoted 4 lines

At this point, NFS server developers don't need to care what the underlying filesystem format provides - the xattrs provide the crash detection and enumeration the NFS server functionality requires.

Doesn't the filesystem already detect when it's been mounted after an unclean shutdown?

Not every filesystem will be able to guarantee unclean shutdown detection at the next mount. That's the whole problem - NFS developers are asking for something that cannot be provided as generic functionality by individual filesystems, so the NFS server application is going to have to work around any filesytem that cannot provide the information it needs.

e.g. ext4 has it journal replayed by the userspace tools prior to mount, so when it then gets mounted by the kernel it's seen as a clean mount.

If we shut an XFS filesystem down due to a filesystem corruption or failed IO to the journal code, the kernel might not be able to replay the journal on mount (i.e. it is corrupt). We then run xfs_repair, and that fixes the corruption issue and -cleans the log-. When we next mount the filesystem, it results in a _clean mount_, and the kernel filesystem code can not signal to NFS that an unclean mount occurred and so it should bump it's crash counter.

IOWs, this whole "filesystems need to tell NFS about crashes" propagates all the way through *every filesystem tool chain*, not just the kernel mount code. And we most certainly don't control every 3rd party application that walks around in the filesystem on disk format, and so there are -zero- guarantees that the kernel filesystem mount code can give that an unclean shutdown occurred prior to the current mount.

And then for niche NFS server applications (like transparent fail-over between HA NFS servers) there are even more rigid constraints on NFS change attributes. And you're asking local filesystems to know about these application constraints and bake them into their on-disk format again.

This whole discussion has come about because we baked certain behaviour for NFS into the on-disk format many, many years ago, and it's only now that it is considered inadequate for *new* NFS application related functionality (e.g. fscache integration and cache validity across server side mount cycles).

We've learnt a valuable lesson from this: don't bake application specific persistent metadata requirements into the on-disk format because when the application needs to change, it requires every filesystem that supports taht application level functionality to change their on-disk formats...

Quoted 3 lines

I'm not sure what good we'll get out of bolting this scheme onto the NFS server, when the filesystem could just as easily give us this info.

The xattr scheme guarantees the correct application behaviour that the NFS server requires, all at the NFS application level without requiring local filesystems to support the NFS requirements in their on-disk format. THe NFS server controls the format and versioning of it's on-disk persistent metadata (i.e. the xattrs it uses) and so any changes to the application level requirements of that functionality are now completely under the control of the application.

i.e. the application gets to manage version control, backwards and forwards compatibility of it's persistent metadata, etc. What you are asking is that every local filesystem takes responsibility for managing the long term persistent metadata that only NFS requires. It's more complex to do this at the filesystem level, and we have to replicate the same work for every filesystem that is going to support this on-disk functionality.

Using xattrs means the functionality is implemented once, it's common across all local filesystems, and no exportable filesystem needs to know anything about it as it's all self-contained in the NFS server code. THe code is smaller, easier to maintain, consistent across all systems, easy to test, etc.

It also can be implemented and rolled out *immediately* to all existing supported NFS server implementations, without having to wait months/years (or never!) for local filesystem on-disk format changes to roll out to production systems.

Asking individual filesystems to implement application specific persistent metadata is a *last resort* and should only be done if correctness or performance cannot be obtained in any other way.

So, yeah, the only sane direction to take here is to use xattrs to store this NFS application level information. It's less work for everyone, and in the long term it means when the NFS application requirements change again, we don't need to modify the on-disk format of multiple local filesystems.

Quoted 5 lines

In any case, the main problem at this point is not so much in detecting when there has been an unclean shutdown, but rather what to do when there is one. We need to to advance the presented change attributes beyond the largest possible one that may have been handed out prior to the crash.

Sure, but you're missing my point: by using xattrs for detection, you don't need to involve anything to do with local filesystems at all.

Quoted 4 lines

How do we determine what that offset should be? Your last email suggested that there really is no limit to the number of i_version bumps that can happen in memory before one of them makes it to disk. What can we do to address that?

<shrug>

I'm just pointing out problems I see when defining this as behaviour for on-disk format purposes. If we define it as part of the on-disk format, then we have to be concerned about how it may be used outside the scope of just the NFS server application.

However, If NFS keeps this metadata and functionaly entirely contained at the application level via xattrs, I really don't care what algorithm NFS developers decides to use for their crash sequencing. It's not my concern at this point, and that's precisely why NFS should be using xattrs for this NFS specific functionality.

Quoted 12 lines · Dave Chinner

Quoted 4 lines

How do we determine what that offset should be? Your last email suggested that there really is no limit to the number of i_version bumps that can happen in memory before one of them makes it to disk. What can we do to address that?

<shrug>

I'm just pointing out problems I see when defining this as behaviour for on-disk format purposes. If we define it as part of the on-disk format, then we have to be concerned about how it may be used outside the scope of just the NFS server application.

Quoted 6 lines

However, If NFS keeps this metadata and functionaly entirely contained at the application level via xattrs, I really don't care what algorithm NFS developers decides to use for their crash sequencing. It's not my concern at this point, and that's precisely why NFS should be using xattrs for this NFS specific functionality.

I get it: you'd rather not have to deal with what you see as an NFS problem, but I don't get how what you're proposing solves anything. We might be able to use that scheme to detect crashes, but that's only part of the problem (and it's a relatively simple part of the problem to solve, really).

Maybe you can clarify it for me:

Suppose we go with what you're saying and store some information in xattrs that allows us to detect crashes in some fashion. The server crashes and comes back up and we detect that there was a crash earlier.

What does nfsd need to do now to ensure that it doesn't hand out a duplicate change attribute?

Quoted 26 lines · Jeff Layton

Quoted 12 lines · Dave Chinner

Quoted 4 lines

How do we determine what that offset should be? Your last email suggested that there really is no limit to the number of i_version bumps that can happen in memory before one of them makes it to disk. What can we do to address that?

<shrug>

I'm just pointing out problems I see when defining this as behaviour for on-disk format purposes. If we define it as part of the on-disk format, then we have to be concerned about how it may be used outside the scope of just the NFS server application.

Quoted 6 lines

However, If NFS keeps this metadata and functionaly entirely contained at the application level via xattrs, I really don't care what algorithm NFS developers decides to use for their crash sequencing. It's not my concern at this point, and that's precisely why NFS should be using xattrs for this NFS specific functionality.

I get it: you'd rather not have to deal with what you see as an NFS problem, but I don't get how what you're proposing solves anything. We might be able to use that scheme to detect crashes, but that's only part of the problem (and it's a relatively simple part of the problem to solve, really).

Quoted 8 lines

Maybe you can clarify it for me:

Suppose we go with what you're saying and store some information in xattrs that allows us to detect crashes in some fashion. The server crashes and comes back up and we detect that there was a crash earlier.

What does nfsd need to do now to ensure that it doesn't hand out a duplicate change attribute?

As I've already stated, the NFS server can hold the persistent NFS crash counter value in a second xattr that it bumps whenever it detects a crash and hence we take the local filesystem completely out of the equation. How the crash counter is then used by the nfsd to fold it into the NFS protocol change attribute is a nfsd problem, not a local filesystem problem.

If you're worried about maximum number of writes outstanding vs i_version bumps that are held in memory, then *bound the maximum number of uncommitted i_version changes that the NFS server will allow to build up in memory*. By moving the crash counter to being a NFS server only function, the NFS server controls the entire algorithm and it doesn't have to care about external 3rd party considerations like local filesystems have to.

e.g. The NFS server can track the i_version values when the NFSD syncs/commits a given inode. The nfsd can sample i_version it when calls ->commit_metadata or flushed data on the inode, and then when it peeks at i_version when gathering post-op attrs (or any other getattr op) it can decide that there is too much in-memory change (e.g. 10,000 counts since last sync) and sync the inode.

i.e. the NFS server can trivially cap the maximum number of uncommitted NFS change attr bumps it allows to build up in memory. At that point, the NFS server has a bound "maximum write count" that can be used in conjunction with the xattr based crash counter to determine how the change_attr is bumped by the crash counter.

Quoted 36 lines · Dave Chinner

Quoted 26 lines · Jeff Layton

Quoted 12 lines · Dave Chinner

Quoted 4 lines

How do we determine what that offset should be? Your last email suggested that there really is no limit to the number of i_version bumps that can happen in memory before one of them makes it to disk. What can we do to address that?

<shrug>

I'm just pointing out problems I see when defining this as behaviour for on-disk format purposes. If we define it as part of the on-disk format, then we have to be concerned about how it may be used outside the scope of just the NFS server application.

Quoted 6 lines

However, If NFS keeps this metadata and functionaly entirely contained at the application level via xattrs, I really don't care what algorithm NFS developers decides to use for their crash sequencing. It's not my concern at this point, and that's precisely why NFS should be using xattrs for this NFS specific functionality.

I get it: you'd rather not have to deal with what you see as an NFS problem, but I don't get how what you're proposing solves anything. We might be able to use that scheme to detect crashes, but that's only part of the problem (and it's a relatively simple part of the problem to solve, really).

Quoted 8 lines

Maybe you can clarify it for me:

Suppose we go with what you're saying and store some information in xattrs that allows us to detect crashes in some fashion. The server crashes and comes back up and we detect that there was a crash earlier.

What does nfsd need to do now to ensure that it doesn't hand out a duplicate change attribute?

Quoted 7 lines

As I've already stated, the NFS server can hold the persistent NFS crash counter value in a second xattr that it bumps whenever it detects a crash and hence we take the local filesystem completely out of the equation. How the crash counter is then used by the nfsd to fold it into the NFS protocol change attribute is a nfsd problem, not a local filesystem problem.

Ok, assuming you mean put this in an xattr that lives at the root of the export? We only need this for IS_I_VERSION filesystems (btrfs, xfs, and ext4), and they all support xattrs so this scheme should work.

Quoted 8 lines

If you're worried about maximum number of writes outstanding vs i_version bumps that are held in memory, then *bound the maximum number of uncommitted i_version changes that the NFS server will allow to build up in memory*. By moving the crash counter to being a NFS server only function, the NFS server controls the entire algorithm and it doesn't have to care about external 3rd party considerations like local filesystems have to.

Yeah, this is the bigger consideration.

Quoted 7 lines

e.g. The NFS server can track the i_version values when the NFSD syncs/commits a given inode. The nfsd can sample i_version it when calls ->commit_metadata or flushed data on the inode, and then when it peeks at i_version when gathering post-op attrs (or any other getattr op) it can decide that there is too much in-memory change (e.g. 10,000 counts since last sync) and sync the inode.

Quoted 5 lines

i.e. the NFS server can trivially cap the maximum number of uncommitted NFS change attr bumps it allows to build up in memory. At that point, the NFS server has a bound "maximum write count" that can be used in conjunction with the xattr based crash counter to determine how the change_attr is bumped by the crash counter.

Well, not "trivially". This is the bit where we have to grow struct inode (or the fs-specific inode), as we'll need to know what the latest on-disk value is for the inode.

I'm leaning toward doing this on the query side. Basically, when nfsd goes to query the i_version, it'll check the delta between the current version and the latest one on disk. If it's bigger than X then we'd just return NFS4ERR_DELAY to the client.

If the delta is >X/2, maybe it can kick off a workqueue job or something that calls write_inode with WB_SYNC_ALL to try to get the thing onto the platter ASAP.

Quoted 23 lines · Jeff Layton

Quoted 7 lines · Dave Chinner

e.g. The NFS server can track the i_version values when the NFSD syncs/commits a given inode. The nfsd can sample i_version it when calls ->commit_metadata or flushed data on the inode, and then when it peeks at i_version when gathering post-op attrs (or any other getattr op) it can decide that there is too much in-memory change (e.g. 10,000 counts since last sync) and sync the inode.

Quoted 5 lines

i.e. the NFS server can trivially cap the maximum number of uncommitted NFS change attr bumps it allows to build up in memory. At that point, the NFS server has a bound "maximum write count" that can be used in conjunction with the xattr based crash counter to determine how the change_attr is bumped by the crash counter.

Well, not "trivially". This is the bit where we have to grow struct inode (or the fs-specific inode), as we'll need to know what the latest on-disk value is for the inode.

I'm leaning toward doing this on the query side. Basically, when nfsd goes to query the i_version, it'll check the delta between the current version and the latest one on disk. If it's bigger than X then we'd just return NFS4ERR_DELAY to the client.

Quoted 3 lines

If the delta is >X/2, maybe it can kick off a workqueue job or something that calls write_inode with WB_SYNC_ALL to try to get the thing onto the platter ASAP.

Still looking at this bit too. Probably we can just kick off a WB_SYNC_NONE filemap_fdatawrite at that point and hope for the best?

Quoted 24 lines · Jeff Layton

Quoted 23 lines · Jeff Layton

Quoted 7 lines · Dave Chinner

e.g. The NFS server can track the i_version values when the NFSD syncs/commits a given inode. The nfsd can sample i_version it when calls ->commit_metadata or flushed data on the inode, and then when it peeks at i_version when gathering post-op attrs (or any other getattr op) it can decide that there is too much in-memory change (e.g. 10,000 counts since last sync) and sync the inode.

Quoted 5 lines

i.e. the NFS server can trivially cap the maximum number of uncommitted NFS change attr bumps it allows to build up in memory. At that point, the NFS server has a bound "maximum write count" that can be used in conjunction with the xattr based crash counter to determine how the change_attr is bumped by the crash counter.

Well, not "trivially". This is the bit where we have to grow struct inode (or the fs-specific inode), as we'll need to know what the latest on-disk value is for the inode.

I'm leaning toward doing this on the query side. Basically, when nfsd goes to query the i_version, it'll check the delta between the current version and the latest one on disk. If it's bigger than X then we'd just return NFS4ERR_DELAY to the client.

Quoted 6 lines

Quoted 3 lines

If the delta is >X/2, maybe it can kick off a workqueue job or something that calls write_inode with WB_SYNC_ALL to try to get the thing onto the platter ASAP.

Still looking at this bit too. Probably we can just kick off a WB_SYNC_NONE filemap_fdatawrite at that point and hope for the best?

"Hope" is not a great assurance regarding data integrity ;)

Anyway, it
depends on how you imagine the "i_version on disk" is going to be maintained. It could be maintained by NFSD inside commit_inode_metadata() - fetch current i_version value before asking filesystem for the sync and by the time commit_metadata() returns we know that value is on disk. If we detect the current - on_disk is > X/2, we call commit_inode_metadata() and we are done. It is not even *that* expensive because usually filesystems optimize away unnecessary IO when the inode didn't change since last time it got synced.

Quoted 43 lines · Jeff Layton

Quoted 36 lines · Jeff Layton

Quoted 12 lines · Dave Chinner

e.g. The NFS server can track the i_version values when the NFSD
syncs/commits a given inode. The nfsd can sample i_version it when
calls ->commit_metadata or flushed data on the inode, and then when
it peeks at i_version when gathering post-op attrs (or any other
getattr op) it can decide that there is too much in-memory change
(e.g. 10,000 counts since last sync) and sync the inode.

Quoted 8 lines

i.e. the NFS server can trivially cap the maximum number of uncommitted NFS change attr bumps it allows to build up in memory.
At that point, the NFS server has a bound "maximum write count" that
can be used in conjunction with the xattr based crash counter to
determine how the change_attr is bumped by the crash counter.

Well, not "trivially". This is the bit where we have to grow struct
inode (or the fs-specific inode), as we'll need to know what the latest
on-disk value is for the inode.

I'm leaning toward doing this on the query side. Basically, when nfsd
goes to query the i_version, it'll check the delta between the current
version and the latest one on disk. If it's bigger than X then we'd just
return NFS4ERR_DELAY to the client.

Quoted 5 lines

If the delta is >X/2, maybe it can kick off a workqueue job or something
that calls write_inode with WB_SYNC_ALL to try to get the thing onto the
platter ASAP.

Quoted 3 lines

Still looking at this bit too. Probably we can just kick off a WB_SYNC_NONE filemap_fdatawrite at that point and hope for the best?

"Hope" is not a great assurance regarding data integrity ;) Anyway, it
depends on how you imagine the "i_version on disk" is going to be maintained. It could be maintained by NFSD inside commit_inode_metadata() -
fetch current i_version value before asking filesystem for the sync and by the
time commit_metadata() returns we know that value is on disk. If we detect the
current - on_disk is > X/2, we call commit_inode_metadata() and we are
done. It is not even *that* expensive because usually filesystems optimize
away unnecessary IO when the inode didn't change since last time it got
synced.

Quoted 48 lines · Jan Kara

Quoted 43 lines · Jeff Layton

Quoted 36 lines · Jeff Layton

Quoted 12 lines · Dave Chinner

e.g. The NFS server can track the i_version values when the NFSD
syncs/commits a given inode. The nfsd can sample i_version it when
calls ->commit_metadata or flushed data on the inode, and then when
it peeks at i_version when gathering post-op attrs (or any other
getattr op) it can decide that there is too much in-memory change
(e.g. 10,000 counts since last sync) and sync the inode.

Quoted 8 lines

i.e. the NFS server can trivially cap the maximum number of uncommitted NFS change attr bumps it allows to build up in memory.
At that point, the NFS server has a bound "maximum write count" that
can be used in conjunction with the xattr based crash counter to
determine how the change_attr is bumped by the crash counter.

Well, not "trivially". This is the bit where we have to grow struct
inode (or the fs-specific inode), as we'll need to know what the latest
on-disk value is for the inode.

I'm leaning toward doing this on the query side. Basically, when nfsd
goes to query the i_version, it'll check the delta between the current
version and the latest one on disk. If it's bigger than X then we'd just
return NFS4ERR_DELAY to the client.

Quoted 5 lines

If the delta is >X/2, maybe it can kick off a workqueue job or something
that calls write_inode with WB_SYNC_ALL to try to get the thing onto the
platter ASAP.

Quoted 3 lines

Still looking at this bit too. Probably we can just kick off a WB_SYNC_NONE filemap_fdatawrite at that point and hope for the best?

Quoted 17 lines

"Hope" is not a great assurance regarding data integrity ;) Anyway, it
depends on how you imagine the "i_version on disk" is going to be maintained. It could be maintained by NFSD inside commit_inode_metadata() -
fetch current i_version value before asking filesystem for the sync and by the
time commit_metadata() returns we know that value is on disk. If we detect the
current - on_disk is > X/2, we call commit_inode_metadata() and we are
done. It is not even *that* expensive because usually filesystems optimize
away unnecessary IO when the inode didn't change since last time it got
synced.

Note that these approaches requiring 3rd party help in order to track i_version integrity across filesystem crashes all make the idea of adding i_version to statx() a no-go.

It is one thing for knfsd to add specialised machinery for integrity checking, but if all applications need to do so, then they are highly unlikely to want to adopt this attribute.

-----Original Message-----
From: Jeff Layton [mailto:[email protected]]
Sent: Friday, September 23, 2022 6:50 AM
To: Trond Myklebust <[email protected]>; [email protected]
Cc: [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [man-pages RFC PATCH v4] statx, inode: document the new STATX_INO_VERSION field

Quoted 54 lines · Trond Myklebust

Quoted 37 lines · Jan Kara

Quoted 28 lines · Jeff Layton

Quoted 27 lines · Jeff Layton

Quoted 8 lines · Dave Chinner

e.g. The NFS server can track the i_version values when the NFSD syncs/commits a given inode. The nfsd can sample i_version it when calls ->commit_metadata or flushed data on the inode, and then when it peeks at i_version when gathering post-op attrs (or any other getattr op) it can decide that there is too much in-memory change (e.g. 10,000 counts since last sync) and sync the inode.

Quoted 8 lines

i.e. the NFS server can trivially cap the maximum number of uncommitted NFS change attr bumps it allows to build up in memory.
At that point, the NFS server has a bound "maximum write count" that
can be used in conjunction with the xattr based crash counter to determine how the change_attr is bumped by the crash counter.

Well, not "trivially". This is the bit where we have to grow struct inode (or the fs-specific inode), as we'll need to know what the latest on-disk value is for the inode.

I'm leaning toward doing this on the query side. Basically, when nfsd goes to query the i_version, it'll check the delta between the current version and the latest one on disk. If it's bigger than X then we'd just return NFS4ERR_DELAY to the client.

Quoted 7 lines

Quoted 3 lines

If the delta is >X/2, maybe it can kick off a workqueue job or something that calls write_inode with WB_SYNC_ALL to try to get the thing onto the platter ASAP.

Still looking at this bit too. Probably we can just kick off a WB_SYNC_NONE filemap_fdatawrite at that point and hope for the best?

Quoted 11 lines

"Hope" is not a great assurance regarding data integrity ;) Anyway, it depends on how you imagine the "i_version on disk" is going to be maintained. It could be maintained by NFSD inside commit_inode_metadata() -
fetch current i_version value before asking filesystem for the sync and by the time commit_metadata() returns we know that value is on disk. If we detect the current - on_disk is > X/2, we call commit_inode_metadata() and we are done. It is not even *that* expensive because usually filesystems optimize away unnecessary IO when the inode didn't change since last time it got synced.

Note that these approaches requiring 3rd party help in order to track i_version integrity across filesystem crashes all make the idea of adding i_version to statx() a no-go.

Quoted 5 lines

It is one thing for knfsd to add specialised machinery for integrity checking, but if all applications need to do so, then they are highly unlikely to want to adopt this attribute.

Absolutely. That is the downside of this approach, but the priority here

always been to improve nfsd. If we don't get the ability to present this

statx, then so be it. Later on, I suppose we can move that handling into