Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Chuck Lever]

On Tue, Apr 25, 2023 at 04:01:01PM +0300, Amir Goldstein wrote:
> 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/
> [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(-)

Hello Amir-

Note that the maintainers of exportfs are Jeff and myself. Please
copy us on future versions of this patch series. Thanks!

[cel@klimt even-releases]$ scripts/get_maintainer.pl fs/exportfs/
Chuck Lever <chuck.lever@oracle.com> (supporter:KERNEL NFSD, SUNRPC, AND LOCKD SERVERS)
Jeff Layton <jlayton@kernel.org> (supporter:KERNEL NFSD, SUNRPC, AND LOCKD SERVERS)
linux-nfs@vger.kernel.org (open list:KERNEL NFSD, SUNRPC, AND LOCKD SERVERS)
linux-kernel@vger.kernel.org (open list)

Re: [RFC][PATCH 1/4] exportfs: change connectable argument to bit flags

[Chuck Lever]

On Tue, Apr 25, 2023 at 04:01:02PM +0300, Amir Goldstein wrote:
> Convert the bool connectable arguemnt into a bit flags argument and
> define the EXPORT_FS_CONNECTABLE flag as a requested property of the
> file handle.
> 
> We are going to add a flag for requesting non-decodeable file handles.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/exportfs/expfs.c      | 11 +++++++++--
>  fs/nfsd/nfsfh.c          |  5 +++--
>  include/linux/exportfs.h |  6 ++++--
>  3 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index ab88d33d106c..bf1b4925fedd 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -393,14 +393,21 @@ int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
>  }
>  EXPORT_SYMBOL_GPL(exportfs_encode_inode_fh);
>  
> +/**
> + * exportfs_encode_fh - encode a file handle from dentry
> + * @dentry:  the object to encode
> + * @fid:     where to store the file handle fragment
> + * @max_len: maximum length to store there
> + * @flags:   properties of the requrested file handle

Thanks for adding the KDOC comment!

/requrested/requested

And please add:

 *
 * Returns an enum fid_type or a negative errno

> + */
>  int exportfs_encode_fh(struct dentry *dentry, struct fid *fid, int *max_len,
> -		int connectable)
> +		       int flags)
>  {
>  	int error;
>  	struct dentry *p = NULL;
>  	struct inode *inode = dentry->d_inode, *parent = NULL;
>  
> -	if (connectable && !S_ISDIR(inode->i_mode)) {
> +	if ((flags & EXPORT_FH_CONNECTABLE) && !S_ISDIR(inode->i_mode)) {
>  		p = dget_parent(dentry);
>  		/*
>  		 * note that while p might've ceased to be our parent already,
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index ccd8485fee04..31e4505c0df3 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -414,10 +414,11 @@ static void _fh_update(struct svc_fh *fhp, struct svc_export *exp,
>  		struct fid *fid = (struct fid *)
>  			(fhp->fh_handle.fh_fsid + fhp->fh_handle.fh_size/4 - 1);
>  		int maxsize = (fhp->fh_maxsize - fhp->fh_handle.fh_size)/4;
> -		int subtreecheck = !(exp->ex_flags & NFSEXP_NOSUBTREECHECK);
> +		int fh_flags = (exp->ex_flags & NFSEXP_NOSUBTREECHECK) ? 0 :
> +				EXPORT_FH_CONNECTABLE;
>  
>  		fhp->fh_handle.fh_fileid_type =
> -			exportfs_encode_fh(dentry, fid, &maxsize, subtreecheck);
> +			exportfs_encode_fh(dentry, fid, &maxsize, fh_flags);
>  		fhp->fh_handle.fh_size += maxsize * 4;
>  	} else {
>  		fhp->fh_handle.fh_fileid_type = FILEID_ROOT;
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index 601700fedc91..2b1048238170 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -135,6 +135,8 @@ struct fid {
>  	};
>  };
>  
> +#define EXPORT_FH_CONNECTABLE	0x1
> +
>  /**
>   * struct export_operations - for nfsd to communicate with file systems
>   * @encode_fh:      encode a file handle fragment from a dentry
> @@ -150,7 +152,7 @@ struct fid {
>   * encode_fh:
>   *    @encode_fh should store in the file handle fragment @fh (using at most
>   *    @max_len bytes) information that can be used by @decode_fh to recover the
> - *    file referred to by the &struct dentry @de.  If the @connectable flag is
> + *    file referred to by the &struct dentry @de.  If @flag has CONNECTABLE bit
>   *    set, the encode_fh() should store sufficient information so that a good
>   *    attempt can be made to find not only the file but also it's place in the
>   *    filesystem.   This typically means storing a reference to de->d_parent in
> @@ -226,7 +228,7 @@ struct export_operations {
>  extern int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
>  				    int *max_len, struct inode *parent);
>  extern int exportfs_encode_fh(struct dentry *dentry, struct fid *fid,
> -	int *max_len, int connectable);
> +			      int *max_len, int flags);
>  extern struct dentry *exportfs_decode_fh_raw(struct vfsmount *mnt,
>  					     struct fid *fid, int fh_len,
>  					     int fileid_type,
> -- 
> 2.34.1
> 

Re: [RFC][PATCH 2/4] exportfs: add explicit flag to request non-decodeable file handles

[Chuck Lever]

On Tue, Apr 25, 2023 at 04:01:03PM +0300, Amir Goldstein wrote:
> So far, all callers of exportfs_encode_inode_fh(), except for fsnotify's
> show_mark_fhandle(), check that filesystem can decode file handles, but
> we would like to add more callers that do not require a file handle that
> can be decoded.
> 
> Introduce a flag to explicitly request a file handle that may not to be
> decoded later and a wrapper exportfs_encode_fid() that sets this flag
> and convert show_mark_fhandle() to use the new wrapper.
> 
> This will be used to allow adding fanotify support to filesystems that
> do not support NFS export.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  Documentation/filesystems/nfs/exporting.rst |  4 ++--
>  fs/exportfs/expfs.c                         | 18 ++++++++++++++++--
>  fs/notify/fanotify/fanotify.c               |  4 ++--
>  fs/notify/fdinfo.c                          |  2 +-
>  include/linux/exportfs.h                    | 12 +++++++++++-
>  5 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/filesystems/nfs/exporting.rst b/Documentation/filesystems/nfs/exporting.rst
> index 0e98edd353b5..3d97b8d8f735 100644
> --- a/Documentation/filesystems/nfs/exporting.rst
> +++ b/Documentation/filesystems/nfs/exporting.rst
> @@ -122,8 +122,8 @@ are exportable by setting the s_export_op field in the struct
>  super_block.  This field must point to a "struct export_operations"
>  struct which has the following members:
>  
> - encode_fh  (optional)
> -    Takes a dentry and creates a filehandle fragment which can later be used
> +  encode_fh (optional)
> +    Takes a dentry and creates a filehandle fragment which may later be used
>      to find or create a dentry for the same object.  The default
>      implementation creates a filehandle fragment that encodes a 32bit inode
>      and generation number for the inode encoded, and if necessary the
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index bf1b4925fedd..1b35dda5bdda 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -381,11 +381,25 @@ static int export_encode_fh(struct inode *inode, struct fid *fid,
>  	return type;
>  }
>  
> +/**
> + * exportfs_encode_inode_fh - encode a file handle from inode
> + * @inode:   the object to encode
> + * @fid:     where to store the file handle fragment
> + * @max_len: maximum length to store there
> + * @flags:   properties of the requrested file handle
> + */

Same comment here: /requrested/requested and add a " * Returns an ..."

>  int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
> -			     int *max_len, struct inode *parent)
> +			     int *max_len, struct inode *parent, int flags)
>  {
>  	const struct export_operations *nop = inode->i_sb->s_export_op;
>  
> +	/*
> +	 * If a decodeable file handle was requested, we need to make sure that
> +	 * filesystem can decode file handles.
> +	 */
> +	if (nop && !(flags & EXPORT_FH_FID) && !nop->fh_to_dentry)
> +		return -EOPNOTSUPP;
> +
>  	if (nop && nop->encode_fh)
>  		return nop->encode_fh(inode, fid->raw, max_len, parent);
>  
> @@ -416,7 +430,7 @@ int exportfs_encode_fh(struct dentry *dentry, struct fid *fid, int *max_len,
>  		parent = p->d_inode;
>  	}
>  
> -	error = exportfs_encode_inode_fh(inode, fid, max_len, parent);
> +	error = exportfs_encode_inode_fh(inode, fid, max_len, parent, flags);
>  	dput(p);
>  
>  	return error;
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 29bdd99b29fa..d1a49f5b6e6d 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -380,7 +380,7 @@ static int fanotify_encode_fh_len(struct inode *inode)
>  	if (!inode)
>  		return 0;
>  
> -	exportfs_encode_inode_fh(inode, NULL, &dwords, NULL);
> +	exportfs_encode_inode_fh(inode, NULL, &dwords, NULL, 0);
>  	fh_len = dwords << 2;
>  
>  	/*
> @@ -443,7 +443,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
>  	}
>  
>  	dwords = fh_len >> 2;
> -	type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);
> +	type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL, 0);
>  	err = -EINVAL;
>  	if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
>  		goto out_err;
> diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
> index 55081ae3a6ec..5c430736ec12 100644
> --- a/fs/notify/fdinfo.c
> +++ b/fs/notify/fdinfo.c
> @@ -50,7 +50,7 @@ static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
>  	f.handle.handle_bytes = sizeof(f.pad);
>  	size = f.handle.handle_bytes >> 2;
>  
> -	ret = exportfs_encode_inode_fh(inode, (struct fid *)f.handle.f_handle, &size, NULL);
> +	ret = exportfs_encode_fid(inode, (struct fid *)f.handle.f_handle, &size);
>  	if ((ret == FILEID_INVALID) || (ret < 0)) {
>  		WARN_ONCE(1, "Can't encode file handler for inotify: %d\n", ret);
>  		return;
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index 2b1048238170..635e89e1dae7 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -136,6 +136,7 @@ struct fid {
>  };
>  
>  #define EXPORT_FH_CONNECTABLE	0x1
> +#define EXPORT_FH_FID		0x2
>  
>  /**
>   * struct export_operations - for nfsd to communicate with file systems
> @@ -226,9 +227,18 @@ struct export_operations {
>  };
>  
>  extern int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
> -				    int *max_len, struct inode *parent);
> +				    int *max_len, struct inode *parent,
> +				    int flags);
>  extern int exportfs_encode_fh(struct dentry *dentry, struct fid *fid,
>  			      int *max_len, int flags);
> +
> +static inline int exportfs_encode_fid(struct inode *inode, struct fid *fid,
> +				      int *max_len)
> +{
> +	return exportfs_encode_inode_fh(inode, fid, max_len, NULL,
> +					EXPORT_FH_FID);
> +}
> +
>  extern struct dentry *exportfs_decode_fh_raw(struct vfsmount *mnt,
>  					     struct fid *fid, int fh_len,
>  					     int fileid_type,
> -- 
> 2.34.1
> 

Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Amir Goldstein]

On Wed, Apr 26, 2023 at 4:47 PM Chuck Lever <cel@kernel.org> wrote:
>
> On Tue, Apr 25, 2023 at 04:01:01PM +0300, Amir Goldstein wrote:
> > 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/
> > [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(-)
>
> Hello Amir-
>
> Note that the maintainers of exportfs are Jeff and myself. Please
> copy us on future versions of this patch series. Thanks!

Sorry. Will do.
Thanks for the review!

Amir.

Re: [RFC][PATCH 4/4] fanotify: support reporting non-decodeable file handles

[Amir Goldstein]

s_export_op

On Thu, Apr 27, 2023 at 2:48 PM Jan Kara <jack@suse.cz> wrote:
>
> On Tue 25-04-23 16:01:05, Amir Goldstein wrote:
> > fanotify users do not always need to decode the file handles reported
> > with FAN_REPORT_FID.
> >
> > Relax the restriction that filesystem needs to support NFS export and
> > allow reporting file handles from filesystems that only support ecoding
> > unique file handles.
> >
> > For such filesystems, users will have to use the AT_HANDLE_FID of
> > name_to_handle_at(2) if they want to compare the object in path to the
> > object fid reported in an event.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ...
> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index 8f430bfad487..a5af84cbb30d 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -1586,11 +1586,9 @@ static int fanotify_test_fid(struct dentry *dentry)
> >        * We need to make sure that the file system supports at least
> >        * encoding a file handle so user can use name_to_handle_at() to
> >        * compare fid returned with event to the file handle of watched
> > -      * objects. However, name_to_handle_at() requires that the
> > -      * filesystem also supports decoding file handles.
> > +      * objects, but it does not need to support decoding file handles.
> >        */
> > -     if (!dentry->d_sb->s_export_op ||
> > -         !dentry->d_sb->s_export_op->fh_to_dentry)
> > +     if (!dentry->d_sb->s_export_op)
> >               return -EOPNOTSUPP;
>
> So AFAICS the only thing you require is that s_export_op is set to
> *something* as exportfs_encode_inode_fh() can deal with NULL ->encode_fh
> just fine without any filesystem cooperation. What is the reasoning behind
> the dentry->d_sb->s_export_op check? Is there an implicit expectation that
> if s_export_op is set to something, the filesystem has sensible
> i_generation? Or is it just a caution that you don't want the functionality
> to be enabled for unexpected filesystems?

A little bit of both.
Essentially, I do not want to use the generic encoding unless the filesystem
opted-in to say "This is how objects should be identified".

The current fs that have s_export_op && !s_export_op->encode_fh
practically make that statement because they support NFS export
(i.e. they implement fh_to_dentry()).

I don't like the implicit fallback to generic encoding, especially when
introducing this new functionality of encode_fid().

Before posting this patch set I had two earlier revisions.
One that changed the encode_fh() to mandatory and converted
all the INO32_GEN fs to explicitly set
s_export_op.encode_fh = generic_encode_ino32_fh,
And one that marked all the INO32_GEN fs with
s_export_op.flags = EXPORT_OP_ENCODE_INO32_GEN
in both cases there was no blind fallback to INO32_GEN.

But in the end, these added noise without actual value so
I dropped them, because the d_sb->s_export_op check is anyway
a pretty strong indication for opt-in to export fids.

CC exportfs maintainers in case they have an opinion one
way or the other.

> In either case it would be good
> to capture the reasoning either in a comment or the changelog...
>

Will do.

Thanks,
Amir.

Re: [RFC][PATCH 4/4] fanotify: support reporting non-decodeable file handles

[Amir Goldstein]

On Thu, Apr 27, 2023 at 3:28 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> s_export_op
>
> On Thu, Apr 27, 2023 at 2:48 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Tue 25-04-23 16:01:05, Amir Goldstein wrote:
> > > fanotify users do not always need to decode the file handles reported
> > > with FAN_REPORT_FID.
> > >
> > > Relax the restriction that filesystem needs to support NFS export and
> > > allow reporting file handles from filesystems that only support ecoding
> > > unique file handles.
> > >
> > > For such filesystems, users will have to use the AT_HANDLE_FID of
> > > name_to_handle_at(2) if they want to compare the object in path to the
> > > object fid reported in an event.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ...
> > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > > index 8f430bfad487..a5af84cbb30d 100644
> > > --- a/fs/notify/fanotify/fanotify_user.c
> > > +++ b/fs/notify/fanotify/fanotify_user.c
> > > @@ -1586,11 +1586,9 @@ static int fanotify_test_fid(struct dentry *dentry)
> > >        * We need to make sure that the file system supports at least
> > >        * encoding a file handle so user can use name_to_handle_at() to
> > >        * compare fid returned with event to the file handle of watched
> > > -      * objects. However, name_to_handle_at() requires that the
> > > -      * filesystem also supports decoding file handles.
> > > +      * objects, but it does not need to support decoding file handles.
> > >        */
> > > -     if (!dentry->d_sb->s_export_op ||
> > > -         !dentry->d_sb->s_export_op->fh_to_dentry)
> > > +     if (!dentry->d_sb->s_export_op)
> > >               return -EOPNOTSUPP;
> >
> > So AFAICS the only thing you require is that s_export_op is set to
> > *something* as exportfs_encode_inode_fh() can deal with NULL ->encode_fh
> > just fine without any filesystem cooperation. What is the reasoning behind
> > the dentry->d_sb->s_export_op check? Is there an implicit expectation that
> > if s_export_op is set to something, the filesystem has sensible
> > i_generation? Or is it just a caution that you don't want the functionality
> > to be enabled for unexpected filesystems?
>
> A little bit of both.
> Essentially, I do not want to use the generic encoding unless the filesystem
> opted-in to say "This is how objects should be identified".
>
> The current fs that have s_export_op && !s_export_op->encode_fh
> practically make that statement because they support NFS export
> (i.e. they implement fh_to_dentry()).
>
> I don't like the implicit fallback to generic encoding, especially when
> introducing this new functionality of encode_fid().
>
> Before posting this patch set I had two earlier revisions.
> One that changed the encode_fh() to mandatory and converted
> all the INO32_GEN fs to explicitly set
> s_export_op.encode_fh = generic_encode_ino32_fh,
> And one that marked all the INO32_GEN fs with
> s_export_op.flags = EXPORT_OP_ENCODE_INO32_GEN
> in both cases there was no blind fallback to INO32_GEN.
>
> But in the end, these added noise without actual value so
> I dropped them, because the d_sb->s_export_op check is anyway
> a pretty strong indication for opt-in to export fids.
>
> CC exportfs maintainers in case they have an opinion one
> way or the other.
>

BTW, the other reason I chose this requirement is circular -
this is the same requirement for exporting fids to user with
AT_HANDLE_FID and the two FAN_REPORT_FID should be
aligned with users ability to get fid with name_to_handle_at().

Another reasonable requirement would have been:
* !AT_HANDLE_FID requires ->fh_to_dentry (as current code)
* AT_HANDLE_FID requires either ->fh_to_dentry or encode_fh

Thanks,
Amir.

Re: [RFC][PATCH 4/4] fanotify: support reporting non-decodeable file handles

[Jan Kara]

On Thu 27-04-23 15:28:23, Amir Goldstein wrote:
> s_export_op
> 
> On Thu, Apr 27, 2023 at 2:48 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Tue 25-04-23 16:01:05, Amir Goldstein wrote:
> > > fanotify users do not always need to decode the file handles reported
> > > with FAN_REPORT_FID.
> > >
> > > Relax the restriction that filesystem needs to support NFS export and
> > > allow reporting file handles from filesystems that only support ecoding
> > > unique file handles.
> > >
> > > For such filesystems, users will have to use the AT_HANDLE_FID of
> > > name_to_handle_at(2) if they want to compare the object in path to the
> > > object fid reported in an event.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ...
> > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > > index 8f430bfad487..a5af84cbb30d 100644
> > > --- a/fs/notify/fanotify/fanotify_user.c
> > > +++ b/fs/notify/fanotify/fanotify_user.c
> > > @@ -1586,11 +1586,9 @@ static int fanotify_test_fid(struct dentry *dentry)
> > >        * We need to make sure that the file system supports at least
> > >        * encoding a file handle so user can use name_to_handle_at() to
> > >        * compare fid returned with event to the file handle of watched
> > > -      * objects. However, name_to_handle_at() requires that the
> > > -      * filesystem also supports decoding file handles.
> > > +      * objects, but it does not need to support decoding file handles.
> > >        */
> > > -     if (!dentry->d_sb->s_export_op ||
> > > -         !dentry->d_sb->s_export_op->fh_to_dentry)
> > > +     if (!dentry->d_sb->s_export_op)
> > >               return -EOPNOTSUPP;
> >
> > So AFAICS the only thing you require is that s_export_op is set to
> > *something* as exportfs_encode_inode_fh() can deal with NULL ->encode_fh
> > just fine without any filesystem cooperation. What is the reasoning behind
> > the dentry->d_sb->s_export_op check? Is there an implicit expectation that
> > if s_export_op is set to something, the filesystem has sensible
> > i_generation? Or is it just a caution that you don't want the functionality
> > to be enabled for unexpected filesystems?
> 
> A little bit of both.
> Essentially, I do not want to use the generic encoding unless the filesystem
> opted-in to say "This is how objects should be identified".
> 
> The current fs that have s_export_op && !s_export_op->encode_fh
> practically make that statement because they support NFS export
> (i.e. they implement fh_to_dentry()).

Makes sense.

> I don't like the implicit fallback to generic encoding, especially when
> introducing this new functionality of encode_fid().
> 
> Before posting this patch set I had two earlier revisions.
> One that changed the encode_fh() to mandatory and converted
> all the INO32_GEN fs to explicitly set
> s_export_op.encode_fh = generic_encode_ino32_fh,
> And one that marked all the INO32_GEN fs with
> s_export_op.flags = EXPORT_OP_ENCODE_INO32_GEN
> in both cases there was no blind fallback to INO32_GEN.
> 
> But in the end, these added noise without actual value so
> I dropped them, because the d_sb->s_export_op check is anyway
> a pretty strong indication for opt-in to export fids.
> 
> CC exportfs maintainers in case they have an opinion one
> way or the other.

Yeah, I agree with your judgement. I just wanted to make sure I understand
everything properly.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR