| Message ID | 577E219402000078000FBDB6@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/07/16 08:32, Jan Beulich wrote: > We must not skip the transaction_end() call for a failed > XS_TRANSACTION_START. The removed code fragment got introduced by > commit 027bd7e899 ("xen/xenbus: Avoid synchronous wait on XenBus > stalling shutdown/restart") without its description really indicating > why it was added (and hence I can't identify whether a more complex > change might be needed here). If sending the XS_TRANSACTION_END message failed, then the transaction is still open and transaction_end() should not be called. However, if sending an XS_TRANSACTION_START failed, then transaction_end() should be called. So, yes a more complex fix is needed here. David
>>> On 07.07.16 at 13:36, <david.vrabel@citrix.com> wrote: > On 07/07/16 08:32, Jan Beulich wrote: >> We must not skip the transaction_end() call for a failed >> XS_TRANSACTION_START. The removed code fragment got introduced by >> commit 027bd7e899 ("xen/xenbus: Avoid synchronous wait on XenBus >> stalling shutdown/restart") without its description really indicating >> why it was added (and hence I can't identify whether a more complex >> change might be needed here). > > If sending the XS_TRANSACTION_END message failed, then the transaction > is still open and transaction_end() should not be called. > > However, if sending an XS_TRANSACTION_START failed, then > transaction_end() should be called. > > So, yes a more complex fix is needed here. Well, both of the things you name are what happens with the patch in place. So if those two conditions are all that needs to be satisfied, then no more complex change is needed afaict (and was the behavior before the cross referenced commit) - the question really is whether that other commit meant to deal with something _beyond_ those two things. Jan
On 07/07/16 13:09, Jan Beulich wrote: >>>> On 07.07.16 at 13:36, <david.vrabel@citrix.com> wrote: >> On 07/07/16 08:32, Jan Beulich wrote: >>> We must not skip the transaction_end() call for a failed >>> XS_TRANSACTION_START. The removed code fragment got introduced by >>> commit 027bd7e899 ("xen/xenbus: Avoid synchronous wait on XenBus >>> stalling shutdown/restart") without its description really indicating >>> why it was added (and hence I can't identify whether a more complex >>> change might be needed here). >> >> If sending the XS_TRANSACTION_END message failed, then the transaction >> is still open and transaction_end() should not be called. >> >> However, if sending an XS_TRANSACTION_START failed, then >> transaction_end() should be called. >> >> So, yes a more complex fix is needed here. > > Well, both of the things you name are what happens with the patch > in place. So if those two conditions are all that needs to be satisfied, > then no more complex change is needed afaict (and was the behavior > before the cross referenced commit) - the question really is whether > that other commit meant to deal with something _beyond_ those two > things. You call transaction_end() if msg->type == XS_TRANSACTION_END, even if xb_write() returned an error. David
>>> On 07.07.16 at 14:17, <david.vrabel@citrix.com> wrote: > On 07/07/16 13:09, Jan Beulich wrote: >>>>> On 07.07.16 at 13:36, <david.vrabel@citrix.com> wrote: >>> On 07/07/16 08:32, Jan Beulich wrote: >>>> We must not skip the transaction_end() call for a failed >>>> XS_TRANSACTION_START. The removed code fragment got introduced by >>>> commit 027bd7e899 ("xen/xenbus: Avoid synchronous wait on XenBus >>>> stalling shutdown/restart") without its description really indicating >>>> why it was added (and hence I can't identify whether a more complex >>>> change might be needed here). >>> >>> If sending the XS_TRANSACTION_END message failed, then the transaction >>> is still open and transaction_end() should not be called. >>> >>> However, if sending an XS_TRANSACTION_START failed, then >>> transaction_end() should be called. >>> >>> So, yes a more complex fix is needed here. >> >> Well, both of the things you name are what happens with the patch >> in place. So if those two conditions are all that needs to be satisfied, >> then no more complex change is needed afaict (and was the behavior >> before the cross referenced commit) - the question really is whether >> that other commit meant to deal with something _beyond_ those two >> things. > > You call transaction_end() if msg->type == XS_TRANSACTION_END, even if > xb_write() returned an error. When xb_write() returns an error, msg->type gets set to XS_ERROR. Jan
On 07/07/16 13:23, Jan Beulich wrote: >>>> On 07.07.16 at 14:17, <david.vrabel@citrix.com> wrote: >> On 07/07/16 13:09, Jan Beulich wrote: >>>>>> On 07.07.16 at 13:36, <david.vrabel@citrix.com> wrote: >>>> On 07/07/16 08:32, Jan Beulich wrote: >>>>> We must not skip the transaction_end() call for a failed >>>>> XS_TRANSACTION_START. The removed code fragment got introduced by >>>>> commit 027bd7e899 ("xen/xenbus: Avoid synchronous wait on XenBus >>>>> stalling shutdown/restart") without its description really indicating >>>>> why it was added (and hence I can't identify whether a more complex >>>>> change might be needed here). >>>> >>>> If sending the XS_TRANSACTION_END message failed, then the transaction >>>> is still open and transaction_end() should not be called. >>>> >>>> However, if sending an XS_TRANSACTION_START failed, then >>>> transaction_end() should be called. >>>> >>>> So, yes a more complex fix is needed here. >>> >>> Well, both of the things you name are what happens with the patch >>> in place. So if those two conditions are all that needs to be satisfied, >>> then no more complex change is needed afaict (and was the behavior >>> before the cross referenced commit) - the question really is whether >>> that other commit meant to deal with something _beyond_ those two >>> things. >> >> You call transaction_end() if msg->type == XS_TRANSACTION_END, even if >> xb_write() returned an error. > > When xb_write() returns an error, msg->type gets set to XS_ERROR. So? if ((msg->type == XS_TRANSACTION_END) || (...)) transaction_end(); We don't check msg->type for XS_TRANSACTION_END messages. David
On 07/07/16 14:13, David Vrabel wrote: > On 07/07/16 13:23, Jan Beulich wrote: >>>>> On 07.07.16 at 14:17, <david.vrabel@citrix.com> wrote: >>> On 07/07/16 13:09, Jan Beulich wrote: >>>>>>> On 07.07.16 at 13:36, <david.vrabel@citrix.com> wrote: >>>>> On 07/07/16 08:32, Jan Beulich wrote: >>>>>> We must not skip the transaction_end() call for a failed >>>>>> XS_TRANSACTION_START. The removed code fragment got introduced by >>>>>> commit 027bd7e899 ("xen/xenbus: Avoid synchronous wait on XenBus >>>>>> stalling shutdown/restart") without its description really indicating >>>>>> why it was added (and hence I can't identify whether a more complex >>>>>> change might be needed here). >>>>> >>>>> If sending the XS_TRANSACTION_END message failed, then the transaction >>>>> is still open and transaction_end() should not be called. >>>>> >>>>> However, if sending an XS_TRANSACTION_START failed, then >>>>> transaction_end() should be called. >>>>> >>>>> So, yes a more complex fix is needed here. >>>> >>>> Well, both of the things you name are what happens with the patch >>>> in place. So if those two conditions are all that needs to be satisfied, >>>> then no more complex change is needed afaict (and was the behavior >>>> before the cross referenced commit) - the question really is whether >>>> that other commit meant to deal with something _beyond_ those two >>>> things. >>> >>> You call transaction_end() if msg->type == XS_TRANSACTION_END, even if >>> xb_write() returned an error. >> >> When xb_write() returns an error, msg->type gets set to XS_ERROR. > > So? > > if ((msg->type == XS_TRANSACTION_END) || > (...)) > transaction_end(); > > We don't check msg->type for XS_TRANSACTION_END messages. Sorry, being stupid. Yeah, the change is fine but it needs a better commit message. David
>>> On 07.07.16 at 15:22, <david.vrabel@citrix.com> wrote: > On 07/07/16 14:13, David Vrabel wrote: >> On 07/07/16 13:23, Jan Beulich wrote: >>>>>> On 07.07.16 at 14:17, <david.vrabel@citrix.com> wrote: >>>> On 07/07/16 13:09, Jan Beulich wrote: >>>>>>>> On 07.07.16 at 13:36, <david.vrabel@citrix.com> wrote: >>>>>> On 07/07/16 08:32, Jan Beulich wrote: >>>>>>> We must not skip the transaction_end() call for a failed >>>>>>> XS_TRANSACTION_START. The removed code fragment got introduced by >>>>>>> commit 027bd7e899 ("xen/xenbus: Avoid synchronous wait on XenBus >>>>>>> stalling shutdown/restart") without its description really indicating >>>>>>> why it was added (and hence I can't identify whether a more complex >>>>>>> change might be needed here). >>>>>> >>>>>> If sending the XS_TRANSACTION_END message failed, then the transaction >>>>>> is still open and transaction_end() should not be called. >>>>>> >>>>>> However, if sending an XS_TRANSACTION_START failed, then >>>>>> transaction_end() should be called. >>>>>> >>>>>> So, yes a more complex fix is needed here. >>>>> >>>>> Well, both of the things you name are what happens with the patch >>>>> in place. So if those two conditions are all that needs to be satisfied, >>>>> then no more complex change is needed afaict (and was the behavior >>>>> before the cross referenced commit) - the question really is whether >>>>> that other commit meant to deal with something _beyond_ those two >>>>> things. >>>> >>>> You call transaction_end() if msg->type == XS_TRANSACTION_END, even if >>>> xb_write() returned an error. >>> >>> When xb_write() returns an error, msg->type gets set to XS_ERROR. >> >> So? >> >> if ((msg->type == XS_TRANSACTION_END) || >> (...)) >> transaction_end(); >> >> We don't check msg->type for XS_TRANSACTION_END messages. > > Sorry, being stupid. Yeah, the change is fine but it needs a better > commit message. I can certainly omit the part in parentheses. I don't think I should omit the reference to the original commit having introduced the issue. And without a more specific hint I also don't know what else may need changing. I'm sorry, I know I'm not doing very well in writing commit messages to your liking. Jan
On 07/07/16 14:35, Jan Beulich wrote: >>>> On 07.07.16 at 15:22, <david.vrabel@citrix.com> wrote: >> On 07/07/16 14:13, David Vrabel wrote: >>> On 07/07/16 13:23, Jan Beulich wrote: >>>>>>> On 07.07.16 at 14:17, <david.vrabel@citrix.com> wrote: >>>>> On 07/07/16 13:09, Jan Beulich wrote: >>>>>>>>> On 07.07.16 at 13:36, <david.vrabel@citrix.com> wrote: >>>>>>> On 07/07/16 08:32, Jan Beulich wrote: >>>>>>>> We must not skip the transaction_end() call for a failed >>>>>>>> XS_TRANSACTION_START. The removed code fragment got introduced by >>>>>>>> commit 027bd7e899 ("xen/xenbus: Avoid synchronous wait on XenBus >>>>>>>> stalling shutdown/restart") without its description really indicating >>>>>>>> why it was added (and hence I can't identify whether a more complex >>>>>>>> change might be needed here). >>>>>>> >>>>>>> If sending the XS_TRANSACTION_END message failed, then the transaction >>>>>>> is still open and transaction_end() should not be called. >>>>>>> >>>>>>> However, if sending an XS_TRANSACTION_START failed, then >>>>>>> transaction_end() should be called. >>>>>>> >>>>>>> So, yes a more complex fix is needed here. >>>>>> >>>>>> Well, both of the things you name are what happens with the patch >>>>>> in place. So if those two conditions are all that needs to be satisfied, >>>>>> then no more complex change is needed afaict (and was the behavior >>>>>> before the cross referenced commit) - the question really is whether >>>>>> that other commit meant to deal with something _beyond_ those two >>>>>> things. >>>>> >>>>> You call transaction_end() if msg->type == XS_TRANSACTION_END, even if >>>>> xb_write() returned an error. >>>> >>>> When xb_write() returns an error, msg->type gets set to XS_ERROR. >>> >>> So? >>> >>> if ((msg->type == XS_TRANSACTION_END) || >>> (...)) >>> transaction_end(); >>> >>> We don't check msg->type for XS_TRANSACTION_END messages. >> >> Sorry, being stupid. Yeah, the change is fine but it needs a better >> commit message. > > I can certainly omit the part in parentheses. I don't think I should > omit the reference to the original commit having introduced the issue. > And without a more specific hint I also don't know what else may > need changing. I'm sorry, I know I'm not doing very well in writing > commit messages to your liking. I rewrote it as: xenbus: don't bail early from xenbus_dev_request_and_reply() xenbus_dev_request_and_reply() needs to track whether a transaction is open. For XS_TRANSACTION_START messages it calls transaction_start() and for XS_TRANSACTION_END messages it calls transaction_end(). If sending an XS_TRANSACTION_START message fails or responds with an an error, the transaction is not open and transaction_end() must be called. If sending an XS_TRANSACTION_END message fails, the transaction is still open, but if an error response is returned the transaction is closed. Commit 027bd7e89906 ("xen/xenbus: Avoid synchronous wait on XenBus stalling shutdown/restart") introduced a regression where failed XS_TRANSACTION_START messages were leaving the transaction open. This can cause problems with suspend (and migration) as all transaction must be closed before suspending. It appears that the problematic change was added accidentally, so just remove it. David
--- 4.7-rc6-xen.orig/drivers/xen/xenbus/xenbus_xs.c +++ 4.7-rc6-xen/drivers/xen/xenbus/xenbus_xs.c @@ -249,9 +249,6 @@ void *xenbus_dev_request_and_reply(struc 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)))
We must not skip the transaction_end() call for a failed XS_TRANSACTION_START. The removed code fragment got introduced by commit 027bd7e899 ("xen/xenbus: Avoid synchronous wait on XenBus stalling shutdown/restart") without its description really indicating why it was added (and hence I can't identify whether a more complex change might be needed here). Signed-off-by: Jan Beulich <jbeulich@suse.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- drivers/xen/xenbus/xenbus_xs.c | 3 --- 1 file changed, 3 deletions(-)