[v2,6/9] block: Use common req handling for discard
diff mbox series

Message ID 20180705073701.10558-7-famz@redhat.com
State New
Headers show
Series
  • block: Fix dst reading after tail copy offloading
Related show

Commit Message

Fam Zheng July 5, 2018, 7:36 a.m. UTC
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(-)

Comments

Kevin Wolf July 5, 2018, 12:44 p.m. UTC | #1
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?

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

Kevin
Fam Zheng July 6, 2018, 6:51 a.m. UTC | #2
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
Kevin Wolf July 6, 2018, 8:54 a.m. UTC | #3
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 <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.

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

Patch
diff mbox series

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);
+            /* fall through, to set dirty bits */
+        case BDRV_TRACKED_DISCARD:
+            bdrv_set_dirty(bs, req->offset, req->bytes);
+            break;
+        default:
+            break;
+        }
+    }
 }
 
 /*
@@ -2643,10 +2654,7 @@  int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes)
     ret = bdrv_check_byte_request(bs, offset, bytes);
     if (ret < 0) {
         return ret;
-    } else if (bs->read_only) {
-        return -EPERM;
     }
-    assert(!(bs->open_flags & BDRV_O_INACTIVE));
 
     /* Do nothing if disabled.  */
     if (!(bs->open_flags & BDRV_O_UNMAP)) {
@@ -2670,7 +2678,7 @@  int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes)
     bdrv_inc_in_flight(bs);
     tracked_request_begin(&req, bs, offset, bytes, BDRV_TRACKED_DISCARD);
 
-    ret = notifier_with_return_list_notify(&bs->before_write_notifiers, &req);
+    ret = bdrv_co_write_req_prepare(child, &req, 0);
     if (ret < 0) {
         goto out;
     }
@@ -2736,8 +2744,7 @@  int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int bytes)
     }
     ret = 0;
 out:
-    atomic_inc(&bs->write_gen);
-    bdrv_set_dirty(bs, req.offset, req.bytes);
+    bdrv_co_write_req_finish(child, &req, ret);
     tracked_request_end(&req);
     bdrv_dec_in_flight(bs);
     return ret;