Re: [Qemu-block] [PATCH v2 6/9] block: Use common req handling for discard

[Kevin Wolf]

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 <address@hidden>
> > > ---
> > >  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