From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48588) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dZckT-0001tn-5t for qemu-devel@nongnu.org; Mon, 24 Jul 2017 08:46:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dZckP-0001Bb-6p for qemu-devel@nongnu.org; Mon, 24 Jul 2017 08:46:09 -0400 Received: from mailhub.sw.ru ([195.214.232.25]:5739 helo=relay.sw.ru) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dZckO-0001AS-Mc for qemu-devel@nongnu.org; Mon, 24 Jul 2017 08:46:05 -0400 From: Vladimir Sementsov-Ogievskiy References: <20170606162652.112122-1-vsementsov@virtuozzo.com> <20170606162652.112122-2-vsementsov@virtuozzo.com> <50f19b1d-9736-b8b3-5616-67c3393336f3@redhat.com> <7c610bc7-a4f7-e6d6-2464-7bbb1b24002b@virtuozzo.com> Message-ID: <0e18dedc-c1ce-6b00-fbae-70af28d86d6b@virtuozzo.com> Date: Mon, 24 Jul 2017 15:45:52 +0300 MIME-Version: 1.0 In-Reply-To: <7c610bc7-a4f7-e6d6-2464-7bbb1b24002b@virtuozzo.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-devel@nongnu.org, qemu-block@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, mreitz@redhat.com, den@openvz.org, Eric Blake 12.07.2017 18:18, Vladimir Sementsov-Ogievskiy wrote: > 30.06.2017 03:27, John Snow wrote: >> >> On 06/06/2017 12:26 PM, Vladimir Sementsov-Ogievskiy wrote: >>> The function should collect statistics, about used/unused by top-level >>> format driver space (in its .file) and allocation status >>> (data/zero/discarded/after-eof) of corresponding areas in this .file. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>> --- >>> block.c | 16 ++++++++++++++ >>> include/block/block.h | 3 +++ >>> include/block/block_int.h | 2 ++ >>> qapi/block-core.json | 55 >>> +++++++++++++++++++++++++++++++++++++++++++++++ >>> 4 files changed, 76 insertions(+) >>> >>> diff --git a/block.c b/block.c >>> index 50ba264143..7d720ae0c2 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -3407,6 +3407,22 @@ int64_t >>> bdrv_get_allocated_file_size(BlockDriverState *bs) >>> } >>> /** >>> + * Collect format allocation info. See BlockFormatAllocInfo >>> definition in >>> + * qapi/block-core.json. >>> + */ >>> +int bdrv_get_format_alloc_stat(BlockDriverState *bs, >>> BlockFormatAllocInfo *bfai) >>> +{ >>> + BlockDriver *drv = bs->drv; >>> + if (!drv) { >>> + return -ENOMEDIUM; >>> + } >>> + if (drv->bdrv_get_format_alloc_stat) { >>> + return drv->bdrv_get_format_alloc_stat(bs, bfai); >>> + } >>> + return -ENOTSUP; >>> +} >>> + >>> +/** >>> * Return number of sectors on success, -errno on error. >>> */ >>> int64_t bdrv_nb_sectors(BlockDriverState *bs) >>> diff --git a/include/block/block.h b/include/block/block.h >>> index 9b355e92d8..646376a772 100644 >>> --- a/include/block/block.h >>> +++ b/include/block/block.h >>> @@ -335,6 +335,9 @@ typedef enum { >>> int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, >>> BdrvCheckMode fix); >>> +int bdrv_get_format_alloc_stat(BlockDriverState *bs, >>> + BlockFormatAllocInfo *bfai); >>> + >>> /* The units of offset and total_work_size may be chosen >>> arbitrarily by the >>> * block driver; total_work_size may change during the course of >>> the amendment >>> * operation */ >>> diff --git a/include/block/block_int.h b/include/block/block_int.h >>> index 8d3724cce6..458c715e99 100644 >>> --- a/include/block/block_int.h >>> +++ b/include/block/block_int.h >>> @@ -208,6 +208,8 @@ struct BlockDriver { >>> int64_t (*bdrv_getlength)(BlockDriverState *bs); >>> bool has_variable_length; >>> int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs); >>> + int (*bdrv_get_format_alloc_stat)(BlockDriverState *bs, >>> + BlockFormatAllocInfo *bfai); >>> int coroutine_fn >>> (*bdrv_co_pwritev_compressed)(BlockDriverState *bs, >>> uint64_t offset, uint64_t bytes, QEMUIOVector *qiov); >>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>> index ea0b3e8b13..fd7b52bd69 100644 >>> --- a/qapi/block-core.json >>> +++ b/qapi/block-core.json >>> @@ -139,6 +139,61 @@ >>> '*format-specific': 'ImageInfoSpecific' } } >>> ## >>> +# @BlockFormatAllocInfo: >>> +# >>> +# >>> +# Allocation relations between format file and underlying protocol >>> file. >>> +# All fields are in bytes. >>> +# >> I guess this is a relation in the sense that the format differentiates >> between used-unused and the protocol differentiates between >> data-zero-trim which gives us the 2D matrix, showing a relation between >> "two" files. >> >> I find the use of the word "file" here a bit of a misdirection, though, >> as qcow2 (or any other format) in this sense is not a file but rather a >> schema used for interpreting data, and the file itself belongs solely to >> the protocol. >> >> I might suggest phrasing this as ... >> >> "Allocation information of an underlying protocol file, as partitioned >> by a format driver's utilization of said allocations." >> >> Maybe that's too wordy. Eric? >> >>> +# There are two types of the format file portions: 'used' and >>> 'unused'. It's up >>> +# to the format how to interpret these types. For now the only >>> format supporting >> "format file" seems misleading for similar reasons to my objection >> above, which is that the format doesn't have a file, exactly. It's an >> abstract schema. >> >> "It's up to the format how to interpret these types" is also a bit too >> vague to help inform readers what the types mean, IMO. >> >> Here's an attempt: >> >> "Allocations may be considered either used or unused by the format >> driver interpreting those allocations. It is at the discretion of the >> format driver (e.g. qcow2) which regions of its backing storage are >> considered in-use or not." > > So we are saying about "allocations". But unallocated data may be > used/unused too, so, > can we call unallocated areas "allocations"? John? Note: we can add this with x- prefix or something like this if this simplifies things (and adjust doc later). > >> >> I'm trying to clarify that it is not up to the qcow2 driver what "used" >> or "unused" *means* but rather that only the qcow2 driver itself can >> know which segments are used or unused, as these are semantic >> distinctions that belong to the format driver layer. Does that make >> sense? >> >> That's the spirit of my suggestion. >> >>> +# the feature is Qcow2 and for this case 'used' are clusters with >>> positive >>> +# refcount and unused a clusters with zero refcount. Described >>> portions include >>> +# all format file allocations, not only virtual disk data >>> (metadata, internal >>> +# snapshots, etc. are included). >>> +# >> "For now, the only format driver supporting this feature is Qcow2 which >> is a cluster based format. Clusters considered in-use by qcow2 are those >> with a non-zero refcount in the format metadata. All other clusters, if >> present, are considered unused." >> >> (Your original description is actually pretty clear.) >> >> I might add some further examples to illustrate the abstraction boundary >> we're targeting: >> >> "Examples of unused allocations for the Qcow2 format are leaked >> clusters, pre-allocated clusters, and recently freed clusters." >> >>> +# For the underlying file there are native block-status types of >>> the portions: >> How about "underlying protocol file" or "underlying storage protocol" or >> something that uses the word "protocol" to make it very clear about when >> we're talking about a format (qcow2) and when we're talking about the >> storage/protocol file itself (raw posix) >> >>> +# - data: allocated data >> ACK. My favorite kind of data. Easy to understand for idiots like me. >> >>> +# - zero: read-as-zero holes >> This is not *necessarily* a hole, at the discretion of the protocol. >> Depending on how we've backed the qcow2 we might not actually know how >> the zeroes are stored, or if they are stored. All we know is that the >> storage protocol here knows that this data happens to be zero. >> >> I find the usage of "hole" here to be misleading, as it suggests >> naturally either filesystem sparse allocations (which is correct, >> incidentally) but also qcow2 holes, which doesn't have anything to do >> with zeroes, necessarily. >> >>> +# - discarded: not allocated >> This might be OK; I don't have a better suggestion. "not allocated" is >> again protocol-dependent, but I can't think of a better way to phrase >> this, actually... >> >>> +# 4th additional type is 'overrun', which is for the format file >>> portions beyond >>> +# the end of the underlying file. >>> +# >> "Which is data referenced by the format driver located beyond EOF of the >> protocol file." >> >> The key thing I am trying to illustrate in the phrase is that the format >> file specifies or alludes to the existence of data that is beyond the >> EOF for the protocol file. >> >> I think -- though I cannot prove -- that this is almost certainly a >> special case of read-as-zero. If that is the case, perhaps we could >> mention as much. >> >> An example here would be really illustrative: >> >> "For example, a partially allocated cluster at the end of a QCOW2 file, >> where Qcow2 generally operates on complete clusters." >> >>> +# So, the fields are: >>> +# >>> +# @used-data: used by the format file and backed by data in the >>> underlying file >>> +# >>> +# @used-zero: used by the format file and backed by a hole in the >>> underlying >>> +# file >>> +# >> Maybe "backed by zeroes in the underlying file; which may be a >> filesystem hole for e.g. POSIX files." >> >>> +# @used-discarded: used by the format file but actually unallocated >>> in the >>> +# underlying file >>> +# >> Which would almost certainly be an error, right? Mentioning as much >> might be good. >> >>> +# @used-overrun: used by the format file beyond the end of the >>> underlying file >>> +# >> Which may or may not be an error, depending on how the protocol file >> (for the format driver?) handles reads to areas out of bounds. >> >>> +# @unused-data: allocated data in the underlying file not used by >>> the format >>> +# >>> +# @unused-zero: holes in the underlying file not used by the format >>> file >>> +# >>> +# @unused-discarded: unallocated areas in the underlying file not >>> used by the >>> +# format file >>> +# >>> +# Note: sum of 6 fields {used,unused}-{data,zero,discarded} is >>> equal to the >>> +# length of the underlying file. >>> +# >>> +# Since: 2.10 >>> +# >>> +## >>> +{ 'struct': 'BlockFormatAllocInfo', >>> + 'data': {'used-data': 'uint64', >>> + 'used-zero': 'uint64', >>> + 'used-discarded': 'uint64', >>> + 'used-overrun': 'uint64', >>> + 'unused-data': 'uint64', >>> + 'unused-zero': 'uint64', >>> + 'unused-discarded': 'uint64' } } >>> + >>> +## >>> # @ImageCheck: >>> # >>> # Information about a QEMU image file check >>> >> All of my suggestions here are purely on phrasings. The mechanics of >> this patch are now clear to me and I think it is useful information to >> have in qemu. >> >> Thanks for putting up with my questions! > > -- Best regards, Vladimir