Konrad Rzeszutek Wilk wrote on 2013-11-09:
> The 'read_reply' works with 'process_msg' to read of a reply in XenBus.
> 'process_msg' is running from within the 'xenbus' thread. Whenever a
> message shows up in XenBus it is put on a xs_state.reply_list list and
> 'read_reply' picks it up.
>
> The problem is if the backend domain or the xenstored process is killed.
> In which case 'xenbus' is still awaiting - and 'read_reply' if called
> - stuck forever waiting for the reply_list to have some contents.
>
> This is normally not a problem - as the backend domain can come back
> or the xenstored process can be restarted. However if the domain is in
> process of being powered off/restarted/halted - there is no point of
> waiting on it coming back - as we are effectively being terminated and
> should not impede the progress.
>
Hi, Konrad,
Is this patch applied in Linux upstream tree? I didn't find it with latest Linux upstream source.
> This patch solves this problem by checking the 'system_state' value to
> see if we are in heading towards death. We also make the wait
> mechanism a bit more asynchronous.
>
> Fixes-Bug: http://bugs.xenproject.org/xen/bug/8
> Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
> ---
> drivers/xen/xenbus/xenbus_xs.c | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
> diff --git a/drivers/xen/xenbus/xenbus_xs.c
> b/drivers/xen/xenbus/xenbus_xs.c index b6d5fff..4f22706 100644
> --- a/drivers/xen/xenbus/xenbus_xs.c
> +++ b/drivers/xen/xenbus/xenbus_xs.c
> @@ -148,9 +148,24 @@ static void *read_reply(enum xsd_sockmsg_type
> *type, unsigned int *len)
>
> while (list_empty(&xs_state.reply_list)) {
> spin_unlock(&xs_state.reply_lock);
> - /* XXX FIXME: Avoid synchronous wait for response here. */
> - wait_event(xs_state.reply_waitq, -
> !list_empty(&xs_state.reply_list));
> + wait_event_timeout(xs_state.reply_waitq, +
> !list_empty(&xs_state.reply_list), + msecs_to_jiffies(500)); +
> + /* + * If we are in the process of being shut-down there is + *
> no point of trying to contact XenBus - it is either + * killed
> (xenstored application) or the other domain + * has been killed or is
> unreachable. + */ + switch (system_state) { + case SYSTEM_POWER_OFF:
> + case SYSTEM_RESTART: + case SYSTEM_HALT: + return ERR_PTR(-EIO);
> + default: + break; + }
> spin_lock(&xs_state.reply_lock);
> }
> @@ -215,6 +230,9 @@ void *xenbus_dev_request_and_reply(struct
> xsd_sockmsg *msg)
>
> mutex_unlock(&xs_state.request_mutex);
> + if (IS_ERR(ret))
> + return ret;
> +
> if ((msg->type == XS_TRANSACTION_END) ||
> ((req_msg.type == XS_TRANSACTION_START) &&
> (msg->type == XS_ERROR)))
Best regards,
Yang
"Zhang, Yang Z" <[email protected]> wrote:
>Konrad Rzeszutek Wilk wrote on 2013-11-09:
>> The 'read_reply' works with 'process_msg' to read of a reply in
>XenBus.
>> 'process_msg' is running from within the 'xenbus' thread. Whenever a
>> message shows up in XenBus it is put on a xs_state.reply_list list
>and
>> 'read_reply' picks it up.
>>
>> The problem is if the backend domain or the xenstored process is
>killed.
>> In which case 'xenbus' is still awaiting - and 'read_reply' if called
>> - stuck forever waiting for the reply_list to have some contents.
>>
>> This is normally not a problem - as the backend domain can come back
>> or the xenstored process can be restarted. However if the domain is
>in
>> process of being powered off/restarted/halted - there is no point of
>> waiting on it coming back - as we are effectively being terminated
>and
>> should not impede the progress.
>>
>
>Hi, Konrad,
>
>Is this patch applied in Linux upstream tree? I didn't find it with
>latest Linux upstream source.
>
No. It needs rework.
>> This patch solves this problem by checking the 'system_state' value
>to
>> see if we are in heading towards death. We also make the wait
>> mechanism a bit more asynchronous.
>>
>> Fixes-Bug: http://bugs.xenproject.org/xen/bug/8
>> Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
>> ---
>> drivers/xen/xenbus/xenbus_xs.c | 24 +++++++++++++++++++++---
>> 1 file changed, 21 insertions(+), 3 deletions(-)
>> diff --git a/drivers/xen/xenbus/xenbus_xs.c
>> b/drivers/xen/xenbus/xenbus_xs.c index b6d5fff..4f22706 100644
>> --- a/drivers/xen/xenbus/xenbus_xs.c
>> +++ b/drivers/xen/xenbus/xenbus_xs.c
>> @@ -148,9 +148,24 @@ static void *read_reply(enum xsd_sockmsg_type
>> *type, unsigned int *len)
>>
>> while (list_empty(&xs_state.reply_list)) {
>> spin_unlock(&xs_state.reply_lock);
>> - /* XXX FIXME: Avoid synchronous wait for response here. */
>> - wait_event(xs_state.reply_waitq, -
>> !list_empty(&xs_state.reply_list));
>> + wait_event_timeout(xs_state.reply_waitq, +
>> !list_empty(&xs_state.reply_list), + msecs_to_jiffies(500)); +
>> + /* + * If we are in the process of being shut-down there is +
>*
>> no point of trying to contact XenBus - it is either + * killed
>> (xenstored application) or the other domain + * has been killed or
>is
>> unreachable. + */ + switch (system_state) { + case
>SYSTEM_POWER_OFF:
>> + case SYSTEM_RESTART: + case SYSTEM_HALT: + return
>ERR_PTR(-EIO);
>> + default: + break; + }
>> spin_lock(&xs_state.reply_lock);
>> }
>> @@ -215,6 +230,9 @@ void *xenbus_dev_request_and_reply(struct
>> xsd_sockmsg *msg)
>>
>> mutex_unlock(&xs_state.request_mutex);
>> + if (IS_ERR(ret))
>> + return ret;
>> +
>> if ((msg->type == XS_TRANSACTION_END) ||
>> ((req_msg.type == XS_TRANSACTION_START) &&
>> (msg->type == XS_ERROR)))
>
>
>Best regards,
>Yang