Re: [Qemu-devel] [PATCH v8 4/9] ide: account UNMAP (TRIM) operations

[Anton Nefedov <anton.nefedov@virtuozzo.com>]

On 12/8/2019 9:16 PM, Max Reitz wrote:
> On 16.05.19 16:33, Anton Nefedov wrote:
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   hw/ide/core.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 6afadf894f..3a7ac93777 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -441,6 +441,14 @@ static void ide_issue_trim_cb(void *opaque, int ret)
>>       TrimAIOCB *iocb = opaque;
>>       IDEState *s = iocb->s;
>>   
>> +    if (iocb->i >= 0) {
>> +        if (ret >= 0) {
>> +            block_acct_done(blk_get_stats(s->blk), &s->acct);
>> +        } else {
>> +            block_acct_failed(blk_get_stats(s->blk), &s->acct);
> 
> Hmm, in other places (ide_handle_rw_error() here or
> scsi_handle_rw_error() in scsi-disk) only report this with
> BLOCK_ERROR_ACTION_REPORT.
> 
> So I’m wondering whether the same should be done here.
> 

Many other places do not check the action:
scsi_{dma|read|write}_complete(), hw/ide/atapi.c calls

That feels somewhat weird to me, to account the operation complete when
it's not.
(But I don't really know the use cases of BLOCK_ERROR_ACTION_IGNORE).

Can it be that the error action is only checked so that the operation is
not accounted twice (as there might be block_acct_done() later in the
common path with ACTION_IGNORE)

/Anton