Re: [PATCH 7/8] virtiofs: Add new notification type FUSE_NOTIFY_LOCK

[Miklos Szeredi <miklos@szeredi.hu>]

On Thu, 7 Oct 2021 at 16:22, Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Thu, Oct 07, 2021 at 03:45:40PM +0200, Miklos Szeredi wrote:
> > On Wed, 6 Oct 2021 at 18:13, Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > On Wed, Oct 06, 2021 at 03:02:36PM +0200, Miklos Szeredi wrote:
> > > > On Thu, 30 Sept 2021 at 16:39, Vivek Goyal <vgoyal@redhat.com> wrote:
> > > > >
> > > > > Add a new notification type FUSE_NOTIFY_LOCK. This notification can be
> > > > > sent by file server to signifiy that a previous locking request has
> > > > > completed and actual caller should be woken up.
> > > >
> > > > Shouldn't this also be generic instead of lock specific?
> > > >
> > > > I.e. generic header  + original outarg.
> > >
> > > Hi Miklos,
> > >
> > > I am not sure I understand the idea. Can you please elaborate a bit more.
> > >
> > > IIUC, "fuse_out_header + original outarg"  is format for responding
> > > to regular fuse requests. If we use that it will become like responding
> > > to same request twice. First time we responded with ->error=1 so that
> > > caller can wait and second time we respond with actual outarg (if
> > > there is one depending on the type of request).
> > >
> > > IOW, this will become more like implementing blocking of request in
> > > client in a more generic manner.
> > >
> > > But outarg, depends on type of request (In case of locking there is
> > > none). And outarg memory is allocated by driver and filled by server.
> > > In case of notifications, driver is allocating the memory but it
> > > does not know what will come in notification and how much memory
> > > to allocate. So it relies on device telling it how much memory
> > > to allocate in general so that bunch of pre-defined notification
> > > types can fit in (fs->notify_buf_size).
> > >
> > > I modeled this on the same lines as other fuse notifications where
> > > server sends notifications with following format.
> > >
> > > fuse_out_header + <structure based on notification type>
> > >
> > > out_header->unique is 0 for notifications to differentiate notifications
> > > from request reply.
> > >
> > > out_header->error contains the code of actual notification being sent.
> > > ex. FUSE_NOTIFY_INVAL_INODE or FUSE_NOTIFY_LOCK or FUSE_NOTIFY_DELETE.
> > > Right now virtiofs supports only one notification type. But in future
> > > we can introduce more types (to support inotify stuff etc).
> > >
> > > In short, I modeled this on existing notion of fuse notifications
> > > (and not fuse reply). And given notifications are asynchronous,
> > > we don't know what were original outarg. In fact they might
> > > be generated not necessarily in response to a request. And that's
> > > why this notion of defining a type of notification (FUSE_NOTIFY_LOCK)
> > > and then let driver decide how to handle this notification.
> > >
> > > I might have completely misunderstood your suggestion. Please help
> > > me understand.
> >
> > Okay, so we are expecting this mechanism to be only used for blocking
> > locks.
>
> Yes, as of now it is only being used only for blocking locks. So there
> are two parts to it.
>
> A. For a blocking operation, server can reply with error=1, and that's
>    a signal to client to wait for a notification to arrive later. And
>    fuse client will not complete the request and instead will queue it
>    in one of the internal lists.
>
> B. Later server will send a fuse notification event (FUSE_NOTIFY_LOCK)
>    when it has acquired the lock. This notification will have unique
>    number of request for which this notification has been generated.
>    Fuse client will search for the request with expected unique number
>    in the list and complete the request.
>
> I think part A is generic in the sense it could be used for other
> kind of blocking requests as well down the line, where server is
> doing the blocking operation on behalf of client and will send
> notification later. Part B is very specific to blocking locks though.

I don't really get why B is specific to blocking locks. But anyway...
we are only implementing it for blocking locks for now.

>
> > That makes sense, but then locking ops should be setting a
> > flag indicating that this is locking op.  I.e. in fuse_setlk():
> >
> >     args.blocking_lock = true;
> >
> > And this should be verified when the reply with the positive error comes back.
>
> So this args.blocking_lock, goes to server as well? Or this is something
> internal to fuse client so that client can decide whether ->error=1 is
> a valid response or not. IOW, client is trying to do verification
> whether server should have generated ->error=1 or not for this specific
> request.

Right, it's for the client.

Thanks,
Miklos