Re: [Virtio-fs] [PATCH 4/4] virtiofsd: Implement blocking posix locks
* Vivek Goyal (vgo...@redhat.com) wrote:
> On Tue, Nov 26, 2019 at 01:02:29PM +, Dr. David Alan Gilbert wrote:
>
> [..]
> > > > > @@ -1950,21 +1948,54 @@ static void lo_setlk(fuse_req_t req,
> > > > > fuse_ino_t ino,
> > > > >
> > > > > if (!plock) {
> > > > > saverr = ret;
> > > > > + pthread_mutex_unlock(>plock_mutex);
> > > > > goto out;
> > > > > }
> > > > >
> > > > > + /*
> > > > > + * plock is now released when inode is going away. We already
> > > > > have
> > > > > + * a reference on inode, so it is guaranteed that plock->fd is
> > > > > + * still around even after dropping inode->plock_mutex lock
> > > > > + */
> > > > > + ofd = plock->fd;
> > > > > + pthread_mutex_unlock(>plock_mutex);
> > > > > +
> > > > > + /*
> > > > > + * If this lock request can block, request caller to wait for
> > > > > + * notification. Do not access req after this. Once lock is
> > > > > + * available, send a notification instead.
> > > > > + */
> > > > > + if (sleep && lock->l_type != F_UNLCK) {
> > > > > + /*
> > > > > + * If notification queue is not enabled, can't support
> > > > > async
> > > > > + * locks.
> > > > > + */
> > > > > + if (!se->notify_enabled) {
> > > > > + saverr = EOPNOTSUPP;
> > > > > + goto out;
> > > > > + }
> > > > > + async_lock = true;
> > > > > + unique = req->unique;
> > > > > + fuse_reply_wait(req);
> > > > > + }
> > > > > /* TODO: Is it alright to modify flock? */
> > > > > lock->l_pid = 0;
> > > > > - ret = fcntl(plock->fd, F_OFD_SETLK, lock);
> > > > > + if (async_lock)
> > > > > + ret = fcntl(ofd, F_OFD_SETLKW, lock);
> > > > > + else
> > > > > + ret = fcntl(ofd, F_OFD_SETLK, lock);
> > > >
> > > > What happens if the guest is rebooted after it's asked
> > > > for, but not been granted a lock?
> > >
> > > I think a regular reboot can't be done till a request is pending, because
> > > virtio-fs can't be unmounted and unmount will wait for all pending
> > > requests to finish.
> > >
> > > Destroying qemu will destroy deamon too.
> > >
> > > Are there any other reboot paths I have missed.
> >
> > Yes, there are a few other ways the guest can reboot:
> > a) A echo b > /proc/sysrq-trigger
>
> I tried it. Both qemu and virtiofsd hang. virtiofsd wants to stop a
> queue. And that tries to stop thrad pool. But one of the threads in
> thread pool is blocked on setlkw. So g_thread_pool_free() hangs.
>
> I am not seeing any option in glib thread pool API to stop or send
> signal to threads which are blocked.
Is there a way to setup pthread_cancel ? The upstream libfuse code
has somec ases where it enables cancellation very carefully around
something that might block, does it, then disables cancellation.
Dave
> Thanks
> Vivek
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Virtio-fs] [PATCH 4/4] virtiofsd: Implement blocking posix locks
On Tue, Nov 26, 2019 at 01:02:29PM +, Dr. David Alan Gilbert wrote:
[..]
> > > > @@ -1950,21 +1948,54 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t
> > > > ino,
> > > >
> > > > if (!plock) {
> > > > saverr = ret;
> > > > + pthread_mutex_unlock(>plock_mutex);
> > > > goto out;
> > > > }
> > > >
> > > > + /*
> > > > +* plock is now released when inode is going away. We already
> > > > have
> > > > +* a reference on inode, so it is guaranteed that plock->fd is
> > > > +* still around even after dropping inode->plock_mutex lock
> > > > +*/
> > > > + ofd = plock->fd;
> > > > + pthread_mutex_unlock(>plock_mutex);
> > > > +
> > > > + /*
> > > > +* If this lock request can block, request caller to wait for
> > > > +* notification. Do not access req after this. Once lock is
> > > > +* available, send a notification instead.
> > > > +*/
> > > > + if (sleep && lock->l_type != F_UNLCK) {
> > > > + /*
> > > > +* If notification queue is not enabled, can't support
> > > > async
> > > > +* locks.
> > > > +*/
> > > > + if (!se->notify_enabled) {
> > > > + saverr = EOPNOTSUPP;
> > > > + goto out;
> > > > + }
> > > > + async_lock = true;
> > > > + unique = req->unique;
> > > > + fuse_reply_wait(req);
> > > > + }
> > > > /* TODO: Is it alright to modify flock? */
> > > > lock->l_pid = 0;
> > > > - ret = fcntl(plock->fd, F_OFD_SETLK, lock);
> > > > + if (async_lock)
> > > > + ret = fcntl(ofd, F_OFD_SETLKW, lock);
> > > > + else
> > > > + ret = fcntl(ofd, F_OFD_SETLK, lock);
> > >
> > > What happens if the guest is rebooted after it's asked
> > > for, but not been granted a lock?
> >
> > I think a regular reboot can't be done till a request is pending, because
> > virtio-fs can't be unmounted and unmount will wait for all pending
> > requests to finish.
> >
> > Destroying qemu will destroy deamon too.
> >
> > Are there any other reboot paths I have missed.
>
> Yes, there are a few other ways the guest can reboot:
> a) A echo b > /proc/sysrq-trigger
I tried it. Both qemu and virtiofsd hang. virtiofsd wants to stop a
queue. And that tries to stop thrad pool. But one of the threads in
thread pool is blocked on setlkw. So g_thread_pool_free() hangs.
I am not seeing any option in glib thread pool API to stop or send
signal to threads which are blocked.
Thanks
Vivek
Re: [Virtio-fs] [PATCH 4/4] virtiofsd: Implement blocking posix locks
* Vivek Goyal (vgo...@redhat.com) wrote:
> On Fri, Nov 22, 2019 at 05:47:32PM +, Dr. David Alan Gilbert wrote:
>
> [..]
> > > +static int virtio_send_notify_msg(struct fuse_session *se, struct iovec
> > > *iov,
> > > + int count)
> > > +{
> > > +struct fv_QueueInfo *qi;
> > > +VuDev *dev = >virtio_dev->dev;
> > > +VuVirtq *q;
> > > +FVRequest *req;
> > > +VuVirtqElement *elem;
> > > +unsigned int in_num, bad_in_num = 0, bad_out_num = 0;
> > > +struct fuse_out_header *out = iov[0].iov_base;
> > > +size_t in_len, tosend_len = iov_size(iov, count);
> > > +struct iovec *in_sg;
> > > +int ret = 0;
> > > +
> > > +/* Notifications have unique == 0 */
> > > +assert (!out->unique);
> > > +
> > > +if (!se->notify_enabled)
> > > +return -EOPNOTSUPP;
> > > +
> > > +/* If notifications are enabled, queue index 1 is notification queue
> > > */
> > > +qi = se->virtio_dev->qi[1];
> > > +q = vu_get_queue(dev, qi->qidx);
> > > +
> > > +pthread_rwlock_rdlock(>virtio_dev->vu_dispatch_rwlock);
> > > +pthread_mutex_lock(>vq_lock);
> > > +/* Pop an element from queue */
> > > +req = vu_queue_pop(dev, q, sizeof(FVRequest), _in_num,
> > > _out_num);
> >
> > You don't need bad_in_num/bad_out_num - just pass NULL for both; they're
> > only needed if you expect to read/write data that's not mappable (i.e.
> > in our direct write case).
>
> Will do.
>
> [..]
> > > @@ -1950,21 +1948,54 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t
> > > ino,
> > >
> > > if (!plock) {
> > > saverr = ret;
> > > + pthread_mutex_unlock(>plock_mutex);
> > > goto out;
> > > }
> > >
> > > + /*
> > > + * plock is now released when inode is going away. We already have
> > > + * a reference on inode, so it is guaranteed that plock->fd is
> > > + * still around even after dropping inode->plock_mutex lock
> > > + */
> > > + ofd = plock->fd;
> > > + pthread_mutex_unlock(>plock_mutex);
> > > +
> > > + /*
> > > + * If this lock request can block, request caller to wait for
> > > + * notification. Do not access req after this. Once lock is
> > > + * available, send a notification instead.
> > > + */
> > > + if (sleep && lock->l_type != F_UNLCK) {
> > > + /*
> > > + * If notification queue is not enabled, can't support async
> > > + * locks.
> > > + */
> > > + if (!se->notify_enabled) {
> > > + saverr = EOPNOTSUPP;
> > > + goto out;
> > > + }
> > > + async_lock = true;
> > > + unique = req->unique;
> > > + fuse_reply_wait(req);
> > > + }
> > > /* TODO: Is it alright to modify flock? */
> > > lock->l_pid = 0;
> > > - ret = fcntl(plock->fd, F_OFD_SETLK, lock);
> > > + if (async_lock)
> > > + ret = fcntl(ofd, F_OFD_SETLKW, lock);
> > > + else
> > > + ret = fcntl(ofd, F_OFD_SETLK, lock);
> >
> > What happens if the guest is rebooted after it's asked
> > for, but not been granted a lock?
>
> I think a regular reboot can't be done till a request is pending, because
> virtio-fs can't be unmounted and unmount will wait for all pending
> requests to finish.
>
> Destroying qemu will destroy deamon too.
>
> Are there any other reboot paths I have missed.
Yes, there are a few other ways the guest can reboot:
a) A echo b > /proc/sysrq-trigger
b) Telling qemu to do a reset
probably a few more as well; but they all end up with the daemon
still running over the same connection. See
'virtiofsd: Handle hard reboot' where I handle that case where
a FUSE_INIT turns up unexpectedly.
Dave
> Thanks
> Vivek
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Virtio-fs] [PATCH 4/4] virtiofsd: Implement blocking posix locks
On Fri, Nov 22, 2019 at 05:47:32PM +, Dr. David Alan Gilbert wrote:
[..]
> > +static int virtio_send_notify_msg(struct fuse_session *se, struct iovec
> > *iov,
> > + int count)
> > +{
> > +struct fv_QueueInfo *qi;
> > +VuDev *dev = >virtio_dev->dev;
> > +VuVirtq *q;
> > +FVRequest *req;
> > +VuVirtqElement *elem;
> > +unsigned int in_num, bad_in_num = 0, bad_out_num = 0;
> > +struct fuse_out_header *out = iov[0].iov_base;
> > +size_t in_len, tosend_len = iov_size(iov, count);
> > +struct iovec *in_sg;
> > +int ret = 0;
> > +
> > +/* Notifications have unique == 0 */
> > +assert (!out->unique);
> > +
> > +if (!se->notify_enabled)
> > +return -EOPNOTSUPP;
> > +
> > +/* If notifications are enabled, queue index 1 is notification queue */
> > +qi = se->virtio_dev->qi[1];
> > +q = vu_get_queue(dev, qi->qidx);
> > +
> > +pthread_rwlock_rdlock(>virtio_dev->vu_dispatch_rwlock);
> > +pthread_mutex_lock(>vq_lock);
> > +/* Pop an element from queue */
> > +req = vu_queue_pop(dev, q, sizeof(FVRequest), _in_num,
> > _out_num);
>
> You don't need bad_in_num/bad_out_num - just pass NULL for both; they're
> only needed if you expect to read/write data that's not mappable (i.e.
> in our direct write case).
Will do.
[..]
> > @@ -1950,21 +1948,54 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino,
> >
> > if (!plock) {
> > saverr = ret;
> > + pthread_mutex_unlock(>plock_mutex);
> > goto out;
> > }
> >
> > + /*
> > +* plock is now released when inode is going away. We already have
> > +* a reference on inode, so it is guaranteed that plock->fd is
> > +* still around even after dropping inode->plock_mutex lock
> > +*/
> > + ofd = plock->fd;
> > + pthread_mutex_unlock(>plock_mutex);
> > +
> > + /*
> > +* If this lock request can block, request caller to wait for
> > +* notification. Do not access req after this. Once lock is
> > +* available, send a notification instead.
> > +*/
> > + if (sleep && lock->l_type != F_UNLCK) {
> > + /*
> > +* If notification queue is not enabled, can't support async
> > +* locks.
> > +*/
> > + if (!se->notify_enabled) {
> > + saverr = EOPNOTSUPP;
> > + goto out;
> > + }
> > + async_lock = true;
> > + unique = req->unique;
> > + fuse_reply_wait(req);
> > + }
> > /* TODO: Is it alright to modify flock? */
> > lock->l_pid = 0;
> > - ret = fcntl(plock->fd, F_OFD_SETLK, lock);
> > + if (async_lock)
> > + ret = fcntl(ofd, F_OFD_SETLKW, lock);
> > + else
> > + ret = fcntl(ofd, F_OFD_SETLK, lock);
>
> What happens if the guest is rebooted after it's asked
> for, but not been granted a lock?
I think a regular reboot can't be done till a request is pending, because
virtio-fs can't be unmounted and unmount will wait for all pending
requests to finish.
Destroying qemu will destroy deamon too.
Are there any other reboot paths I have missed.
Thanks
Vivek
Re: [Virtio-fs] [PATCH 4/4] virtiofsd: Implement blocking posix locks
On Fri, Nov 22, 2019 at 10:53:24AM +, 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), _in_num,
> > _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
