| Message ID | 1464815413-613-10-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 01.06.2016 um 23:10 hat Eric Blake geschrieben: > Another step on our continuing quest to switch to byte-based > interfaces. > > Kill an abuse of the comma operator while at it (fortunately, > the semantics were still right). Also, the test for requests > not aligned to clusters should be applied always, not just > when a backing file is present. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > block/qed.c | 33 +++++++++++++++------------------ > 1 file changed, 15 insertions(+), 18 deletions(-) > > diff --git a/block/qed.c b/block/qed.c > index 0ab5b40..45ec13d 100644 > --- a/block/qed.c > +++ b/block/qed.c > @@ -1419,7 +1419,7 @@ typedef struct { > bool done; > } QEDWriteZeroesCB; > > -static void coroutine_fn qed_co_write_zeroes_cb(void *opaque, int ret) > +static void coroutine_fn qed_co_pwrite_zeroes_cb(void *opaque, int ret) > { > QEDWriteZeroesCB *cb = opaque; > > @@ -1430,10 +1430,10 @@ static void coroutine_fn qed_co_write_zeroes_cb(void *opaque, int ret) > } > } > > -static int coroutine_fn bdrv_qed_co_write_zeroes(BlockDriverState *bs, > - int64_t sector_num, > - int nb_sectors, > - BdrvRequestFlags flags) > +static int coroutine_fn bdrv_qed_co_pwrite_zeroes(BlockDriverState *bs, > + int64_t offset, > + int count, > + BdrvRequestFlags flags) > { > BlockAIOCB *blockacb; > BDRVQEDState *s = bs->opaque; > @@ -1441,25 +1441,22 @@ static int coroutine_fn bdrv_qed_co_write_zeroes(BlockDriverState *bs, > QEMUIOVector qiov; > struct iovec iov; > > - /* Refuse if there are untouched backing file sectors */ > - if (bs->backing) { > - if (qed_offset_into_cluster(s, sector_num * BDRV_SECTOR_SIZE) != 0) { > - return -ENOTSUP; > - } > - if (qed_offset_into_cluster(s, nb_sectors * BDRV_SECTOR_SIZE) != 0) { > - return -ENOTSUP; > - } > + /* Fall back if the request is not */ ...aligned? > + if (qed_offset_into_cluster(s, offset) || > + qed_offset_into_cluster(s, count)) { > + return -ENOTSUP; > } This is obviously correct and almost as obviously suboptimal compared to the original version (we need cluster alignment with a backing file, but without a backing file, sector alignment would be enough). But as this is QED, which is only supported for compatibility these days, simpler if slightly suboptimal code is okay with me. Kevin
On 06/02/2016 05:16 AM, Kevin Wolf wrote: > Am 01.06.2016 um 23:10 hat Eric Blake geschrieben: >> Another step on our continuing quest to switch to byte-based >> interfaces. >> >> Kill an abuse of the comma operator while at it (fortunately, >> the semantics were still right). Also, the test for requests >> not aligned to clusters should be applied always, not just >> when a backing file is present. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> block/qed.c | 33 +++++++++++++++------------------ >> 1 file changed, 15 insertions(+), 18 deletions(-) >> - } >> + /* Fall back if the request is not */ > > ...aligned? Yes, thanks. > >> + if (qed_offset_into_cluster(s, offset) || >> + qed_offset_into_cluster(s, count)) { >> + return -ENOTSUP; >> } > > This is obviously correct and almost as obviously suboptimal compared to > the original version (we need cluster alignment with a backing file, but > without a backing file, sector alignment would be enough). Does QED support per-sector zeroing, or is it only per-cluster zero like qcow2? /me checks the spec only per-cluster zeroing > > But as this is QED, which is only supported for compatibility these > days, simpler if slightly suboptimal code is okay with me. Widening a request to cluster boundaries is only possible if you know the cluster is otherwise unallocated or already reads as zeroes, and unless qed tracks zero sectors (as opposed to zero clusters), I argue that this was actually a bug fix, even when the request was sector-aligned, since we lack the check for cluster remains unallocated/zero before widening, the way qcow2 does it. Sub-optimal but safe is better than incorrectly optimized.
Am 02.06.2016 um 14:40 hat Eric Blake geschrieben: > On 06/02/2016 05:16 AM, Kevin Wolf wrote: > > Am 01.06.2016 um 23:10 hat Eric Blake geschrieben: > >> Another step on our continuing quest to switch to byte-based > >> interfaces. > >> > >> Kill an abuse of the comma operator while at it (fortunately, > >> the semantics were still right). Also, the test for requests > >> not aligned to clusters should be applied always, not just > >> when a backing file is present. > >> > >> Signed-off-by: Eric Blake <eblake@redhat.com> > >> --- > >> block/qed.c | 33 +++++++++++++++------------------ > >> 1 file changed, 15 insertions(+), 18 deletions(-) > > >> - } > >> + /* Fall back if the request is not */ > > > > ...aligned? > > Yes, thanks. > > > > >> + if (qed_offset_into_cluster(s, offset) || > >> + qed_offset_into_cluster(s, count)) { > >> + return -ENOTSUP; > >> } > > > > This is obviously correct and almost as obviously suboptimal compared to > > the original version (we need cluster alignment with a backing file, but > > without a backing file, sector alignment would be enough). > > Does QED support per-sector zeroing, or is it only per-cluster zero like > qcow2? > > /me checks the spec > > only per-cluster zeroing > > > > > But as this is QED, which is only supported for compatibility these > > days, simpler if slightly suboptimal code is okay with me. > > Widening a request to cluster boundaries is only possible if you know > the cluster is otherwise unallocated or already reads as zeroes, and > unless qed tracks zero sectors (as opposed to zero clusters), I argue > that this was actually a bug fix, even when the request was > sector-aligned, since we lack the check for cluster remains > unallocated/zero before widening, the way qcow2 does it. Sub-optimal > but safe is better than incorrectly optimized. It actually checks whether the cluster is already allocated; in that case, qed implements a fallback itself. So I think it was working. Just as usual with the callback based qed code, it's not very easy to read. Kevin
diff --git a/block/qed.c b/block/qed.c index 0ab5b40..45ec13d 100644 --- a/block/qed.c +++ b/block/qed.c @@ -1419,7 +1419,7 @@ typedef struct { bool done; } QEDWriteZeroesCB; -static void coroutine_fn qed_co_write_zeroes_cb(void *opaque, int ret) +static void coroutine_fn qed_co_pwrite_zeroes_cb(void *opaque, int ret) { QEDWriteZeroesCB *cb = opaque; @@ -1430,10 +1430,10 @@ static void coroutine_fn qed_co_write_zeroes_cb(void *opaque, int ret) } } -static int coroutine_fn bdrv_qed_co_write_zeroes(BlockDriverState *bs, - int64_t sector_num, - int nb_sectors, - BdrvRequestFlags flags) +static int coroutine_fn bdrv_qed_co_pwrite_zeroes(BlockDriverState *bs, + int64_t offset, + int count, + BdrvRequestFlags flags) { BlockAIOCB *blockacb; BDRVQEDState *s = bs->opaque; @@ -1441,25 +1441,22 @@ static int coroutine_fn bdrv_qed_co_write_zeroes(BlockDriverState *bs, QEMUIOVector qiov; struct iovec iov; - /* Refuse if there are untouched backing file sectors */ - if (bs->backing) { - if (qed_offset_into_cluster(s, sector_num * BDRV_SECTOR_SIZE) != 0) { - return -ENOTSUP; - } - if (qed_offset_into_cluster(s, nb_sectors * BDRV_SECTOR_SIZE) != 0) { - return -ENOTSUP; - } + /* Fall back if the request is not */ + if (qed_offset_into_cluster(s, offset) || + qed_offset_into_cluster(s, count)) { + return -ENOTSUP; } /* Zero writes start without an I/O buffer. If a buffer becomes necessary * then it will be allocated during request processing. */ - iov.iov_base = NULL, - iov.iov_len = nb_sectors * BDRV_SECTOR_SIZE, + iov.iov_base = NULL; + iov.iov_len = count; qemu_iovec_init_external(&qiov, &iov, 1); - blockacb = qed_aio_setup(bs, sector_num, &qiov, nb_sectors, - qed_co_write_zeroes_cb, &cb, + blockacb = qed_aio_setup(bs, offset >> BDRV_SECTOR_BITS, &qiov, + count >> BDRV_SECTOR_BITS, + qed_co_pwrite_zeroes_cb, &cb, QED_AIOCB_WRITE | QED_AIOCB_ZERO); if (!blockacb) { return -EIO; @@ -1664,7 +1661,7 @@ static BlockDriver bdrv_qed = { .bdrv_co_get_block_status = bdrv_qed_co_get_block_status, .bdrv_aio_readv = bdrv_qed_aio_readv, .bdrv_aio_writev = bdrv_qed_aio_writev, - .bdrv_co_write_zeroes = bdrv_qed_co_write_zeroes, + .bdrv_co_pwrite_zeroes = bdrv_qed_co_pwrite_zeroes, .bdrv_truncate = bdrv_qed_truncate, .bdrv_getlength = bdrv_qed_getlength, .bdrv_get_info = bdrv_qed_get_info,
Another step on our continuing quest to switch to byte-based interfaces. Kill an abuse of the comma operator while at it (fortunately, the semantics were still right). Also, the test for requests not aligned to clusters should be applied always, not just when a backing file is present. Signed-off-by: Eric Blake <eblake@redhat.com> --- block/qed.c | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-)