Am 06.07.2018 um 08:51 hat Fam Zheng geschrieben: > On Thu, 07/05 14:44, Kevin Wolf wrote: > > Am 05.07.2018 um 09:36 hat Fam Zheng geschrieben: > > > Reuse the new bdrv_co_write_req_prepare/finish helpers. The variation > > > here is that discard requests don't affect bs->wr_highest_offset. > > > > > > Signed-off-by: Fam Zheng <f...@redhat.com> > > > --- > > > block/io.c | 21 ++++++++++++++------- > > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > > > diff --git a/block/io.c b/block/io.c > > > index f06978dda0..912fcb962a 100644 > > > --- a/block/io.c > > > +++ b/block/io.c > > > @@ -1582,7 +1582,18 @@ bdrv_co_write_req_finish(BdrvChild *child, > > > BdrvTrackedRequest *req, int ret) > > > bdrv_parent_cb_resize(bs); > > > bdrv_dirty_bitmap_truncate(bs, end_sector << BDRV_SECTOR_BITS); > > > } > > > - bdrv_set_dirty(bs, req->offset, req->bytes); > > > + if (req->bytes) { > > > + switch (req->type) { > > > + case BDRV_TRACKED_WRITE: > > > + stat64_max(&bs->wr_highest_offset, req->offset + req->bytes); > > > > You forgot to remove this line above, so now this one is redundant and > > we still execute it always. > > > > Apart from that, why shouldn't discard be included in > > bs->wr_highest_offset? It's an access to an area in the image that must > > be present, so it indicates a larger file size, doesn't it? > > I'm not sure. wr_highest_offset is used for management to allocate disk > space. A > discard request is on the contrary for releasing disk space. Since guest is > allowed to discard unallocated sectors even though it should be nop in the > backend, such an operation shouldn't cause a user visible change in > @wr_highest_offset in QMP.
It's a tough call. I think wr_highest_offset is more about the file size than about allocation status. Though as long as discard can't grow the image if you discard beyond EOF, not increasing wr_highest_offset is indeed more consistent in a way. Kevin