| Message ID | 1397771992-31126-3-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 17.04.2014 um 23:59 hat Max Reitz geschrieben: > Implement bdrv_make_empty() by making all clusters in the image fall > through to the backing file (via the now modified discard). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 1e7b7d5..4d70665 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2006,6 +2006,27 @@ fail: > return ret; > } > > +static int qcow2_make_empty(BlockDriverState *bs) > +{ > + uint64_t start_sector; > + int ret = 0; > + > + /* The step taken here may not exceed INT_MAX when multiplied by > + * BDRV_SECTOR_SIZE. 64k is arbitrary, but works well. */ So you really mean something like this? int max_sectors_per_iteration = (INT_MAX / BDRV_SECTOR_SIZE); > + for (start_sector = 0; start_sector < bs->total_sectors; > + start_sector += 65536) > + { > + ret = qcow2_discard_clusters(bs, start_sector * BDRV_SECTOR_SIZE, > + MIN(65536, bs->total_sectors - start_sector), > + QCOW2_DISCARD_REQUEST, true); QCOW2_DISCARD_REQUEST is for requests that come from the guest. SNAPSHOT would be a nice fit if we reinterpreted it so that it doesn't only refer to internal snapshots but also to external ones. I would be okay with OTHER as well if you prefer. Perhaps a look at the defaults can help us: SNAPSHOT defaults to on, OTHER to off. I think it totally makes sense to physically discard clusters in qcow2_make_empty() in almost every case, so that might be a good reason to use SNAPSHOT here. > + if (ret < 0) { > + break; > + } > + } > + > + return ret; > +} > + Kevin
On 22.04.2014 16:23, Kevin Wolf wrote: > Am 17.04.2014 um 23:59 hat Max Reitz geschrieben: >> Implement bdrv_make_empty() by making all clusters in the image fall >> through to the backing file (via the now modified discard). >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block/qcow2.c | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 1e7b7d5..4d70665 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -2006,6 +2006,27 @@ fail: >> return ret; >> } >> >> +static int qcow2_make_empty(BlockDriverState *bs) >> +{ >> + uint64_t start_sector; >> + int ret = 0; >> + >> + /* The step taken here may not exceed INT_MAX when multiplied by >> + * BDRV_SECTOR_SIZE. 64k is arbitrary, but works well. */ > So you really mean something like this? > > int max_sectors_per_iteration = (INT_MAX / BDRV_SECTOR_SIZE); I like 64k. ;-) Yes, you're right, using an arbitrary value is probably not for the best. >> + for (start_sector = 0; start_sector < bs->total_sectors; >> + start_sector += 65536) >> + { >> + ret = qcow2_discard_clusters(bs, start_sector * BDRV_SECTOR_SIZE, >> + MIN(65536, bs->total_sectors - start_sector), >> + QCOW2_DISCARD_REQUEST, true); > QCOW2_DISCARD_REQUEST is for requests that come from the guest. SNAPSHOT > would be a nice fit if we reinterpreted it so that it doesn't only refer > to internal snapshots but also to external ones. I would be okay with > OTHER as well if you prefer. > > Perhaps a look at the defaults can help us: SNAPSHOT defaults to on, > OTHER to off. I think it totally makes sense to physically discard > clusters in qcow2_make_empty() in almost every case, so that might be a > good reason to use SNAPSHOT here. My problem with using SNAPSHOT would be that this function is that there is no indication of this function being meant for committing snapshots only; however, in practice it is obviously only used for that purpose, thus I can accept it. But I do like reasoning with the defaults. ;-) I'll go for SNAPSHOT then and include an explanatory comment. Max >> + if (ret < 0) { >> + break; >> + } >> + } >> + >> + return ret; >> +} >> + > Kevin
Am 22.04.2014 um 18:02 hat Max Reitz geschrieben: > On 22.04.2014 16:23, Kevin Wolf wrote: > >QCOW2_DISCARD_REQUEST is for requests that come from the guest. SNAPSHOT > >would be a nice fit if we reinterpreted it so that it doesn't only refer > >to internal snapshots but also to external ones. I would be okay with > >OTHER as well if you prefer. > > > >Perhaps a look at the defaults can help us: SNAPSHOT defaults to on, > >OTHER to off. I think it totally makes sense to physically discard > >clusters in qcow2_make_empty() in almost every case, so that might be a > >good reason to use SNAPSHOT here. > > My problem with using SNAPSHOT would be that this function is that > there is no indication of this function being meant for committing > snapshots only; however, in practice it is obviously only used for > that purpose, thus I can accept it. It all depends on what you call a snapshot. I think an image with a backing file constitutes a snapshot pretty much by definition. But I know that some people differentiate snapshots and templates, or things like that. > But I do like reasoning with the defaults. ;-) :-) > I'll go for SNAPSHOT then and include an explanatory comment. Great, thanks. Kevin
diff --git a/block/qcow2.c b/block/qcow2.c index 1e7b7d5..4d70665 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2006,6 +2006,27 @@ fail: return ret; } +static int qcow2_make_empty(BlockDriverState *bs) +{ + uint64_t start_sector; + int ret = 0; + + /* The step taken here may not exceed INT_MAX when multiplied by + * BDRV_SECTOR_SIZE. 64k is arbitrary, but works well. */ + for (start_sector = 0; start_sector < bs->total_sectors; + start_sector += 65536) + { + ret = qcow2_discard_clusters(bs, start_sector * BDRV_SECTOR_SIZE, + MIN(65536, bs->total_sectors - start_sector), + QCOW2_DISCARD_REQUEST, true); + if (ret < 0) { + break; + } + } + + return ret; +} + static coroutine_fn int qcow2_co_flush_to_os(BlockDriverState *bs) { BDRVQcowState *s = bs->opaque; @@ -2388,6 +2409,7 @@ static BlockDriver bdrv_qcow2 = { .bdrv_co_discard = qcow2_co_discard, .bdrv_truncate = qcow2_truncate, .bdrv_write_compressed = qcow2_write_compressed, + .bdrv_make_empty = qcow2_make_empty, .bdrv_snapshot_create = qcow2_snapshot_create, .bdrv_snapshot_goto = qcow2_snapshot_goto,
Implement bdrv_make_empty() by making all clusters in the image fall through to the backing file (via the now modified discard). Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/qcow2.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)