On 8/10/2018 6:03 PM, Kevin Wolf wrote: > Am 08.10.2018 um 16:38 hat Anton Nefedov geschrieben: >> 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 <anton.nefe...@virtuozzo.com> >>>> Reviewed-by: Alberto Garcia <be...@igalia.com> >>>> --- >>>> 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() ) > > Yes, commit caeadbc8ba4 was already wrong in assuming that there is > only one possible source for -EINVAL. > >>> 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. > > blk_aio_discard() can fail with -EINVAL, too, so getting this error code > from a TRIM command doesn't mean anything. It can still have multiple > possible sources. >
I meant that common ide_dma_cb() should account EINVAL (along with other errors) as failed, unless it's TRIM, which means it's already accounted (either invalid or failed) > Maybe we just need to remember somewhere whether we already accounted > for a request (maybe an additional field in BlockAcctCookie? Or change > the type to BLOCK_ACCT_ALREADY_ACCOUNTED?) and then make an additional > block_account_one_io() call a no-op for such requests. > > Kevin > Maybe even resetting to BLOCK_ACCT_NONE == 0. It should also protect from accounting uninitialized cookie. /Anton