Message ID | 1331112179-12726-1-git-send-email-wdongxu@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
On Wed, Mar 7, 2012 at 9:22 AM, Dong Xu Wang <wdongxu@linux.vnet.ibm.com> wrote: > From: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> > > Discussion can be found at: > http://patchwork.ozlabs.org/patch/128730/ > > This patch add image fragmentation statistics while using qemu-img info. > > Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> > --- > block.c | 13 +++++++++++++ > block.h | 7 +++++++ > block_int.h | 1 + > qemu-img.c | 9 +++++++++ > 4 files changed, 30 insertions(+), 0 deletions(-) > > diff --git a/block.c b/block.c > index 52ffe14..947607b 100644 > --- a/block.c > +++ b/block.c > @@ -2588,6 +2588,19 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) > return drv->bdrv_get_info(bs, bdi); > } > > +int bdrv_get_fragment(BlockDriverState *bs, BlockFragInfo *bfi) bdrv_get_fraginfo() makes it clearer that this function gets a summary of fragmentation information. bdrv_get_fragment() makes me think it gets one specific "fragment". > +{ > + BlockDriver *drv = bs->drv; > + if (!drv) { > + return -ENOMEDIUM; > + } > + if (!drv->bdrv_get_fragment) { > + return -ENOTSUP; > + } > + memset(bfi, 0, sizeof(*bfi)); > + return drv->bdrv_get_fragment(bs, bfi); For now this .drv_get_fraginfo() interface makes sense but if we merge the QCOW2<->QED in-place conversion patch series in the future it will be possible to implement this in a generic way because image formats will expose their guest -> host mapping. > @@ -1126,6 +1127,14 @@ static int img_info(int argc, char **argv) > printf("cluster_size: %d\n", bdi.cluster_size); > } > } > + if (bdrv_get_fragment(bs, &bfi) >= 0) { I think we need a separate sub-command for fragmentation info: qemu-img fraginfo <image-file> Utilities that invoke qemu-img info want it to be fast. Reading all metadata from a large image can take several seconds. Since many qemu-img info users don't need to see the fragmentation information, it makes sense to put it in a new sub-command. > + if (bfi.total_clusters != 0 && bfi.allocated_clusters != 0) { > + printf("%lld/%lld = %0.2f%% allocated, %0.2f%% fragmented\n", Please use the PRId64 macro instead of %lld because some compilers warn when uint64_t is not typedefed to long long int. Stefan
Am 12.03.2012 14:07, schrieb Stefan Hajnoczi: > On Wed, Mar 7, 2012 at 9:22 AM, Dong Xu Wang <wdongxu@linux.vnet.ibm.com> wrote: >> From: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> >> >> Discussion can be found at: >> http://patchwork.ozlabs.org/patch/128730/ >> >> This patch add image fragmentation statistics while using qemu-img info. >> >> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> >> --- >> block.c | 13 +++++++++++++ >> block.h | 7 +++++++ >> block_int.h | 1 + >> qemu-img.c | 9 +++++++++ >> 4 files changed, 30 insertions(+), 0 deletions(-) >> >> diff --git a/block.c b/block.c >> index 52ffe14..947607b 100644 >> --- a/block.c >> +++ b/block.c >> @@ -2588,6 +2588,19 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) >> return drv->bdrv_get_info(bs, bdi); >> } >> >> +int bdrv_get_fragment(BlockDriverState *bs, BlockFragInfo *bfi) > > bdrv_get_fraginfo() makes it clearer that this function gets a summary > of fragmentation information. bdrv_get_fragment() makes me think it > gets one specific "fragment". > >> +{ >> + BlockDriver *drv = bs->drv; >> + if (!drv) { >> + return -ENOMEDIUM; >> + } >> + if (!drv->bdrv_get_fragment) { >> + return -ENOTSUP; >> + } >> + memset(bfi, 0, sizeof(*bfi)); >> + return drv->bdrv_get_fragment(bs, bfi); > > For now this .drv_get_fraginfo() interface makes sense but if we merge > the QCOW2<->QED in-place conversion patch series in the future it will > be possible to implement this in a generic way because image formats > will expose their guest -> host mapping. We can probably resurrect Devin's patches for this part even now instead of introducing an interface that we don't really want. > >> @@ -1126,6 +1127,14 @@ static int img_info(int argc, char **argv) >> printf("cluster_size: %d\n", bdi.cluster_size); >> } >> } >> + if (bdrv_get_fragment(bs, &bfi) >= 0) { > > I think we need a separate sub-command for fragmentation info: > > qemu-img fraginfo <image-file> > > Utilities that invoke qemu-img info want it to be fast. Reading all > metadata from a large image can take several seconds. Since many > qemu-img info users don't need to see the fragmentation information, > it makes sense to put it in a new sub-command. Yes. If we wanted to merge it into an existing qemu-img subcommand, I think check would be the one, as it scans the whole image already today and fragmentation is something that could be added fairly easily. Kevin
On Mon, Mar 12, 2012 at 1:14 PM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 12.03.2012 14:07, schrieb Stefan Hajnoczi: >> On Wed, Mar 7, 2012 at 9:22 AM, Dong Xu Wang <wdongxu@linux.vnet.ibm.com> wrote: >>> @@ -1126,6 +1127,14 @@ static int img_info(int argc, char **argv) >>> printf("cluster_size: %d\n", bdi.cluster_size); >>> } >>> } >>> + if (bdrv_get_fragment(bs, &bfi) >= 0) { >> >> I think we need a separate sub-command for fragmentation info: >> >> qemu-img fraginfo <image-file> >> >> Utilities that invoke qemu-img info want it to be fast. Reading all >> metadata from a large image can take several seconds. Since many >> qemu-img info users don't need to see the fragmentation information, >> it makes sense to put it in a new sub-command. > > Yes. If we wanted to merge it into an existing qemu-img subcommand, I > think check would be the one, as it scans the whole image already today > and fragmentation is something that could be added fairly easily. In that case we might not even need a separate interface/struct. This would just be part of check. Does that sound good? Stefan
Am 12.03.2012 14:26, schrieb Stefan Hajnoczi: > On Mon, Mar 12, 2012 at 1:14 PM, Kevin Wolf <kwolf@redhat.com> wrote: >> Am 12.03.2012 14:07, schrieb Stefan Hajnoczi: >>> On Wed, Mar 7, 2012 at 9:22 AM, Dong Xu Wang <wdongxu@linux.vnet.ibm.com> wrote: >>>> @@ -1126,6 +1127,14 @@ static int img_info(int argc, char **argv) >>>> printf("cluster_size: %d\n", bdi.cluster_size); >>>> } >>>> } >>>> + if (bdrv_get_fragment(bs, &bfi) >= 0) { >>> >>> I think we need a separate sub-command for fragmentation info: >>> >>> qemu-img fraginfo <image-file> >>> >>> Utilities that invoke qemu-img info want it to be fast. Reading all >>> metadata from a large image can take several seconds. Since many >>> qemu-img info users don't need to see the fragmentation information, >>> it makes sense to put it in a new sub-command. >> >> Yes. If we wanted to merge it into an existing qemu-img subcommand, I >> think check would be the one, as it scans the whole image already today >> and fragmentation is something that could be added fairly easily. > > In that case we might not even need a separate interface/struct. This > would just be part of check. > > Does that sound good? Sure, that would be the only way to take advantage of the scan that bdrv_check already performs. Kevin
diff --git a/block.c b/block.c index 52ffe14..947607b 100644 --- a/block.c +++ b/block.c @@ -2588,6 +2588,19 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) return drv->bdrv_get_info(bs, bdi); } +int bdrv_get_fragment(BlockDriverState *bs, BlockFragInfo *bfi) +{ + BlockDriver *drv = bs->drv; + if (!drv) { + return -ENOMEDIUM; + } + if (!drv->bdrv_get_fragment) { + return -ENOTSUP; + } + memset(bfi, 0, sizeof(*bfi)); + return drv->bdrv_get_fragment(bs, bfi); +} + int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, int64_t pos, int size) { diff --git a/block.h b/block.h index 48d0bf3..d76d386 100644 --- a/block.h +++ b/block.h @@ -17,6 +17,12 @@ typedef struct BlockDriverInfo { int64_t vm_state_offset; } BlockDriverInfo; +typedef struct BlockFragInfo { + uint64_t allocated_clusters; + uint64_t total_clusters; + uint64_t fragmented_clusters; +} BlockFragInfo; + typedef struct QEMUSnapshotInfo { char id_str[128]; /* unique snapshot id */ /* the following fields are informative. They are not needed for @@ -290,6 +296,7 @@ const char *bdrv_get_device_name(BlockDriverState *bs); int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, const uint8_t *buf, int nb_sectors); int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi); +int bdrv_get_fragment(BlockDriverState *bs, BlockFragInfo *bfi); const char *bdrv_get_encrypted_filename(BlockDriverState *bs); void bdrv_get_backing_filename(BlockDriverState *bs, diff --git a/block_int.h b/block_int.h index b460c36..339a5ac 100644 --- a/block_int.h +++ b/block_int.h @@ -179,6 +179,7 @@ struct BlockDriver { int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs, const char *snapshot_name); int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi); + int (*bdrv_get_fragment)(BlockDriverState *bs, BlockFragInfo *bdi); int (*bdrv_save_vmstate)(BlockDriverState *bs, const uint8_t *buf, int64_t pos, int size); diff --git a/qemu-img.c b/qemu-img.c index 8df3564..17731a9 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1075,6 +1075,7 @@ static int img_info(int argc, char **argv) char backing_filename[1024]; char backing_filename2[1024]; BlockDriverInfo bdi; + BlockFragInfo bfi; fmt = NULL; for(;;) { @@ -1126,6 +1127,14 @@ static int img_info(int argc, char **argv) printf("cluster_size: %d\n", bdi.cluster_size); } } + if (bdrv_get_fragment(bs, &bfi) >= 0) { + if (bfi.total_clusters != 0 && bfi.allocated_clusters != 0) { + printf("%lld/%lld = %0.2f%% allocated, %0.2f%% fragmented\n", + bfi.allocated_clusters, bfi.total_clusters, + bfi.allocated_clusters * 100.0 / bfi.total_clusters, + bfi.fragmented_clusters * 100.0 / bfi.allocated_clusters); + } + } bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_filename)); if (backing_filename[0] != '\0') { path_combine(backing_filename2, sizeof(backing_filename2),