From: "Durrant, Paul" <email@example.com> To: "Marek Marczykowski-Górecki" <firstname.lastname@example.org>, "Michael Brown" <email@example.com>, "firstname.lastname@example.org" <email@example.com> Cc: "firstname.lastname@example.org" <email@example.com>, "firstname.lastname@example.org" <email@example.com>, "firstname.lastname@example.org" <email@example.com>, "firstname.lastname@example.org" <email@example.com> Subject: RE: [PATCH] xen-netback: Check for hotplug-status existence before watching Date: Tue, 11 May 2021 07:06:55 +0000 [thread overview] Message-ID: <df9e9a32b0294aee814eeb58d2d71edd@EX13D32EUC003.ant.amazon.com> (raw) In-Reply-To: <YJmMvTkp2Y1hlLLm@mail-itl> > -----Original Message----- > From: Marek Marczykowski-Górecki <firstname.lastname@example.org> > Sent: 10 May 2021 20:43 > To: Michael Brown <email@example.com>; firstname.lastname@example.org > Cc: email@example.com; firstname.lastname@example.org; email@example.com; firstname.lastname@example.org; Durrant, > Paul <email@example.com> > Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching > > On Mon, May 10, 2021 at 08:06:55PM +0100, Michael Brown wrote: > > If you have a suggested patch, I'm happy to test that it doesn't reintroduce > > the regression bug that was fixed by this commit. > > Actually, I've just tested with a simple reloading xen-netfront module. It > seems in this case, the hotplug script is not re-executed. In fact, I > think it should not be re-executed at all, since the vif interface > remains in place (it just gets NO-CARRIER flag). > > This brings a question, why removing hotplug-status in the first place? > The interface remains correctly configured by the hotplug script after > all. From the commit message: > > xen-netback: remove 'hotplug-status' once it has served its purpose > > Removing the 'hotplug-status' node in netback_remove() is wrong; the script > may not have completed. Only remove the node once the watch has fired and > has been unregistered. > > I think the intention was to remove 'hotplug-status' node _later_ in > case of quickly adding and removing the interface. Is that right, Paul? The removal was done to allow unbind/bind to function correctly. IIRC before the original patch doing a bind would stall forever waiting for the hotplug status to change, which would never happen. > In that case, letting hotplug_status_changed() remove the entry wont > work, because the watch was unregistered few lines earlier in > netback_remove(). And keeping the watch is not an option, because the > whole backend_info struct is going to be free-ed already. > > If my guess about the original reason for the change is right, I think > it should be fixed at the hotplug script level - it should check if the > device is still there before writing 'hotplug-status' node. > I'm not sure if doing it race-free is possible from a shell script (I think it > requires doing xenstore read _and_ write in a single transaction). But > in the worst case, the aftermath of loosing the race is leaving stray > 'hotplug-status' xenstore node - not ideal, but also less harmful than > failing to bring up an interface. At this point, the toolstack could cleanup > it later, perhaps while setting up that interface again (if it gets > re-connected)? > > Anyway, perhaps the best thing to do now, is to revert both commits, and > think of an alternative solution for the original issue? That of course > assumes I guessed correctly why it was done in the first place... > Simply reverting everything would likely break the ability to do unbind and bind (which is useful e.g to allow update the netback module whilst guests are still running) so I don't think that's an option. Paul
next prev parent reply other threads:[~2021-05-11 7:33 UTC|newest] Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-10 18:25 xen-netback hotplug-status regression bug Michael Brown 2021-04-13 7:12 ` Paul Durrant 2021-04-13 10:48 ` Michael Brown 2021-04-13 10:55 ` Paul Durrant 2021-04-13 15:14 ` Michael Brown 2021-04-13 15:25 ` [PATCH] xen-netback: Check for hotplug-status existence before watching Michael Brown 2021-04-13 19:12 ` Paul Durrant 2021-04-13 22:30 ` patchwork-bot+netdevbpf 2021-05-10 18:32 ` Marek Marczykowski-Górecki 2021-05-10 18:47 ` Michael Brown 2021-05-10 18:53 ` Marek Marczykowski-Górecki 2021-05-10 19:06 ` Michael Brown 2021-05-10 19:42 ` Marek Marczykowski-Górecki 2021-05-11 7:06 ` Durrant, Paul [this message] 2021-05-11 10:40 ` Marek Marczykowski-Górecki 2021-05-11 10:45 ` Marek Marczykowski-Górecki 2021-05-11 12:46 ` Durrant, Paul 2021-05-17 21:43 ` Marek Marczykowski-Górecki 2021-05-17 21:51 ` Michael Brown 2021-05-17 21:58 ` Marek Marczykowski-Górecki 2021-05-18 6:57 ` Paul Durrant 2021-05-18 9:18 ` Marek Marczykowski-Górecki [not found] ` <887f9533f5c54bfabfbff7231eb99b08@EX13D32EUC003.ant.amazon.com> [not found] ` <YKOMpXwcnr9QiXy8@mail-itl> [not found] ` <2c23e102b6254e42877eb1e8fe68a4f7@EX13D32EUC003.ant.amazon.com> 2021-05-18 10:42 ` Marek Marczykowski-Górecki
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=df9e9a32b0294aee814eeb58d2d71edd@EX13D32EUC003.ant.amazon.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='RE: [PATCH] xen-netback: Check for hotplug-status existence before watching' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).