Re: [PATCH 1/4] vhost-user-blk: delay vhost_user_blk_disconnect

[Li Feng]

Hi Norwitz ，
Thanks for your good suggestion.

I got this fix from net/vhost-user.c, it has the same issue with this case.
Your suggestion is a good option.
I'm trying to do some work. but there is another crash issue ...
I need some time to make your idea be workable.

This is the net/vhost-user patch:

commit e7c83a885f865128ae3cf1946f8cb538b63cbfba
Author: Marc-André Lureau <address@hidden>
Date: Mon Feb 27 14:49:56 2017 +0400

vhost-user: delay vhost_user_stop

Since commit b0a335e351103bf92f3f9d0bd5759311be8156ac, a socket write
may trigger a disconnect events, calling vhost_user_stop() and clearing
all the vhost_dev strutures holding data that vhost.c functions expect
to remain valid. Delay the cleanup to keep the vhost_dev structure
valid during the vhost.c functions.



Feng Li

Raphael Norwitz <address@hidden> 于2020年4月17日周五 上午11:41写道：



>
> On Wed, Apr 15, 2020 at 11:28:23AM +0800, Li Feng wrote:
> >
> >      switch (event) {
> >      case CHR_EVENT_OPENED:
> > @@ -363,7 +376,16 @@ static void vhost_user_blk_event(void *opaque, 
> > QEMUChrEvent event)
> >          }
> >          break;
> >      case CHR_EVENT_CLOSED:
> > -        vhost_user_blk_disconnect(dev);
> > +        /*
> > +         * a close event may happen during a read/write, but vhost
> > +         * code assumes the vhost_dev remains setup, so delay the
> > +         * stop & clear to idle.
> > +         */
> > +        ctx = qemu_get_current_aio_context();
> > +
> > +        qemu_chr_fe_set_handlers(&s->chardev,  NULL, NULL, NULL,
> > +                                 NULL, NULL, NULL, false);
> > +        aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
>
> This seems a bit racy. What’s to stop the async operation from executing 
> before
> the next read?
>
> If the issue is just that the vhost_dev state is being destroyed too early, 
> why
> don’t we rather move the vhost_dev_cleanup() call from 
> vhost_user_blk_disconnect()
> to vhost_user_blk_connect()? We may need to add some state to tell if this is 
> the
> first connect or a reconnect so we don’t call vhost_dev_cleanup() on initial
> connect, but as long as we call vhost_dev_cleanup() before vhost_dev_init()
> every time the device reconnects it shouldn’t matter that we keep that state
> around.
>
> Thoughts?
>
> >          break;
> >      case CHR_EVENT_BREAK:
> >      case CHR_EVENT_MUX_IN:

-- 
The SmartX email address is only for business purpose. Any sent message 
that is not related to the business is not authorized or permitted by 
SmartX.
本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.