On Thu, 2023-04-27 at 22:11 +0300, Amir Goldstein wrote:

handle_bytes

On Thu, Apr 27, 2023 at 7:36 PM Jeff Layton [off-list ref] wrote:

quoted

On Thu, 2023-04-27 at 18:52 +0300, Amir Goldstein wrote:

quoted

On Thu, Apr 27, 2023 at 6:13 PM Jeff Layton [off-list ref] wrote:

quoted

On Tue, 2023-04-25 at 16:01 +0300, Amir Goldstein wrote:

quoted

Jan,

Following up on the FAN_REPORT_ANY_FID proposal [1], here is a shot at an
alternative proposal to seamlessly support more filesystems.

While fanotify relaxes the requirements for filesystems to support
reporting fid to require only the ->encode_fh() operation, there are
currently no new filesystems that meet the relaxed requirements.

I will shortly post patches that allow overlayfs to meet the new
requirements with default overlay configurations.

The overlay and vfs/fanotify patch sets are completely independent.
The are both available on my github branch [2] and there is a simple
LTP test variant that tests reporting fid from overlayfs [3], which
also demonstrates the minor UAPI change of name_to_handle_at(2) for
requesting a non-decodeable file handle by userspace.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/20230417162721.ouzs33oh6mb7vtft@quack3/ (local)
[2] https://github.com/amir73il/linux/commits/exportfs_encode_fid
[3] https://github.com/amir73il/ltp/commits/exportfs_encode_fid

Amir Goldstein (4):
  exportfs: change connectable argument to bit flags
  exportfs: add explicit flag to request non-decodeable file handles
  exportfs: allow exporting non-decodeable file handles to userspace
  fanotify: support reporting non-decodeable file handles

 Documentation/filesystems/nfs/exporting.rst |  4 +--
 fs/exportfs/expfs.c                         | 29 ++++++++++++++++++---
 fs/fhandle.c                                | 20 ++++++++------
 fs/nfsd/nfsfh.c                             |  5 ++--
 fs/notify/fanotify/fanotify.c               |  4 +--
 fs/notify/fanotify/fanotify_user.c          |  6 ++---
 fs/notify/fdinfo.c                          |  2 +-
 include/linux/exportfs.h                    | 18 ++++++++++---
 include/uapi/linux/fcntl.h                  |  5 ++++
 9 files changed, 67 insertions(+), 26 deletions(-)

This set looks fairly benign to me, so ACK on the general concept.

Thanks!

quoted

I am starting to dislike how the AT_* flags are turning into a bunch of
flags that only have meanings on certain syscalls. I don't see a cleaner
way to handle it though.

Yeh, it's not great.

There is also a way to extend the existing API with:

Perhstruct file_handle {
        unsigned int handle_bytes:8;
        unsigned int handle_flags:24;
        int handle_type;
        unsigned char f_handle[];
};

AFAICT, this is guaranteed to be backward compat
with old kernels and old applications.

That could work. It would probably look cleaner as a union though.
Something like this maybe?

union {
        unsigned int legacy_handle_bytes;
        struct {
                u8      handle_bytes;
                u8      __reserved;
                u16     handle_flags;
        };
}

I have no problem with the union, but does this struct
guarantee that the lowest byte of legacy_handle_bytes
is in handle_bytes for all architectures?

That is a very good point. 

That's the reason I went with

struct {
         unsigned int handle_bytes:8;
         unsigned int handle_flags:24;
}

Is there a problem with this approach?

I just have a natural aversion to bitfields.

What you're proposing would work fine, I think. You won't be able to
take a pointer into the bitfield of course, but that's not necessarily a
showstopper for an "interface struct" like file_handle.

quoted

quoted

        unsigned int handle_bytes:8;
        unsigned int handle_flags:24;

__reserved must be zeroed (for now). You could consider using it for
some other purpose later.

It's a little ugly as an API but it would be backward compatible, given
that we never use the high bits today anyway.

Callers might need to deal with an -EINVAL when they try to pass non-
zero handle_flags to existing kernels, since you'd trip the
MAX_HANDLE_SZ check that's there today.

Exactly.

quoted

quoted

It also may not be a bad idea that the handle_flags could
be used to request specific fh properties (FID) and can also
describe the properties of the returned fh (i.e. non-decodeable)
that could also be respected by open_by_handle_at().

For backward compact, kernel will only set handle_flags in
response if new flags were set in the request.

Do you consider this extension better than AT_HANDLE_FID
or worse? At least it is an API change that is contained within the
exportfs subsystem, without polluting the AT_ flags global namespace.

Personally, yes. I think adding a struct file_handle_v2 would be cleaner
and allows for expanding the API later through new flags.

I agree.
I will give it a try.

Cool.
-- 
Jeff Layton [off-list ref]