Re: [Qemu-devel] [PATCH v9 3/9] block: add empty account cookie type

[Anton Nefedov]

On 9/9/2019 5:54 PM, Alberto Garcia wrote:
> On Fri 06 Sep 2019 06:01:14 PM CEST, Anton Nefedov wrote:
>> This adds some protection from accounting uninitialized cookie.
>> That is, block_acct_failed/done without previous block_acct_start;
>> in that case, cookie probably holds values from previous operation.
>>
>> (Note: it might also be uninitialized holding garbage value and there
>> is still "< BLOCK_MAX_IOTYPE" assertion for that.  So
>> block_acct_failed/done without previous block_acct_start should be
>> used with caution.)
>>
>> Currently this is particularly useful in ide code where it's hard to
>> keep track whether the request started accounting or not. For example,
>> trim requests do the accounting separately.
> 
> Sorry if I'm understanding it wrong, but it sounds like you know that
> there's a bug in the ide code (where you call block_acct_done() without
> having it initialized it first), and the purpose of the this patch is to
> hide the bug ?
> 

hi,

not really; in the existing code, I can't see block_acct_done() without
block_acct_start(), but there might be double-accounting though;
e.g. ide_atapi_cmd_read_dma_cb(): it can account the same operation
twice like
   ide_handle_rw_error();
   goto eot;
   block_acct_failed();

The patch should solve it.

The commit message is misleading, sorry. I'll change to:

 > Each block_acct_done/failed call is designed to correspond to a
 > previous block_acct_start call, which initializes the stats cookie.
 > However sometimes it is not the case, e.g. some error paths might
 > report the same cookie twice because it is hard to accurately track if
 > the cookie was reported yet or not.

 > This patch cleans the cookie after report.
 > (Note: block_acct_failed/done without a previous block_acct_start at
 > all should be avoided. Uninitialized cookie might hold a garbage value
 > and there is still "< BLOCK_MAX_IOTYPE" assertion for that)

 > It will be particularly useful in ide code where it's hard to
 > keep track whether the request done its accounting or not: in the
 > following patch of the series, trim requests will do the accounting
 > separately.

/Anton