diff mbox series

[RFC,09/22] nbd: Add writethrough to block-export-add

Message ID 20200813162935.210070-10-kwolf@redhat.com
State New, archived
Headers show
Series block/export: Add infrastructure and QAPI for block exports | expand

Commit Message

Kevin Wolf Aug. 13, 2020, 4:29 p.m. UTC
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(-)

Comments

Max Reitz Aug. 17, 2020, 12:56 p.m. UTC | #1
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?

Anyway, @writable might be a similar story.  A @read-only may make sense
in general, I think.

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.)

Max
Kevin Wolf Aug. 17, 2020, 1:13 p.m. UTC | #2
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.

> 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.

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.

> 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?

Kevin
Max Reitz Aug. 17, 2020, 1:51 p.m. UTC | #3
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.

>> 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.


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.)

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.)

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...)


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?

>> 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.

Max
Kevin Wolf Aug. 17, 2020, 2:32 p.m. UTC | #4
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? :-)

> 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.

> 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.

> 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.

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.

> >> 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.

Kevin
Max Reitz Aug. 17, 2020, 3:35 p.m. UTC | #5
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
Eric Blake Aug. 19, 2020, 8:05 p.m. UTC | #6
On 8/17/20 7:56 AM, Max Reitz wrote:
> 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>
>> ---

>> +++ 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?
> 
> Anyway, @writable might be a similar story.  A @read-only may make sense
> in general, I think.

And maybe even auto-read-only.  As for @writable vs. @read-only, that's 
a choice of spelling, but we don't want both; there's also the question 
of which default is saner (having to opt-in to allowing writes is more 
verbose than defaulting to allowing writes when possible, but arguably 
saner as it is less risk of unintended writes when you forgot to specify 
the option; @auto-read-only can help in choosing nicer defaults).

> 
> 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.)

NBD isn't resizable yet, but extending the protocol to let it become so 
is on my TODO list.
Eric Blake Aug. 19, 2020, 8:13 p.m. UTC | #7
On 8/13/20 11:29 AM, 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(-)
> 

> +++ b/blockdev-nbd.c
> @@ -218,7 +218,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
>   
>       exp = nbd_export_new(bs, arg->name, arg->description, arg->bitmap,
>                            !arg->writable, !arg->writable,
> -                         NULL, false, errp);
> +                         NULL, exp_args->writethrough, errp);

It would someday be nice to get rid of exp_args->has_writethrough in 
QAPI generated code, but that's independent of this series.  Meanwhile, 
you are safe in relying on writethrough being false when 
has_writethrough is false.

This change makes sense to me interface-wise.  QAPI-wise, should we try 
harder to make writethrough settable only for writable exports (and an 
error for read-only exports)?  I'm not sure what QAPI contortions would 
have to look like to make that enforced by the QMP parser; but it's also 
not the end of the world if we just always permit the optional field and 
then apply a post-parse semantic check.
diff mbox series

Patch

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'
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 28159a92b2..17417c1b6b 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -218,7 +218,7 @@  BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
 
     exp = nbd_export_new(bs, arg->name, arg->description, arg->bitmap,
                          !arg->writable, !arg->writable,
-                         NULL, false, errp);
+                         NULL, exp_args->writethrough, errp);
     if (!exp) {
         goto out;
     }