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

[Fam Zheng <famz@redhat.com>]

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 <famz@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.

Fam

> 
> > +            /* fall through, to set dirty bits */
> > +        case BDRV_TRACKED_DISCARD:
> > +            bdrv_set_dirty(bs, req->offset, req->bytes);
> > +            break;
> > +        default:
> > +            break;
> > +        }
> > +    }
> >  }
> 
> Kevin