| Message ID | 20191219143415.28490-1-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi/block: fix nbd-server-add spec | expand |
On Thu, Dec 19, 2019 at 4:34 PM Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > > "NAME" here may be interpreted like it should match @name, which is > export name. But it was never mentioned in such way. Make it obvious, > that actual "<dirty-bitmap-export-name>" (see docs/interop/nbd.txt) > will match @bitmap parameter. But this is wrong, dirty-bitmap-export-name does not mean the actual bitmap name but the name exposed to the NBD client, which can be anything. > Fixes: 5fcbeb06812685a2 > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > > Hi all. > > This patch follows discussion on Nir's patch > [PATCH] block: nbd: Fix dirty bitmap context name > ( https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg04309.html ) > > Let's just fix qapi spec now. But qapi documents a better behavior for users. We should fix the code instead to mach the docs. With this we still have the issue of leaking internal bitmap name to users who do not control the name, and do not care about it. > qapi/block.json | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/qapi/block.json b/qapi/block.json > index 145c268bb6..8042ef78f0 100644 > --- a/qapi/block.json > +++ b/qapi/block.json > @@ -255,7 +255,8 @@ > > # @bitmap: Also export the dirty bitmap reachable from @device, so the > # NBD client can use NBD_OPT_SET_META_CONTEXT with > -# "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0) > +# "qemu:dirty-bitmap:BITMAP" to inspect the bitmap (BITMAP here > +# matches @bitmap parameter). (since 4.0) > # > # Returns: error if the server is not running, or export with the same name > # already exists. > -- > 2.21.0 >
19.12.2019 17:42, Nir Soffer wrote: > On Thu, Dec 19, 2019 at 4:34 PM Vladimir Sementsov-Ogievskiy > <vsementsov@virtuozzo.com> wrote: >> >> "NAME" here may be interpreted like it should match @name, which is >> export name. But it was never mentioned in such way. Make it obvious, >> that actual "<dirty-bitmap-export-name>" (see docs/interop/nbd.txt) >> will match @bitmap parameter. > > But this is wrong, dirty-bitmap-export-name does not mean the actual bitmap > name but the name exposed to the NBD client, which can be anything. Yes. What is wrong? It can be enything. Currently by default it is bitmap name. It purely documented (okay, even confusingly documented), but it was so since 4.0. And existing users obviously knows how it work (otherwise, they can't use the feature) So, I think it's OK to fix spec to directly show implementation, that was here since feature introducing. > >> Fixes: 5fcbeb06812685a2 >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> >> Hi all. >> >> This patch follows discussion on Nir's patch >> [PATCH] block: nbd: Fix dirty bitmap context name >> ( https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg04309.html ) >> >> Let's just fix qapi spec now. > > But qapi documents a better behavior for users. We should fix the code instead > to mach the docs. 1. Using disk name as a bitmap name is a bad behavior, as they are completely different concepts. Especially keeping in mind that user already knows disk name anyway and no reason to write this export name inside metadata context of this export. 2. It's not directly documented. You assume that NAME == @name. I understand that it may be assumed.. But it's not documented. 3. It's never worked like you write. So if we change the behavior, we'll break existing users. > > With this we still have the issue of leaking internal bitmap name to > users who do not > control the name, and do not care about it. > >> qapi/block.json | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/qapi/block.json b/qapi/block.json >> index 145c268bb6..8042ef78f0 100644 >> --- a/qapi/block.json >> +++ b/qapi/block.json >> @@ -255,7 +255,8 @@ >> >> # @bitmap: Also export the dirty bitmap reachable from @device, so the >> # NBD client can use NBD_OPT_SET_META_CONTEXT with >> -# "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0) >> +# "qemu:dirty-bitmap:BITMAP" to inspect the bitmap (BITMAP here >> +# matches @bitmap parameter). (since 4.0) >> # >> # Returns: error if the server is not running, or export with the same name >> # already exists. >> -- >> 2.21.0 >> >
On Thu, Dec 19, 2019 at 5:00 PM Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > > 19.12.2019 17:42, Nir Soffer wrote: > > On Thu, Dec 19, 2019 at 4:34 PM Vladimir Sementsov-Ogievskiy > > <vsementsov@virtuozzo.com> wrote: > >> > >> "NAME" here may be interpreted like it should match @name, which is > >> export name. But it was never mentioned in such way. Make it obvious, > >> that actual "<dirty-bitmap-export-name>" (see docs/interop/nbd.txt) > >> will match @bitmap parameter. > > > > But this is wrong, dirty-bitmap-export-name does not mean the actual bitmap > > name but the name exposed to the NBD client, which can be anything. > > Yes. What is wrong? It can be enything. Currently by default it is bitmap name. > It purely documented (okay, even confusingly documented), but it was so since > 4.0. And existing users obviously knows how it work (otherwise, they can't use > the feature) > > So, I think it's OK to fix spec to directly show implementation, that was here > since feature introducing. > > > > >> Fixes: 5fcbeb06812685a2 > >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > >> --- > >> > >> Hi all. > >> > >> This patch follows discussion on Nir's patch > >> [PATCH] block: nbd: Fix dirty bitmap context name > >> ( https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg04309.html ) > >> > >> Let's just fix qapi spec now. > > > > But qapi documents a better behavior for users. We should fix the code instead > > to mach the docs. > > 1. Using disk name as a bitmap name is a bad behavior, as they are completely > different concepts. Especially keeping in mind that user already knows disk name anyway > and no reason to write this export name inside metadata context of this export. The different concept is expressed by the "qemu:dirty-bitmap:" prefix. "qemu:dirty-bitmap:export-name" means the dirty bitmap for this export. > 2. It's not directly documented. You assume that NAME == @name. I understand that > it may be assumed.. But it's not documented. But NAME is likely to be understood as the name argument, and unlikely to be the bitmap name. > 3. It's never worked like you write. So if we change the behavior, we'll break > existing users. Do we have existing users? isn't this new feature in 4.2? Before we had experimental x-block-dirty-bitmap APIs, which are stable, so users could not depend on them. > > With this we still have the issue of leaking internal bitmap name to > > users who do not > > control the name, and do not care about it. > > > >> qapi/block.json | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/qapi/block.json b/qapi/block.json > >> index 145c268bb6..8042ef78f0 100644 > >> --- a/qapi/block.json > >> +++ b/qapi/block.json > >> @@ -255,7 +255,8 @@ > >> > >> # @bitmap: Also export the dirty bitmap reachable from @device, so the > >> # NBD client can use NBD_OPT_SET_META_CONTEXT with > >> -# "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0) > >> +# "qemu:dirty-bitmap:BITMAP" to inspect the bitmap (BITMAP here > >> +# matches @bitmap parameter). (since 4.0) > >> # > >> # Returns: error if the server is not running, or export with the same name > >> # already exists. > >> -- > >> 2.21.0 > >> > > > > > -- > Best regards, > Vladimir
19.12.2019 18:08, Nir Soffer wrote: > On Thu, Dec 19, 2019 at 5:00 PM Vladimir Sementsov-Ogievskiy > <vsementsov@virtuozzo.com> wrote: >> >> 19.12.2019 17:42, Nir Soffer wrote: >>> On Thu, Dec 19, 2019 at 4:34 PM Vladimir Sementsov-Ogievskiy >>> <vsementsov@virtuozzo.com> wrote: >>>> >>>> "NAME" here may be interpreted like it should match @name, which is >>>> export name. But it was never mentioned in such way. Make it obvious, >>>> that actual "<dirty-bitmap-export-name>" (see docs/interop/nbd.txt) >>>> will match @bitmap parameter. >>> >>> But this is wrong, dirty-bitmap-export-name does not mean the actual bitmap >>> name but the name exposed to the NBD client, which can be anything. >> >> Yes. What is wrong? It can be enything. Currently by default it is bitmap name. >> It purely documented (okay, even confusingly documented), but it was so since >> 4.0. And existing users obviously knows how it work (otherwise, they can't use >> the feature) >> >> So, I think it's OK to fix spec to directly show implementation, that was here >> since feature introducing. >> >>> >>>> Fixes: 5fcbeb06812685a2 >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> >>>> Hi all. >>>> >>>> This patch follows discussion on Nir's patch >>>> [PATCH] block: nbd: Fix dirty bitmap context name >>>> ( https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg04309.html ) >>>> >>>> Let's just fix qapi spec now. >>> >>> But qapi documents a better behavior for users. We should fix the code instead >>> to mach the docs. >> >> 1. Using disk name as a bitmap name is a bad behavior, as they are completely >> different concepts. Especially keeping in mind that user already knows disk name anyway >> and no reason to write this export name inside metadata context of this export. > > The different concept is expressed by the "qemu:dirty-bitmap:" prefix. > "qemu:dirty-bitmap:export-name" means the dirty bitmap for this export. Why do you think so? Did you read NBD specification? Metadata context is always owned by some export. Do you mean that there will be metadata contexts qemu:dirty-bitmap:export-A qemu:dirty-bitmap:export-B both defined for export-A? > >> 2. It's not directly documented. You assume that NAME == @name. I understand that >> it may be assumed.. But it's not documented. > > But NAME is likely to be understood as the name argument, and unlikely to be the > bitmap name. Yes likely. But it's still bad specification, which should be fixed. > >> 3. It's never worked like you write. So if we change the behavior, we'll break >> existing users. > > Do we have existing users? isn't this new feature in 4.2? No, it's since 4.0 > > Before we had experimental x-block-dirty-bitmap APIs, which are stable, so users > could not depend on them. > >>> With this we still have the issue of leaking internal bitmap name to >>> users who do not >>> control the name, and do not care about it. >>> >>>> qapi/block.json | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/qapi/block.json b/qapi/block.json >>>> index 145c268bb6..8042ef78f0 100644 >>>> --- a/qapi/block.json >>>> +++ b/qapi/block.json >>>> @@ -255,7 +255,8 @@ >>>> >>>> # @bitmap: Also export the dirty bitmap reachable from @device, so the >>>> # NBD client can use NBD_OPT_SET_META_CONTEXT with >>>> -# "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0) >>>> +# "qemu:dirty-bitmap:BITMAP" to inspect the bitmap (BITMAP here >>>> +# matches @bitmap parameter). (since 4.0) >>>> # >>>> # Returns: error if the server is not running, or export with the same name >>>> # already exists. >>>> -- >>>> 2.21.0 >>>> >>> >> >> >> -- >> Best regards, >> Vladimir >
On Thu, Dec 19, 2019 at 5:25 PM Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > > 19.12.2019 18:08, Nir Soffer wrote: > > On Thu, Dec 19, 2019 at 5:00 PM Vladimir Sementsov-Ogievskiy > > <vsementsov@virtuozzo.com> wrote: > >> > >> 19.12.2019 17:42, Nir Soffer wrote: > >>> On Thu, Dec 19, 2019 at 4:34 PM Vladimir Sementsov-Ogievskiy > >>> <vsementsov@virtuozzo.com> wrote: > >>>> > >>>> "NAME" here may be interpreted like it should match @name, which is > >>>> export name. But it was never mentioned in such way. Make it obvious, > >>>> that actual "<dirty-bitmap-export-name>" (see docs/interop/nbd.txt) > >>>> will match @bitmap parameter. > >>> > >>> But this is wrong, dirty-bitmap-export-name does not mean the actual bitmap > >>> name but the name exposed to the NBD client, which can be anything. > >> > >> Yes. What is wrong? It can be enything. Currently by default it is bitmap name. > >> It purely documented (okay, even confusingly documented), but it was so since > >> 4.0. And existing users obviously knows how it work (otherwise, they can't use > >> the feature) > >> > >> So, I think it's OK to fix spec to directly show implementation, that was here > >> since feature introducing. > >> > >>> > >>>> Fixes: 5fcbeb06812685a2 > >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > >>>> --- > >>>> > >>>> Hi all. > >>>> > >>>> This patch follows discussion on Nir's patch > >>>> [PATCH] block: nbd: Fix dirty bitmap context name > >>>> ( https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg04309.html ) > >>>> > >>>> Let's just fix qapi spec now. > >>> > >>> But qapi documents a better behavior for users. We should fix the code instead > >>> to mach the docs. > >> > >> 1. Using disk name as a bitmap name is a bad behavior, as they are completely > >> different concepts. Especially keeping in mind that user already knows disk name anyway > >> and no reason to write this export name inside metadata context of this export. > > > > The different concept is expressed by the "qemu:dirty-bitmap:" prefix. > > "qemu:dirty-bitmap:export-name" means the dirty bitmap for this export. > > Why do you think so? Did you read NBD specification? Yes - the name of the bitmap does not have any meaning. But for nbd_server_add we allow only single bitmap for export. > Metadata context is always owned by some export. Of course. > Do you mean that there will bemetadata contexts > > qemu:dirty-bitmap:export-A > qemu:dirty-bitmap:export-B > > both defined for export-A? It does not make sense, but it is valid. > >> 2. It's not directly documented. You assume that NAME == @name. I understand that > >> it may be assumed.. But it's not documented. > > > > But NAME is likely to be understood as the name argument, and unlikely to be the > > bitmap name. > > Yes likely. But it's still bad specification, which should be fixed. If we cannot change the current behavior since it will break current users, I agree fixing the spec to describe the current behavior is a good idea. > > > >> 3. It's never worked like you write. So if we change the behavior, we'll break > >> existing users. > > > > Do we have existing users? isn't this new feature in 4.2? > > No, it's since 4.0 > > > > > Before we had experimental x-block-dirty-bitmap APIs, which are stable, so users > > could not depend on them. > > > >>> With this we still have the issue of leaking internal bitmap name to > >>> users who do not > >>> control the name, and do not care about it. > >>> > >>>> qapi/block.json | 3 ++- > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/qapi/block.json b/qapi/block.json > >>>> index 145c268bb6..8042ef78f0 100644 > >>>> --- a/qapi/block.json > >>>> +++ b/qapi/block.json > >>>> @@ -255,7 +255,8 @@ > >>>> > >>>> # @bitmap: Also export the dirty bitmap reachable from @device, so the > >>>> # NBD client can use NBD_OPT_SET_META_CONTEXT with > >>>> -# "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0) > >>>> +# "qemu:dirty-bitmap:BITMAP" to inspect the bitmap (BITMAP here > >>>> +# matches @bitmap parameter). (since 4.0) > >>>> # > >>>> # Returns: error if the server is not running, or export with the same name > >>>> # already exists. > >>>> -- > >>>> 2.21.0 > >>>> > >>> > >> > >> > >> -- > >> Best regards, > >> Vladimir > > > > > -- > Best regards, > Vladimir
19.12.2019 19:14, Nir Soffer wrote: > On Thu, Dec 19, 2019 at 5:25 PM Vladimir Sementsov-Ogievskiy > <vsementsov@virtuozzo.com> wrote: >> >> 19.12.2019 18:08, Nir Soffer wrote: >>> On Thu, Dec 19, 2019 at 5:00 PM Vladimir Sementsov-Ogievskiy >>> <vsementsov@virtuozzo.com> wrote: >>>> >>>> 19.12.2019 17:42, Nir Soffer wrote: >>>>> On Thu, Dec 19, 2019 at 4:34 PM Vladimir Sementsov-Ogievskiy >>>>> <vsementsov@virtuozzo.com> wrote: >>>>>> >>>>>> "NAME" here may be interpreted like it should match @name, which is >>>>>> export name. But it was never mentioned in such way. Make it obvious, >>>>>> that actual "<dirty-bitmap-export-name>" (see docs/interop/nbd.txt) >>>>>> will match @bitmap parameter. >>>>> >>>>> But this is wrong, dirty-bitmap-export-name does not mean the actual bitmap >>>>> name but the name exposed to the NBD client, which can be anything. >>>> >>>> Yes. What is wrong? It can be enything. Currently by default it is bitmap name. >>>> It purely documented (okay, even confusingly documented), but it was so since >>>> 4.0. And existing users obviously knows how it work (otherwise, they can't use >>>> the feature) >>>> >>>> So, I think it's OK to fix spec to directly show implementation, that was here >>>> since feature introducing. >>>> >>>>> >>>>>> Fixes: 5fcbeb06812685a2 >>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>>>> --- >>>>>> >>>>>> Hi all. >>>>>> >>>>>> This patch follows discussion on Nir's patch >>>>>> [PATCH] block: nbd: Fix dirty bitmap context name >>>>>> ( https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg04309.html ) >>>>>> >>>>>> Let's just fix qapi spec now. >>>>> >>>>> But qapi documents a better behavior for users. We should fix the code instead >>>>> to mach the docs. >>>> >>>> 1. Using disk name as a bitmap name is a bad behavior, as they are completely >>>> different concepts. Especially keeping in mind that user already knows disk name anyway >>>> and no reason to write this export name inside metadata context of this export. >>> >>> The different concept is expressed by the "qemu:dirty-bitmap:" prefix. >>> "qemu:dirty-bitmap:export-name" means the dirty bitmap for this export. >> >> Why do you think so? Did you read NBD specification? > > Yes - the name of the bitmap does not have any meaning. it does not have any meaning in your scenario. But in some other, when we need to export several bitmaps, the name will be used. > But for nbd_server_add we allow only single bitmap for export. Yes, Qemu just don't use the whole power of NBD spec. But it may change. > >> Metadata context is always owned by some export. > > Of course. > >> Do you mean that there will bemetadata contexts >> >> qemu:dirty-bitmap:export-A >> qemu:dirty-bitmap:export-B >> >> both defined for export-A? > > It does not make sense, but it is valid. It conflicts with the fact that both contexts are owned by export-A. The idea was that metadata is bound to the export. We should not see metadata of export B in metadata of export A. Moreover, consider that these two exports may have different size. Then how NBD_CMD_BLOCK_STATUS will work? We don't have global nbd server metadata, like you consider it. We only have per-export metadata, which is bound (at least by size) to the export. So it's theoretically possible to export bitmap of another nbd-export (if it has the same size), but it breaks the concept. > >>>> 2. It's not directly documented. You assume that NAME == @name. I understand that >>>> it may be assumed.. But it's not documented. >>> >>> But NAME is likely to be understood as the name argument, and unlikely to be the >>> bitmap name. >> >> Yes likely. But it's still bad specification, which should be fixed. > > If we cannot change the current behavior since it will break current users, > I agree fixing the spec to describe the current behavior is a good idea. > > > > >>> >>>> 3. It's never worked like you write. So if we change the behavior, we'll break >>>> existing users. >>> >>> Do we have existing users? isn't this new feature in 4.2? >> >> No, it's since 4.0 >> >>> >>> Before we had experimental x-block-dirty-bitmap APIs, which are stable, so users >>> could not depend on them. >>> >>>>> With this we still have the issue of leaking internal bitmap name to >>>>> users who do not >>>>> control the name, and do not care about it. >>>>> >>>>>> qapi/block.json | 3 ++- >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/qapi/block.json b/qapi/block.json >>>>>> index 145c268bb6..8042ef78f0 100644 >>>>>> --- a/qapi/block.json >>>>>> +++ b/qapi/block.json >>>>>> @@ -255,7 +255,8 @@ >>>>>> >>>>>> # @bitmap: Also export the dirty bitmap reachable from @device, so the >>>>>> # NBD client can use NBD_OPT_SET_META_CONTEXT with >>>>>> -# "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0) >>>>>> +# "qemu:dirty-bitmap:BITMAP" to inspect the bitmap (BITMAP here >>>>>> +# matches @bitmap parameter). (since 4.0) >>>>>> # >>>>>> # Returns: error if the server is not running, or export with the same name >>>>>> # already exists. >>>>>> -- >>>>>> 2.21.0 >>>>>> >>>>> >>>> >>>> >>>> -- >>>> Best regards, >>>> Vladimir >>> >> >> >> -- >> Best regards, >> Vladimir >
On 12/19/19 9:08 AM, Nir Soffer wrote: >>>> Let's just fix qapi spec now. >>> >>> But qapi documents a better behavior for users. We should fix the code instead >>> to mach the docs. >> >> 1. Using disk name as a bitmap name is a bad behavior, as they are completely >> different concepts. Especially keeping in mind that user already knows disk name anyway >> and no reason to write this export name inside metadata context of this export. > > The different concept is expressed by the "qemu:dirty-bitmap:" prefix. > "qemu:dirty-bitmap:export-name" means the dirty bitmap for this export. > >> 2. It's not directly documented. You assume that NAME == @name. I understand that >> it may be assumed.. But it's not documented. > > But NAME is likely to be understood as the name argument, and unlikely to be the > bitmap name. That's a misunderstanding due to poor docs. The bitmap name has always been what was exposed, ever since we promoted things to stable by getting rid of x-. > >> 3. It's never worked like you write. So if we change the behavior, we'll break >> existing users. > > Do we have existing users? isn't this new feature in 4.2? No, the feature stems back to 4.0, when we got rid of x-. There are other reasons that dirty bitmaps aren't really usable for incremental backups without qemu 4.2, but qemu 4.0 was the first time we exposed a stable interface for a bitmap over an NBD export, and that release used the bitmap name (and not the export name), so at this point, a code change would break expectations of any 4.0 client using bitmaps for other reasons. Libvirt currently has absolute control over the bitmap name (my initial code in libvirt created a temporary bitmap with my desired name, then merged the contents from the permanent bitmaps corresponding to the actual libvirt Checkpoint objects into the temporary, so that it could then call nbd-export-add with the temporary bitmap name). But, as you point out... > > Before we had experimental x-block-dirty-bitmap APIs, which are stable, so users > could not depend on them. The unstable x-block-dirty-bitmap APIs _did_ have a way to export a user-controlled name SEPARATE from the bitmap name. At the time I was removing the x- prefix, I asked if anyone had a strong use case for keeping that functionality. No one spoke up in favor of keeping it (Nikolay mentioned using the old interface, but not being stumped by its removal), so we nuked it at the time. We can always add it back (now that it sounds like you have a use case where it may be more compelling), but it was easier to stabilize less and add more later as needed, than to stabilize too much and regret that we had to support the flexibility that no one needed. https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg02373.html https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg01970.html
On 12/19/19 10:14 AM, Nir Soffer wrote: >>>> 1. Using disk name as a bitmap name is a bad behavior, as they are completely >>>> different concepts. Especially keeping in mind that user already knows disk name anyway >>>> and no reason to write this export name inside metadata context of this export. >>> >>> The different concept is expressed by the "qemu:dirty-bitmap:" prefix. >>> "qemu:dirty-bitmap:export-name" means the dirty bitmap for this export. >> >> Why do you think so? Did you read NBD specification? > > Yes - the name of the bitmap does not have any meaning. > But for nbd_server_add we allow only single bitmap for export. Just because qemu is currently limited to only exposing one bitmap at the moment does not mean that a future version can't expose multiple bitmaps. It may very well be that we have reason to expose both "qemu:dirty-bitmap:timeA" and "qemu:dirty-bitmap:timeB" on the same export, for exposing two bitmaps at once. To get to that point, we'd have to refactor the QAPI command to allow attaching more than one bitmap at the time of creating the NBD export, but it's not impossible. > >> Metadata context is always owned by some export. > > Of course. > >> Do you mean that there will bemetadata contexts >> >> qemu:dirty-bitmap:export-A >> qemu:dirty-bitmap:export-B >> >> both defined for export-A? > > It does not make sense, but it is valid. If an image has multiple bitmaps, exposing all of those as separate contexts at the same time for a single export can indeed make sense. > >>>> 2. It's not directly documented. You assume that NAME == @name. I understand that >>>> it may be assumed.. But it's not documented. >>> >>> But NAME is likely to be understood as the name argument, and unlikely to be the >>> bitmap name. >> >> Yes likely. But it's still bad specification, which should be fixed. > > If we cannot change the current behavior since it will break current users, > I agree fixing the spec to describe the current behavior is a good idea. We need the doc fix. Whether we also want an additional fix adding an optional parameter allowing user control over the export name is also under debate (the fact that the old x-nbd-server-add-bitmap supported it shows that it may be useful, but it is not minimal, and as I pointed out at the time of removing x-, libvirt can always control what name is exposed by creating a temporary bitmap and merging from other bitmaps into the temporary). We also have to think about a future of parallel backup jobs: libvirt can create a single temporary bitmap to expose whatever name it wants under one job, but if libvirt wants to expose the SAME user-visible name to two parallel jobs, it cannot create two bitmaps with the same name, so having a way to set the user-visible name of an arbitrary bitmap when producing the NBD export makes sense on that front. >>> >>>> 3. It's never worked like you write. So if we change the behavior, we'll break >>>> existing users. >>> >>> Do we have existing users? isn't this new feature in 4.2? >> >> No, it's since 4.0 As long as altering the exported name is controlled by a new optional parameter, it does not hurt older 4.0 clients that do not use the new parameter.
21.12.2019 1:04, Eric Blake wrote: > On 12/19/19 10:14 AM, Nir Soffer wrote: > >>>>> 1. Using disk name as a bitmap name is a bad behavior, as they are completely >>>>> different concepts. Especially keeping in mind that user already knows disk name anyway >>>>> and no reason to write this export name inside metadata context of this export. >>>> >>>> The different concept is expressed by the "qemu:dirty-bitmap:" prefix. >>>> "qemu:dirty-bitmap:export-name" means the dirty bitmap for this export. >>> >>> Why do you think so? Did you read NBD specification? >> >> Yes - the name of the bitmap does not have any meaning. >> But for nbd_server_add we allow only single bitmap for export. > > Just because qemu is currently limited to only exposing one bitmap at the moment does not mean that a future version can't expose multiple bitmaps. It may very well be that we have reason to expose both "qemu:dirty-bitmap:timeA" and "qemu:dirty-bitmap:timeB" on the same export, for exposing two bitmaps at once. To get to that point, we'd have to refactor the QAPI command to allow attaching more than one bitmap at the time of creating the NBD export, but it's not impossible. > >> >>> Metadata context is always owned by some export. >> >> Of course. >> >>> Do you mean that there will bemetadata contexts >>> >>> qemu:dirty-bitmap:export-A >>> qemu:dirty-bitmap:export-B >>> >>> both defined for export-A? >> >> It does not make sense, but it is valid. > > If an image has multiple bitmaps, exposing all of those as separate contexts at the same time for a single export can indeed make sense. > >> >>>>> 2. It's not directly documented. You assume that NAME == @name. I understand that >>>>> it may be assumed.. But it's not documented. >>>> >>>> But NAME is likely to be understood as the name argument, and unlikely to be the >>>> bitmap name. >>> >>> Yes likely. But it's still bad specification, which should be fixed. >> >> If we cannot change the current behavior since it will break current users, >> I agree fixing the spec to describe the current behavior is a good idea. > > We need the doc fix. Do you like the patch starting this thread? It's a fix we are talking about.. > Whether we also want an additional fix adding an optional parameter allowing user control over the export name is also under debate (the fact that the old x-nbd-server-add-bitmap supported it shows that it may be useful, but it is not minimal, and as I pointed out at the time of removing x-, libvirt can always control what name is exposed by creating a temporary bitmap and merging from other bitmaps into the temporary). > > We also have to think about a future of parallel backup jobs: libvirt can create a single temporary bitmap to expose whatever name it wants under one job, but if libvirt wants to expose the SAME user-visible name to two parallel jobs, it cannot create two bitmaps with the same name, so having a way to set the user-visible name of an arbitrary bitmap when producing the NBD export makes sense on that front. > > >>>> >>>>> 3. It's never worked like you write. So if we change the behavior, we'll break >>>>> existing users. >>>> >>>> Do we have existing users? isn't this new feature in 4.2? >>> >>> No, it's since 4.0 > > As long as altering the exported name is controlled by a new optional parameter, it does not hurt older 4.0 clients that do not use the new parameter. >
diff --git a/qapi/block.json b/qapi/block.json index 145c268bb6..8042ef78f0 100644 --- a/qapi/block.json +++ b/qapi/block.json @@ -255,7 +255,8 @@ # @bitmap: Also export the dirty bitmap reachable from @device, so the # NBD client can use NBD_OPT_SET_META_CONTEXT with -# "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0) +# "qemu:dirty-bitmap:BITMAP" to inspect the bitmap (BITMAP here +# matches @bitmap parameter). (since 4.0) # # Returns: error if the server is not running, or export with the same name # already exists.
"NAME" here may be interpreted like it should match @name, which is export name. But it was never mentioned in such way. Make it obvious, that actual "<dirty-bitmap-export-name>" (see docs/interop/nbd.txt) will match @bitmap parameter. Fixes: 5fcbeb06812685a2 Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- Hi all. This patch follows discussion on Nir's patch [PATCH] block: nbd: Fix dirty bitmap context name ( https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg04309.html ) Let's just fix qapi spec now. qapi/block.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)