Re: [PATCH] qemu-nbd: set timeout to qemu-nbd socket

[luzhipeng]

在 2022/9/26 20:44, Vladimir Sementsov-Ogievskiy 写道:
> On 9/26/22 14:34, Denis V. Lunev wrote:
> > On 9/26/22 12:05, Vladimir Sementsov-Ogievskiy wrote:
> > > [+ Den]
> > >
> > > On 9/25/22 16:53, luzhipeng wrote:
> > > > From: lu zhipeng <luzhipeng@cestc.cn>
> > > >
> > > > Prevent the NBD socket stuck all the time, So
> > > > set timeout.
> > > >
> > > > Signed-off-by: lu zhipeng <luzhipeng@cestc.cn>
> > > > ---
> > > >   nbd/client.c | 8 ++++++++
> > > >   1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/nbd/client.c b/nbd/client.c
> > > > index 30d5383cb1..89dde53a0f 100644
> > > > --- a/nbd/client.c
> > > > +++ b/nbd/client.c
> > > > @@ -24,6 +24,8 @@
> > > >   #include "nbd-internal.h"
> > > >   #include "qemu/cutils.h"
> > > >   +#define NBD_DEFAULT_TIMEOUT 30
> > > > +
> > > >   /* Definitions for opaque data types */
> > > >     static QTAILQ_HEAD(, NBDExport) exports = 
> > > > QTAILQ_HEAD_INITIALIZER(exports);
> > > > @@ -1301,6 +1303,12 @@ int nbd_init(int fd, QIOChannelSocket *sioc, 
> > > > NBDExportInfo *info,
> > > >           }
> > > >       }
> > > >   +    if (ioctl(fd, NBD_SET_TIMEOUT, NBD_DEFAULT_TIMEOUT) < 0) {
> > > > +        int serrno = errno;
> > > > +        error_setg(errp, "Failed setting timeout");
> > > > +        return -serrno;
> > > > +    }
> > > > +
> > > >       trace_nbd_init_finish();
> > > >         return 0;
> > >
> > >
> > > Personally, I don't see a problem in enabling timeout by default.. 
> > > But probably we need a new option instead?
> > I believe that this should be the same story as we have had with
> > KEEPALIVE. This should be set as an option and downstream
> > will change its default when necessary.
>
> It's also interesting, how NBD_SET_TIMEOUT would interfere with 
> keep-alive options set on the socket. Isn't existing keep-alive option 
> already enough, do we need both timeouts?
>
> (and yes, if we need both ways for different cases, we definitely should 
> keep a possibility for the user to enable only one timeout, so now I 
> agree, that we need an option for this new feature)
keep-alive is only valid for tcp mode, unix domain is not valid