Re: [PATCH v2 4/4] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value

[Max Reitz]

On 17.01.20 00:26, Alberto Garcia wrote:
> On Tue 14 Jan 2020 03:15:48 PM CET, Max Reitz wrote:
>>> @@ -219,7 +219,7 @@ static int l2_load(BlockDriverState *bs, uint64_t 
>>> offset,
>>>   * Writes one sector of the L1 table to the disk (can't update single 
>>> entries
>>>   * and we really don't want bdrv_pread to perform a read-modify-write)
>>>   */
>>> -#define L1_ENTRIES_PER_SECTOR (512 / 8)
>>> +#define L1_ENTRIES_PER_SECTOR (BDRV_SECTOR_SIZE / 8)
>>>  int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index)
>>
>> Here it’s because the comment is wrong: “Can’t update single entries” –
>> yes, we can.  We’d just have to do a bdrv_pwrite() to a single entry.
> 
> What's the point of qcow2_write_l1_entry() then?

I think the point was that we couldn’t, for a long time, because the
block layer only provided sector-granularity access.  This function
simply was never changed when the block layer gained the ability to do
byte-granularity I/O.

(We’d still need this function, but only for the endian swap, I think.)

>>> @@ -3836,7 +3837,7 @@ qcow2_co_copy_range_from(BlockDriverState *bs,
>>>          case QCOW2_CLUSTER_NORMAL:
>>>              child = s->data_file;
>>>              copy_offset += offset_into_cluster(s, src_offset);
>>> -            if ((copy_offset & 511) != 0) {
>>> +            if (!QEMU_IS_ALIGNED(copy_offset, BDRV_SECTOR_SIZE)) {
>>
>> Hm.  I don’t get this one.
> 
> Checking the code (e.g. block_copy_do_copy()) it seems that the whole
> chunk must be cluster aligned so I don't get this one either.

Hm, how did you get to block_copy_do_copy()?  That’s part of the
block-copy infrastructure that’s only used for the backup job, as far as
I’m aware.  It’s different from copy_range.

I don’t see any limitation for copy_range.  I suppose maybe it doesn’t
work for anything that isn’t aligned to physical sectors?  But the qcow2
driver shouldn’t care about that.

On thing’s for sure, the raw driver doesn’t care about it.

Max

signature.asc
Description: OpenPGP digital signature