[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 09/22] nbd: Add writethrough to block-export-add
From: |
Max Reitz |
Subject: |
Re: [RFC PATCH 09/22] nbd: Add writethrough to block-export-add |
Date: |
Mon, 17 Aug 2020 17:35:23 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 |
On 17.08.20 16:32, Kevin Wolf wrote:
> Am 17.08.2020 um 15:51 hat Max Reitz geschrieben:
>> On 17.08.20 15:13, Kevin Wolf wrote:
>>> Am 17.08.2020 um 14:56 hat Max Reitz geschrieben:
>>>> On 13.08.20 18:29, Kevin Wolf wrote:
>>>>> qemu-nbd allows use of writethrough cache modes, which mean that write
>>>>> requests made through NBD will cause a flush before they complete.
>>>>> Expose the same functionality in block-export-add.
>>>>>
>>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>>>> ---
>>>>> qapi/block-export.json | 7 ++++++-
>>>>> blockdev-nbd.c | 2 +-
>>>>> 2 files changed, 7 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/qapi/block-export.json b/qapi/block-export.json
>>>>> index 1fdc55c53a..4ce163411f 100644
>>>>> --- a/qapi/block-export.json
>>>>> +++ b/qapi/block-export.json
>>>>> @@ -167,10 +167,15 @@
>>>>> # Describes a block export, i.e. how single node should be exported on an
>>>>> # external interface.
>>>>> #
>>>>> +# @writethrough: If true, caches are flushed after every write request
>>>>> to the
>>>>> +# export before completion is signalled. (since: 5.2;
>>>>> +# default: false)
>>>>> +#
>>>>> # Since: 4.2
>>>>> ##
>>>>> { 'union': 'BlockExportOptions',
>>>>> - 'base': { 'type': 'BlockExportType' },
>>>>> + 'base': { 'type': 'BlockExportType',
>>>>> + '*writethrough': 'bool' },
>>>>> 'discriminator': 'type',
>>>>> 'data': {
>>>>> 'nbd': 'BlockExportOptionsNbd'
>>>>
>>>> Hm. I find it weird to have @writethrough in the base but @device in
>>>> the specialized class.
>>>>
>>>> I think everything that will be common to all block exports should be in
>>>> the base, and that probably includes a node-name. I’m aware that will
>>>> make things more tedious in the code, but perhaps it would be a nicer
>>>> interface in the end. Or is the real problem that that would create
>>>> problems in the storage daemon’s command line interface, because then
>>>> the specialized (legacy) NBD interface would no longer be compatible
>>>> with the new generalized block export interface?
>>>
>>> Indeed. I think patch 15 has what you're looking for.
>>
>> Great. :)
>>
>> Discussions where both participants have the same opinion from the
>> start are the best ones.
>
> Makes things a lot easier.
>
> Maybe I should try to move patch 15 earlier. The series is mostly just
> in the order that I wrote things, but there were also a few nasty
> dependencies in the part the generalises things from NBD to BlockExport.
> So I'm not sure if this is a patch that can be moved.
>
>>>> Anyway, @writable might be a similar story. A @read-only may make sense
>>>> in general, I think.
>>>
>>> Pulling @writable up is easier than a @read-only, but that's a naming
>>> detail.
>>
>> Sure.
>>
>>> In general I agree, but this part isn't addressed in this series yet.
>>> Part of the reason why this is an RFC was to find out if I should
>>> include things like this or if we'll do it when we add another export
>>> type or common functionality that needs the same option.
>>
>> Sure, sure.
>
> So should I or not? :-)
Can we delay it until after this series? I.e., as long as it retains
the name “writable”, would pulling it up into BlockExportOptions a
compatible change?
If so, then I suppose we could do it afterwards. But I think it does
make the most sense to “just” do it as part of this series.
>> Meta: I personally don’t like RFC patches very much. RFC to me means
>> everything is fair game, and reviewers should be free to let their
>> thoughts wander and come up with perhaps wild ideas, just trying to
>> gauge what everyone thinks.
>>
>> When I’m the submitter, I tend to get defensive then, because I’ve
>> invested time in writing the code already, so I tend to be biased
>> against fundamental changes. (Horrible personal trait. I’m working
>> on it.)
>
> This makes sense. Nobody likes having to rewrite their RFC series.
>
> But there is one thing I dread even more: Polishing the RFC series for
> another week until I can send it out as non-RFC and _then_ having to
> rewrite it.
Yes. Especially bad with tests.
>> As a reviewer, the code and thus some fleshed out design is there
>> already, so it’s difficult to break free from that and find completely
>> different solutions to the original problem.
>> (I kind of ventured in that direction for this patch, and it seems like
>> you immediately noticed that my response was different from usual and
>> pointed out the RFC status, perhaps to make me feel more comfortable in
>> questioning the fundamental design more. Which I noticed, hence this
>> wall of text.)
>
> Basically just telling you that I was already interested in your input
> for this point specifically when I sent the series.
OK :)
>> Perhaps I’m wrong. Perhaps it’s just myself (the points I’ve just
>> listed are definitely my own personal weaknesses), but I can’t help but
>> project and assume that others may feel similar, at least in part.
>> So I feel like RFCs that consist of patches tend to at least lock me in
>> to the solution that’s present. I find them difficult to handle, both
>> as a submitter and as a reviewer.
>>
>> All in all, that means on either side I tend to handle patch RFCs as
>> “Standard series, just tests missing”. Not sure if that’s ideal. Or
>> maybe that’s exactly what patch RFCs are?
>>
>> (Of course, it can and should be argued that even for standard series, I
>> shouldn’t be afraid of questioning the fundamental design still. But
>> that’s hard...)
>
> I usually send RFC patches when I know that I wouldn't consider them
> mergable yet, but I don't want to invest the time to polish them before
> I know that other people agree with the approach and the time won't be
> wasted.
>
>> But, well. The alternative is writing pure design RFCs, and then you
>> tend to get weeks of slow discussion, drawing everything out. Which
>> isn’t ideal either. Or is that just a baseless prejudice I have?
>
> In many cases (and I think this is one of them in large parts), I only
> really learn what the series will look like when I write it.
That’s true. With a pure design RFC, it’s often difficult to know even
the scope of the design until you’ve begun to write code. So there’s a
danger of just writing a bunch of uncontroversial basic design stuff
because one has no idea of what may actually become problematic and
questionable. :/
> I could have sent a design RFC for the QAPI part, but I didn't expect
> this to be contentious because it's just the normal add/del/query thing
> that exists for pretty much everything else, too.
Yeah, the functions themselves are clear.
Hm. Perhaps software engineering just is actually difficult, and
there’s no way around it.
>>>> Basically, I think that the export code should be separate from the code
>>>> setting up the BlockBackend that should be exported, so all options
>>>> regarding that BB should be common; and those options are @node-name,
>>>> @writethrough, and @read-only. (And perhaps other things like
>>>> @resizable, too, even though that isn’t something to consider for NBD.)
>>>
>>> Do you mean that the BlockBackend should already be created by the
>>> generic block export layer?
>>
>> It would certainly be nice, if it were feasible, don’t you think?
>>
>> We don’t have to bend backwards for it, but maybe it would force us to
>> bring the natural separation of block device and export parameters to
>> the interface.
>
> I can try. I seem to remember that you had a reason not to do this the
> last time we discussed generalised exports, but I'm not sure what it
> was.
>
> The obvious one could be that the block export layer doesn't know which
> permissions are needed. But it can always start with minimal permissions
> and let the driver do a blk_set_perm() if it needs more.
Trying sounds good. Since there shouldn’t be consequences for the QMP
interface, we™ can always try again later (i.e., when adding more export
types).
Max
signature.asc
Description: OpenPGP digital signature
- Re: [RFC PATCH 07/22] block/export: Remove magic from block-export-add, (continued)
[RFC PATCH 09/22] nbd: Add writethrough to block-export-add, Kevin Wolf, 2020/08/13
Re: [RFC PATCH 09/22] nbd: Add writethrough to block-export-add, Eric Blake, 2020/08/19
Re: [RFC PATCH 09/22] nbd: Add writethrough to block-export-add, Eric Blake, 2020/08/19
[RFC PATCH 08/22] nbd: Add max-connections to nbd-server-start, Kevin Wolf, 2020/08/13
[RFC PATCH 10/22] nbd: Remove NBDExport.close callback, Kevin Wolf, 2020/08/13