Re: [Qemu-devel] [RFC 1/3] block: Add ImageRotationalInfo

[Kevin Wolf]

Am 31.05.2019 um 13:51 hat Max Reitz geschrieben:
> On 30.05.19 00:10, Kevin Wolf wrote:
> > Am 24.05.2019 um 19:28 hat Max Reitz geschrieben:
> >> This enum indicates whether a file is stored on a rotating disk or a
> >> solid-state drive.  Drivers report it via the .bdrv_get_info() callback,
> >> and if they do not, the global bdrv_get_info() implementation
> >> automatically takes it from bs->file or bs->backing, if available.
> > 
> > Good that you wrote "bs->file or bs->backing" explicitly. Otherwise, I
> > might have missed that it begs one big question: What is the correct
> > answer for a qcow2 file that has bs->file on an SSD, but bs->backing on
> > a rotating disk?
> > 
> > I don't think there is a correct answer for the whole device, so maybe
> > this information shouldn't be per device in BlockDriverInfo, but per
> > block in bdrv_co_block_status() (optionally determined if the caller
> > requests it)?
> 
> I think that’s taking it a bit too far.  There is no heavy implication
> in making the wrong choice here, it’s just a performance problem.
> Having to call block_status for every block where we want to know what
> to do seems like the opposite of performance optimization to me.  (We
> could add a flag to block_status to only query that status, but that
> sounds plainly wrong.)
> 
> So, in this series I decided that since all writes go to bs->file, that
> seemed like what mostly determines the behavior of @bs.  (After my “Deal
> with filters” series, that would become a decision of bdrv_storage_bs()
> vs. bdrv_filtered_cow_bs().)

Okay, if we consider the existing qcow2 case as the only case, writes
are what matters. Then we can ignore backing files. (However, it
shouldn't check bs->file, but s->data_file, I think.)

> (Note that it has to get even funnier with vmdk, if your extents are on
> an HDD, but your descriptor file is on an SSD.  I don’t care too much
> about vmdk’s performance, though.)
> 
> In my v1, I’ll add a per-node @rotational parameter with which the user
> can override the status we guessed.  In fact, currently, my commit
> message explicitly notes that case:
> 
> https://git.xanclic.moe/XanClic/qemu/commit/0834f1ce77b4c27f0c00f1e4fbee099278e530b2
> 
> (Point 4)
> 
> (from
> https://git.xanclic.moe/XanClic/qemu/commits/branch/spinning-rust-next)
> 
> 
> Alternatively to making bs->file take precedence over bs->backing, we
> could also just set the status to unknown if bs->file and bs->backing
> differ in their status.
> 
> I think it’s generally better to prefer what bs->file says.  This is an
> optimization case, so I think it’s more important to get it right most
> of the time (and guess wrong sometimes) than to stop guessing in all
> cases where we could be wrong.

Fair enough, but let's improve the documentation (both QAPI schema and
comments in the code) to be explicit about these details, how the result
is determined for nodes with multiple children, and what it means and
doesn't mean therefore.

Kevin

signature.asc
Description: PGP signature