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
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
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
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
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
© 2016 - 2025 Red Hat, Inc.