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

[Vivek Goyal <vgoyal@redhat.com>]

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.

> 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.

In case of virtiofs server is trusted entity. So this kind of verification
might be nice to have but not necessary.

Thanks
Vivek