[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: |
Kevin Wolf |
Subject: |
Re: [PATCH v2 4/4] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value |
Date: |
Fri, 17 Jan 2020 12:51:04 +0100 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
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
- Re: [PATCH v2 1/4] qcow2: Require that the virtual size is a multiple of the sector size, (continued)
[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, 2020/01/17
- Re: [PATCH v2 4/4] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value,
Kevin Wolf <=
- 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
[PATCH v2 2/4] qcow2: Don't round the L1 table allocation up to the sector size, Alberto Garcia, 2020/01/09