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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

31.10.2018 14:34, Anton Nefedov wrote:
> This adds some protection from accounting unitialized cookie.

uninitialized

> 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 unitialized holding garbage value and there is

and here.

>   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.

so, is it an existing bug? Could you please give an example in the code of such wrong (before the patch) accounting?

> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>   include/block/accounting.h | 1 +
>   block/accounting.c         | 6 ++++++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index ba8b04d572..878b4c3581 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -33,6 +33,7 @@ typedef struct BlockAcctTimedStats BlockAcctTimedStats;
>   typedef struct BlockAcctStats BlockAcctStats;
>   
>   enum BlockAcctType {
> +    BLOCK_ACCT_NONE = 0,
>       BLOCK_ACCT_READ,
>       BLOCK_ACCT_WRITE,
>       BLOCK_ACCT_FLUSH,
> diff --git a/block/accounting.c b/block/accounting.c
> index 70a3d9a426..8d41c8a83a 100644
> --- a/block/accounting.c
> +++ b/block/accounting.c
> @@ -195,6 +195,10 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
>   
>       assert(cookie->type < BLOCK_MAX_IOTYPE);
>   
> +    if (cookie->type == BLOCK_ACCT_NONE) {
> +        return;
> +    }
> +
>       qemu_mutex_lock(&stats->lock);
>   
>       if (failed) {
> @@ -217,6 +221,8 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
>       }
>   
>       qemu_mutex_unlock(&stats->lock);
> +
> +    cookie->type = BLOCK_ACCT_NONE;
>   }
>   
>   void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie)
> 


-- 
Best regards,
Vladimir