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?
Juergen
> -----Original Message-----
> From: Jürgen Groß <[email protected]>
> Sent: 09 December 2019 11:55
> To: Roger Pau Monné <[email protected]>; Durrant, Paul
> <[email protected]>
> Cc: [email protected]; [email protected]; Stefano
> Stabellini <[email protected]>; Boris Ostrovsky
> <[email protected]>
> 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.
Paul
>
>
> Juergen
On 09.12.19 13:03, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jürgen Groß <[email protected]>
>> Sent: 09 December 2019 11:55
>> To: Roger Pau Monné <[email protected]>; Durrant, Paul
>> <[email protected]>
>> Cc: [email protected]; [email protected]; Stefano
>> Stabellini <[email protected]>; Boris Ostrovsky
>> <[email protected]>
>> 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. 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.
Juergen
> -----Original Message-----
> From: Jürgen Groß <[email protected]>
> Sent: 09 December 2019 12:09
> To: Durrant, Paul <[email protected]>; Roger Pau Monné
> <[email protected]>
> Cc: [email protected]; [email protected]; Stefano
> Stabellini <[email protected]>; Boris Ostrovsky
> <[email protected]>
> 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ß <[email protected]>
> >> Sent: 09 December 2019 11:55
> >> To: Roger Pau Monné <[email protected]>; Durrant, Paul
> >> <[email protected]>
> >> Cc: [email protected]; [email protected];
> Stefano
> >> Stabellini <[email protected]>; Boris Ostrovsky
> >> <[email protected]>
> >> 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.
> 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.
Paul
>
> Juergen
On 09.12.19 13:19, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jürgen Groß <[email protected]>
>> Sent: 09 December 2019 12:09
>> To: Durrant, Paul <[email protected]>; Roger Pau Monné
>> <[email protected]>
>> Cc: [email protected]; [email protected]; Stefano
>> Stabellini <[email protected]>; Boris Ostrovsky
>> <[email protected]>
>> 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ß <[email protected]>
>>>> Sent: 09 December 2019 11:55
>>>> To: Roger Pau Monné <[email protected]>; Durrant, Paul
>>>> <[email protected]>
>>>> Cc: [email protected]; [email protected];
>> Stefano
>>>> Stabellini <[email protected]>; Boris Ostrovsky
>>>> <[email protected]>
>>>> 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
> -----Original Message-----
> From: Jürgen Groß <[email protected]>
> Sent: 09 December 2019 13:39
> To: Durrant, Paul <[email protected]>; Roger Pau Monné
> <[email protected]>
> Cc: [email protected]; [email protected]; Stefano
> Stabellini <[email protected]>; Boris Ostrovsky
> <[email protected]>
> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
> closed
>
> On 09.12.19 13:19, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Jürgen Groß <[email protected]>
> >> Sent: 09 December 2019 12:09
> >> To: Durrant, Paul <[email protected]>; Roger Pau Monné
> >> <[email protected]>
> >> Cc: [email protected]; [email protected];
> Stefano
> >> Stabellini <[email protected]>; Boris Ostrovsky
> >> <[email protected]>
> >> 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ß <[email protected]>
> >>>> Sent: 09 December 2019 11:55
> >>>> To: Roger Pau Monné <[email protected]>; Durrant, Paul
> >>>> <[email protected]>
> >>>> Cc: [email protected]; [email protected];
> >> Stefano
> >>>> Stabellini <[email protected]>; Boris Ostrovsky
> >>>> <[email protected]>
> >>>> 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".
That depends on the toolstack. IIUC the only 'supported' toolstack is xl/libxl, which will set 'state' to 5 and 'online' to 0 to initiate an unplug.
Paul
>
>
> Juergen