All of lore.kernel.org
 help / color / mirror / Atom feed
From: Vivek Goyal <vgoyal@redhat.com>
To: Stefan Hajnoczi <stefanha@redhat.com>
Cc: virtio-fs@redhat.com, qemu-devel@nongnu.org, miklos@szeredi.hu
Subject: Re: [Virtio-fs] [PATCH 4/4] virtiofsd: Implement blocking posix locks
Date: Mon, 25 Nov 2019 10:38:45 -0500	[thread overview]
Message-ID: <20191125153845.GB13247@redhat.com> (raw)
In-Reply-To: <20191122105324.GE464656@stefanha-x1.localdomain>

On Fri, Nov 22, 2019 at 10:53:24AM +0000, Stefan Hajnoczi wrote:
> On Fri, Nov 15, 2019 at 03:55:43PM -0500, Vivek Goyal wrote:
> > diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c
> > index d4a42d9804..f706e440bf 100644
> > --- a/contrib/virtiofsd/fuse_lowlevel.c
> > +++ b/contrib/virtiofsd/fuse_lowlevel.c
> > @@ -183,7 +183,8 @@ int fuse_send_reply_iov_nofree(fuse_req_t req, int error, struct iovec *iov,
> >  {
> >  	struct fuse_out_header out;
> >  
> > -	if (error <= -1000 || error > 0) {
> > +	/* error = 1 has been used to signal client to wait for notificaiton */
> > +	if (error <= -1000 || error > 1) {
> >  		fuse_log(FUSE_LOG_ERR, "fuse: bad error value: %i\n",	error);
> >  		error = -ERANGE;
> >  	}
> 
> What is this?

When a waiting lock request comes in, we need a way to reply back saying
wait for the notification. So I used value "1" for the
fuse_out_header->error field for this purpose. As of now, 0 is returned
for success and negative values for error code. So positive values seem
to be unused.

> 
> > +int fuse_lowlevel_notify_lock(struct fuse_session *se, uint64_t req_id,
> > +			      int32_t error)
> > +{
> > +	struct fuse_notify_lock_out outarg;
> 
> Missing = {} initialization to avoid information leaks to the guest.

Will do.

> 
> > @@ -1704,6 +1720,15 @@ int fuse_lowlevel_notify_delete(struct fuse_session *se,
> >  int fuse_lowlevel_notify_store(struct fuse_session *se, fuse_ino_t ino,
> >  			       off_t offset, struct fuse_bufvec *bufv,
> >  			       enum fuse_buf_copy_flags flags);
> > +/**
> > + * Notify event related to previous lock request
> > + *
> > + * @param se the session object
> > + * @param req_id the id of the request which requested setlkw
> 
> The rest of the code calls this id "unique":

Will change it.

> 
>   + * @param req_unique the unique id of the setlkw request
> 
> > +    /* Pop an element from queue */
> > +    req = vu_queue_pop(dev, q, sizeof(FVRequest), &bad_in_num, &bad_out_num);
> > +    if (!req) {
> > +        /* TODO: Implement some sort of ring buffer and queue notifications
> > +	 * on that and send these later when notification queue has space
> > +	 * available.
> > +	 */
> > +        return -ENOSPC;
> 
> Ah, I thought the point of the notifications processing thread was
> exactly this case.  It could wake any threads waiting for buffers.
> 
> This wakeup could be implemented with a condvar - no ring buffer
> necessary.

I was thinking that thread sending notification should not block. It can
just queue the notification reuqest and some other thread (including
notification thread could send it later). Number of pre-allocated buffers
could be of fixed and we will drop notifications if guest is not
responding. This will also take care of concerns w.r.t rogue guest
blocking filesystem code in daemon.

Anyway, this is a TODO item and not implemented yet. 

Thanks
Vivek


  reply	other threads:[~2019-11-25 15:38 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-15 20:55 [PATCH 0/4] [RFC] virtiofsd, vhost-user-fs: Add support for notification queue Vivek Goyal
2019-11-15 20:55 ` [Virtio-fs] " Vivek Goyal
2019-11-15 20:55 ` [PATCH 1/4] virtiofsd: Release file locks using F_UNLCK Vivek Goyal
2019-11-15 20:55   ` [Virtio-fs] " Vivek Goyal
2019-11-22 10:07   ` Stefan Hajnoczi
2019-11-22 10:07     ` [Virtio-fs] " Stefan Hajnoczi
2019-11-22 13:45     ` Vivek Goyal
2019-11-22 13:45       ` [Virtio-fs] " Vivek Goyal
2019-11-15 20:55 ` [PATCH 2/4] virtiofd: Create a notification queue Vivek Goyal
2019-11-15 20:55   ` [Virtio-fs] " Vivek Goyal
2019-11-22 10:19   ` Stefan Hajnoczi
2019-11-22 10:19     ` [Virtio-fs] " Stefan Hajnoczi
2019-11-22 14:47     ` Vivek Goyal
2019-11-22 14:47       ` [Virtio-fs] " Vivek Goyal
2019-11-22 17:29       ` Dr. David Alan Gilbert
2019-11-22 17:29         ` [Virtio-fs] " Dr. David Alan Gilbert
2019-11-15 20:55 ` [PATCH 3/4] virtiofsd: Specify size of notification buffer using config space Vivek Goyal
2019-11-15 20:55   ` [Virtio-fs] " Vivek Goyal
2019-11-22 10:33   ` Stefan Hajnoczi
2019-11-22 10:33     ` [Virtio-fs] " Stefan Hajnoczi
2019-11-25 14:57     ` Vivek Goyal
2019-11-25 14:57       ` [Virtio-fs] " Vivek Goyal
2019-11-15 20:55 ` [PATCH 4/4] virtiofsd: Implement blocking posix locks Vivek Goyal
2019-11-15 20:55   ` [Virtio-fs] " Vivek Goyal
2019-11-22 10:53   ` Stefan Hajnoczi
2019-11-22 10:53     ` [Virtio-fs] " Stefan Hajnoczi
2019-11-25 15:38     ` Vivek Goyal [this message]
2019-11-22 17:47   ` Dr. David Alan Gilbert
2019-11-22 17:47     ` [Virtio-fs] " Dr. David Alan Gilbert
2019-11-25 15:44     ` Vivek Goyal
2019-11-26 13:02       ` Dr. David Alan Gilbert
2019-11-27 19:08         ` Vivek Goyal
2019-12-09 11:06           ` Dr. David Alan Gilbert

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191125153845.GB13247@redhat.com \
    --to=vgoyal@redhat.com \
    --cc=miklos@szeredi.hu \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanha@redhat.com \
    --cc=virtio-fs@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.