Message ID | 1461706338-20219-7-git-send-email-mreitz@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Am 26.04.2016 um 23:32 hat Max Reitz geschrieben: > In order to allow block drivers to use that function, it needs to be > public. In order to be useful, it needs to take a parameter which allows > the caller to specify whether the runtime options allowed by the block > driver are actually significant for the guest-visible BDS content. > > Signed-off-by: Max Reitz <mreitz@redhat.com> Is this actually good enough? I expect that many drivers will have some options that are significant and other options that aren't. We already have some (Quorum: children are significant, rewrite-corrupted isn't), but as we convert more things to proper options, we'll get more of them (raw-posix: filename is significant, aio=native isn't). We might actually need to pass a list of significant fields instead that append_open_options() can use. Kevin > block.c | 9 ++++++--- > include/block/block_int.h | 3 +++ > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/block.c b/block.c > index 511a0b1..b3ff08f 100644 > --- a/block.c > +++ b/block.c > @@ -3872,7 +3872,10 @@ static bool append_open_options(QDict *d, BlockDriverState *bs) > return found_any; > } > > -static void bdrv_default_refresh_format_filename(BlockDriverState *bs) > +/* @options_significant must be true if any of the driver-specific runtime > + * options may change the guest-visible content of the BDS */ > +void bdrv_default_refresh_format_filename(BlockDriverState *bs, > + bool options_significant) > { > BlockDriver *drv = bs->drv; > QDict *opts; > @@ -3885,7 +3888,7 @@ static void bdrv_default_refresh_format_filename(BlockDriverState *bs) > } > > opts = qdict_new(); > - has_open_options = append_open_options(opts, bs); > + has_open_options = append_open_options(opts, bs) && options_significant; > > has_open_options |= bs->backing_overridden; > > /* If no specific options have been given for this BDS, the filename of > @@ -3997,7 +4000,7 @@ void bdrv_refresh_filename(BlockDriverState *bs) > QDECREF(opts); > } else if (bs->file) { > /* Try to reconstruct valid information from the underlying file */ > - bdrv_default_refresh_format_filename(bs); > + bdrv_default_refresh_format_filename(bs, true); > } else if (!bs->full_open_options && qdict_size(bs->options)) { > bdrv_default_refresh_protocol_filename(bs); > } > diff --git a/include/block/block_int.h b/include/block/block_int.h > index d73e9ce..eb3665a 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -532,6 +532,9 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size, > void bdrv_set_io_limits(BlockDriverState *bs, > ThrottleConfig *cfg); > > +void bdrv_default_refresh_format_filename(BlockDriverState *bs, > + bool options_significant); > + > > /** > * bdrv_add_before_write_notifier: > -- > 2.8.0 >
On 02.05.2016 17:36, Kevin Wolf wrote: > Am 26.04.2016 um 23:32 hat Max Reitz geschrieben: >> In order to allow block drivers to use that function, it needs to be >> public. In order to be useful, it needs to take a parameter which allows >> the caller to specify whether the runtime options allowed by the block >> driver are actually significant for the guest-visible BDS content. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> > > Is this actually good enough? I expect that many drivers will have some > options that are significant and other options that aren't. We already > have some (Quorum: children are significant, rewrite-corrupted isn't), > but as we convert more things to proper options, we'll get more of them > (raw-posix: filename is significant, aio=native isn't). > > We might actually need to pass a list of significant fields instead that > append_open_options() can use. Well, in theory, every driver with insignificant options would just implement .bdrv_refresh_filename() however it's needed. Making bdrv_default_refresh_format_filename() function public is just a way of keeping that implementation very simple for some drivers that only have insignificant options. I'm not opposed to extending this function in the future when it actually makes sense. Right now I don't think it does. The only thing that changes if a significant option is detected is that no plain filename is generated; however, for Quorum we can never generate such a filename. Therefore, we cannot use this function for Quorum anyway. However, instead of extending this function, it may make more sense then to introduce a new field to the BlockDriver struct which is a NULL-terminated array of significant option names, or something like that. If .bdrv_refresh_filename is NULL but that array pointer is not, then the default implementation could behave accordingly. But this is something I'd defer to the future, too, unless you can point out a current block driver that would benefit from this functionality (I don't think Quorum does, as I said above). Max
Am 03.05.2016 um 13:19 hat Max Reitz geschrieben: > On 02.05.2016 17:36, Kevin Wolf wrote: > > Am 26.04.2016 um 23:32 hat Max Reitz geschrieben: > >> In order to allow block drivers to use that function, it needs to be > >> public. In order to be useful, it needs to take a parameter which allows > >> the caller to specify whether the runtime options allowed by the block > >> driver are actually significant for the guest-visible BDS content. > >> > >> Signed-off-by: Max Reitz <mreitz@redhat.com> > > > > Is this actually good enough? I expect that many drivers will have some > > options that are significant and other options that aren't. We already > > have some (Quorum: children are significant, rewrite-corrupted isn't), > > but as we convert more things to proper options, we'll get more of them > > (raw-posix: filename is significant, aio=native isn't). > > > > We might actually need to pass a list of significant fields instead that > > append_open_options() can use. > > Well, in theory, every driver with insignificant options would just > implement .bdrv_refresh_filename() however it's needed. Making > bdrv_default_refresh_format_filename() function public is just a way of > keeping that implementation very simple for some drivers that only have > insignificant options. > > I'm not opposed to extending this function in the future when it > actually makes sense. Right now I don't think it does. The only thing > that changes if a significant option is detected is that no plain > filename is generated; however, for Quorum we can never generate such a > filename. Therefore, we cannot use this function for Quorum anyway. If you integrate it into append_open_options(), I suppose it would also mean that insignificant options are dropped from the json: description, i.e. Quorum would return a json: object with all children, but not the rewrite-corrupted setting. Which I think would be a good thing. Kevin > However, instead of extending this function, it may make more sense then > to introduce a new field to the BlockDriver struct which is a > NULL-terminated array of significant option names, or something like > that. If .bdrv_refresh_filename is NULL but that array pointer is not, > then the default implementation could behave accordingly. But this is > something I'd defer to the future, too, unless you can point out a > current block driver that would benefit from this functionality (I don't > think Quorum does, as I said above). > > Max
On 03.05.2016 15:31, Kevin Wolf wrote: > Am 03.05.2016 um 13:19 hat Max Reitz geschrieben: >> On 02.05.2016 17:36, Kevin Wolf wrote: >>> Am 26.04.2016 um 23:32 hat Max Reitz geschrieben: >>>> In order to allow block drivers to use that function, it needs to be >>>> public. In order to be useful, it needs to take a parameter which allows >>>> the caller to specify whether the runtime options allowed by the block >>>> driver are actually significant for the guest-visible BDS content. >>>> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>> >>> Is this actually good enough? I expect that many drivers will have some >>> options that are significant and other options that aren't. We already >>> have some (Quorum: children are significant, rewrite-corrupted isn't), >>> but as we convert more things to proper options, we'll get more of them >>> (raw-posix: filename is significant, aio=native isn't). >>> >>> We might actually need to pass a list of significant fields instead that >>> append_open_options() can use. >> >> Well, in theory, every driver with insignificant options would just >> implement .bdrv_refresh_filename() however it's needed. Making >> bdrv_default_refresh_format_filename() function public is just a way of >> keeping that implementation very simple for some drivers that only have >> insignificant options. >> >> I'm not opposed to extending this function in the future when it >> actually makes sense. Right now I don't think it does. The only thing >> that changes if a significant option is detected is that no plain >> filename is generated; however, for Quorum we can never generate such a >> filename. Therefore, we cannot use this function for Quorum anyway. > > If you integrate it into append_open_options(), I suppose it would also > mean that insignificant options are dropped from the json: description, > i.e. Quorum would return a json: object with all children, but not the > rewrite-corrupted setting. Which I think would be a good thing. I'm not sure I do. :-) At least right now the JSON version is supposed to contain all options, be they significant or not. Let me try to remember my reasoning: Ideally, we want to get a filename which *exactly* results in the same BDS that we have. This should always be possible if instead of a plain filename one specifies options, e.g. using a JSON filename. However, such a JSON filename (or giving options using the dot syntax or as JSON with blockdev-add) is cumbersome. In many simple cases, we can (re-)construct a plain filename which yields exactly the same BDS, though. That's nice so that's what we try to do first. In some cases, it is impossible to construct a plain filename which yields a BDS that will return the same data when accessed. Then, we just cannot give such a filename and have to stick to a JSON filename, there's no way around this. However, sometimes we are in a gray area. We can construct a plain filename which yields a slightly different BDS than the one we have; but it will return the same data when accessed and thus it is "close enough". We then have to make a tradeoff between getting exactly the same BDS and having a nice and simple filename. I opted for the latter. However, if we do have to emit a JSON filename at some point in the tree I think we've basically "lost" already. If we get to that point, we may as well just emit all the options that have been used to construct the BDS, even if they don't change the data it yields. Max
Am 03.05.2016 um 15:48 hat Max Reitz geschrieben: > On 03.05.2016 15:31, Kevin Wolf wrote: > > Am 03.05.2016 um 13:19 hat Max Reitz geschrieben: > >> On 02.05.2016 17:36, Kevin Wolf wrote: > >>> Am 26.04.2016 um 23:32 hat Max Reitz geschrieben: > >>>> In order to allow block drivers to use that function, it needs to be > >>>> public. In order to be useful, it needs to take a parameter which allows > >>>> the caller to specify whether the runtime options allowed by the block > >>>> driver are actually significant for the guest-visible BDS content. > >>>> > >>>> Signed-off-by: Max Reitz <mreitz@redhat.com> > >>> > >>> Is this actually good enough? I expect that many drivers will have some > >>> options that are significant and other options that aren't. We already > >>> have some (Quorum: children are significant, rewrite-corrupted isn't), > >>> but as we convert more things to proper options, we'll get more of them > >>> (raw-posix: filename is significant, aio=native isn't). > >>> > >>> We might actually need to pass a list of significant fields instead that > >>> append_open_options() can use. > >> > >> Well, in theory, every driver with insignificant options would just > >> implement .bdrv_refresh_filename() however it's needed. Making > >> bdrv_default_refresh_format_filename() function public is just a way of > >> keeping that implementation very simple for some drivers that only have > >> insignificant options. > >> > >> I'm not opposed to extending this function in the future when it > >> actually makes sense. Right now I don't think it does. The only thing > >> that changes if a significant option is detected is that no plain > >> filename is generated; however, for Quorum we can never generate such a > >> filename. Therefore, we cannot use this function for Quorum anyway. > > > > If you integrate it into append_open_options(), I suppose it would also > > mean that insignificant options are dropped from the json: description, > > i.e. Quorum would return a json: object with all children, but not the > > rewrite-corrupted setting. Which I think would be a good thing. > > I'm not sure I do. :-) > > At least right now the JSON version is supposed to contain all options, > be they significant or not. Let me try to remember my reasoning: > > Ideally, we want to get a filename which *exactly* results in the same > BDS that we have. Here I'm not sure we do. :-) We already don't do this. We filter out any options that are parsed by the block layer. For example, we don't include the node name or caching options. If we really wanted to represend the BDS as exactly as we can, this wouldn't be right and we'd have to fix it. But as I see it, what we were really after when we implemented things this way was that we distinguish options that are conceptually part of some address that points to the image data (which I thought matches your "significant" options) and other options that just influence our access patterns and what we do with the image at this address. The filename (json: or not) consists then only of the address part, as the other options can differ between qemu invocations without actually changing which image we see. I don't expect something called a filename (json: exists just because a plain filename can't represent everything) to contain various runtime configuration settings, but just a pure pointer to the image. > This should always be possible if instead of a plain > filename one specifies options, e.g. using a JSON filename. However, > such a JSON filename (or giving options using the dot syntax or as JSON > with blockdev-add) is cumbersome. > > In many simple cases, we can (re-)construct a plain filename which > yields exactly the same BDS, though. That's nice so that's what we try > to do first. > > In some cases, it is impossible to construct a plain filename which > yields a BDS that will return the same data when accessed. Then, we just > cannot give such a filename and have to stick to a JSON filename, > there's no way around this. > > However, sometimes we are in a gray area. We can construct a plain > filename which yields a slightly different BDS than the one we have; but > it will return the same data when accessed and thus it is "close > enough". We then have to make a tradeoff between getting exactly the > same BDS and having a nice and simple filename. I opted for the latter. I can imagine that there are use cases for some mechanism to return the JSON object that creates exactly the same BDS, like you seem to be envisioning here. I just doubt that it's useful in those cases where we really wanted a filename and have to go for JSON because we can't do anything more user friendly. > However, if we do have to emit a JSON filename at some point in the tree > I think we've basically "lost" already. If we get to that point, we may > as well just emit all the options that have been used to construct the > BDS, even if they don't change the data it yields. In places where you want a filename (which is mostly, if not exclusively, messages for the user), emitting everything may just make an already unfriendly message even worse. Kevin
On 03.05.2016 16:34, Kevin Wolf wrote: > Am 03.05.2016 um 15:48 hat Max Reitz geschrieben: >> On 03.05.2016 15:31, Kevin Wolf wrote: >>> Am 03.05.2016 um 13:19 hat Max Reitz geschrieben: >>>> On 02.05.2016 17:36, Kevin Wolf wrote: >>>>> Am 26.04.2016 um 23:32 hat Max Reitz geschrieben: >>>>>> In order to allow block drivers to use that function, it needs to be >>>>>> public. In order to be useful, it needs to take a parameter which allows >>>>>> the caller to specify whether the runtime options allowed by the block >>>>>> driver are actually significant for the guest-visible BDS content. >>>>>> >>>>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>>> >>>>> Is this actually good enough? I expect that many drivers will have some >>>>> options that are significant and other options that aren't. We already >>>>> have some (Quorum: children are significant, rewrite-corrupted isn't), >>>>> but as we convert more things to proper options, we'll get more of them >>>>> (raw-posix: filename is significant, aio=native isn't). >>>>> >>>>> We might actually need to pass a list of significant fields instead that >>>>> append_open_options() can use. >>>> >>>> Well, in theory, every driver with insignificant options would just >>>> implement .bdrv_refresh_filename() however it's needed. Making >>>> bdrv_default_refresh_format_filename() function public is just a way of >>>> keeping that implementation very simple for some drivers that only have >>>> insignificant options. >>>> >>>> I'm not opposed to extending this function in the future when it >>>> actually makes sense. Right now I don't think it does. The only thing >>>> that changes if a significant option is detected is that no plain >>>> filename is generated; however, for Quorum we can never generate such a >>>> filename. Therefore, we cannot use this function for Quorum anyway. >>> >>> If you integrate it into append_open_options(), I suppose it would also >>> mean that insignificant options are dropped from the json: description, >>> i.e. Quorum would return a json: object with all children, but not the >>> rewrite-corrupted setting. Which I think would be a good thing. >> >> I'm not sure I do. :-) >> >> At least right now the JSON version is supposed to contain all options, >> be they significant or not. Let me try to remember my reasoning: >> >> Ideally, we want to get a filename which *exactly* results in the same >> BDS that we have. > > Here I'm not sure we do. :-) > > We already don't do this. We filter out any options that are parsed by > the block layer. For example, we don't include the node name or caching > options. If we really wanted to represend the BDS as exactly as we can, > this wouldn't be right and we'd have to fix it. > > But as I see it, what we were really after when we implemented things > this way was that we distinguish options that are conceptually part of > some address that points to the image data (which I thought matches your > "significant" options) and other options that just influence our access > patterns and what we do with the image at this address. > > The filename (json: or not) consists then only of the address part, as > the other options can differ between qemu invocations without actually > changing which image we see. I don't expect something called a filename > (json: exists just because a plain filename can't represent everything) > to contain various runtime configuration settings, but just a pure > pointer to the image. > >> This should always be possible if instead of a plain >> filename one specifies options, e.g. using a JSON filename. However, >> such a JSON filename (or giving options using the dot syntax or as JSON >> with blockdev-add) is cumbersome. >> >> In many simple cases, we can (re-)construct a plain filename which >> yields exactly the same BDS, though. That's nice so that's what we try >> to do first. >> >> In some cases, it is impossible to construct a plain filename which >> yields a BDS that will return the same data when accessed. Then, we just >> cannot give such a filename and have to stick to a JSON filename, >> there's no way around this. >> >> However, sometimes we are in a gray area. We can construct a plain >> filename which yields a slightly different BDS than the one we have; but >> it will return the same data when accessed and thus it is "close >> enough". We then have to make a tradeoff between getting exactly the >> same BDS and having a nice and simple filename. I opted for the latter. > > I can imagine that there are use cases for some mechanism to return the > JSON object that creates exactly the same BDS, like you seem to be > envisioning here. I just doubt that it's useful in those cases where we > really wanted a filename and have to go for JSON because we can't do > anything more user friendly. > >> However, if we do have to emit a JSON filename at some point in the tree >> I think we've basically "lost" already. If we get to that point, we may >> as well just emit all the options that have been used to construct the >> BDS, even if they don't change the data it yields. > > In places where you want a filename (which is mostly, if not > exclusively, messages for the user), emitting everything may just make > an already unfriendly message even worse. Well, I don't particularly care either way. Thus, I'd be fine with removing insignificant options even from full_open_options if you deem that useful, but that is something we don't do already so we'd have to implement it. I guess I can implement it in this series if I decide to address the issue you originally raised here ("Is this good enough?"). If it gets too much, I'd rather handle it in a follow-up, though. Max
diff --git a/block.c b/block.c index 511a0b1..b3ff08f 100644 --- a/block.c +++ b/block.c @@ -3872,7 +3872,10 @@ static bool append_open_options(QDict *d, BlockDriverState *bs) return found_any; } -static void bdrv_default_refresh_format_filename(BlockDriverState *bs) +/* @options_significant must be true if any of the driver-specific runtime + * options may change the guest-visible content of the BDS */ +void bdrv_default_refresh_format_filename(BlockDriverState *bs, + bool options_significant) { BlockDriver *drv = bs->drv; QDict *opts; @@ -3885,7 +3888,7 @@ static void bdrv_default_refresh_format_filename(BlockDriverState *bs) } opts = qdict_new(); - has_open_options = append_open_options(opts, bs); + has_open_options = append_open_options(opts, bs) && options_significant; has_open_options |= bs->backing_overridden; /* If no specific options have been given for this BDS, the filename of @@ -3997,7 +4000,7 @@ void bdrv_refresh_filename(BlockDriverState *bs) QDECREF(opts); } else if (bs->file) { /* Try to reconstruct valid information from the underlying file */ - bdrv_default_refresh_format_filename(bs); + bdrv_default_refresh_format_filename(bs, true); } else if (!bs->full_open_options && qdict_size(bs->options)) { bdrv_default_refresh_protocol_filename(bs); } diff --git a/include/block/block_int.h b/include/block/block_int.h index d73e9ce..eb3665a 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -532,6 +532,9 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size, void bdrv_set_io_limits(BlockDriverState *bs, ThrottleConfig *cfg); +void bdrv_default_refresh_format_filename(BlockDriverState *bs, + bool options_significant); + /** * bdrv_add_before_write_notifier:
In order to allow block drivers to use that function, it needs to be public. In order to be useful, it needs to take a parameter which allows the caller to specify whether the runtime options allowed by the block driver are actually significant for the guest-visible BDS content. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 9 ++++++--- include/block/block_int.h | 3 +++ 2 files changed, 9 insertions(+), 3 deletions(-)