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

[luzhipeng <luzhipeng@cestc.cn>]

在 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