Re: [virtio-dev] [PATCH v2 1/2] content: add virtio file system device

[Stefan Hajnoczi <stefanha@redhat.com>]

On Thu, Feb 14, 2019 at 08:20:58AM +0100, Miklos Szeredi wrote:
> On Thu, Feb 14, 2019 at 4:33 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Wed, Feb 13, 2019 at 05:47:19PM +0100, Paolo Bonzini wrote:
> > > On 13/02/19 07:33, Stefan Hajnoczi wrote:
> > > > +Notifications are different from normal requests because they only contain
> > > > +device writable fields.  The driver sends notification replies on one of the
> > > > +request queues.  The format of notification requests is as follows:
> > > > +
> > > > +\begin{lstlisting}
> > > > +struct virtio_fs_notification_req {
> > > > +        // Device-writable part
> > > > +        struct fuse_out_header out;
> > > > +        u8 dataout[];
> > > > +};
> > > > +\end{lstlisting}
> > > > +
> > > > +\field{out} is the completion header common to all types of FUSE requests.  The
> > > > +\field{out.unique} field is 0 and the \field{out.error} field contains a
> > > > +FUSE_NOTIFY_* code.
> > > > +
> > > > +\field{dataout} consists of request-specific data, if any.  This is identical
> > > > +to the data written to the /dev/fuse device by a FUSE daemon.
> > > > +
> > >
> > > What happens if notifications are lost because no request was there?
> > > virtio-scsi has a flag for that, would it make sense to add it here too?
> >
> > The FUSE protocol assumes notification delivery is reliable.  Some
> > notifications can be dropped with no or little impact on functionality,
> > but others cannot because it would cause a hung operation.
> >
> > Therefore the device must hold notifications until the driver makes
> > buffers available.  The question becomes what happens when the device
> > runs out of space to hold notifications.  At this point the device must
> > be reset because no further progress is possible.
> >
> > (In our current implementation notifications aren't used at all, but the
> > virtio-fs spec should allow for it so that the full FUSE protocol is
> > available for future applications.)
> >
> > Miklos: What do you think from the FUSE protocol point of view?
> 
> I  think notifications would be limited in functionality in the virtio
> case.  E.g. FUSE_NOTIFY_INVAL_INODE could be used if the server
> detects that cached attributes have become invalid.  If this is a best
> effort thing (doesn't block other clients) then it's okay.  But if
> it's for implementing strong coherency, then it doesn't work that
> well, since the broken client can block other clients from making
> progress.
>
> So I'm not sure.  Probably easier to leave notifications out of the
> implementation and the spec, until an actual use case arises.

I'd still like to discuss the options because it would be a real problem
to spec the device without notifications and then find out the design
cannot be extended when we need them.

When the device runs out of space to queue notifications for a slow
client, that client must be kicked out so that others can continue.
This seems like the most robust way to keep the file system available.
Only the client that couldn't keep up is hurt.

In our implementation each virtiofsd has a single client, so I'm not
sure the denial-of-service you described can occur (is that something
that involves the ireg daemon?).  The good thing is this means that
virtiofsd may block until its sole client replenishes notifications
buffers.  No other clients are hurt!

Stefan

Attachment: signature.asc
Description: PGP signature