| Message ID | 20180705073701.10558-7-famz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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;
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(-)