qemu-nbd: set timeout to qemu-nbd socket

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

Posted by luzhipeng 1 year, 7 months ago

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;
-- 
2.31.1

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

Posted by Vladimir Sementsov-Ogievskiy 1 year, 7 months ago

[+ 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?


-- 
Best regards,
Vladimir

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

Posted by Denis V. Lunev 1 year, 7 months ago

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.

Den

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

Posted by Vladimir Sementsov-Ogievskiy 1 year, 7 months ago

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

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

Posted by luzhipeng 1 year, 7 months ago


在 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, but not for unix sockets

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

Posted by luzhipeng 1 year, 7 months ago


在 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