On 7/29/19 6:09 AM, Stefan Hajnoczi wrote:
> On Fri, Jul 26, 2019 at 04:18:46PM -0400, John Snow wrote:
>> Paolo, Stefan and Kevin: can I loop you in here? I'm quite uncertain
>> about this and I'd like to clear this up quickly if it's possible:
>>
>> On 7/25/19 8:58 PM, John Snow wrote:
>>>
>>>
>>> On 7/7/19 10:55 PM, shaju.abra...@nutanix.com wrote:
>>>> From: Shaju Abraham <shaju.abra...@nutanix.com>
>>>>
>>>> During the IDE DMA transfer for a ISCSI target,when libiscsi encounters
>>>> a SENSE KEY error, it sets the task->sense to the value "COMMAND ABORTED".
>>>> The function iscsi_translate_sense() later translaters this error to
>>>> -ECANCELED
>>>> and this value is passed to the callback function. In the case of IDE DMA
>>>> read
>>>> or write, the callback function returns immediately if the value of the ret
>>>> argument is -ECANCELED.
>>>> Later when ide_cancel_dma_sync() function is invoked the assertion
>>>> "s->bus->dma->aiocb == ((void *)0)" fails and the qemu process gets
>>>> terminated.
>>>> Fix the issue by making the value of s->bus->dma->aiocb = NULL when
>>>> -ECANCELED is passed to the callback.
>>>>
>>>> Signed-off-by: Shaju Abraham <shaju.abra...@nutanix.com>
>>>> ---
>>>> hw/ide/core.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>>> index 6afadf8..78ea357 100644
>>>> --- a/hw/ide/core.c
>>>> +++ b/hw/ide/core.c
>>>> @@ -841,6 +841,7 @@ static void ide_dma_cb(void *opaque, int ret)
>>>> bool stay_active = false;
>>>>
>>>> if (ret == -ECANCELED) {
>>>> + s->bus->dma->aiocb = NULL;
>>>> return;
>>>> }
>>>>
>>>>
>>>
>>> The part that makes me nervous here is that I can't remember why we do
>>> NO cleanup whatsoever for the ECANCELED case.
>>>
>>> commit 0d910cfeaf2076b116b4517166d5deb0fea76394
>>> Author: Fam Zheng <f...@redhat.com>
>>> Date: Thu Sep 11 13:41:07 2014 +0800
>>>
>>> ide/ahci: Check for -ECANCELED in aio callbacks
>>>
>>>
>>> ... This looks like we never expected the aio callbacks to ever get
>>> called with ECANCELED, so we treat this as a QEMU-internal signal.
>>>
>>> It looks like we expect these callbacks to do NOTHING in this case; but
>>> I'm not sure where the IDE state machine does its cleanup otherwise.
>>> (The DMA might have been canceled, but the DMA and IDE state machines
>>> still need to exit their loop.)
>>>
>>> If you take a look at this patch from 2014 though, there are many other
>>> spots where we have littered ECANCELED checks that might also cause
>>> problems if we're receiving error codes we thought we couldn't get normally.
>>>
>>> I am worried this patch papers over something worse.
>>>
>> I'm not clear why Fam's patch adds a do-nothing return to the ide_dma_cb
>> if it's invoked with ECANCELED: shouldn't it be the case that the IDE
>> state machine needs to know that a transfer it was relying on to service
>> an ATA command was canceled and treat it like an error?
>>
>> Why was it ever correct to ignore these? Is it because we only ever
>> canceled DMA during reset/shutdown/etc?
>>
>> It appears as if iscsi requests can actually genuinely return an
>> ECANCELED errno, so there are likely several places in the IDE code that
>> need to accommodate this from happening.
>>
>> The easiest fix LOOKS like just deleting the special-casing of ECANCELED
>> altogether and letting the error pathways handle things as normal.
>>
>> Am I mistaken?
>
> I think your instincts are right that there are deeper issues. The
> first step would be test cases, then you can be sure various scenarios
> have been handled correctly.
>
Suggestions? I'm not sure what's supposed to work and in what way here.
I guess this stuff was introduced for bdrv_aio_cancel_async, but it's
not immediately clear what's supposed to happen when you call that.
> I noticed that ide_sector_read_cb(), ide_sector_write_cb(), and
> ide_flush_cb() all differ in whether they reset s->pio_aiocb and
> s->status before returning early due to -ECANCELED. That must be a bug.
>
> I didn't look at the ide_dma_cb() code path.
>
> Stefan
>
Hm ...
It looks like canceling the ide_dma_cb AIOCB objects doesn't do anything
too useful?
dma_blk_io and friends will establish dma_aio_cancel as the async cancel
callback. So if we do cancel these objects, we're going to call this
function:
static void dma_aio_cancel(BlockAIOCB *acb)
{
DMAAIOCB *dbs = container_of(acb, DMAAIOCB, common);
trace_dma_aio_cancel(dbs);
if (dbs->acb) {
blk_aio_cancel_async(dbs->acb);
}
if (dbs->bh) {
cpu_unregister_map_client(dbs->bh);
qemu_bh_delete(dbs->bh);
dbs->bh = NULL;
}
}
but there's no cancel callback for the lower layer in dbs->acb, so
that's just a nop, I think -- blk_aio_prwv doesn't offer an asynchronous
cancel mechanism.
Next, we'll unschedule the BH if there is one. I think the only case
where there is one is the reschedule_dma case of dma_blk_cb. (I'm not
too familiar with these DMA helpers: in what cases do we expect the iov
to be empty?)
So it looks like this cancellation will produce one of two effects,
depending on when it's invoked:
1) We'll stall the DMA permanently by deleting that BH, because
dma_complete will never get invoked and therefore nobody will ever call
ide_dma_cb with any return value of any kind. The IDE state machine
likely just hangs waiting for the DMA to finish until the guest OS
decides to reset the errant controller.
2) The DMA will continue blissfully unaware it was canceled, because the
lower AIOCB has no cancel method, and so will finish, call back to
dma_blk_cb, and continue the transfer loop unaware.
... Does your reading align with mine?
If it does -- if there are indeed no places in the code today that
artificially inject -ECANCELED -- I need to remove these special stanzas
from the IDE code and allow the IDE state machine to handle these errors
as true errors.
I'm just not confident enough in my unwinding of the DMA callback
spaghetti, though.
--js
