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

[Kevin Wolf <kwolf@redhat.com>]

Am 08.10.2018 um 17:25 hat Anton Nefedov geschrieben:
> 
> 
> 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.nefedov@virtuozzo.com>
> >>>> Reviewed-by: Alberto Garcia <berto@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)

Oh, you would already account for failure in ide_issue_trim_cb(), too,
but only if it's EINVAL? That feels like it would complicate the code
quite a bit.

And actually, didn't commit caeadbc8ba4 break werror=stop for requests
returning -EINVAL because we don't call ide_handle_rw_error() any more?

> > 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.
> 
> Maybe even resetting to BLOCK_ACCT_NONE == 0. It should also protect
> from accounting uninitialized cookie.

That sounds good to me.

Kevin