Re: [Xen-devel] [PATCH] libxl: create backend/ xenstore dir for driver domains

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Mar 23, 2020 at 04:35:12PM +0100, Roger Pau Monné wrote:
> On Mon, Jan 06, 2020 at 05:03:40PM +0100, Marek Marczykowski-Górecki wrote:
> > Alternatively, toolstack could wait for the actual backend node to be
> > removed (by the driver domain), and then cleanup the parent directory (if
> > empty).
> 
> I'm not sure you need to cleanup the parent directory, 

You do, that's why this is an issue. Otherwise empty directories will
accumulate there, leading to various issues (inability to list, running
out of watches for monitoring them etc).

Example state:

/local/domain/5/backend = ""
/local/domain/5/backend/vif = ""
/local/domain/5/backend/vif/6 = ""
/local/domain/5/backend/vif/7 = ""
/local/domain/5/backend/vif/7/0 = ""
/local/domain/5/backend/vif/7/0/frontend = "/local/domain/7/device/vif/0"
/local/domain/5/backend/vif/7/0/frontend-id = "7"
/local/domain/5/backend/vif/7/0/online = "1"
/local/domain/5/backend/vif/7/0/state = "4"
/local/domain/5/backend/vif/7/0/script = "/etc/xen/scripts/vif-route-qubes"
/local/domain/5/backend/vif/7/0/mac = "00:16:3e:5e:6c:00"
/local/domain/5/backend/vif/7/0/ip = "10.137.0.49 fd09:24ef:4179::a89:31"
/local/domain/5/backend/vif/7/0/bridge = "xenbr0"
/local/domain/5/backend/vif/7/0/handle = "0"
/local/domain/5/backend/vif/7/0/type = "vif"
/local/domain/5/backend/vif/7/0/feature-sg = "1"
/local/domain/5/backend/vif/7/0/feature-gso-tcpv4 = "1"
/local/domain/5/backend/vif/7/0/feature-gso-tcpv6 = "1"
/local/domain/5/backend/vif/7/0/feature-ipv6-csum-offload = "1"
/local/domain/5/backend/vif/7/0/feature-rx-copy = "1"
/local/domain/5/backend/vif/7/0/feature-rx-flip = "0"
/local/domain/5/backend/vif/7/0/feature-multicast-control = "1"
/local/domain/5/backend/vif/7/0/feature-dynamic-multicast-control = "1"
/local/domain/5/backend/vif/7/0/feature-split-event-channels = "1"
/local/domain/5/backend/vif/7/0/multi-queue-max-queues = "2"
/local/domain/5/backend/vif/7/0/feature-ctrl-ring = "1"
/local/domain/5/backend/vif/7/0/hotplug-status = "connected"
/local/domain/5/backend/vif/8 = ""
/local/domain/5/backend/vif/11 = ""
/local/domain/5/backend/vif/12 = ""
/local/domain/5/backend/vif/17 = ""
/local/domain/5/backend/vif/20 = ""
/local/domain/5/backend/vif/23 = ""
/local/domain/5/backend/vif/26 = ""
/local/domain/5/backend/vif/28 = ""
/local/domain/5/backend/vif/29 = ""
/local/domain/5/backend/vif/30 = ""
/local/domain/5/backend/vif/33 = ""
/local/domain/5/backend/vif/34 = ""
(...)
/local/domain/5/backend/vif/416 = ""

> albeit it
> wouldn't hurt. It needs to be done in a transaction though, so that
> you don't race with new additions to it.

Good point.

> > I don't find it particularly appealing, as every contact with
> > libxl async code reduce overall happiness...
> > 
> > >  * There needs to be a way to deal with a broken/unresponsive driver
> > >    domain.  That will involve not waiting for the backend so must
> > >    involve simply deleting the backend from xenstore.
> > 
> > It's already there: if driver domain fails to set .../state = 6 within
> > a timeout, toolstack will forcibly remove the entry.
> 
> Would it work to change this and instead of monitor .../state = 6
> monitor that the parent directory still exist?

That could be a good idea, to avoid introducing yet another (set of)
callback. I'll look into it, it may require different handling of
dom0/non-dom0 backend.

> > > Is the distinction here between "xl shutdown" and "xl destroy", on the
> > > actual guest domain, good enough ?  Hopefully if the driver domain
> > > sees the backend directory simply vanish it can destructively tear
> > > everything down ?
> > 
> > In the past this lead to multiple issues, where hotplug script didn't
> > know which device actually was removed. In some cases I needed to
> > workaround this by saving xenstore dump into a file in an "online"
> > hotplug script, but it is very ugly solution.
> 
> Removing the whole directory without giving time to the driver domain
> to execute it's hotplug scripts can indeed lead to issues, as there's
> no guarantee that the hotplug script won't use data in xenstore in
> order to perform the cleanup IIRC.

Yes, that's what 546678c6a60f64fb186640460dfa69a837c8fba5 fixed, but not
removing it too early.

> My preferred option would be to wait for the backend directory to be
> removed by the driver domain, as I think it's the cleanest and likely
> safest approach.
> 
> Thanks, Roger.
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature