[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 18/54] block: Default .bdrv_child_perm() for for
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH 18/54] block: Default .bdrv_child_perm() for format drivers |
Date: |
Mon, 27 Feb 2017 15:15:22 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 |
On 27.02.2017 15:05, Kevin Wolf wrote:
> Am 27.02.2017 um 13:34 hat Max Reitz geschrieben:
>> On 27.02.2017 13:33, Kevin Wolf wrote:
>>> Am 25.02.2017 um 12:57 hat Max Reitz geschrieben:
>>>> On 21.02.2017 15:58, Kevin Wolf wrote:
>>>>> Almost all format drivers have the same characteristics as far as
>>>>> permissions are concerned: They have one or more children for storing
>>>>> their own data and, more importantly, metadata (can be written to and
>>>>> grow even without external write requests, must be protected against
>>>>> other writers and present consistent data) and optionally a backing file
>>>>> (this is just data, so like for a filter, it only depends on what the
>>>>> parent nodes need).
>>>>>
>>>>> This provides a default implementation that can be shared by most of
>>>>> our format drivers.
>>>>>
>>>>> Signed-off-by: Kevin Wolf <address@hidden>
>>>>> ---
>>>>> block.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>>> include/block/block_int.h | 8 ++++++++
>>>>> 2 files changed, 50 insertions(+)
>>>>>
>>>>> diff --git a/block.c b/block.c
>>>>> index 523cbd3..f2e7178 100644
>>>>> --- a/block.c
>>>>> +++ b/block.c
>>>>> @@ -1554,6 +1554,48 @@ void bdrv_filter_default_perms(BlockDriverState
>>>>> *bs, BdrvChild *c,
>>>>> (c->shared_perm & DEFAULT_PERM_UNCHANGED);
>>>>> }
>>>>>
>>>>> +void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
>>>>> + const BdrvChildRole *role,
>>>>> + uint64_t perm, uint64_t shared,
>>>>> + uint64_t *nperm, uint64_t *nshared)
>>>>> +{
>>>>> + bool backing = (role == &child_backing);
>>>>> + assert(role == &child_backing || role == &child_file);
>>>>> +
>>>>> + if (!backing) {
>>>>> + /* Apart from the modifications below, the same permissions are
>>>>> + * forwarded and left alone as for filters */
>>>>> + bdrv_filter_default_perms(bs, c, role, perm, shared, &perm,
>>>>> &shared);
>>>>> +
>>>>> + /* Format drivers may touch metadata even if the guest doesn't
>>>>> write */
>>>>> + if (!bdrv_is_read_only(bs)) {
>>>>> + perm |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
>>>>> + }
>>>>> +
>>>>> + /* bs->file always needs to be consistent because of the
>>>>> metadata. We
>>>>> + * can never allow other users to resize or write to it. */
>>>>> + perm |= BLK_PERM_CONSISTENT_READ;
>>>>> + shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
>>>>> + } else {
>>>>> + /* We want consistent read from backing files if the parent
>>>>> needs it.
>>>>> + * No other operations are performed on backing files. */
>>>>> + perm &= BLK_PERM_CONSISTENT_READ;
>>>>> +
>>>>> + /* If the parent can deal with changing data, we're okay with a
>>>>> + * writable and resizable backing file. */
>>>>> + if (shared & BLK_PERM_WRITE) {
>>>>> + shared = BLK_PERM_WRITE | BLK_PERM_RESIZE;
>>>>
>>>> Wouldn't this break CONSISTENT_READ?
>>>
>>> WRITE (even for multiple users) and CONSISTENT_READ aren't mutually
>>> exclusive. I was afraid that I didn't define CONSISTENT_READ right, but
>>> it appears that the definition is fine:
>>>
>>> * A user that has the "permission" of consistent reads is guaranteed that
>>> * their view of the contents of the block device is complete and
>>> * self-consistent, representing the contents of a disk at a specific
>>> * point.
>>
>> Right, but writes to the backing file at least to me appear to be a
>> different matter. If those don't break CONSISTENT_READ, then I don't see
>> how commit breaks CONSISTENT_READ for the intermediate nodes.
>
> There's probably multiple ways to interpret such actions. You could
> understand a commit job as writing the desired image to the base node
> and at the same time it's a shared writer for the intermediate nodes
> that happens to write garbage.
Agreed.
> The question is if this is a useful way
> of seeing it when the job is to prevent accidental data corruption.
Agreed. But then I would infer that any write to a backing file breaks
CONSISTENT_READ on the overlay.
> Note that we need writable backing files for commit, so taking away
> BLK_PERM_WRITE from shared wouldn't work. We could probably make it
> dependent on cleared CONSISTENT_READ (commit jobs don't require this
> anyway), if you think that the current version is too permissive.
I agree that we need to be able to share WRITE for a backing file. But I
think this should only be set if the overlay's parents do not require
CONSISTENT_READ from the overlay.
Max
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH 15/54] block: Involve block drivers in permission granting, (continued)
[Qemu-devel] [PATCH 14/54] block: Let callers request permissions when attaching a child node, Kevin Wolf, 2017/02/21
[Qemu-devel] [PATCH 18/54] block: Default .bdrv_child_perm() for format drivers, Kevin Wolf, 2017/02/21
[Qemu-devel] [PATCH 19/54] block: Request child permissions in format drivers, Kevin Wolf, 2017/02/21
[Qemu-devel] [PATCH 20/54] vvfat: Implement .bdrv_child_perm(), Kevin Wolf, 2017/02/21
[Qemu-devel] [PATCH 21/54] block: Require .bdrv_child_perm() with child nodes, Kevin Wolf, 2017/02/21
[Qemu-devel] [PATCH 22/54] block: Request real permissions in bdrv_attach_child(), Kevin Wolf, 2017/02/21