Re: [Qemu-block] [PATCH v4 3/8] ide: account UNMAP (TRIM) operations

[Anton Nefedov]

On 4/10/2018 6:33 PM, Kevin Wolf wrote:
> Am 21.08.2018 um 11:46 hat Anton Nefedov geschrieben:
>> Signed-off-by: Anton Nefedov <address@hidden>
>> Reviewed-by: Alberto Garcia <address@hidden>
>> ---
>>   hw/ide/core.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 2c62efc..352429b 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -440,6 +440,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);
>> +        }
>> +    }
>> +
>>       if (ret >= 0) {
>>           while (iocb->j < iocb->qiov->niov) {
>>               int j = iocb->j;
>> @@ -461,6 +469,9 @@ static void ide_issue_trim_cb(void *opaque, int ret)
>>                       goto done;
>>                   }
>>   
>> +                block_acct_start(blk_get_stats(s->blk), &s->acct,
>> +                                 count << BDRV_SECTOR_BITS, 
>> BLOCK_ACCT_UNMAP);
>> +
>>                   /* Got an entry! Submit and exit.  */
>>                   iocb->aiocb = blk_aio_pdiscard(s->blk,
>>                                                  sector << BDRV_SECTOR_BITS,
>> @@ -845,6 +856,7 @@ static void ide_dma_cb(void *opaque, int ret)
>>       }
>>   
>>       if (ret == -EINVAL) {
>> +        block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_UNMAP);
> 
> This looks wrong to me, ide_dma_cb() is not only called for unmap, but
> also for reads and writes, and each of them could return -EINVAL.
> 

Stating here BLOCK_ACCT_UNMAP is definitely a blunder :(

> Also, -EINVAL doesn't necessarily mean that the guest driver did
> something wrong, it could also be the result of a host problem.
> Therefore, it isn't right to call block_acct_invalid() here - especially
> since the request may already have been accounted for as either done or
> failed in ide_issue_trim_cb().
> 

Couldn't be accounted done with such retcode;
and it seems I shouldnt do block_acct_failed() there anyway - or it's
accounted twice: there and in ide_dma_cb()->ide_handle_rw_error()

But if EINVAL (from further layers) should not be accounted as an
invalid op, then it should be accounted failed instead, the thing that
current code does not do.
(and which brings us back to possible double-accounting if we account
invalid in ide_issue_trim_cb() )

> Instead, I think it would be better to immediately account for invalid
> requests in ide_issue_trim_cb() where iocb->ret = -EINVAL is set and we
> know for sure that indeed !ide_sect_range_ok() is the cause for the
> -EINVAL return code.
> 
So I guess yes, move acct_invalid in ide_issue_trim_cb() and leave
acct_failed there, and filter off TRIM commands in the common
accounting.

/Anton