[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v4 3/8] ide: account UNMAP (TRIM) operations
From: |
Anton Nefedov |
Subject: |
Re: [Qemu-block] [PATCH v4 3/8] ide: account UNMAP (TRIM) operations |
Date: |
Mon, 8 Oct 2018 15:25:56 +0000 |
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 <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() )
>
> 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