Re: [Qemu-devel] [PATCH 1/6] block: Add bdrv_refresh_filename()

[Kevin Wolf]

Am 20.08.2014 um 21:06 hat Max Reitz geschrieben:
> On 20.08.2014 17:07, Kevin Wolf wrote:
> >Am 18.07.2014 um 20:24 hat Max Reitz geschrieben:
> >>Some block devices may not have a filename in their BDS; and for some,
> >>there may not even be a normal filename at all. To work around this, add
> >>a function which tries to construct a valid filename for the
> >>BDS.filename field.
> >>
> >>If a filename exists or a block driver is able to reconstruct a valid
> >>filename (which is placed in BDS.exact_filename), this can directly be
> >>used.
> >>
> >>If no filename can be constructed, we can still construct an options
> >>QDict which is then converted to a JSON object and prefixed with the
> >>"json:" pseudo protocol prefix. The QDict is placed in
> >>BDS.full_open_options.
> >>
> >>For most block drivers, this process can be done automatically; those
> >>that need special handling may define a .bdrv_refresh_filename() method
> >>to fill BDS.exact_filename and BDS.full_open_options themselves.
> >>
> >>Signed-off-by: Max Reitz <address@hidden>
> >>---
> >>In this version, bdrv_refresh_filename() leaves the filename unmodified
> >>if neither a new filename nor an options QDict can be generated. Another
> >>idea would be to clear the filename in this case as it is probably
> >>obsolete then. I was not sure which to pick, so I just used the first
> >>version I wrote.
> >To be honest, many things in this patch don't feel quite right. This
> >isn't necessarily your fault, I can imagine that the infrastructure is
> >just lacking the right properties for you to use.
> >
> >My hope is that soon bs->options would be the only BDS field keeping
> >configuration information and that bs->filename would go away. Now with
> >this patch series we get both of them duplicated instead. I'm not quite
> >sure if this is progress, but it may still be an acceptable intermediate
> >step.
> 
> This code just needs some way to cache the filename and I thought it
> fine to use the filename field for that purpose. If we remove it,
> we'll have to reconstruct it every time (recursively through all the
> BDS layers) when someone needs it.

Hm. Thinking more about it, the part that I really dislike isn't even
that bs->filename exists and is used as a cache. It's just that the
filename argument of bdrv_open() is used to initialise it instead of
only using the options QDict. But that's an independent problem that
isn't made worse by this series.

I guess it's time to dig out the next part of my bdrv_open() series and
complete that work...

> We may be able to cull many such places (where the filename is
> needed), but considering the processing time, I don't think it will
> get any better than using a cache array (such as BDS.filename).
> 
> So for me, the advantages of dumping BDS.filename would be: We don't
> have to consider when the filename field needs to an update; we save
> some memory (negligible, imho).
> 
> The advantages of keeping it, on the other hand, are: It's easy to
> read the filename (no need to change existing code); we may save
> some processing time (probably also negligible, if done right).
> 
> After considering this, I guess we'd have to look at all the places
> which use BDS.filename, how many of those we can cull and how often
> the rest is invoked. If BDS.filename is only rarely really needed,
> we can really remove it and fully replace it by a callback (which
> basically is bdrv_refresh_filename()).

Most of it is probably in monitor command implementations.

Kevin