Re: [PATCH v0] vhost: make SET_VRING_ADDR, SET_[PROTOCOL_]FEATEURES send replies

[Michael S. Tsirkin]

On Wed, Jul 07, 2021 at 05:58:50PM +0300, Denis Plotnikov wrote:
> 
> On 07.07.2021 17:39, Michael S. Tsirkin wrote:
> > On Wed, Jul 07, 2021 at 03:19:20PM +0300, Denis Plotnikov wrote:
> > > On 07.07.2021 13:10, Michael S. Tsirkin wrote:
> > > > On Fri, Jun 25, 2021 at 11:52:10AM +0300, Denis Plotnikov wrote:
> > > > > On vhost-user-blk migration, qemu normally sends a number of commands
> > > > > to enable logging if VHOST_USER_PROTOCOL_F_LOG_SHMFD is negotiated.
> > > > > Qemu sends VHOST_USER_SET_FEATURES to enable buffers logging and
> > > > > VHOST_USER_SET_FEATURES per each started ring to enable "used ring"
> > > > > data logging.
> > > > > The issue is that qemu doesn't wait for reply from the vhost daemon
> > > > > for these commands which may result in races between qemu expectation
> > > > > of logging starting and actual login starting in vhost daemon.
> > > > Could you be more explicit please? What kind of race have you
> > > > observed? Getting a reply slows down the setup considerably and
> > > > should not be done lightly.
> > > I'm talking about the vhost-user-blk case. On migration setup, we enable
> > > logging by sending VHOST_USER_SET_FEATURES. The command doesn't arrive to 
> > > a
> > > vhost-user-blk daemon immediately and the daemon needs some time turn the
> > > logging on internally. If qemu doesn't wait for reply, after sending the
> > > command qemu may start migrate memory pages. At this time the logging may
> > > not be actually turned on in the daemon  but some guest pages, which the
> > > daemon is about to write to, may be already transferred without logging 
> > > to a
> > > destination. Since the logging wasn't turned on, those pages won't be
> > > transferred again as dirty. So we may end up with corrupted data on the
> > > destination.
> > > 
> > > Have I managed to explain the case clearly?
> > > 
> > > Thanks!
> > > 
> > > Denis
> > OK so this is just about enabling logging. It would be cleaner to
> > defer migrating memory until response ... if that is too hard,
> > at least document why we are doing this please.
> > And, let's wait for an ack just in that case then - why not?
> > 
> > And what about VHOST_USER_SET_PROTOCOL_FEATURES?
> 
> The code uses the same path for both VHOST_USER_SET_PROTOCOL_FEATURES and
> VHOST_USER_SET_FEATURES via vhost_user_set_u64(). So, I decided to suggest
> adding reply to both of them, so both feature setting commands work
> similarly as it doesn't contradicts with vhost-user spec.
> 
> I'm not sure that it worth doing that, so if you think it's not I'll just
> remove them.
> 
> 
> Denis


I'm inclined to say let's not add to the latency of setting up the
device unnecessarily.

> > 
> > 
> > > > Thanks!
> > > > 
> > > > > To resolve this issue, this patch makes qemu wait for the commands 
> > > > > result
> > > > > explicilty if VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated.
> > > > > Also, this patch adds the reply waiting for 
> > > > > VHOST_USER_SET_PROTOCOL_FEATURES
> > > > > command to make the features setting functions work similary.
> > > > > 
> > > > > Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
> > > > > ---
> > > > >    hw/virtio/vhost-user.c | 20 ++++++++++++++++++++
> > > > >    1 file changed, 20 insertions(+)
> > > > > 
> > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > > index ee57abe04526..e47b82adab00 100644
> > > > > --- a/hw/virtio/vhost-user.c
> > > > > +++ b/hw/virtio/vhost-user.c
> > > > > @@ -1105,10 +1105,20 @@ static int vhost_user_set_vring_addr(struct 
> > > > > vhost_dev *dev,
> > > > >            .hdr.size = sizeof(msg.payload.addr),
> > > > >        };
> > > > > +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> > > > > +                                              
> > > > > VHOST_USER_PROTOCOL_F_REPLY_ACK);
> > > > > +    if (reply_supported) {
> > > > > +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> > > > > +    }
> > > > > +
> > > > >        if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> > > > >            return -1;
> > > > >        }
> > > > > +    if (reply_supported) {
> > > > > +        return process_message_reply(dev, &msg);
> > > > > +    }
> > > > > +
> > > > >        return 0;
> > > > >    }
> > > > > @@ -1297,10 +1307,20 @@ static int vhost_user_set_u64(struct 
> > > > > vhost_dev *dev, int request, uint64_t u64)
> > > > >            .hdr.size = sizeof(msg.payload.u64),
> > > > >        };
> > > > > +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> > > > > +                                              
> > > > > VHOST_USER_PROTOCOL_F_REPLY_ACK);
> > > > > +    if (reply_supported) {
> > > > > +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> > > > > +    }
> > > > > +
> > > > >        if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> > > > >            return -1;
> > > > >        }
> > > > > +    if (reply_supported) {
> > > > > +        return process_message_reply(dev, &msg);
> > > > > +    }
> > > > > +
> > > > >        return 0;
> > > > >    }
> > > > > -- 
> > > > > 2.25.1