diff mbox series

[v5,2/4,for-3.0] qcow2: Options' documentation fixes

Message ID 20180725142758.9980-3-lbloch@janustech.com (mailing list archive)
State New, archived
Headers show
Series Introduction of l2-cache-full option for qcow2 images | expand

Commit Message

Leonid Bloch July 25, 2018, 2:27 p.m. UTC
Signed-off-by: Leonid Bloch <lbloch@janustech.com>
---
 docs/qcow2-cache.txt |  3 +++
 qemu-options.hx      | 10 ++++++----
 2 files changed, 9 insertions(+), 4 deletions(-)

Comments

Kevin Wolf July 26, 2018, 10:02 a.m. UTC | #1
Am 25.07.2018 um 16:27 hat Leonid Bloch geschrieben:
> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
> ---
>  docs/qcow2-cache.txt |  3 +++
>  qemu-options.hx      | 10 ++++++----
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
> index 8a09a5cc5f..3673f2be0e 100644
> --- a/docs/qcow2-cache.txt
> +++ b/docs/qcow2-cache.txt
> @@ -130,6 +130,9 @@ There are a few things that need to be taken into account:
>     memory as possible to the L2 cache before increasing the refcount
>     cache size.
>  
> +- At most two of "l2-cache-size", "refcount-cache-size", and "cache-size"
> +  can be set simultaneously.

The indentation is off here, the other list items have one space more.

>  Unlike L2 tables, refcount blocks are not used during normal I/O but
>  only during allocations and internal snapshots. In most cases they are
>  accessed sequentially (even during random guest I/O) so increasing the
> diff --git a/qemu-options.hx b/qemu-options.hx
> index b1bf0f485f..13ece21cb6 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -752,15 +752,17 @@ image file)
>  
>  @item cache-size
>  The maximum total size of the L2 table and refcount block caches in bytes
> -(default: 1048576 bytes or 8 clusters, whichever is larger)

I think it would be good to still say something about the default.
Maybe something like "default: the sum of l2-cache-size and
refcount-cache-size"?

>  @item l2-cache-size
> -The maximum size of the L2 table cache in bytes
> -(default: 4/5 of the total cache size)
> +The maximum size of the L2 table cache.

Why did you remove "in bytes" and add a period which the other options
don't have? I prefer the old version of this line.

> +(default: if cache-size is not defined - 1048576 bytes or 8 clusters, whichever
> +is larger; otherwise, as large as possible or needed within the cache-size,
> +while permitting the requested or the minimal refcount cache size)
>  
>  @item refcount-cache-size
>  The maximum size of the refcount block cache in bytes
> -(default: 1/5 of the total cache size)
> +(default: 4 times the cluster size, or any portion of the cache-size, if it is
> +specified and large enough, left over after allocating the full L2 cache)

I found the second part hard to understand. Maybe "4 times the cluster
size; or if both cache-size and l2-cache-size are given, the part of
the cache-size that is not used for the L2 cache yet."?

Kevin
Eric Blake July 26, 2018, 2:20 p.m. UTC | #2
On 07/26/2018 05:02 AM, Kevin Wolf wrote:
> Am 25.07.2018 um 16:27 hat Leonid Bloch geschrieben:
>> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
>> ---
>>   docs/qcow2-cache.txt |  3 +++
>>   qemu-options.hx      | 10 ++++++----
>>   2 files changed, 9 insertions(+), 4 deletions(-)
>>

>> +++ b/qemu-options.hx
>> @@ -752,15 +752,17 @@ image file)
>>   
>>   @item cache-size
>>   The maximum total size of the L2 table and refcount block caches in bytes
>> -(default: 1048576 bytes or 8 clusters, whichever is larger)
> 
> I think it would be good to still say something about the default.
> Maybe something like "default: the sum of l2-cache-size and
> refcount-cache-size"?

Except what happens if you specify only one of l2-cache-size or 
refcount-cache-size? Is the defaulted cache-size then just that one size 
you specified (and the other cache ignored), or is the total cache size 
still defaulted to the 1M/8-cluster size (assuming its larger than the 
other size specified)?

> 
>>   @item l2-cache-size
>> -The maximum size of the L2 table cache in bytes
>> -(default: 4/5 of the total cache size)
>> +The maximum size of the L2 table cache.
> 
> Why did you remove "in bytes" and add a period which the other options
> don't have? I prefer the old version of this line.
> 
>> +(default: if cache-size is not defined - 1048576 bytes or 8 clusters, whichever
>> +is larger; otherwise, as large as possible or needed within the cache-size,
>> +while permitting the requested or the minimal refcount cache size)
>>   
>>   @item refcount-cache-size
>>   The maximum size of the refcount block cache in bytes
>> -(default: 1/5 of the total cache size)
>> +(default: 4 times the cluster size, or any portion of the cache-size, if it is
>> +specified and large enough, left over after allocating the full L2 cache)
> 
> I found the second part hard to understand. Maybe "4 times the cluster
> size; or if both cache-size and l2-cache-size are given, the part of
> the cache-size that is not used for the L2 cache yet."?
> 
> Kevin
>
Leonid Bloch July 26, 2018, 2:27 p.m. UTC | #3
On 07/26/2018 01:02 PM, Kevin Wolf wrote:
> Am 25.07.2018 um 16:27 hat Leonid Bloch geschrieben:
>> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
>> ---
>>   docs/qcow2-cache.txt |  3 +++
>>   qemu-options.hx      | 10 ++++++----
>>   2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
>> index 8a09a5cc5f..3673f2be0e 100644
>> --- a/docs/qcow2-cache.txt
>> +++ b/docs/qcow2-cache.txt
>> @@ -130,6 +130,9 @@ There are a few things that need to be taken into account:
>>      memory as possible to the L2 cache before increasing the refcount
>>      cache size.
>>   
>> +- At most two of "l2-cache-size", "refcount-cache-size", and "cache-size"
>> +  can be set simultaneously.
> 
> The indentation is off here, the other list items have one space more.
> 
>>   Unlike L2 tables, refcount blocks are not used during normal I/O but
>>   only during allocations and internal snapshots. In most cases they are
>>   accessed sequentially (even during random guest I/O) so increasing the
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index b1bf0f485f..13ece21cb6 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -752,15 +752,17 @@ image file)
>>   
>>   @item cache-size
>>   The maximum total size of the L2 table and refcount block caches in bytes
>> -(default: 1048576 bytes or 8 clusters, whichever is larger)
> 
> I think it would be good to still say something about the default.
> Maybe something like "default: the sum of l2-cache-size and
> refcount-cache-size"?

Yes, that sounds good!

> 
>>   @item l2-cache-size
>> -The maximum size of the L2 table cache in bytes
>> -(default: 4/5 of the total cache size)
>> +The maximum size of the L2 table cache.
> 
> Why did you remove "in bytes" and add a period which the other options
> don't have? I prefer the old version of this line.

I removed "in bytes" because it also accepts k, M, G, ... but on a 
second thought, these are amounts of bytes as well, so changing back to 
the old version.

>> +(default: if cache-size is not defined - 1048576 bytes or 8 clusters, whichever
>> +is larger; otherwise, as large as possible or needed within the cache-size,
>> +while permitting the requested or the minimal refcount cache size)
>>   
>>   @item refcount-cache-size
>>   The maximum size of the refcount block cache in bytes
>> -(default: 1/5 of the total cache size)
>> +(default: 4 times the cluster size, or any portion of the cache-size, if it is
>> +specified and large enough, left over after allocating the full L2 cache)
> 
> I found the second part hard to understand. Maybe "4 times the cluster
> size; or if both cache-size and l2-cache-size are given, the part of
> the cache-size that is not used for the L2 cache yet."?

That is not accurate. Because even if l2-cache-size is not given and 
cache-size is large enough to accommodate enough L2 cache to cover the 
entire image, plus the minimal amount of refcount cache with room to 
spare - refcount cache will use all the rest of the cache-size. How about:

"default: 4 times the cluster size; or if cache-size is specified, the 
part of it which is not used for the L2 cache"

Leonid.

> Kevin
>
Leonid Bloch July 26, 2018, 2:30 p.m. UTC | #4
On 07/26/2018 05:20 PM, Eric Blake wrote:
> On 07/26/2018 05:02 AM, Kevin Wolf wrote:
>> Am 25.07.2018 um 16:27 hat Leonid Bloch geschrieben:
>>> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
>>> ---
>>>   docs/qcow2-cache.txt |  3 +++
>>>   qemu-options.hx      | 10 ++++++----
>>>   2 files changed, 9 insertions(+), 4 deletions(-)
>>>
> 
>>> +++ b/qemu-options.hx
>>> @@ -752,15 +752,17 @@ image file)
>>>   @item cache-size
>>>   The maximum total size of the L2 table and refcount block caches in 
>>> bytes
>>> -(default: 1048576 bytes or 8 clusters, whichever is larger)
>>
>> I think it would be good to still say something about the default.
>> Maybe something like "default: the sum of l2-cache-size and
>> refcount-cache-size"?
> 
> Except what happens if you specify only one of l2-cache-size or 
> refcount-cache-size? Is the defaulted cache-size then just that one size 
> you specified (and the other cache ignored), or is the total cache size 
> still defaulted to the 1M/8-cluster size (assuming its larger than the 
> other size specified)?

I think that the meaning here is the sum of the actual l2-cache-size and 
refcount-cache-size. Not the specified options, but the actual caches.

Leonid.
Kevin Wolf July 26, 2018, 2:46 p.m. UTC | #5
Am 26.07.2018 um 16:27 hat Leonid Bloch geschrieben:
> On 07/26/2018 01:02 PM, Kevin Wolf wrote:
> > Am 25.07.2018 um 16:27 hat Leonid Bloch geschrieben:
> > > Signed-off-by: Leonid Bloch <lbloch@janustech.com>
> > > ---
> > >   docs/qcow2-cache.txt |  3 +++
> > >   qemu-options.hx      | 10 ++++++----
> > >   2 files changed, 9 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
> > > index 8a09a5cc5f..3673f2be0e 100644
> > > --- a/docs/qcow2-cache.txt
> > > +++ b/docs/qcow2-cache.txt
> > > @@ -130,6 +130,9 @@ There are a few things that need to be taken into account:
> > >      memory as possible to the L2 cache before increasing the refcount
> > >      cache size.
> > > +- At most two of "l2-cache-size", "refcount-cache-size", and "cache-size"
> > > +  can be set simultaneously.
> > 
> > The indentation is off here, the other list items have one space more.
> > 
> > >   Unlike L2 tables, refcount blocks are not used during normal I/O but
> > >   only during allocations and internal snapshots. In most cases they are
> > >   accessed sequentially (even during random guest I/O) so increasing the
> > > diff --git a/qemu-options.hx b/qemu-options.hx
> > > index b1bf0f485f..13ece21cb6 100644
> > > --- a/qemu-options.hx
> > > +++ b/qemu-options.hx
> > > @@ -752,15 +752,17 @@ image file)
> > >   @item cache-size
> > >   The maximum total size of the L2 table and refcount block caches in bytes
> > > -(default: 1048576 bytes or 8 clusters, whichever is larger)
> > 
> > I think it would be good to still say something about the default.
> > Maybe something like "default: the sum of l2-cache-size and
> > refcount-cache-size"?
> 
> Yes, that sounds good!
> 
> > 
> > >   @item l2-cache-size
> > > -The maximum size of the L2 table cache in bytes
> > > -(default: 4/5 of the total cache size)
> > > +The maximum size of the L2 table cache.
> > 
> > Why did you remove "in bytes" and add a period which the other options
> > don't have? I prefer the old version of this line.
> 
> I removed "in bytes" because it also accepts k, M, G, ... but on a second
> thought, these are amounts of bytes as well, so changing back to the old
> version.
> 
> > > +(default: if cache-size is not defined - 1048576 bytes or 8 clusters, whichever
> > > +is larger; otherwise, as large as possible or needed within the cache-size,
> > > +while permitting the requested or the minimal refcount cache size)
> > >   @item refcount-cache-size
> > >   The maximum size of the refcount block cache in bytes
> > > -(default: 1/5 of the total cache size)
> > > +(default: 4 times the cluster size, or any portion of the cache-size, if it is
> > > +specified and large enough, left over after allocating the full L2 cache)
> > 
> > I found the second part hard to understand. Maybe "4 times the cluster
> > size; or if both cache-size and l2-cache-size are given, the part of
> > the cache-size that is not used for the L2 cache yet."?
> 
> That is not accurate. Because even if l2-cache-size is not given and
> cache-size is large enough to accommodate enough L2 cache to cover the
> entire image, plus the minimal amount of refcount cache with room to spare -
> refcount cache will use all the rest of the cache-size.

Oh, you're right! I wasn't aware that we consider the image size there.

> How about:
> 
> "default: 4 times the cluster size; or if cache-size is specified, the part
> of it which is not used for the L2 cache"

Even simpler, easier to understand and more accurate. I like it.

Kevin
Leonid Bloch July 26, 2018, 2:51 p.m. UTC | #6
>> How about:
>>
>> "default: 4 times the cluster size; or if cache-size is specified, the part
>> of it which is not used for the L2 cache"
> 
> Even simpler, easier to understand and more accurate. I like it.

Thanks!

> 
> Kevin
>
diff mbox series

Patch

diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
index 8a09a5cc5f..3673f2be0e 100644
--- a/docs/qcow2-cache.txt
+++ b/docs/qcow2-cache.txt
@@ -130,6 +130,9 @@  There are a few things that need to be taken into account:
    memory as possible to the L2 cache before increasing the refcount
    cache size.
 
+- At most two of "l2-cache-size", "refcount-cache-size", and "cache-size"
+  can be set simultaneously.
+
 Unlike L2 tables, refcount blocks are not used during normal I/O but
 only during allocations and internal snapshots. In most cases they are
 accessed sequentially (even during random guest I/O) so increasing the
diff --git a/qemu-options.hx b/qemu-options.hx
index b1bf0f485f..13ece21cb6 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -752,15 +752,17 @@  image file)
 
 @item cache-size
 The maximum total size of the L2 table and refcount block caches in bytes
-(default: 1048576 bytes or 8 clusters, whichever is larger)
 
 @item l2-cache-size
-The maximum size of the L2 table cache in bytes
-(default: 4/5 of the total cache size)
+The maximum size of the L2 table cache.
+(default: if cache-size is not defined - 1048576 bytes or 8 clusters, whichever
+is larger; otherwise, as large as possible or needed within the cache-size,
+while permitting the requested or the minimal refcount cache size)
 
 @item refcount-cache-size
 The maximum size of the refcount block cache in bytes
-(default: 1/5 of the total cache size)
+(default: 4 times the cluster size, or any portion of the cache-size, if it is
+specified and large enough, left over after allocating the full L2 cache)
 
 @item cache-clean-interval
 Clean unused entries in the L2 and refcount caches. The interval is in seconds.