* [Qemu-devel] [PATCH v2 0/5] Remove bdrv_read() and bdrv_write()
@ 2019-05-01 18:13 ` Alberto Garcia
0 siblings, 0 replies; 22+ messages in thread
From: Alberto Garcia @ 2019-05-01 18:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Max Reitz, Kevin Wolf, Eric Blake
Hi,
this API only had a few users left so it can be easily removed.
Regards,
Berto
v2:
- Patch 2: Add QEMU_BUILD_BUG_ON(sizeof(VdiHeader) != 512) [Kevin]
Correct size parameter in bdrv_pwrite() call [Kevin]
Return 0 on success, never a positive number [Kevin]
- Patch 3: Check that the return values of bdrv_pread()/bdrv_pwrite()
are strictly < 0 for errors, not simply != 0 [Kevin]
- Patch 4: Reuse the documenation of bdrv_write() for bdrv_pwrite()
v1: https://lists.gnu.org/archive/html/qemu-block/2019-04/msg00832.html
Output of git backport-diff against v1:
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
001/5:[----] [--] 'qcow2: Replace bdrv_write() with bdrv_pwrite()'
002/5:[0006] [FC] 'vdi: Replace bdrv_{read,write}() with bdrv_{pread,pwrite}()'
003/5:[0004] [FC] 'vvfat: Replace bdrv_{read,write}() with bdrv_{pread,pwrite}()'
004/5:[0007] [FC] 'block: Remove bdrv_read() and bdrv_write()'
005/5:[----] [--] 'qcow2: Remove BDRVQcow2State.cluster_sectors'
Alberto Garcia (5):
qcow2: Replace bdrv_write() with bdrv_pwrite()
vdi: Replace bdrv_{read,write}() with bdrv_{pread,pwrite}()
vvfat: Replace bdrv_{read,write}() with bdrv_{pread,pwrite}()
block: Remove bdrv_read() and bdrv_write()
qcow2: Remove BDRVQcow2State.cluster_sectors
block/io.c | 43 +++++++------------------------------------
block/qcow2-refcount.c | 4 ++--
block/qcow2.c | 1 -
block/qcow2.h | 1 -
block/vdi.c | 15 +++++++++------
block/vvfat.c | 12 +++++++-----
include/block/block.h | 4 ----
7 files changed, 25 insertions(+), 55 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v2 0/5] Remove bdrv_read() and bdrv_write()
@ 2019-05-01 18:13 ` Alberto Garcia
0 siblings, 0 replies; 22+ messages in thread
From: Alberto Garcia @ 2019-05-01 18:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, qemu-block, Max Reitz
Hi,
this API only had a few users left so it can be easily removed.
Regards,
Berto
v2:
- Patch 2: Add QEMU_BUILD_BUG_ON(sizeof(VdiHeader) != 512) [Kevin]
Correct size parameter in bdrv_pwrite() call [Kevin]
Return 0 on success, never a positive number [Kevin]
- Patch 3: Check that the return values of bdrv_pread()/bdrv_pwrite()
are strictly < 0 for errors, not simply != 0 [Kevin]
- Patch 4: Reuse the documenation of bdrv_write() for bdrv_pwrite()
v1: https://lists.gnu.org/archive/html/qemu-block/2019-04/msg00832.html
Output of git backport-diff against v1:
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
001/5:[----] [--] 'qcow2: Replace bdrv_write() with bdrv_pwrite()'
002/5:[0006] [FC] 'vdi: Replace bdrv_{read,write}() with bdrv_{pread,pwrite}()'
003/5:[0004] [FC] 'vvfat: Replace bdrv_{read,write}() with bdrv_{pread,pwrite}()'
004/5:[0007] [FC] 'block: Remove bdrv_read() and bdrv_write()'
005/5:[----] [--] 'qcow2: Remove BDRVQcow2State.cluster_sectors'
Alberto Garcia (5):
qcow2: Replace bdrv_write() with bdrv_pwrite()
vdi: Replace bdrv_{read,write}() with bdrv_{pread,pwrite}()
vvfat: Replace bdrv_{read,write}() with bdrv_{pread,pwrite}()
block: Remove bdrv_read() and bdrv_write()
qcow2: Remove BDRVQcow2State.cluster_sectors
block/io.c | 43 +++++++------------------------------------
block/qcow2-refcount.c | 4 ++--
block/qcow2.c | 1 -
block/qcow2.h | 1 -
block/vdi.c | 15 +++++++++------
block/vvfat.c | 12 +++++++-----
include/block/block.h | 4 ----
7 files changed, 25 insertions(+), 55 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v2 1/5] qcow2: Replace bdrv_write() with bdrv_pwrite()
@ 2019-05-01 18:13 ` Alberto Garcia
0 siblings, 0 replies; 22+ messages in thread
From: Alberto Garcia @ 2019-05-01 18:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Max Reitz, Kevin Wolf, Eric Blake
There's only one bdrv_write() call left in the qcow2 code, and it can
be trivially replaced with the byte-based bdrv_pwrite().
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/qcow2-refcount.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index e0fe322500..83f66eed7a 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2409,8 +2409,8 @@ write_refblocks:
on_disk_refblock = (void *)((char *) *refcount_table +
refblock_index * s->cluster_size);
- ret = bdrv_write(bs->file, refblock_offset / BDRV_SECTOR_SIZE,
- on_disk_refblock, s->cluster_sectors);
+ ret = bdrv_pwrite(bs->file, refblock_offset, on_disk_refblock,
+ s->cluster_size);
if (ret < 0) {
fprintf(stderr, "ERROR writing refblock: %s\n", strerror(-ret));
goto fail;
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v2 1/5] qcow2: Replace bdrv_write() with bdrv_pwrite()
@ 2019-05-01 18:13 ` Alberto Garcia
0 siblings, 0 replies; 22+ messages in thread
From: Alberto Garcia @ 2019-05-01 18:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, qemu-block, Max Reitz
There's only one bdrv_write() call left in the qcow2 code, and it can
be trivially replaced with the byte-based bdrv_pwrite().
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/qcow2-refcount.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index e0fe322500..83f66eed7a 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2409,8 +2409,8 @@ write_refblocks:
on_disk_refblock = (void *)((char *) *refcount_table +
refblock_index * s->cluster_size);
- ret = bdrv_write(bs->file, refblock_offset / BDRV_SECTOR_SIZE,
- on_disk_refblock, s->cluster_sectors);
+ ret = bdrv_pwrite(bs->file, refblock_offset, on_disk_refblock,
+ s->cluster_size);
if (ret < 0) {
fprintf(stderr, "ERROR writing refblock: %s\n", strerror(-ret));
goto fail;
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v2 2/5] vdi: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()
@ 2019-05-01 18:13 ` Alberto Garcia
0 siblings, 0 replies; 22+ messages in thread
From: Alberto Garcia @ 2019-05-01 18:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Max Reitz, Kevin Wolf, Eric Blake
There's only a couple of bdrv_read() and bdrv_write() calls left in
the vdi code, and they can be trivially replaced with the byte-based
bdrv_pread() and bdrv_pwrite().
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/vdi.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index e1c42ad732..9caeb50dd1 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -171,6 +171,8 @@ typedef struct {
uint64_t unused2[7];
} QEMU_PACKED VdiHeader;
+QEMU_BUILD_BUG_ON(sizeof(VdiHeader) != 512);
+
typedef struct {
/* The block map entries are little endian (even in memory). */
uint32_t *bmap;
@@ -384,7 +386,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
logout("\n");
- ret = bdrv_read(bs->file, 0, (uint8_t *)&header, 1);
+ ret = bdrv_pread(bs->file, 0, (uint8_t *)&header, sizeof(header));
if (ret < 0) {
goto fail;
}
@@ -484,8 +486,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
goto fail;
}
- ret = bdrv_read(bs->file, s->bmap_sector, (uint8_t *)s->bmap,
- bmap_size);
+ ret = bdrv_pread(bs->file, header.offset_bmap, (uint8_t *)s->bmap,
+ bmap_size * SECTOR_SIZE);
if (ret < 0) {
goto fail_free_bmap;
}
@@ -704,7 +706,7 @@ nonallocating_write:
assert(VDI_IS_ALLOCATED(bmap_first));
*header = s->header;
vdi_header_to_le(header);
- ret = bdrv_write(bs->file, 0, block, 1);
+ ret = bdrv_pwrite(bs->file, 0, block, sizeof(VdiHeader));
g_free(block);
block = NULL;
@@ -722,10 +724,11 @@ nonallocating_write:
base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
logout("will write %u block map sectors starting from entry %u\n",
n_sectors, bmap_first);
- ret = bdrv_write(bs->file, offset, base, n_sectors);
+ ret = bdrv_pwrite(bs->file, offset * SECTOR_SIZE, base,
+ n_sectors * SECTOR_SIZE);
}
- return ret;
+ return ret < 0 ? ret : 0;
}
static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v2 2/5] vdi: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()
@ 2019-05-01 18:13 ` Alberto Garcia
0 siblings, 0 replies; 22+ messages in thread
From: Alberto Garcia @ 2019-05-01 18:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, qemu-block, Max Reitz
There's only a couple of bdrv_read() and bdrv_write() calls left in
the vdi code, and they can be trivially replaced with the byte-based
bdrv_pread() and bdrv_pwrite().
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/vdi.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index e1c42ad732..9caeb50dd1 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -171,6 +171,8 @@ typedef struct {
uint64_t unused2[7];
} QEMU_PACKED VdiHeader;
+QEMU_BUILD_BUG_ON(sizeof(VdiHeader) != 512);
+
typedef struct {
/* The block map entries are little endian (even in memory). */
uint32_t *bmap;
@@ -384,7 +386,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
logout("\n");
- ret = bdrv_read(bs->file, 0, (uint8_t *)&header, 1);
+ ret = bdrv_pread(bs->file, 0, (uint8_t *)&header, sizeof(header));
if (ret < 0) {
goto fail;
}
@@ -484,8 +486,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
goto fail;
}
- ret = bdrv_read(bs->file, s->bmap_sector, (uint8_t *)s->bmap,
- bmap_size);
+ ret = bdrv_pread(bs->file, header.offset_bmap, (uint8_t *)s->bmap,
+ bmap_size * SECTOR_SIZE);
if (ret < 0) {
goto fail_free_bmap;
}
@@ -704,7 +706,7 @@ nonallocating_write:
assert(VDI_IS_ALLOCATED(bmap_first));
*header = s->header;
vdi_header_to_le(header);
- ret = bdrv_write(bs->file, 0, block, 1);
+ ret = bdrv_pwrite(bs->file, 0, block, sizeof(VdiHeader));
g_free(block);
block = NULL;
@@ -722,10 +724,11 @@ nonallocating_write:
base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
logout("will write %u block map sectors starting from entry %u\n",
n_sectors, bmap_first);
- ret = bdrv_write(bs->file, offset, base, n_sectors);
+ ret = bdrv_pwrite(bs->file, offset * SECTOR_SIZE, base,
+ n_sectors * SECTOR_SIZE);
}
- return ret;
+ return ret < 0 ? ret : 0;
}
static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v2 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()
@ 2019-05-01 18:13 ` Alberto Garcia
0 siblings, 0 replies; 22+ messages in thread
From: Alberto Garcia @ 2019-05-01 18:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Max Reitz, Kevin Wolf, Eric Blake
There's only a couple of bdrv_read() and bdrv_write() calls left in
the vvfat code, and they can be trivially replaced with the byte-based
bdrv_pread() and bdrv_pwrite().
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/vvfat.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index 5f66787890..253cc716dd 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1494,8 +1494,8 @@ static int vvfat_read(BlockDriverState *bs, int64_t sector_num,
DLOG(fprintf(stderr, "sectors %" PRId64 "+%" PRId64
" allocated\n", sector_num,
n >> BDRV_SECTOR_BITS));
- if (bdrv_read(s->qcow, sector_num, buf + i * 0x200,
- n >> BDRV_SECTOR_BITS)) {
+ if (bdrv_pread(s->qcow, sector_num * BDRV_SECTOR_SIZE,
+ buf + i * 0x200, n) < 0) {
return -1;
}
i += (n >> BDRV_SECTOR_BITS) - 1;
@@ -1983,8 +1983,9 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
if (res) {
return -1;
}
- res = bdrv_write(s->qcow, offset, s->cluster_buffer, 1);
- if (res) {
+ res = bdrv_pwrite(s->qcow, offset * BDRV_SECTOR_SIZE,
+ s->cluster_buffer, BDRV_SECTOR_SIZE);
+ if (res < 0) {
return -2;
}
}
@@ -3050,7 +3051,8 @@ DLOG(checkpoint());
* Use qcow backend. Commit later.
*/
DLOG(fprintf(stderr, "Write to qcow backend: %d + %d\n", (int)sector_num, nb_sectors));
- ret = bdrv_write(s->qcow, sector_num, buf, nb_sectors);
+ ret = bdrv_pwrite(s->qcow, sector_num * BDRV_SECTOR_SIZE, buf,
+ nb_sectors * BDRV_SECTOR_SIZE);
if (ret < 0) {
fprintf(stderr, "Error writing to qcow backend\n");
return ret;
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v2 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()
@ 2019-05-01 18:13 ` Alberto Garcia
0 siblings, 0 replies; 22+ messages in thread
From: Alberto Garcia @ 2019-05-01 18:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, qemu-block, Max Reitz
There's only a couple of bdrv_read() and bdrv_write() calls left in
the vvfat code, and they can be trivially replaced with the byte-based
bdrv_pread() and bdrv_pwrite().
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/vvfat.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index 5f66787890..253cc716dd 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1494,8 +1494,8 @@ static int vvfat_read(BlockDriverState *bs, int64_t sector_num,
DLOG(fprintf(stderr, "sectors %" PRId64 "+%" PRId64
" allocated\n", sector_num,
n >> BDRV_SECTOR_BITS));
- if (bdrv_read(s->qcow, sector_num, buf + i * 0x200,
- n >> BDRV_SECTOR_BITS)) {
+ if (bdrv_pread(s->qcow, sector_num * BDRV_SECTOR_SIZE,
+ buf + i * 0x200, n) < 0) {
return -1;
}
i += (n >> BDRV_SECTOR_BITS) - 1;
@@ -1983,8 +1983,9 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
if (res) {
return -1;
}
- res = bdrv_write(s->qcow, offset, s->cluster_buffer, 1);
- if (res) {
+ res = bdrv_pwrite(s->qcow, offset * BDRV_SECTOR_SIZE,
+ s->cluster_buffer, BDRV_SECTOR_SIZE);
+ if (res < 0) {
return -2;
}
}
@@ -3050,7 +3051,8 @@ DLOG(checkpoint());
* Use qcow backend. Commit later.
*/
DLOG(fprintf(stderr, "Write to qcow backend: %d + %d\n", (int)sector_num, nb_sectors));
- ret = bdrv_write(s->qcow, sector_num, buf, nb_sectors);
+ ret = bdrv_pwrite(s->qcow, sector_num * BDRV_SECTOR_SIZE, buf,
+ nb_sectors * BDRV_SECTOR_SIZE);
if (ret < 0) {
fprintf(stderr, "Error writing to qcow backend\n");
return ret;
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v2 4/5] block: Remove bdrv_read() and bdrv_write()
@ 2019-05-01 18:13 ` Alberto Garcia
0 siblings, 0 replies; 22+ messages in thread
From: Alberto Garcia @ 2019-05-01 18:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Max Reitz, Kevin Wolf, Eric Blake
No one is using these functions anymore, all callers have switched to
the byte-based bdrv_pread() and bdrv_pwrite()
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/io.c | 43 +++++++------------------------------------
include/block/block.h | 4 ----
2 files changed, 7 insertions(+), 40 deletions(-)
diff --git a/block/io.c b/block/io.c
index dfc153b8d8..396d5364ba 100644
--- a/block/io.c
+++ b/block/io.c
@@ -837,42 +837,6 @@ static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
return rwco.ret;
}
-/*
- * Process a synchronous request using coroutines
- */
-static int bdrv_rw_co(BdrvChild *child, int64_t sector_num, uint8_t *buf,
- int nb_sectors, bool is_write, BdrvRequestFlags flags)
-{
- QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf,
- nb_sectors * BDRV_SECTOR_SIZE);
-
- if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
- return -EINVAL;
- }
-
- return bdrv_prwv_co(child, sector_num << BDRV_SECTOR_BITS,
- &qiov, is_write, flags);
-}
-
-/* return < 0 if error. See bdrv_write() for the return codes */
-int bdrv_read(BdrvChild *child, int64_t sector_num,
- uint8_t *buf, int nb_sectors)
-{
- return bdrv_rw_co(child, sector_num, buf, nb_sectors, false, 0);
-}
-
-/* Return < 0 if error. Important errors are:
- -EIO generic I/O error (may happen for all errors)
- -ENOMEDIUM No media inserted.
- -EINVAL Invalid sector number or nb_sectors
- -EACCES Trying to write a read-only device
-*/
-int bdrv_write(BdrvChild *child, int64_t sector_num,
- const uint8_t *buf, int nb_sectors)
-{
- return bdrv_rw_co(child, sector_num, (uint8_t *)buf, nb_sectors, true, 0);
-}
-
int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
int bytes, BdrvRequestFlags flags)
{
@@ -935,6 +899,7 @@ int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
return qiov->size;
}
+/* See bdrv_pwrite() for the return codes */
int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes)
{
QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
@@ -958,6 +923,12 @@ int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
return qiov->size;
}
+/* Return no. of bytes on success or < 0 on error. Important errors are:
+ -EIO generic I/O error (may happen for all errors)
+ -ENOMEDIUM No media inserted.
+ -EINVAL Invalid offset or number of bytes
+ -EACCES Trying to write a read-only device
+*/
int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes)
{
QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
diff --git a/include/block/block.h b/include/block/block.h
index c7a26199aa..5e2b98b0ee 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -316,10 +316,6 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue, Error **errp);
void bdrv_reopen_commit(BDRVReopenState *reopen_state);
void bdrv_reopen_abort(BDRVReopenState *reopen_state);
-int bdrv_read(BdrvChild *child, int64_t sector_num,
- uint8_t *buf, int nb_sectors);
-int bdrv_write(BdrvChild *child, int64_t sector_num,
- const uint8_t *buf, int nb_sectors);
int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
int bytes, BdrvRequestFlags flags);
int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags);
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v2 4/5] block: Remove bdrv_read() and bdrv_write()
@ 2019-05-01 18:13 ` Alberto Garcia
0 siblings, 0 replies; 22+ messages in thread
From: Alberto Garcia @ 2019-05-01 18:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, qemu-block, Max Reitz
No one is using these functions anymore, all callers have switched to
the byte-based bdrv_pread() and bdrv_pwrite()
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/io.c | 43 +++++++------------------------------------
include/block/block.h | 4 ----
2 files changed, 7 insertions(+), 40 deletions(-)
diff --git a/block/io.c b/block/io.c
index dfc153b8d8..396d5364ba 100644
--- a/block/io.c
+++ b/block/io.c
@@ -837,42 +837,6 @@ static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
return rwco.ret;
}
-/*
- * Process a synchronous request using coroutines
- */
-static int bdrv_rw_co(BdrvChild *child, int64_t sector_num, uint8_t *buf,
- int nb_sectors, bool is_write, BdrvRequestFlags flags)
-{
- QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf,
- nb_sectors * BDRV_SECTOR_SIZE);
-
- if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
- return -EINVAL;
- }
-
- return bdrv_prwv_co(child, sector_num << BDRV_SECTOR_BITS,
- &qiov, is_write, flags);
-}
-
-/* return < 0 if error. See bdrv_write() for the return codes */
-int bdrv_read(BdrvChild *child, int64_t sector_num,
- uint8_t *buf, int nb_sectors)
-{
- return bdrv_rw_co(child, sector_num, buf, nb_sectors, false, 0);
-}
-
-/* Return < 0 if error. Important errors are:
- -EIO generic I/O error (may happen for all errors)
- -ENOMEDIUM No media inserted.
- -EINVAL Invalid sector number or nb_sectors
- -EACCES Trying to write a read-only device
-*/
-int bdrv_write(BdrvChild *child, int64_t sector_num,
- const uint8_t *buf, int nb_sectors)
-{
- return bdrv_rw_co(child, sector_num, (uint8_t *)buf, nb_sectors, true, 0);
-}
-
int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
int bytes, BdrvRequestFlags flags)
{
@@ -935,6 +899,7 @@ int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
return qiov->size;
}
+/* See bdrv_pwrite() for the return codes */
int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes)
{
QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
@@ -958,6 +923,12 @@ int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
return qiov->size;
}
+/* Return no. of bytes on success or < 0 on error. Important errors are:
+ -EIO generic I/O error (may happen for all errors)
+ -ENOMEDIUM No media inserted.
+ -EINVAL Invalid offset or number of bytes
+ -EACCES Trying to write a read-only device
+*/
int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes)
{
QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
diff --git a/include/block/block.h b/include/block/block.h
index c7a26199aa..5e2b98b0ee 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -316,10 +316,6 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue, Error **errp);
void bdrv_reopen_commit(BDRVReopenState *reopen_state);
void bdrv_reopen_abort(BDRVReopenState *reopen_state);
-int bdrv_read(BdrvChild *child, int64_t sector_num,
- uint8_t *buf, int nb_sectors);
-int bdrv_write(BdrvChild *child, int64_t sector_num,
- const uint8_t *buf, int nb_sectors);
int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
int bytes, BdrvRequestFlags flags);
int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags);
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v2 5/5] qcow2: Remove BDRVQcow2State.cluster_sectors
@ 2019-05-01 18:13 ` Alberto Garcia
0 siblings, 0 replies; 22+ messages in thread
From: Alberto Garcia @ 2019-05-01 18:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Max Reitz, Kevin Wolf, Eric Blake
The last user of this field disappeared when we replace the
sector-based bdrv_write() with the byte-based bdrv_pwrite().
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/qcow2.c | 1 -
block/qcow2.h | 1 -
2 files changed, 2 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index a520d116ef..8e024007db 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1259,7 +1259,6 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
s->cluster_bits = header.cluster_bits;
s->cluster_size = 1 << s->cluster_bits;
- s->cluster_sectors = 1 << (s->cluster_bits - BDRV_SECTOR_BITS);
/* Initialise version 3 header fields */
if (header.version == 2) {
diff --git a/block/qcow2.h b/block/qcow2.h
index fdee297f33..e62508d1ce 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -266,7 +266,6 @@ typedef struct Qcow2BitmapHeaderExt {
typedef struct BDRVQcow2State {
int cluster_bits;
int cluster_size;
- int cluster_sectors;
int l2_slice_size;
int l2_bits;
int l2_size;
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v2 5/5] qcow2: Remove BDRVQcow2State.cluster_sectors
@ 2019-05-01 18:13 ` Alberto Garcia
0 siblings, 0 replies; 22+ messages in thread
From: Alberto Garcia @ 2019-05-01 18:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, qemu-block, Max Reitz
The last user of this field disappeared when we replace the
sector-based bdrv_write() with the byte-based bdrv_pwrite().
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/qcow2.c | 1 -
block/qcow2.h | 1 -
2 files changed, 2 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index a520d116ef..8e024007db 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1259,7 +1259,6 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
s->cluster_bits = header.cluster_bits;
s->cluster_size = 1 << s->cluster_bits;
- s->cluster_sectors = 1 << (s->cluster_bits - BDRV_SECTOR_BITS);
/* Initialise version 3 header fields */
if (header.version == 2) {
diff --git a/block/qcow2.h b/block/qcow2.h
index fdee297f33..e62508d1ce 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -266,7 +266,6 @@ typedef struct Qcow2BitmapHeaderExt {
typedef struct BDRVQcow2State {
int cluster_bits;
int cluster_size;
- int cluster_sectors;
int l2_slice_size;
int l2_bits;
int l2_size;
--
2.11.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/5] qcow2: Replace bdrv_write() with bdrv_pwrite()
2019-05-01 18:13 ` Alberto Garcia
(?)
@ 2019-05-06 15:58 ` Vladimir Sementsov-Ogievskiy
-1 siblings, 0 replies; 22+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-05-06 15:58 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
01.05.2019 21:13, Alberto Garcia wrote:
> There's only one bdrv_write() call left in the qcow2 code, and it can
> be trivially replaced with the byte-based bdrv_pwrite().
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/5] qcow2: Remove BDRVQcow2State.cluster_sectors
2019-05-01 18:13 ` Alberto Garcia
(?)
@ 2019-05-06 15:59 ` Vladimir Sementsov-Ogievskiy
-1 siblings, 0 replies; 22+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-05-06 15:59 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
01.05.2019 21:13, Alberto Garcia wrote:
> The last user of this field disappeared when we replace the
> sector-based bdrv_write() with the byte-based bdrv_pwrite().
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/5] vdi: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()
2019-05-01 18:13 ` Alberto Garcia
(?)
@ 2019-05-06 16:38 ` Vladimir Sementsov-Ogievskiy
2019-05-08 12:24 ` Kevin Wolf
-1 siblings, 1 reply; 22+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-05-06 16:38 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
01.05.2019 21:13, Alberto Garcia wrote:
> There's only a couple of bdrv_read() and bdrv_write() calls left in
> the vdi code, and they can be trivially replaced with the byte-based
> bdrv_pread() and bdrv_pwrite().
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block/vdi.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/block/vdi.c b/block/vdi.c
> index e1c42ad732..9caeb50dd1 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -171,6 +171,8 @@ typedef struct {
> uint64_t unused2[7];
> } QEMU_PACKED VdiHeader;
>
> +QEMU_BUILD_BUG_ON(sizeof(VdiHeader) != 512);
> +
> typedef struct {
> /* The block map entries are little endian (even in memory). */
> uint32_t *bmap;
> @@ -384,7 +386,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
>
> logout("\n");
>
> - ret = bdrv_read(bs->file, 0, (uint8_t *)&header, 1);
> + ret = bdrv_pread(bs->file, 0, (uint8_t *)&header, sizeof(header));
bdrv_pread parameter buf parameter is void, so (uint8_t *) conversion is not needed
(and even confusing)
> if (ret < 0) {
> goto fail;
> }
> @@ -484,8 +486,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
> goto fail;
> }
>
> - ret = bdrv_read(bs->file, s->bmap_sector, (uint8_t *)s->bmap,
> - bmap_size);
> + ret = bdrv_pread(bs->file, header.offset_bmap, (uint8_t *)s->bmap,
> + bmap_size * SECTOR_SIZE);
and here
> if (ret < 0) {
> goto fail_free_bmap;
> }
> @@ -704,7 +706,7 @@ nonallocating_write:
> assert(VDI_IS_ALLOCATED(bmap_first));
> *header = s->header;
> vdi_header_to_le(header);
> - ret = bdrv_write(bs->file, 0, block, 1);
> + ret = bdrv_pwrite(bs->file, 0, block, sizeof(VdiHeader));
maybe, more self-descriptive:
ret = bdrv_pwrite(bs->file, 0, header, sizeof(*header));
with at least extra conversion dropped:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> g_free(block);
> block = NULL;
>
> @@ -722,10 +724,11 @@ nonallocating_write:
> base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
> logout("will write %u block map sectors starting from entry %u\n",
> n_sectors, bmap_first);
> - ret = bdrv_write(bs->file, offset, base, n_sectors);
> + ret = bdrv_pwrite(bs->file, offset * SECTOR_SIZE, base,
> + n_sectors * SECTOR_SIZE);
> }
>
> - return ret;
> + return ret < 0 ? ret : 0;
> }
>
> static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()
2019-05-01 18:13 ` Alberto Garcia
(?)
@ 2019-05-06 16:47 ` Vladimir Sementsov-Ogievskiy
2019-05-06 17:06 ` Eric Blake
-1 siblings, 1 reply; 22+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-05-06 16:47 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
01.05.2019 21:13, Alberto Garcia wrote:
> There's only a couple of bdrv_read() and bdrv_write() calls left in
> the vvfat code, and they can be trivially replaced with the byte-based
> bdrv_pread() and bdrv_pwrite().
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block/vvfat.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 5f66787890..253cc716dd 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1494,8 +1494,8 @@ static int vvfat_read(BlockDriverState *bs, int64_t sector_num,
> DLOG(fprintf(stderr, "sectors %" PRId64 "+%" PRId64
> " allocated\n", sector_num,
> n >> BDRV_SECTOR_BITS));
> - if (bdrv_read(s->qcow, sector_num, buf + i * 0x200,
> - n >> BDRV_SECTOR_BITS)) {
> + if (bdrv_pread(s->qcow, sector_num * BDRV_SECTOR_SIZE,
> + buf + i * 0x200, n) < 0) {
Shouldn't we use QEMU_ALIGN_DOWN(n, BDRV_SECTOR_SIZE) ?
Could bdrv_is_allocated give unaligned n?
> return -1;
> }
> i += (n >> BDRV_SECTOR_BITS) - 1;
> @@ -1983,8 +1983,9 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
> if (res) {
> return -1;
> }
> - res = bdrv_write(s->qcow, offset, s->cluster_buffer, 1);
> - if (res) {
> + res = bdrv_pwrite(s->qcow, offset * BDRV_SECTOR_SIZE,
> + s->cluster_buffer, BDRV_SECTOR_SIZE);
> + if (res < 0) {
> return -2;
> }
> }
> @@ -3050,7 +3051,8 @@ DLOG(checkpoint());
> * Use qcow backend. Commit later.
> */
> DLOG(fprintf(stderr, "Write to qcow backend: %d + %d\n", (int)sector_num, nb_sectors));
> - ret = bdrv_write(s->qcow, sector_num, buf, nb_sectors);
> + ret = bdrv_pwrite(s->qcow, sector_num * BDRV_SECTOR_SIZE, buf,
> + nb_sectors * BDRV_SECTOR_SIZE);
> if (ret < 0) {
> fprintf(stderr, "Error writing to qcow backend\n");
> return ret;
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] block: Remove bdrv_read() and bdrv_write()
2019-05-01 18:13 ` Alberto Garcia
(?)
@ 2019-05-06 16:52 ` Vladimir Sementsov-Ogievskiy
-1 siblings, 0 replies; 22+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-05-06 16:52 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
01.05.2019 21:13, Alberto Garcia wrote:
> No one is using these functions anymore, all callers have switched to
> the byte-based bdrv_pread() and bdrv_pwrite()
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
However, I doubt that we want to save error codes descriptions. On
the other hand, "Return no. of bytes on success or < 0 on error." is good
addition, but it becomes unrelated to this patch if drop error codes
description.
> ---
> block/io.c | 43 +++++++------------------------------------
> include/block/block.h | 4 ----
> 2 files changed, 7 insertions(+), 40 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index dfc153b8d8..396d5364ba 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -837,42 +837,6 @@ static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
> return rwco.ret;
> }
>
> -/*
> - * Process a synchronous request using coroutines
> - */
> -static int bdrv_rw_co(BdrvChild *child, int64_t sector_num, uint8_t *buf,
> - int nb_sectors, bool is_write, BdrvRequestFlags flags)
> -{
> - QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf,
> - nb_sectors * BDRV_SECTOR_SIZE);
> -
> - if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
> - return -EINVAL;
> - }
> -
> - return bdrv_prwv_co(child, sector_num << BDRV_SECTOR_BITS,
> - &qiov, is_write, flags);
> -}
> -
> -/* return < 0 if error. See bdrv_write() for the return codes */
> -int bdrv_read(BdrvChild *child, int64_t sector_num,
> - uint8_t *buf, int nb_sectors)
> -{
> - return bdrv_rw_co(child, sector_num, buf, nb_sectors, false, 0);
> -}
> -
> -/* Return < 0 if error. Important errors are:
> - -EIO generic I/O error (may happen for all errors)
> - -ENOMEDIUM No media inserted.
> - -EINVAL Invalid sector number or nb_sectors
> - -EACCES Trying to write a read-only device
> -*/
> -int bdrv_write(BdrvChild *child, int64_t sector_num,
> - const uint8_t *buf, int nb_sectors)
> -{
> - return bdrv_rw_co(child, sector_num, (uint8_t *)buf, nb_sectors, true, 0);
> -}
> -
> int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
> int bytes, BdrvRequestFlags flags)
> {
> @@ -935,6 +899,7 @@ int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
> return qiov->size;
> }
>
> +/* See bdrv_pwrite() for the return codes */
> int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes)
> {
> QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
> @@ -958,6 +923,12 @@ int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
> return qiov->size;
> }
>
> +/* Return no. of bytes on success or < 0 on error. Important errors are:
> + -EIO generic I/O error (may happen for all errors)
> + -ENOMEDIUM No media inserted.
> + -EINVAL Invalid offset or number of bytes
> + -EACCES Trying to write a read-only device
> +*/
> int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes)
> {
> QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
> diff --git a/include/block/block.h b/include/block/block.h
> index c7a26199aa..5e2b98b0ee 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -316,10 +316,6 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
> BlockReopenQueue *queue, Error **errp);
> void bdrv_reopen_commit(BDRVReopenState *reopen_state);
> void bdrv_reopen_abort(BDRVReopenState *reopen_state);
> -int bdrv_read(BdrvChild *child, int64_t sector_num,
> - uint8_t *buf, int nb_sectors);
> -int bdrv_write(BdrvChild *child, int64_t sector_num,
> - const uint8_t *buf, int nb_sectors);
> int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
> int bytes, BdrvRequestFlags flags);
> int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags);
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()
2019-05-06 16:47 ` Vladimir Sementsov-Ogievskiy
@ 2019-05-06 17:06 ` Eric Blake
2019-05-06 17:19 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 22+ messages in thread
From: Eric Blake @ 2019-05-06 17:06 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Alberto Garcia, qemu-devel
Cc: Kevin Wolf, qemu-block, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 1905 bytes --]
On 5/6/19 11:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> 01.05.2019 21:13, Alberto Garcia wrote:
>> There's only a couple of bdrv_read() and bdrv_write() calls left in
>> the vvfat code, and they can be trivially replaced with the byte-based
>> bdrv_pread() and bdrv_pwrite().
>>
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> ---
>> block/vvfat.c | 12 +++++++-----
>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index 5f66787890..253cc716dd 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -1494,8 +1494,8 @@ static int vvfat_read(BlockDriverState *bs, int64_t sector_num,
>> DLOG(fprintf(stderr, "sectors %" PRId64 "+%" PRId64
>> " allocated\n", sector_num,
>> n >> BDRV_SECTOR_BITS));
>> - if (bdrv_read(s->qcow, sector_num, buf + i * 0x200,
>> - n >> BDRV_SECTOR_BITS)) {
>> + if (bdrv_pread(s->qcow, sector_num * BDRV_SECTOR_SIZE,
>> + buf + i * 0x200, n) < 0) {
>
> Shouldn't we use QEMU_ALIGN_DOWN(n, BDRV_SECTOR_SIZE) ?
No, n should already be aligned, which makes align_down a no-op.
> Could bdrv_is_allocated give unaligned n?
>
Yes, bdrv_is_allocated can return unaligned n in some situations; I had
a patch that didn't make 4.0 that would add bdrv_block_status_aligned
for cases where we need to guarantee that different alignment of a
backing chain doesn't bleed through to the specified alignment of the
current layer. But those situations are rare, and I need to revisit
those and send a v2; so I don't see a problem with this one going in
during the meantime as-is.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()
2019-05-06 17:06 ` Eric Blake
@ 2019-05-06 17:19 ` Vladimir Sementsov-Ogievskiy
2019-05-06 17:39 ` Eric Blake
0 siblings, 1 reply; 22+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-05-06 17:19 UTC (permalink / raw)
To: Eric Blake, Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
06.05.2019 20:06, Eric Blake wrote:
> On 5/6/19 11:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 01.05.2019 21:13, Alberto Garcia wrote:
>>> There's only a couple of bdrv_read() and bdrv_write() calls left in
>>> the vvfat code, and they can be trivially replaced with the byte-based
>>> bdrv_pread() and bdrv_pwrite().
>>>
>>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>>> ---
>>> block/vvfat.c | 12 +++++++-----
>>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/block/vvfat.c b/block/vvfat.c
>>> index 5f66787890..253cc716dd 100644
>>> --- a/block/vvfat.c
>>> +++ b/block/vvfat.c
>>> @@ -1494,8 +1494,8 @@ static int vvfat_read(BlockDriverState *bs, int64_t sector_num,
>>> DLOG(fprintf(stderr, "sectors %" PRId64 "+%" PRId64
>>> " allocated\n", sector_num,
>>> n >> BDRV_SECTOR_BITS));
>>> - if (bdrv_read(s->qcow, sector_num, buf + i * 0x200,
>>> - n >> BDRV_SECTOR_BITS)) {
>>> + if (bdrv_pread(s->qcow, sector_num * BDRV_SECTOR_SIZE,
>>> + buf + i * 0x200, n) < 0) {
>>
>> Shouldn't we use QEMU_ALIGN_DOWN(n, BDRV_SECTOR_SIZE) ?
>
> No, n should already be aligned, which makes align_down a no-op.
>
>> Could bdrv_is_allocated give unaligned n?
>>
>
> Yes, bdrv_is_allocated can return unaligned n in some situations; I had
> a patch that didn't make 4.0 that would add bdrv_block_status_aligned
> for cases where we need to guarantee that different alignment of a
> backing chain doesn't bleed through to the specified alignment of the
> current layer. But those situations are rare, and I need to revisit
> those and send a v2; so I don't see a problem with this one going in
> during the meantime as-is.
>
Than, n is not already aligned, as it comes from bdrv_is_allocated.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()
2019-05-06 17:19 ` Vladimir Sementsov-Ogievskiy
@ 2019-05-06 17:39 ` Eric Blake
0 siblings, 0 replies; 22+ messages in thread
From: Eric Blake @ 2019-05-06 17:39 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Alberto Garcia, qemu-devel
Cc: Kevin Wolf, qemu-block, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]
On 5/6/19 12:19 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>> +++ b/block/vvfat.c
>>>> @@ -1494,8 +1494,8 @@ static int vvfat_read(BlockDriverState *bs, int64_t sector_num,
>>>> DLOG(fprintf(stderr, "sectors %" PRId64 "+%" PRId64
>>>> " allocated\n", sector_num,
>>>> n >> BDRV_SECTOR_BITS));
>>>> - if (bdrv_read(s->qcow, sector_num, buf + i * 0x200,
>>>> - n >> BDRV_SECTOR_BITS)) {
>>>> + if (bdrv_pread(s->qcow, sector_num * BDRV_SECTOR_SIZE,
>>>> + buf + i * 0x200, n) < 0) {
>>>
>>> Shouldn't we use QEMU_ALIGN_DOWN(n, BDRV_SECTOR_SIZE) ?
>>
>> No, n should already be aligned, which makes align_down a no-op.
>>
>>> Could bdrv_is_allocated give unaligned n?
>>>
>>
>> Yes, bdrv_is_allocated can return unaligned n in some situations; I had
>> a patch that didn't make 4.0 that would add bdrv_block_status_aligned
>> for cases where we need to guarantee that different alignment of a
>> backing chain doesn't bleed through to the specified alignment of the
>> current layer. But those situations are rare, and I need to revisit
>> those and send a v2; so I don't see a problem with this one going in
>> during the meantime as-is.
>>
>
> Than, n is not already aligned, as it comes from bdrv_is_allocated.
Note that whether bdrv_is_allocated can return data not aligned to 512
depends on the driver. It is possible when querying file-posix.c, but
only for a POSIX file that encounters EOF mid-sector. However, it is not
possible for the qcow2 driver. The patches I need to rework are worried
more about cases where a block device with request_alignment of 4k can
still see 512-alignment leak through from a backing file. But since
vvfat is grabbing alignment from a qcow2 image, and not a raw POSIX
file, we should never see sub-sector alignment.
So my answers above were terse but correct: bdrv_is_allocated can return
unaligned data in some cases, but vvfat should not be one of those
cases. If you'd like to add an assert instead of a QEMU_ALIGN_DOWN, that
should be reasonable.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/5] vdi: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()
2019-05-06 16:38 ` Vladimir Sementsov-Ogievskiy
@ 2019-05-08 12:24 ` Kevin Wolf
0 siblings, 0 replies; 22+ messages in thread
From: Kevin Wolf @ 2019-05-08 12:24 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Alberto Garcia, qemu-devel, qemu-block, Max Reitz
Am 06.05.2019 um 18:38 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 01.05.2019 21:13, Alberto Garcia wrote:
> > There's only a couple of bdrv_read() and bdrv_write() calls left in
> > the vdi code, and they can be trivially replaced with the byte-based
> > bdrv_pread() and bdrv_pwrite().
> >
> > Signed-off-by: Alberto Garcia <berto@igalia.com>
> > ---
> > block/vdi.c | 15 +++++++++------
> > 1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/block/vdi.c b/block/vdi.c
> > index e1c42ad732..9caeb50dd1 100644
> > --- a/block/vdi.c
> > +++ b/block/vdi.c
> > @@ -171,6 +171,8 @@ typedef struct {
> > uint64_t unused2[7];
> > } QEMU_PACKED VdiHeader;
> >
> > +QEMU_BUILD_BUG_ON(sizeof(VdiHeader) != 512);
> > +
> > typedef struct {
> > /* The block map entries are little endian (even in memory). */
> > uint32_t *bmap;
> > @@ -384,7 +386,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
> >
> > logout("\n");
> >
> > - ret = bdrv_read(bs->file, 0, (uint8_t *)&header, 1);
> > + ret = bdrv_pread(bs->file, 0, (uint8_t *)&header, sizeof(header));
>
> bdrv_pread parameter buf parameter is void, so (uint8_t *) conversion is not needed
> (and even confusing)
>
> > if (ret < 0) {
> > goto fail;
> > }
> > @@ -484,8 +486,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
> > goto fail;
> > }
> >
> > - ret = bdrv_read(bs->file, s->bmap_sector, (uint8_t *)s->bmap,
> > - bmap_size);
> > + ret = bdrv_pread(bs->file, header.offset_bmap, (uint8_t *)s->bmap,
> > + bmap_size * SECTOR_SIZE);
>
> and here
>
> > if (ret < 0) {
> > goto fail_free_bmap;
> > }
> > @@ -704,7 +706,7 @@ nonallocating_write:
> > assert(VDI_IS_ALLOCATED(bmap_first));
> > *header = s->header;
> > vdi_header_to_le(header);
> > - ret = bdrv_write(bs->file, 0, block, 1);
> > + ret = bdrv_pwrite(bs->file, 0, block, sizeof(VdiHeader));
>
> maybe, more self-descriptive:
> ret = bdrv_pwrite(bs->file, 0, header, sizeof(*header));
>
>
> with at least extra conversion dropped:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
I'll drop the casts while applying.
Kevin
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/5] Remove bdrv_read() and bdrv_write()
2019-05-01 18:13 ` Alberto Garcia
` (5 preceding siblings ...)
(?)
@ 2019-05-08 12:27 ` Kevin Wolf
-1 siblings, 0 replies; 22+ messages in thread
From: Kevin Wolf @ 2019-05-08 12:27 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Max Reitz
Am 01.05.2019 um 20:13 hat Alberto Garcia geschrieben:
> Hi,
>
> this API only had a few users left so it can be easily removed.
Thanks, applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-05-08 12:28 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-01 18:13 [Qemu-devel] [PATCH v2 0/5] Remove bdrv_read() and bdrv_write() Alberto Garcia
2019-05-01 18:13 ` Alberto Garcia
2019-05-01 18:13 ` [Qemu-devel] [PATCH v2 1/5] qcow2: Replace bdrv_write() with bdrv_pwrite() Alberto Garcia
2019-05-01 18:13 ` Alberto Garcia
2019-05-06 15:58 ` Vladimir Sementsov-Ogievskiy
2019-05-01 18:13 ` [Qemu-devel] [PATCH v2 2/5] vdi: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}() Alberto Garcia
2019-05-01 18:13 ` Alberto Garcia
2019-05-06 16:38 ` Vladimir Sementsov-Ogievskiy
2019-05-08 12:24 ` Kevin Wolf
2019-05-01 18:13 ` [Qemu-devel] [PATCH v2 3/5] vvfat: " Alberto Garcia
2019-05-01 18:13 ` Alberto Garcia
2019-05-06 16:47 ` Vladimir Sementsov-Ogievskiy
2019-05-06 17:06 ` Eric Blake
2019-05-06 17:19 ` Vladimir Sementsov-Ogievskiy
2019-05-06 17:39 ` Eric Blake
2019-05-01 18:13 ` [Qemu-devel] [PATCH v2 4/5] block: Remove bdrv_read() and bdrv_write() Alberto Garcia
2019-05-01 18:13 ` Alberto Garcia
2019-05-06 16:52 ` Vladimir Sementsov-Ogievskiy
2019-05-01 18:13 ` [Qemu-devel] [PATCH v2 5/5] qcow2: Remove BDRVQcow2State.cluster_sectors Alberto Garcia
2019-05-01 18:13 ` Alberto Garcia
2019-05-06 15:59 ` Vladimir Sementsov-Ogievskiy
2019-05-08 12:27 ` [Qemu-devel] [PATCH v2 0/5] Remove bdrv_read() and bdrv_write() Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.