Am 17.01.2020 um 12:01 hat Max Reitz geschrieben: > 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?
file-posix with a regular file has request_alignment == 1 and wouldn't cause an RMW cycle, I think. I won't try to make a statement about all non-O_DIRECT cases in all protocols. The important point is that some cases exist which would get us RMW. > > 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. Who'll write the patch? :-) Kevin
signature.asc
Description: PGP signature
