Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to closed

[JÃrgen GroÃ]

On 09.12.19 13:19, Durrant, Paul wrote:
> > -----Original Message-----
> > From: JÃrgen Groà <jgross@xxxxxxxx>
> > Sent: 09 December 2019 12:09
> > To: Durrant, Paul <pdurrant@xxxxxxxxxx>; Roger Pau MonnÃ
> > <roger.pau@xxxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx; Stefano
> > Stabellini <sstabellini@xxxxxxxxxx>; Boris Ostrovsky
> > <boris.ostrovsky@xxxxxxxxxx>
> > Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
> > closed
> >
> > On 09.12.19 13:03, Durrant, Paul wrote:
> > > > -----Original Message-----
> > > > From: JÃrgen Groà <jgross@xxxxxxxx>
> > > > Sent: 09 December 2019 11:55
> > > > To: Roger Pau Monnà <roger.pau@xxxxxxxxxx>; Durrant, Paul
> > > > <pdurrant@xxxxxxxxxx>
> > > > Cc: linux-kernel@xxxxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> > Stefano
> > > > Stabellini <sstabellini@xxxxxxxxxx>; Boris Ostrovsky
> > > > <boris.ostrovsky@xxxxxxxxxx>
> > > > Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced
> > to
> > > > closed
> > > >
> > > > On 09.12.19 12:39, Roger Pau Monnà wrote:
> > > > > On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
> > > > > > Only force state to closed in the case when the toolstack may need to
> > > > > > clean up. This can be detected by checking whether the state in
> > > > xenstore
> > > > > > has been set to closing prior to device removal.
> > > > >
> > > > > I'm not sure I see the point of this, I would expect that a failure to
> > > > > probe or the removal of the device would leave the xenbus state as
> > > > > closed, which is consistent with the actual driver state.
> > > > >
> > > > > Can you explain what's the benefit of leaving a device without a
> > > > > driver in such unknown state?
> > > >
> > > > And more concerning: did you check that no frontend/backend is
> > > > relying on the closed state to be visible without closing having been
> > > > set before?
> > >
> > > Blkfront doesn't seem to mind and I believe the Windows PV drivers cope,
> > but I don't really understand the comment since this patch is actually
> > removing a case where the backend transitions directly to closed.
> >
> > I'm not speaking of blkfront/blkback only, but of net, tpm, scsi, pvcall
> > etc. frontends/backends. After all you are modifying a function common
> > to all PV driver pairs.
> >
> > You are removing a state switc to "closed" in case the state was _not_
> > "closing" before.
>
> Yes, which AFAIK is against the intention of the generic PV protocol such that it ever existed anyway.

While this might be the case we should _not_ break any guests
running now. So this kind of reasoning is dangerous.

> > So any PV driver reacting to "closed" of the other end
> > in case the previous state might not have been "closing" before is at
> > risk to misbehave with your patch.
>
> Well, they will see nothing now. If the state was not closing, it gets left alone, so the frontend shouldn't do anything. The only risk that I can see is that some frontend/backend pair needed a direct 4 -> 6 transition to support 'unbind' before but AFAIK nothing has ever supported that, and blk and net crash'n'burn if you try that on upstream as it stands. A clean unplug would always set state to 5 first, since that's part of the unplug protocol.

That was my question: are you sure all current and previous
guest frontends and backends are handling unplug this way?

Not "should handle", but "do handle".


Juergen