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

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2874 bytes --]

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.

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"...)

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.

> >>> @@ -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.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]