Looks fine to me.

In D48570#1112658, @glebius wrote:

In D48570#1112656, @rmacklem wrote:

Looks ok to me but I do not know pthreads.
Maybe markj@ can complete a review, since he
has already commented on the code?

If you don't mind, I'd like to push changes today. I will be online through the weekend and not disrupted by usual work & weekday stuff, so will be able to address any issues that may be discovered.

Looks ok to me but I do not know pthreads.
Maybe markj@ can complete a review, since he
has already commented on the code?

Looks ok to me but I do not know pthreads.

As I mentioned, I am concerned that the "cookie" might end
up referring to the wrong socket, if the kernel code were to
close the socket (freeing the structure) and then another
socreate() happens to get memory at the same address.
Then the daemon replies to the upcall.

I think this looks fine. I cannot exactly remember what
the gssd_syscall("") did (it indicated the daemon was going away),
but I'll assume you no longer need it?

Looks ok to me. I noted a minor style nit. Feel free to ignore it.

Was changing the syscall and upcall arguments really necessary?
I thought it was the upcall RPC transport that needed to change.

When I originally did this, I thought of using the socket pointer as a "unique"
indicator for it in userpsace (for the TCP socket the client/server is using).
I chose not to do so, because I was concerned that the kernel code might...

• close the socket
• create a new socket, that happens to have the same kernel address.

In D48452#1105618, @kib wrote:

Zero st_filerev in vop_stat_helper_pre().
Zero va_filerev before calling VOP_GETATTR() in vfs_stdstat()

In D48452#1105634, @olce wrote:

I'm OK with these changes as they are.

That said, even if I agree there is, e.g., a bug in fusefs as mentioned in inline comments, I'm still wondering how to prevent non-initialized fields bugs in the VFS in a more general fashion. It's unfortunate that C does not allow to specify a default initializer per type. Maybe a follow-up attempt creating a new pre for VOP_GETATTR() would do good, although it will show up as not optimal as the compiler can't elide useless pre-initialization in it (for fields that are effectively filled by the FS' VOP_GETATTR() implementation). We would need a slightly different implementation for it to be able to perform such elisions.

In D48452#1105397, @asomers wrote:

Since st_filerev provides similar information to st_gen, should it also be guarded by PRIV_VFS_GENERATION in vop_stat_helper_post? And does it warrant mention in stat.2 ?

Add an additional flag for the case where ctime is used
to set va_filerev.

In D48436#1105038, @kib wrote:

I see that msdosfs did not update de_modrev, If I fix msdosfs and tmpfs to update vs_filerev, do we still need the flag? Or is it better to keep tmpfs/msdosfs as is and go ahead with the flag?

In D48436#1105044, @asomers wrote:

fusefs currently doesn't set va_filerev at all. I can fix that. But with fuse, ctime updates can come from either the kernel or the server. So the easiest solution would probably be to make va_filerev be a function of ctime. Would that work with the Linux client, assuming we set NFS4_CHANGE_TYPE_IS_TIME_METADATA ?

In D48436#1105027, @kib wrote:

And where is the use of the new flag?
Also, what writable and exportable filesystems do not increment va_filerev on changes? I looked and only seems to find smbfs and tmpfs. It might be simpler to fix tmpfs than adding a flag.

In D48198#1099283, @asomers wrote:

In D48198#1099255, @rmacklem wrote:

Exports, mountd and nfsd can be done in a vnet jail, if that would help?

It certainly would help. Are there any good examples of code that creates a temporary vnet jail with nfs inside?

In D48198#1099254, @asomers wrote:

In D48198#1099253, @rmacklem wrote:

I'm not sure I would have called the nfsd "odd". It is one of two
consumers of the VFS/VOP interface (I suppose you might call
the stacked file systems consumers as well?).

Being one of the two, it is definitely different than the local syscall
interface.

But, anyhow, it is definitely different than the local syscall interface
and not just not using VOP_OPEN/VOP_CLOSE. It uses VOP_LOOKUP,
but in a different way than the local syscalls, particularly w.r.t. name caching,
as another example.

I've noticed that nfsd will do VOP_LOOKUP calls for ".". Are there any other ways that it differs?

Not w.r.t. what it looks up, but in how it is set up.
Compare nfsvno_namei() with namei() in kern/vfs_lookup.c,
for instance.

I'm not sure I would have called the nfsd "odd". It is one of two
consumers of the VFS/VOP interface (I suppose you might call
the stacked file systems consumers as well?).

Looks fine to me.

Oh, and I think just copying fid_data would fix cd9660
as well, for those who don't like __packed.

Btw, I replaced "sizeof(struct fid)" with MAXFIDSZ in the _Static_assert()
and it passed (at least for amd64).

Good catch w.r.t. the _Static_assert(). It does need to be fixed,
even if these changes don't go in.

I'll leave it someone familiar with tmpfs to decide if having
tn_gen wrap around to zero is a real concern.

Just fyi, whenever the "struct fid" changes, the file handles
change. As such, an upgrade of the NFS server requires that
all clients be unmounted/remounted.

Added upper bound check for port# as suggested
by delphi@.

Looks fine to me.