[Virtio-fs] [virtiofsd-rs] Issue opened: [RFE] Reporting failure to generate file handles

[virtiofs-bot at sinrega.org]

The `--inode-file-handles` switch is basically just a best-effort suggestion: If a file handle cannot be generated, we still fall back to keeping an `O_PATH` FD for the inode in question.  The rationale for this is:
1. Vivek proposed a variant where file handles were mandatory, and failure to generate file handles would result in an all-out error, quitting virtiofsd.  However, I could not really imagine a case where this would be preferable behavior over it just working and falling back to `O_PATH` FDs, especially in the original context of using file handles only to reduce the number of open FDs.
2. In order not to quit at runtime, we’d have to somehow find out at start-up whether file handles will work for everything in the shared directory.  Even if that were possible (I don’t believe it is), it would be complex code to write.  So we’d probably end up with complex test code at start-up, and might still run into errors at runtime.

Now two things have happened to change my opinion:
1. There are cases where `--inode-file-handles` should have fixed some concrete issue, but using it didn’t change anything.  For such cases, it would be nice to have some indication that it doesn’t work as expected, so nobody has to run `ls /proc/$(pidof virtiofsd-rs)/fd | wc -l` to find out.
2. It turns out there’s at least one other case for which file handles are useful besides just reducing the number of open FDs; and that is to get around NFS’s file renaming.  For this case, it may indeed be preferable for users to receive an error of some sort instead of us falling back to `O_PATH` FDs and thus potentially exposing the less-than-ideal interaction that will then occur.

I’m now opening this (gitlab) issue to see whether someone has opinions on these (file handle usage) issues.

I think we want different solutions for both.  For the first one, we want to have warnings that sensibly log when we have to fall back.  By “sensibly�, I mean that most of the time, generating a file handle will either always or never work for a given mount, and so we should generally only warn once per mount (unless it’s a one-time issue like `name_to_handle_at()` returning `ENOMEM`).

What I’m much less sure about is how to log these warnings.  Intuitively, I’d log them at *warn* level, but the default level we have is *error*, so it’s unlikely that users would see them.  Should we log them at level *error*?

For the second case (NFS), I would throw an actual error somewhere.  We really shouldn’t quit virtiofsd over it, but we can return `EIO` to the guest or something.  Off the top of my head, I have these rough ideas:
* If generating a file handle fails on an NFS mount (discoverable through mountinfo), we not only log a warning, but return an error to the guest.  That’d be a change in behavior, and perhaps surprising that NFS behaves differently than other filesystems, but it wouldn’t require users to take special considerations.  We’d automatically interpret `--inode-file-handles` as strict as we deem sensible, depending on the situation.
* Add some other mode (like `--inode-file-handles=mandatory` mentioned above) where any error to generate a file handle is passed through to the guest (not necessarily verbatim, perhaps just as a generic `EIO`).  Users would be responsible to choose the behavior they need.  On the plus side, on NFS we’d continue to by default fall back to `O_PATH` FDs as on any other FS, and the user needs to consciously pass a different parameter for more strict behavior.  But that’s also the downside, of course, the user needs to make this conscious decision, and if they don’t, they may still run into the errors stemming from NFS’s file renaming.  Perhaps to encourage users to take this decision, we could log a warning that `--inode-file-handles=mandatory` is recommended when we detect some mount in the shared directory to be NFS, if that parameter wasn’t passed.

Thoughts?
---
https://gitlab.com/virtio-fs/virtiofsd-rs/-/issues/17