tmpfs: Rework file handles

[olce]

Summary

Change 'struct tmpfs_fid_data' to behave consistently with the private
structure other FSes use. In a nutshell, make it a full alias of
'struct fid', instead of just using it to fill 'fid_data'. This implies
adding a length field at start (aliasing 'fid_len' of 'struct fid'), and
filling 'fid_len' with the full size of the aliased structure.

To ensure that the new 'struct tmpfs_fid_data' is smaller than 'struct
fid', which the compile-time assert introduced in commit
91b5592a1e1af974 ("fs: Add static asserts for the size of fid
structures") checks (and thus was not strong enough when added), shrink
the 'tfd_gen' field to 'unsigned int', as it only contains the result of
a call to arc4random(), and move it before 'tfd_gen' to compact the
structure (there is still a 16-bit hole before 'tfd_gen').

A consequence of this change is that copying the 'struct tmpfs_fid_data'
into a stack-allocated variable becomes unnecessary, as all fields are
then naturally aligned (provided that the embedding 'struct fhandle' is
so, which is normally guaranteed by kernel's malloc()).

[rmacklem]

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

I am not sure why others are concerned about using __packed.
The uses are simple and not that frequently executed.

sys/fs/tmpfs/tmpfs.h

219

Although this field is initialized by arc4random(),
it is also incremented.
--> Having it 64bits guarantees it will not wrap around to zero.

[rmacklem]

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

[rmacklem]

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

It might make more sense to convert the other file systems where the
_Static_assert() fails with code that just copies fid_data the way tmpfs does?
(tarfs is the current case)

[rmacklem]

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

[olce]

In D47956#1093709, @rmacklem wrote:

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

Wrapping around doesn't seem to be of a concern, as incrementing is just a shortcut chosen to avoid calling arc4random() each time a tmpfs_node structure is recycled, and I very much doubt that allowing the counter in > 32 bits territory in this case adds any real security. Actually, the shortcut itself looks slightly dangerous, and it may be better to remove it entirely, but that's a different matter.

I am not sure why others are concerned about using __packed.
The uses are simple and not that frequently executed.

I don't think there are many more reasons than that it's better not to use __packed for the compiler to generate better, straightforward code, and that seeing __packed is surprising, as it is usually used for serialization/deserialization issues with other systems/components (it may be argued that this is somehow the case here, as we're in fact doing a kind of ser/des into the generic 'struct fid').

I don't really know how frequent fid building and processing are. Is this done at each NFS access request? In any case, I agree that performance-wise this doesn't matter currently as this is second order or more compared to what is in the implementation of tmpfs_fhtovp() (which scans all existing tmpfs nodes!).

[olce]

In D47956#1093749, @rmacklem wrote:

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

If we don't commit the changes here, yes, that would be enough. You can even commit it in the meantime if you prefer.

It might make more sense to convert the other file systems where the
_Static_assert() fails with code that just copies fid_data the way tmpfs does?
(tarfs is the current case)

I think we should actually do the opposite, i.e., stop copying unaligned structures/fields when what the code does is actually just access the fields once, and rely on __packed in cases where we can't reorder the fields to shrink the structure size enough.

This copying is in fact more or less implementing __packed by hand, and I think source-wise it's worse to do that than just using __packed (there may be different reasons to still want to avoid __packed, e.g., if it is believed that compilers are frequently buggy in this respect, on more exotic architectures).

In D47956#1093750, @rmacklem wrote:

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

That would technically work, but is the same case as tarfs above and I wouldn't do it for the same reasons exposed above.

I'd like to know if some people oppose the use of __packed in these contexts, and why.

[markj]

sys/fs/tmpfs/tmpfs.h

451

I would suggest adding an explicit pad field here, like the generic structure.

sys/fs/tmpfs/tmpfs_vfsops.c

606

Not related to your change, but this loop seems rather suboptimal. It looks like tmpfs has no efficient way to map an inode number to a vnode.

[olce]

Keep same size for the generation number, so as to facilitate a quick commit of the other changes.

[olce]

sys/fs/tmpfs/tmpfs.h

451

Structure is now packed. That said, I'd really want to simply remove the padding field fid_data0 of struct fid, enlarging fid_data to compensate, with the aim to really give freedom to FS about the content (going further, I'd remove also fid_len). Strictly speaking, this is an API break (but not an ABI one), but maybe it would still be acceptable to MFC it as nobody is supposed to ever use fid_data0 directly?

sys/fs/tmpfs/tmpfs_vfsops.c

606

Indeed... I mentioned this problem a few comments ago, saying that it caused the performance of __packed versus aligned fields to become second order at best.

[markj]

sys/fs/tmpfs/tmpfs.h

451

I don't have strong feelings about it. tmpfs filesystems are ephemeral, so probably are not commonly NFS-exported, and clients will need to remount them if the server reboots anyway. So, some of the downsides of changing the layout aren't really relevant for tmpfs. But, as I understand, this change does not fix any extant bug, so might not be worth the risk of merging to a stable branch.

[olce]

sys/fs/tmpfs/tmpfs.h

451

Mmm... It seems the risks are quite low (changes are very limited; there might be compiler bugs).

I can for example avoid MFCing this revision to stable/13, as the code slush is approaching for 13.5.

With the risk assessment written above in mind, I don't see much reason not to MFC to stable/13 after 13.5's branch and to stable/14 relatively soon as the next release from it is far away. I'll be personally testing the patch on stable/14 at the beginning of January. Or do you think the risk assessment above is incomplete or too optimistic?

[markj]

sys/fs/tmpfs/tmpfs.h

451

I would not merge this change to stable/13 at all. IMO we should not merge there unless fixing a concrete bug, which this change does not.

The change is probably low-risk, but I have no experience exporting tmpfs filesystems with NFS, and we have no regression tests covering that case.