Re: [Qemu-devel] [PATCH] qemu-img: return allocated size for block device with qcow2 format

[叶残风 <renyime@gmail.com>]

> 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.
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 <mreitz@redhat.com> 于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 <ivanren@tencent.com>
> > ---
> >  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;
> > +}
> > +
>
>