s_export_op
On Thu, Apr 27, 2023 at 2:48 PM Jan Kara <[email protected]> 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 <[email protected]>
> ...
> > 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.
On Thu, Apr 27, 2023 at 3:28 PM Amir Goldstein <[email protected]> wrote:
>
> s_export_op
>
> On Thu, Apr 27, 2023 at 2:48 PM Jan Kara <[email protected]> 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 <[email protected]>
> > ...
> > > 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.
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 <[email protected]> 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 <[email protected]>
> > ...
> > > 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 <[email protected]>
SUSE Labs, CR