qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH] qemu-img: return allocated size for block devic


From: Max Reitz
Subject: Re: [Qemu-block] [PATCH] qemu-img: return allocated size for block device with qcow2 format
Date: Fri, 4 May 2018 16:27:00 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0

On 2018-05-03 15:08, 叶残风 wrote:
>> The whole implementation reminds me a lot of qcow2's check function,
>> which basically just recalculates the refcounts.  So I'm wondering
>> whether you could just count how many clusters with non-0 refcount there
>> are and thus simplify the implementation dramatically.
> 
> Thanks for your review, Max.
> 
> Yes, just get the highest non-0 refcount cluster index can simply get the
> end offset. But in some situations(such as some errors happen), a cluster
> is indexed in index table, but the refcount may be 0 error, just like the
> qcow2 check inconsistency. So I traverse the whole index part, include L1,
> L2, snapshot and so on.

Yeah, well, then you use qemu-img check to repair it.  The refcount
structure is there to know which clusters are allocated, so we should
use it when we want to know that and not assume it's broken.  If the
user thinks it may be broken, they are free to run qemu-img check to check.

Note that qemu makes sure that no refcount ever is lower than it must
be.  Refcounts are always updated in an order such that if qemu crashes
during some operations the refcount is equal to or higher than what it
needs to be.  Anything else is a bug in qemu (or the result of
lazy-refcounts, but then the image is marked dirty and automatically
repaired when opened R/W the next time).

Max

> I try to reuse the qcow2 check code, but the check routine limit the
> avaliable
> end to SIZE_MAX which work well in file situation, however the block
> device has
> a fix end. And the check routine print a lot check info which I don't need.
> 
> 
> 
>>> +static int64_t qcow2_get_allocated_file_size(BlockDriverState *bs)
>>> +{
>>> +    struct stat st;
>>> +    if (stat(bs->filename, &st) < 0 || !S_ISBLK(st.st_mode)) {
>>> +        goto get_file_size;
>>> +    }
>>
>> This definitely doesn't work because nobody guarantees that bs->filename
>> is something that stat() can work with.  I'm aware that you need to do
>> the S_ISBLK() check somewhere, but the qcow2 driver is not the right
> place.
>>
>> I don't really have a good way around this, though.  These things come
>> to mind:
>> ...
> 
> Yea, thank you for your suggestion. I think a hack to
> qcow2_get_allocated_file_size
> will work well.
>> (3) As a hack, qcow2_get_allocated_file_size() could first always call
>> bdrv_get_allocated_file_size(bs->file->bs), and if that returns 0 (which
>> is absolutely impossible for qcow2 files because they have an image
>> header that takes up some space), we fall back to
>> qcow2_get_block_allocated_size().  While I consider it a hack, I can't
>> come up with a scenario where it wouldn't work.
> 
> 
> Max Reitz <address@hidden <mailto:address@hidden>> 于2018年5月2日
> 周三 下午10:37写道:
> 
>     Hi,
> 
>     [replying to this version because the previous mail doesn't seem to have
>     made it to the mailing lists for whatever reason]
> 
>     On 2018-05-02 15:34, Ivan Ren wrote:
>     > qemu-img info with a block device which has a qcow2 format always
>     > return 0 for disk size, and this can not reflect the qcow2 size
>     > and the used space of the block device. This patch return the
>     > allocated size of qcow2 as the disk size.
> 
>     I'm not quite sure whether you really need this information for block
>     devices (I tend to agree with Eric that wr_highest_cluster is the more
>     important information there), but I can imagine it just being nice
>     to have.
> 
>     So the basic idea makes sense to me, but I think the implementation can
>     be simplified and the reporting in qemu-img should be done differently.
> 
>     >
>     > Signed-off-by: Ivan Ren <address@hidden
>     <mailto:address@hidden>>
>     > ---
>     >  block/qcow2-bitmap.c |  69 +++++++++++++++++
>     >  block/qcow2.c        | 212
>     +++++++++++++++++++++++++++++++++++++++++++++++++++
>     >  block/qcow2.h        |  42 ++++++++++
>     >  3 files changed, 323 insertions(+)
> 
>     The whole implementation reminds me a lot of qcow2's check function,
>     which basically just recalculates the refcounts.  So I'm wondering
>     whether you could just count how many clusters with non-0 refcount there
>     are and thus simplify the implementation dramatically.
> 
>     [...]
> 
>     > +static int64_t qcow2_get_allocated_file_size(BlockDriverState *bs)
>     > +{
>     > +    struct stat st;
>     > +    if (stat(bs->filename, &st) < 0 || !S_ISBLK(st.st_mode)) {
>     > +        goto get_file_size;
>     > +    }
> 
>     This definitely doesn't work because nobody guarantees that bs->filename
>     is something that stat() can work with.  I'm aware that you need to do
>     the S_ISBLK() check somewhere, but the qcow2 driver is not the right
>     place.
> 
>     I don't really have a good way around this, though.  These things come
>     to mind:
> 
>     (1) We could let file-posix report an error for S_ISBLK because the
>     information is known to be usually useless -- but I think that is not
>     quite the right thing to do because maybe some block devices do report
>     useful information there, I don't know.
> 
>     (2) Or we introduce a new field in qemu-img info (and thus in ImageInfo,
>     too, I suppose?).  qemu-img info (or rather bdrv_query_image_info())
>     could detect whether the format layer supports
>     bdrv_get_allocated_file_size, and if so, it emits that information
>     separately from the allocated size of bs->file->bs.  But that would
>     break vmdk...
> 
>     (3) As a hack, qcow2_get_allocated_file_size() could first always call
>     bdrv_get_allocated_file_size(bs->file->bs), and if that returns 0 (which
>     is absolutely impossible for qcow2 files because they have an image
>     header that takes up some space), we fall back to
>     qcow2_get_block_allocated_size().  While I consider it a hack, I can't
>     come up with a scenario where it wouldn't work.
> 
>     Max
> 
>     > +
>     > +    return qcow2_get_block_allocated_size(bs);
>     > +
>     > +get_file_size:
>     > +    if (bs->file) {
>     > +        return bdrv_get_allocated_file_size(bs->file->bs);
>     > +    }
>     > +    return -ENOTSUP;
>     > +}
>     > +
> 


Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]