[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PULL 20/37] qcow2: Give the refcount cach
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PULL 20/37] qcow2: Give the refcount cache the minimum possible size by default |
Date: |
Mon, 28 May 2018 10:38:55 +0200 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
Am 25.05.2018 um 19:10 hat Peter Maydell geschrieben:
> On 15 May 2018 at 16:40, Kevin Wolf <address@hidden> wrote:
> > From: Alberto Garcia <address@hidden>
> >
> > The L2 and refcount caches have default sizes that can be overridden
> > using the l2-cache-size and refcount-cache-size (an additional
> > parameter named cache-size sets the combined size of both caches).
>
> Hi; Coverity raises a nit about this patch (CID 1391229):
CCing Berto as the patch author.
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 2f36e632f9..6d532470a8 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -802,23 +802,30 @@ static void read_cache_sizes(BlockDriverState *bs,
> > QemuOpts *opts,
> > } else if (refcount_cache_size_set) {
> > *l2_cache_size = combined_cache_size - *refcount_cache_size;
> > } else {
> > - *refcount_cache_size = combined_cache_size
> > - / (DEFAULT_L2_REFCOUNT_SIZE_RATIO + 1);
> > - *l2_cache_size = combined_cache_size - *refcount_cache_size;
> > + uint64_t virtual_disk_size = bs->total_sectors *
> > BDRV_SECTOR_SIZE;
> > + uint64_t max_l2_cache = virtual_disk_size / (s->cluster_size /
> > 8);
> > + uint64_t min_refcount_cache =
> > + (uint64_t) MIN_REFCOUNT_CACHE_SIZE * s->cluster_size;
>
> Here we have a (uint64_t) cast that ensures we're doing a 64x64 multiply
> rather than a 32x32 one...
>
> > +
> > + /* Assign as much memory as possible to the L2 cache, and
> > + * use the remainder for the refcount cache */
> > + if (combined_cache_size >= max_l2_cache + min_refcount_cache) {
> > + *l2_cache_size = max_l2_cache;
> > + *refcount_cache_size = combined_cache_size -
> > *l2_cache_size;
> > + } else {
> > + *refcount_cache_size =
> > + MIN(combined_cache_size, min_refcount_cache);
> > + *l2_cache_size = combined_cache_size -
> > *refcount_cache_size;
> > + }
> > }
> > } else {
> > - if (!l2_cache_size_set && !refcount_cache_size_set) {
> > + if (!l2_cache_size_set) {
> > *l2_cache_size = MAX(DEFAULT_L2_CACHE_BYTE_SIZE,
> > (uint64_t)DEFAULT_L2_CACHE_CLUSTERS
> > * s->cluster_size);
> > - *refcount_cache_size = *l2_cache_size
> > - / DEFAULT_L2_REFCOUNT_SIZE_RATIO;
> > - } else if (!l2_cache_size_set) {
> > - *l2_cache_size = *refcount_cache_size
> > - * DEFAULT_L2_REFCOUNT_SIZE_RATIO;
> > - } else if (!refcount_cache_size_set) {
> > - *refcount_cache_size = *l2_cache_size
> > - / DEFAULT_L2_REFCOUNT_SIZE_RATIO;
> > + }
> > + if (!refcount_cache_size_set) {
> > + *refcount_cache_size = MIN_REFCOUNT_CACHE_SIZE *
> > s->cluster_size;
>
> ...but in the else clause down here, we don't have the cast, and
> Coverity complains that we evaluate a 32-bit result and then
> put it in a 64-bit variable. Should this have the (uint64_t)
> cast as well ?
It's a false positive, MIN_REFCOUNT_CACHE_SIZE is 4 and s->cluster_size
is at most 2 MB, so this will never overflow.
I guess we can change the code anyway to silence it?
Kevin
- [Qemu-block] [PULL 11/37] blockjob: Fix assertion in block_job_finalize(), (continued)
- [Qemu-block] [PULL 11/37] blockjob: Fix assertion in block_job_finalize(), Kevin Wolf, 2018/05/15
- [Qemu-block] [PULL 16/37] blockjob: Add block_job_driver(), Kevin Wolf, 2018/05/15
- [Qemu-block] [PULL 14/37] blockjob: Implement block_job_set_speed() centrally, Kevin Wolf, 2018/05/15
- [Qemu-block] [PULL 19/37] specs/qcow2: Clarify that compressed clusters have the COPIED bit reset, Kevin Wolf, 2018/05/15
- [Qemu-block] [PULL 13/37] blockjob: Move RateLimit to BlockJob, Kevin Wolf, 2018/05/15
- [Qemu-block] [PULL 12/37] blockjob: Wrappers for progress counter access, Kevin Wolf, 2018/05/15
- [Qemu-block] [PULL 17/37] iotests: Split 214 off of 122, Kevin Wolf, 2018/05/15
- [Qemu-block] [PULL 22/37] iotests: Add failure matching to common.qemu, Kevin Wolf, 2018/05/15
- [Qemu-block] [PULL 20/37] qcow2: Give the refcount cache the minimum possible size by default, Kevin Wolf, 2018/05/15
[Qemu-block] [PULL 21/37] docs: Document the new default sizes of the qcow2 caches, Kevin Wolf, 2018/05/15
[Qemu-block] [PULL 26/37] block: Add BDRV_REQ_WRITE_UNCHANGED flag, Kevin Wolf, 2018/05/15
[Qemu-block] [PULL 28/37] block/quorum: Support BDRV_REQ_WRITE_UNCHANGED, Kevin Wolf, 2018/05/15
[Qemu-block] [PULL 18/37] Fix error message about compressed clusters with OFLAG_COPIED, Kevin Wolf, 2018/05/15
[Qemu-block] [PULL 23/37] iotests: Skip 181 and 201 without userfaultfd, Kevin Wolf, 2018/05/15
[Qemu-block] [PULL 27/37] block: Set BDRV_REQ_WRITE_UNCHANGED for COR writes, Kevin Wolf, 2018/05/15
[Qemu-block] [PULL 25/37] block: BLK_PERM_WRITE includes ..._UNCHANGED, Kevin Wolf, 2018/05/15