| Message ID | 524e211cb818a20f521d6e271e782ab62b8e5e80.1556732434.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Remove bdrv_read() and bdrv_write() | expand |
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; >
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.
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.
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.
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;
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(-)