| Message ID | 1465917916-22348-7-git-send-email-den@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/14/2016 09:25 AM, Denis V. Lunev wrote: > Signed-off-by: Denis V. Lunev <den@openvz.org> The commit message says what, but not why. It's useful to give reviewers a hint as to why a patch makes sense (such as a future patch being able to use the write notifier to make mirroring more efficient if it knows what is being mirrored). > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > CC: Stefan Hajnoczi <stefanha@redhat.com> > CC: Fam Zheng <famz@redhat.com> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Max Reitz <mreitz@redhat.com> > CC: Jeff Cody <jcody@redhat.com> > CC: Eric Blake <eblake@redhat.com> > --- > block/io.c | 12 +++++++----- > include/block/block_int.h | 1 + > 2 files changed, 8 insertions(+), 5 deletions(-) > > @@ -2228,7 +2230,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, > return 0; > } > > - tracked_request_begin(&req, bs, sector_num, nb_sectors, > + tracked_request_begin(&req, bs, NULL, sector_num, nb_sectors, > BDRV_TRACKED_DISCARD); > bdrv_set_dirty(bs, sector_num, nb_sectors); > > @@ -2331,7 +2333,7 @@ static int bdrv_co_do_ioctl(BlockDriverState *bs, int req, void *buf) > }; > BlockAIOCB *acb; > > - tracked_request_begin(&tracked_req, bs, 0, 0, BDRV_TRACKED_IOCTL); > + tracked_request_begin(&tracked_req, bs, NULL, 0, 0, BDRV_TRACKED_IOCTL); > if (!drv || !drv->bdrv_aio_ioctl) { > co.ret = -ENOTSUP; > goto out; > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 30a9717..72f463a 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -69,6 +69,7 @@ enum BdrvTrackedRequestType { > > typedef struct BdrvTrackedRequest { > BlockDriverState *bs; > + QEMUIOVector *qiov; > int64_t offset; > unsigned int bytes; I guess bytes and qiov->size are not always redundant, because you pass NULL for qiov for a zero or discard operation (an alternative would be to always pass a qiov, even for zero/discard, so that we only need a single size). But I've been pointing out our inconsistent use of qiov for zeroes in multiple places, so I don't think it's worth changing in this series, but in one of its own if we want to do it.
On Tue, Jun 14, 2016 at 06:25:13PM +0300, Denis V. Lunev wrote: > Signed-off-by: Denis V. Lunev <den@openvz.org> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > CC: Stefan Hajnoczi <stefanha@redhat.com> > CC: Fam Zheng <famz@redhat.com> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Max Reitz <mreitz@redhat.com> > CC: Jeff Cody <jcody@redhat.com> > CC: Eric Blake <eblake@redhat.com> > --- > block/io.c | 12 +++++++----- > include/block/block_int.h | 1 + > 2 files changed, 8 insertions(+), 5 deletions(-) This patch is missing a commit description. Why is this necessary? If you add .qiov then can we remove the .bytes field?
On Tue, Jun 14, 2016 at 06:25:13PM +0300, Denis V. Lunev wrote: > Signed-off-by: Denis V. Lunev <den@openvz.org> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > CC: Stefan Hajnoczi <stefanha@redhat.com> > CC: Fam Zheng <famz@redhat.com> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Max Reitz <mreitz@redhat.com> > CC: Jeff Cody <jcody@redhat.com> > CC: Eric Blake <eblake@redhat.com> > --- > block/io.c | 12 +++++++----- > include/block/block_int.h | 1 + > 2 files changed, 8 insertions(+), 5 deletions(-) I read Eric's reply and realized the duplication with my earlier reply. Feel free to ignore mine and just respond to Eric.
On 06/15/2016 12:21 PM, Stefan Hajnoczi wrote: > On Tue, Jun 14, 2016 at 06:25:13PM +0300, Denis V. Lunev wrote: >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> CC: Stefan Hajnoczi <stefanha@redhat.com> >> CC: Fam Zheng <famz@redhat.com> >> CC: Kevin Wolf <kwolf@redhat.com> >> CC: Max Reitz <mreitz@redhat.com> >> CC: Jeff Cody <jcody@redhat.com> >> CC: Eric Blake <eblake@redhat.com> >> --- >> block/io.c | 12 +++++++----- >> include/block/block_int.h | 1 + >> 2 files changed, 8 insertions(+), 5 deletions(-) > This patch is missing a commit description. Why is this necessary? > > If you add .qiov then can we remove the .bytes field? this would not be convenient unfortunately. here I have pointer to qiov and this pointer is not always available. Actually it is available only to the write operation. The purpose is to make data being written available for the notifier. Den
diff --git a/block/io.c b/block/io.c index 2d832aa..d2ad09c 100644 --- a/block/io.c +++ b/block/io.c @@ -368,12 +368,14 @@ static void tracked_request_end(BdrvTrackedRequest *req) */ static void tracked_request_begin(BdrvTrackedRequest *req, BlockDriverState *bs, + QEMUIOVector *qiov, int64_t offset, unsigned int bytes, enum BdrvTrackedRequestType type) { *req = (BdrvTrackedRequest){ .bs = bs, + .qiov = qiov, .offset = offset, .bytes = bytes, .type = type, @@ -1073,7 +1075,7 @@ int coroutine_fn bdrv_co_preadv(BlockDriverState *bs, bytes = ROUND_UP(bytes, align); } - tracked_request_begin(&req, bs, offset, bytes, BDRV_TRACKED_READ); + tracked_request_begin(&req, bs, NULL, offset, bytes, BDRV_TRACKED_READ); ret = bdrv_aligned_preadv(bs, &req, offset, bytes, align, use_local_qiov ? &local_qiov : qiov, flags); @@ -1391,7 +1393,7 @@ int coroutine_fn bdrv_co_pwritev(BlockDriverState *bs, * Pad qiov with the read parts and be sure to have a tracked request not * only for bdrv_aligned_pwritev, but also for the reads of the RMW cycle. */ - tracked_request_begin(&req, bs, offset, bytes, BDRV_TRACKED_WRITE); + tracked_request_begin(&req, bs, qiov, offset, bytes, BDRV_TRACKED_WRITE); if (!qiov) { ret = bdrv_co_do_zero_pwritev(bs, offset, bytes, flags, &req); @@ -2098,7 +2100,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs) return 0; } - tracked_request_begin(&req, bs, 0, 0, BDRV_TRACKED_FLUSH); + tracked_request_begin(&req, bs, NULL, 0, 0, BDRV_TRACKED_FLUSH); /* Write back all layers by calling one driver function */ if (bs->drv->bdrv_co_flush) { @@ -2228,7 +2230,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, return 0; } - tracked_request_begin(&req, bs, sector_num, nb_sectors, + tracked_request_begin(&req, bs, NULL, sector_num, nb_sectors, BDRV_TRACKED_DISCARD); bdrv_set_dirty(bs, sector_num, nb_sectors); @@ -2331,7 +2333,7 @@ static int bdrv_co_do_ioctl(BlockDriverState *bs, int req, void *buf) }; BlockAIOCB *acb; - tracked_request_begin(&tracked_req, bs, 0, 0, BDRV_TRACKED_IOCTL); + tracked_request_begin(&tracked_req, bs, NULL, 0, 0, BDRV_TRACKED_IOCTL); if (!drv || !drv->bdrv_aio_ioctl) { co.ret = -ENOTSUP; goto out; diff --git a/include/block/block_int.h b/include/block/block_int.h index 30a9717..72f463a 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -69,6 +69,7 @@ enum BdrvTrackedRequestType { typedef struct BdrvTrackedRequest { BlockDriverState *bs; + QEMUIOVector *qiov; int64_t offset; unsigned int bytes; enum BdrvTrackedRequestType type;