| Message ID | AANLkTikGa2o-qso-kGWUkh4qN8gtkr6K=p_k1zJe3s48@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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
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
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);