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
signature.asc
Description: PGP signature