Message-ID: <YXgg6VkAqe24Isgt@redhat.com>
Date:   Tue, 26 Oct 2021 11:38:17 -0400
From:   Vivek Goyal <vgoyal@...hat.com>
To:     Amir Goldstein <amir73il@...il.com>
Cc:     Ioannis Angelakopoulos <iangelak@...hat.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        virtio-fs-list <virtio-fs@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Jan Kara <jack@...e.cz>, Al Viro <viro@...iv.linux.org.uk>,
        Miklos Szeredi <miklos@...redi.hu>
Subject: Re: [RFC PATCH 5/7] Fsnotify: Add a wrapper around the fsnotify
 function

On Tue, Oct 26, 2021 at 05:37:55PM +0300, Amir Goldstein wrote:
> On Mon, Oct 25, 2021 at 11:47 PM Ioannis Angelakopoulos
> <iangelak@...hat.com> wrote:
> >
> > Generally, inotify events are generated locally by calling the "fsnotify"
> > function in fs/notify/fsnotify.c and various helper functions. However, now
> > we expect events to arrive from the FUSE server. Thus, without any
> > intervention a user space application will receive two events. One event is
> > generated locally and one arrives from the server.
> >
> > Hence, to avoid duplicate events we need to "suppress" the local events
> > generated by the guest kernel for FUSE inodes. To achieve this we add a
> > wrapper around the "fsnotify" function in fs/notify/fsnotify.c that
> > checks if the remote inotify is enabled and based on the check either it
> > "suppresses" or lets through a local event.
> >
> > The wrapper will be called in the place of the original "fsnotify" call
> > that is responsible for the event notification (now renamed as
> > "__fsnotify").
> >
> > When the remote inotify is not enabled, all local events will be let
> > through as expected. This process is completely transparent to user space.
> >
> > Signed-off-by: Ioannis Angelakopoulos <iangelak@...hat.com>
> > ---
> >  fs/notify/fsnotify.c             | 35 ++++++++++++++++++++++++++++++--
> >  include/linux/fsnotify_backend.h | 14 ++++++++++++-
> >  2 files changed, 46 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> > index 963e6ce75b96..848a824c29c4 100644
> > --- a/fs/notify/fsnotify.c
> > +++ b/fs/notify/fsnotify.c
> > @@ -440,7 +440,7 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
> >  }
> >
> >  /*
> > - * fsnotify - This is the main call to fsnotify.
> > + * __fsnotify - This is the main call to fsnotify.
> >   *
> >   * The VFS calls into hook specific functions in linux/fsnotify.h.
> >   * Those functions then in turn call here.  Here will call out to all of the
> > @@ -459,7 +459,7 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
> >   *             if both are non-NULL event may be reported to both.
> >   * @cookie:    inotify rename cookie
> >   */
> > -int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
> > +int __fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
> >              const struct qstr *file_name, struct inode *inode, u32 cookie)
> >  {
> >         const struct path *path = fsnotify_data_path(data, data_type);
> > @@ -552,6 +552,37 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
> >
> >         return ret;
> >  }
> > +
> > +/*
> > + * Wrapper around fsnotify. The main functionality is to filter local events in
> > + * case the inode belongs to a filesystem that supports remote events
> > + */
> > +int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
> > +            const struct qstr *file_name, struct inode *inode, u32 cookie)
> > +{
> > +
> > +       if (inode != NULL || dir != NULL) {
> > +               /*
> > +                * Check if the fsnotify_event operation is available which
> > +                * will let the remote inotify events go through and suppress
> > +                * the local events
> > +                */
> > +               if (inode && inode->i_op->fsnotify_event) {
> > +                       return inode->i_op->fsnotify_event(mask, data,
> > +                                                          data_type, dir,
> > +                                                          file_name, inode,
> > +                                                          cookie);
> > +               }
> > +               if (dir && dir->i_op->fsnotify_event) {
> > +                       return dir->i_op->fsnotify_event(mask, data,
> > +                                                        data_type, dir,
> > +                                                        file_name, inode,
> > +                                                        cookie);
> > +               }
> > +       }
> > +
> 
> That's not the way to accomplish what you want to do.
> 
> Assuming that we agree to let filesystem silence VFS fsnotify hooks
> it should be done using an inode flag, similar to file flag FMODE_NONOTIFY.

Ok, so basically define a new flag say FMODE_FOO and check that in
fsnotify() and ignore event if flag is set. And filesystem can set
that flag if remote events are enabled. And then vfs will ignore
local events. Sounds reasonable.

> 
> But the reason you want to do that seems a bit odd.

I gave this idea to Ioannis. But defining a inode flag probably is
much cheaper as comapred to inode operation.

> Duplicate events are going to be merged with fanotify and I think that
> you ruled out fanotify for the wrong reasons
> (you should have only ruled out permission events)

Ioannis was looking at fanoity and wondering if fanotify can be supported
as well. fanotify seemed to be much more powerful as compared to inotify
and some of the things looked like not feasible to be supported for
remote filesystems.

But if it is acceptable to support only limited functionality/events, then
it probably is a good idea to keep fanotify in mind and somebody can extend
it for fanotify as well.

Ideally it will be nice to support fanoity as well (as much as possible).
Just that it seemed more complicated. So we thought that let us start
with a simpler API (inotify) and try to implement that first.

I don't understand "Duplicate events are going to be merged with
fanotify". So fanotiy has something to figure out there are duplicate
events and merge these and user space never sees duplicate events.

Vivek

Powered by blists - more mailing lists