Patchwork Re: [PATCH 1/4] block: clarify the meaning of BDRV_O_NOCACHE

login
register
mail settings
Submitter Stefan Hajnoczi
Date March 16, 2011, 9:42 a.m.
Message ID <AANLkTikGa2o-qso-kGWUkh4qN8gtkr6K=p_k1zJe3s48@mail.gmail.com>
Download mbox | patch
Permalink /patch/87200/
State New
Headers show

Comments

Stefan Hajnoczi - March 16, 2011, 9:42 a.m.
On Tue, Mar 15, 2011 at 2:11 PM, Christoph Hellwig <hch@lst.de> wrote:
> Change BDRV_O_NOCACHE to only imply bypassing the host OS file cache,
> but no writeback semantics.  All existing callers are changed to also
> specify BDRV_O_CACHE_WB to give them writeback semantics.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I think there is one hunk missing:

Stefan
Kevin Wolf - March 16, 2011, 9:55 a.m.
Am 16.03.2011 10:42, schrieb Stefan Hajnoczi:
> On Tue, Mar 15, 2011 at 2:11 PM, Christoph Hellwig <hch@lst.de> wrote:
>> Change BDRV_O_NOCACHE to only imply bypassing the host OS file cache,
>> but no writeback semantics.  All existing callers are changed to also
>> specify BDRV_O_CACHE_WB to give them writeback semantics.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> I think there is one hunk missing:
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 75b8bec..db1931b 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -229,7 +229,7 @@ static int qcow2_open(BlockDriverState *bs, int flags)
>      }
> 
>      /* alloc L2 table/refcount block cache */
> -    writethrough = ((flags & BDRV_O_CACHE_MASK) == 0);
> +    writethrough = ((flags & (BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)) == 0);
>      s->l2_table_cache = qcow2_cache_create(bs, L2_CACHE_SIZE, writethrough);
>      s->refcount_block_cache = qcow2_cache_create(bs, REFCOUNT_CACHE_SIZE,
>          writethrough);

This one doesn't make a difference. But the intention could be made
clearer now that all writeback modes have BDRV_O_CACHE_WB set:

    writethrough = ((flags & BDRV_O_CACHE_WB) == 0);

Kevin
Christoph Hellwig - March 16, 2011, 2:08 p.m.
On Wed, Mar 16, 2011 at 09:42:37AM +0000, Stefan Hajnoczi wrote:
> -    writethrough = ((flags & BDRV_O_CACHE_MASK) == 0);
> +    writethrough = ((flags & (BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)) == 0);

or rather

	writethrough = ((flags & (BDRV_O_CACHE_WB) != );

but yes, this code had sneaked in since my initial version.
Stefan Hajnoczi - March 16, 2011, 5 p.m.
On Wed, Mar 16, 2011 at 2:08 PM, Christoph Hellwig <hch@lst.de> wrote:
> On Wed, Mar 16, 2011 at 09:42:37AM +0000, Stefan Hajnoczi wrote:
>> -    writethrough = ((flags & BDRV_O_CACHE_MASK) == 0);
>> +    writethrough = ((flags & (BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)) == 0);
>
> or rather
>
>        writethrough = ((flags & (BDRV_O_CACHE_WB) != );
>
> but yes, this code had sneaked in since my initial version.

My intention was that if we don't care about honoring flushes then we
might as well use Qcow2Cache.  But yes, just checking for cache mode
is the clearest.

Stefan
Kevin Wolf - March 17, 2011, 9:07 a.m.
Am 16.03.2011 18:00, schrieb Stefan Hajnoczi:
> On Wed, Mar 16, 2011 at 2:08 PM, Christoph Hellwig <hch@lst.de> wrote:
>> On Wed, Mar 16, 2011 at 09:42:37AM +0000, Stefan Hajnoczi wrote:
>>> -    writethrough = ((flags & BDRV_O_CACHE_MASK) == 0);
>>> +    writethrough = ((flags & (BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)) == 0);
>>
>> or rather
>>
>>        writethrough = ((flags & (BDRV_O_CACHE_WB) != );
>>
>> but yes, this code had sneaked in since my initial version.
> 
> My intention was that if we don't care about honoring flushes then we
> might as well use Qcow2Cache.  But yes, just checking for cache mode
> is the clearest.

You mean for a possible writethrough mode with BDRV_O_NO_FLUSH set? Such
a mode doesn't make a lot of sense to me.

Kevin
Stefan Hajnoczi - March 17, 2011, 9:18 a.m.
On Thu, Mar 17, 2011 at 9:07 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 16.03.2011 18:00, schrieb Stefan Hajnoczi:
>> On Wed, Mar 16, 2011 at 2:08 PM, Christoph Hellwig <hch@lst.de> wrote:
>>> On Wed, Mar 16, 2011 at 09:42:37AM +0000, Stefan Hajnoczi wrote:
>>>> -    writethrough = ((flags & BDRV_O_CACHE_MASK) == 0);
>>>> +    writethrough = ((flags & (BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)) == 0);
>>>
>>> or rather
>>>
>>>        writethrough = ((flags & (BDRV_O_CACHE_WB) != );
>>>
>>> but yes, this code had sneaked in since my initial version.
>>
>> My intention was that if we don't care about honoring flushes then we
>> might as well use Qcow2Cache.  But yes, just checking for cache mode
>> is the clearest.
>
> You mean for a possible writethrough mode with BDRV_O_NO_FLUSH set? Such
> a mode doesn't make a lot of sense to me.

Yeah, I guess you're right, we can never have BDRV_O_NO_FLUSH without
BDRV_O_CACHE_WB today.  I wasn't thinking end-to-end, just looking at
the BDRV_O_* flag bits.

Stefan

Patch

diff --git a/block/qcow2.c b/block/qcow2.c
index 75b8bec..db1931b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -229,7 +229,7 @@  static int qcow2_open(BlockDriverState *bs, int flags)
     }

     /* alloc L2 table/refcount block cache */
-    writethrough = ((flags & BDRV_O_CACHE_MASK) == 0);
+    writethrough = ((flags & (BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)) == 0);
     s->l2_table_cache = qcow2_cache_create(bs, L2_CACHE_SIZE, writethrough);
     s->refcount_block_cache = qcow2_cache_create(bs, REFCOUNT_CACHE_SIZE,
         writethrough);