[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 4/4] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded
From: |
Max Reitz |
Subject: |
Re: [PATCH v2 4/4] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value |
Date: |
Fri, 17 Jan 2020 12:01:06 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 |
On 17.01.20 10:55, Kevin Wolf wrote:
> Am 17.01.2020 um 10:12 hat Max Reitz geschrieben:
>> 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.)
>
> We still can't do byte-granularity writes with O_DIRECT, because that's
> a kernel requirement.
Ah, yes. But that makes BDRV_SECTOR_SIZE the wrong choice.
> The comment explains that we don't want to do a RMW cycle to write a
> single entry because that would be slower than just writing a whole
> sector. I think this is still accurate. Maybe we should change the
> comment to say "can't necessarily update". (The part that looks really
> wrong in the comment is "bdrv_pread", that should be "bdrv_pwrite"...)
Hm. But we wouldn’t do an RMW cycle without O_DIRECT, would we?
> Now, what's wrong about the logic to avoid the RMW is that it assumes
> a fixed required alignment of 512. What it should do is looking at
> bs->file->bl.request_alignment and rounding accordingly.
Yes.
>>>>> @@ -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.
>
> I don't understand this one either.
Good. :-)
Max
signature.asc
Description: OpenPGP digital signature
- Re: [PATCH v2 1/4] qcow2: Require that the virtual size is a multiple of the sector size, (continued)
- Re: [PATCH v2 1/4] qcow2: Require that the virtual size is a multiple of the sector size, Alberto Garcia, 2020/01/14
- Re: [PATCH v2 1/4] qcow2: Require that the virtual size is a multiple of the sector size, Max Reitz, 2020/01/14
- Re: [PATCH v2 1/4] qcow2: Require that the virtual size is a multiple of the sector size, Alberto Garcia, 2020/01/14
- Re: [PATCH v2 1/4] qcow2: Require that the virtual size is a multiple of the sector size, Max Reitz, 2020/01/14
- Re: [PATCH v2 1/4] qcow2: Require that the virtual size is a multiple of the sector size, Alberto Garcia, 2020/01/16
[PATCH v2 4/4] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value, Alberto Garcia, 2020/01/09
- Re: [PATCH v2 4/4] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value, Max Reitz, 2020/01/14
- Re: [PATCH v2 4/4] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value, Alberto Garcia, 2020/01/16
- Re: [PATCH v2 4/4] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value, Max Reitz, 2020/01/17
- Re: [PATCH v2 4/4] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value, Kevin Wolf, 2020/01/17
- Re: [PATCH v2 4/4] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value,
Max Reitz <=
- Re: [PATCH v2 4/4] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value, Kevin Wolf, 2020/01/17
- Re: [PATCH v2 4/4] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value, Alberto Garcia, 2020/01/17
Re: [PATCH v2 4/4] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value, Alberto Garcia, 2020/01/18