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

[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)


--
Best regards,
Vladimir