Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230425130105.2606684-1-amir73il@gmail.com> <20230425130105.2606684-5-amir73il@gmail.com>
 <20230427114849.cv3kzxk7rvxpohjc@quack3>
In-Reply-To: <20230427114849.cv3kzxk7rvxpohjc@quack3>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Thu, 27 Apr 2023 15:28:23 +0300
Message-ID: <CAOQ4uxhBaZ4_c5Ko6jZ6UzqtB-4spE_xiRC=TNMO8+bwnYMSnA@mail.gmail.com>
Subject: Re: [RFC][PATCH 4/4] fanotify: support reporting non-decodeable file handles
To:     Jan Kara <jack@suse.cz>
Cc:     Christian Brauner <brauner@kernel.org>,
        Miklos Szeredi <miklos@szeredi.hu>,
        linux-unionfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-api@vger.kernel.org, Chuck Lever <cel@kernel.org>,
        Jeff Layton <jlayton@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

s_export_op

On Thu, Apr 27, 2023 at 2:48=E2=80=AFPM 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/fa=
notify_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 *dent=
ry)
> >        * 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 behin=
d
> the dentry->d_sb->s_export_op check? Is there an implicit expectation tha=
t
> 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 functionali=
ty
> to be enabled for unexpected filesystems?

A little bit of both.
Essentially, I do not want to use the generic encoding unless the filesyste=
m
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 =3D generic_encode_ino32_fh,
And one that marked all the INO32_GEN fs with
s_export_op.flags =3D 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.