| Message ID | 01df3140216ac0398bfb3a295c553c42cdf31e5b.1347548248.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/13/2012 12:12 PM, Paolo Bonzini wrote: > Il 13/09/2012 17:49, Jeff Cody ha scritto: >> Block drivers should always open the files in writeback mode (see commit >> e1e9b0ac), so raw-posix/raw-win32 should not parse the BDRV_O_CACHE_WB >> flag. >> >> Signed-off-by: Jeff Cody <jcody@redhat.com> >> --- >> block/raw-posix.c | 3 --- >> block/raw-win32.c | 3 --- >> 2 files changed, 6 deletions(-) >> >> diff --git a/block/raw-posix.c b/block/raw-posix.c >> index 7d3ac9d..4a1047c 100644 >> --- a/block/raw-posix.c >> +++ b/block/raw-posix.c >> @@ -202,9 +202,6 @@ static void raw_parse_flags(int bdrv_flags, int *open_flags) >> if ((bdrv_flags & BDRV_O_NOCACHE)) { >> *open_flags |= O_DIRECT; >> } >> - if (!(bdrv_flags & BDRV_O_CACHE_WB)) { >> - *open_flags |= O_DSYNC; >> - } >> } >> >> static int raw_set_aio(void **aio_ctx, int *use_aio, int bdrv_flags) >> diff --git a/block/raw-win32.c b/block/raw-win32.c >> index 335c06a..78c8306 100644 >> --- a/block/raw-win32.c >> +++ b/block/raw-win32.c >> @@ -92,9 +92,6 @@ static void raw_parse_flags(int flags, int *access_flags, DWORD *overlapped) >> if (flags & BDRV_O_NOCACHE) { >> *overlapped |= FILE_FLAG_NO_BUFFERING; >> } >> - if (!(flags & BDRV_O_CACHE_WB)) { >> - *overlapped |= FILE_FLAG_WRITE_THROUGH; >> - } >> } >> >> static int raw_open(BlockDriverState *bs, const char *filename, int flags) >> > > Why does this matter? If raw-posix was opened directly (i.e. without > the bs->file indirection) this would cause a writethrough file to be > incorrectly opened as writeback. > > Paolo > The problem this patch was trying to work around is that bdrv_open_common() forces BDRV_O_CACHE_WB (commit e1e9b0ac), but that setting is not preserved in bs->open_flags, so it is lost on a reopen. Is there a scenario currently that has raw-posix opened directly as a writethrough file, or were you more concerned with future use?
Il 13/09/2012 19:17, Jeff Cody ha scritto: >> > >> > Why does this matter? If raw-posix was opened directly (i.e. without >> > the bs->file indirection) this would cause a writethrough file to be >> > incorrectly opened as writeback. >> > >> > Paolo >> > > The problem this patch was trying to work around is that > bdrv_open_common() forces BDRV_O_CACHE_WB (commit e1e9b0ac), but that > setting is not preserved in bs->open_flags, so it is lost on a reopen. Perhaps we _should_ preserve that in bs->open_flags, while still using the initial value of BDRV_O_CACHE_WB to initialize bs->enable_write_cache. > Is there a scenario currently that has raw-posix opened directly as a > writethrough file, or were you more concerned with future use? Not for raw-posix, but IIRC some other protocol is opened directly without a format on top. rbd perhaps? I'm concerned of having to work around what seems like a bug elsewhere, in multiple protocols. Paolo
On 09/13/2012 02:56 PM, Paolo Bonzini wrote: > Il 13/09/2012 19:17, Jeff Cody ha scritto: >>>> >>>> Why does this matter? If raw-posix was opened directly (i.e. without >>>> the bs->file indirection) this would cause a writethrough file to be >>>> incorrectly opened as writeback. >>>> >>>> Paolo >>>> >> The problem this patch was trying to work around is that >> bdrv_open_common() forces BDRV_O_CACHE_WB (commit e1e9b0ac), but that >> setting is not preserved in bs->open_flags, so it is lost on a reopen. > > Perhaps we _should_ preserve that in bs->open_flags, while still using > the initial value of BDRV_O_CACHE_WB to initialize bs->enable_write_cache. That would work. Part of the problem is that BDRV_O_CACHE_WB seems overloaded; maybe bdrv_parse_cache_flags() should set a new flag, called BDRV_O_CACHE_WCE, which can be used in lieu of enable_write_cache (similar to getting rid of keep_read_only in favor of BDRV_O_ALLOW_RDWR). And then bdrv_parse_cache_flags() would just not set BDRV_O_CACHE_WB, which can then be used exclusively by the lower layers for their parsing (and bdrv_open_common would just set bs->open_flags to always have BDRV_O_CACHE_WB). Then patch 2/16 would change to having bdrv_set_enable_write_cache() toggle BDRV_O_CACHE_WCE. > >> Is there a scenario currently that has raw-posix opened directly as a >> writethrough file, or were you more concerned with future use? > > Not for raw-posix, but IIRC some other protocol is opened directly > without a format on top. rbd perhaps? I'm concerned of having to work > around what seems like a bug elsewhere, in multiple protocols. > > Paolo >
Il 13/09/2012 21:04, Jeff Cody ha scritto: >> > Perhaps we _should_ preserve that in bs->open_flags, while still using >> > the initial value of BDRV_O_CACHE_WB to initialize bs->enable_write_cache. > That would work. Part of the problem is that BDRV_O_CACHE_WB seems > overloaded; maybe bdrv_parse_cache_flags() should set a new flag, called > BDRV_O_CACHE_WCE, which can be used in lieu of enable_write_cache > (similar to getting rid of keep_read_only in favor of > BDRV_O_ALLOW_RDWR). And then bdrv_parse_cache_flags() would just not > set BDRV_O_CACHE_WB, which can then be used exclusively by the lower > layers for their parsing (and bdrv_open_common would just set > bs->open_flags to always have BDRV_O_CACHE_WB). > > Then patch 2/16 would change to having bdrv_set_enable_write_cache() > toggle BDRV_O_CACHE_WCE. > Yeah, that would work. Alternatively, we can keep this patch and leave bdrv_open_common as is; but then I would also prefer if raw-{posix,win32} took care of setting BDRV_O_CACHE_WB in bs->open_flags, so that the flags are consistent. This setting of BDRV_O_CACHE_WB can be extended later to other formats. Considering this and my comments to patch 3/16 we would have: - after opening with cache=writethrough: bs bs->file enable_write_cache true true BDRV_O_CACHE_WB false true bdrv_enable_write_cache() false true - after opening with cache=writeback: bs bs->file enable_write_cache true true BDRV_O_CACHE_WB true true bdrv_enable_write_cache() true true Paolo
Il 13/09/2012 21:44, Paolo Bonzini ha scritto: > Il 13/09/2012 21:04, Jeff Cody ha scritto: >>>> Perhaps we _should_ preserve that in bs->open_flags, while still using >>>> the initial value of BDRV_O_CACHE_WB to initialize bs->enable_write_cache. >> That would work. Part of the problem is that BDRV_O_CACHE_WB seems >> overloaded; maybe bdrv_parse_cache_flags() should set a new flag, called >> BDRV_O_CACHE_WCE, which can be used in lieu of enable_write_cache >> (similar to getting rid of keep_read_only in favor of >> BDRV_O_ALLOW_RDWR). And then bdrv_parse_cache_flags() would just not >> set BDRV_O_CACHE_WB, which can then be used exclusively by the lower >> layers for their parsing (and bdrv_open_common would just set >> bs->open_flags to always have BDRV_O_CACHE_WB). >> >> Then patch 2/16 would change to having bdrv_set_enable_write_cache() >> toggle BDRV_O_CACHE_WCE. >> > > Yeah, that would work. > > Alternatively, we can keep this patch and leave bdrv_open_common as is; > but then I would also prefer if raw-{posix,win32} took care of setting > BDRV_O_CACHE_WB in bs->open_flags, so that the flags are consistent. > This setting of BDRV_O_CACHE_WB can be extended later to other formats. Hmm, no, what was I thinking... But there is a very simple thing we can do: - patch 2 can be left as is - in patch 3 bdrv_reopen_queue, always add BDRV_O_CACHE_WB to bs_entry->state.flags - in patch 3 bdrv_reopen_commit, always leave BDRV_O_CACHE_WB aside: reopen_state->bs->open_flags = (bs->open_flags & BDRV_O_CACHE_WB) | (reopen_state->flags & ~BDRV_O_CACHE_WB); - this patch can be dropped completely. Paolo
On 09/13/2012 04:29 PM, Paolo Bonzini wrote: > Il 13/09/2012 21:44, Paolo Bonzini ha scritto: >> Il 13/09/2012 21:04, Jeff Cody ha scritto: >>>>> Perhaps we _should_ preserve that in bs->open_flags, while still using >>>>> the initial value of BDRV_O_CACHE_WB to initialize bs->enable_write_cache. >>> That would work. Part of the problem is that BDRV_O_CACHE_WB seems >>> overloaded; maybe bdrv_parse_cache_flags() should set a new flag, called >>> BDRV_O_CACHE_WCE, which can be used in lieu of enable_write_cache >>> (similar to getting rid of keep_read_only in favor of >>> BDRV_O_ALLOW_RDWR). And then bdrv_parse_cache_flags() would just not >>> set BDRV_O_CACHE_WB, which can then be used exclusively by the lower >>> layers for their parsing (and bdrv_open_common would just set >>> bs->open_flags to always have BDRV_O_CACHE_WB). >>> >>> Then patch 2/16 would change to having bdrv_set_enable_write_cache() >>> toggle BDRV_O_CACHE_WCE. >>> >> >> Yeah, that would work. >> >> Alternatively, we can keep this patch and leave bdrv_open_common as is; >> but then I would also prefer if raw-{posix,win32} took care of setting >> BDRV_O_CACHE_WB in bs->open_flags, so that the flags are consistent. >> This setting of BDRV_O_CACHE_WB can be extended later to other formats. > > Hmm, no, what was I thinking... > > But there is a very simple thing we can do: > > - patch 2 can be left as is > > - in patch 3 bdrv_reopen_queue, always add BDRV_O_CACHE_WB to > bs_entry->state.flags > > - in patch 3 bdrv_reopen_commit, always leave BDRV_O_CACHE_WB aside: > > reopen_state->bs->open_flags = > (bs->open_flags & BDRV_O_CACHE_WB) | > (reopen_state->flags & ~BDRV_O_CACHE_WB); > > - this patch can be dropped completely. > Yes, I think that would work. The only thing I don't like is that BDRV_O_CACHE_WB still remains a bit confusing when looking through the code... bs->open_flags does not actually represent the open flags. With what I proposed above, here are the steps needed: - bdrv_parse_cache_flags() sets BDRV_O_CACHE_WCE instead of BDRV_O_CACHE_WB - BDRV_O_CACHE_WCE is used everywhere enable_write_cache was used - bs->enable_write_cache is removed - this patch is dropped - bdrv_open_common() sets bs->open_flags to have BDRV_O_CACHE_WB enabled by default. - The only way BDRV_O_CACHE_WB would get cleared from bs->open_flags now would be if someone explicitly cleared it during a bdrv_reopen(). While there are more changes this way, I think it cleans up the code a bit. The advantage is that bs->open_flags actually reflects the open flags that are currently in use. One disadvantage I see is that it seems a bit odd to have BDRV_O_CACHE_WCE cleared and BDRV_O_CACHE_WB set, until you think about them being intended for different layers. Maybe that is part of the underlying problem - there is one open_flags variable in the BDS, that has some flags intended for all layers in the block stack, and some flags specific to a layer.
Il 13/09/2012 23:45, Jeff Cody ha scritto: > While there are more changes this way, I think it cleans up the code a > bit. The advantage is that bs->open_flags actually reflects the open > flags that are currently in use. One disadvantage I see is that it > seems a bit odd to have BDRV_O_CACHE_WCE cleared and BDRV_O_CACHE_WB > set, until you think about them being intended for different layers. It's more weird to see BDRV_O_CACHE_WCE set and BDRV_O_CACHE_WB cleared. :) > Maybe that is part of the underlying problem - there is one open_flags > variable in the BDS, that has some flags intended for all layers in the > block stack, and some flags specific to a layer. Yes, that's true. Paolo
Am 13.09.2012 18:12, schrieb Paolo Bonzini: > Il 13/09/2012 17:49, Jeff Cody ha scritto: >> Block drivers should always open the files in writeback mode (see commit >> e1e9b0ac), so raw-posix/raw-win32 should not parse the BDRV_O_CACHE_WB >> flag. >> >> Signed-off-by: Jeff Cody <jcody@redhat.com> >> --- >> block/raw-posix.c | 3 --- >> block/raw-win32.c | 3 --- >> 2 files changed, 6 deletions(-) >> >> diff --git a/block/raw-posix.c b/block/raw-posix.c >> index 7d3ac9d..4a1047c 100644 >> --- a/block/raw-posix.c >> +++ b/block/raw-posix.c >> @@ -202,9 +202,6 @@ static void raw_parse_flags(int bdrv_flags, int *open_flags) >> if ((bdrv_flags & BDRV_O_NOCACHE)) { >> *open_flags |= O_DIRECT; >> } >> - if (!(bdrv_flags & BDRV_O_CACHE_WB)) { >> - *open_flags |= O_DSYNC; >> - } >> } >> >> static int raw_set_aio(void **aio_ctx, int *use_aio, int bdrv_flags) >> diff --git a/block/raw-win32.c b/block/raw-win32.c >> index 335c06a..78c8306 100644 >> --- a/block/raw-win32.c >> +++ b/block/raw-win32.c >> @@ -92,9 +92,6 @@ static void raw_parse_flags(int flags, int *access_flags, DWORD *overlapped) >> if (flags & BDRV_O_NOCACHE) { >> *overlapped |= FILE_FLAG_NO_BUFFERING; >> } >> - if (!(flags & BDRV_O_CACHE_WB)) { >> - *overlapped |= FILE_FLAG_WRITE_THROUGH; >> - } >> } >> >> static int raw_open(BlockDriverState *bs, const char *filename, int flags) >> > > Why does this matter? If raw-posix was opened directly (i.e. without > the bs->file indirection) this would cause a writethrough file to be > incorrectly opened as writeback. --verbose, please. I can't see how bs->file is needed here for writethrough semantics. bdrv_open_common() sets bs->enable_write_cache to false and bdrv_co_do_writev() checks it and flushes if necessary. Looks fine to me. In fact, bdrv_open_common() even removes BDRV_O_CACHE_WB, so what Jeff removes here is really dead code (checked with strace: The file isn't opened with O_SYNC even when using -drive format=file). Kevin
Il 14/09/2012 09:27, Kevin Wolf ha scritto: > I can't see how bs->file is needed here for writethrough semantics. > bdrv_open_common() sets bs->enable_write_cache to false and > bdrv_co_do_writev() checks it and flushes if necessary. Looks fine to me. You're right. > In fact, bdrv_open_common() even removes BDRV_O_CACHE_WB, so what Jeff > removes here is really dead code (checked with strace: The file isn't > opened with O_SYNC even when using -drive format=file). Yes, it's dead, on the other hand we still honor BDRV_O_CACHE_WB in all the other protocols. Either we go and touch all the protocols (effectively removing BDRV_O_CACHE_WB from the BlockDriver specification), or treating raw-{posix,win32} specially means we leave bugs everywhere else. Paolo
Am 13.09.2012 23:45, schrieb Jeff Cody: > On 09/13/2012 04:29 PM, Paolo Bonzini wrote: >> Il 13/09/2012 21:44, Paolo Bonzini ha scritto: >>> Il 13/09/2012 21:04, Jeff Cody ha scritto: >>>>>> Perhaps we _should_ preserve that in bs->open_flags, while still using >>>>>> the initial value of BDRV_O_CACHE_WB to initialize bs->enable_write_cache. >>>> That would work. Part of the problem is that BDRV_O_CACHE_WB seems >>>> overloaded; maybe bdrv_parse_cache_flags() should set a new flag, called >>>> BDRV_O_CACHE_WCE, which can be used in lieu of enable_write_cache >>>> (similar to getting rid of keep_read_only in favor of >>>> BDRV_O_ALLOW_RDWR). And then bdrv_parse_cache_flags() would just not >>>> set BDRV_O_CACHE_WB, which can then be used exclusively by the lower >>>> layers for their parsing (and bdrv_open_common would just set >>>> bs->open_flags to always have BDRV_O_CACHE_WB). >>>> >>>> Then patch 2/16 would change to having bdrv_set_enable_write_cache() >>>> toggle BDRV_O_CACHE_WCE. >>>> >>> >>> Yeah, that would work. >>> >>> Alternatively, we can keep this patch and leave bdrv_open_common as is; >>> but then I would also prefer if raw-{posix,win32} took care of setting >>> BDRV_O_CACHE_WB in bs->open_flags, so that the flags are consistent. >>> This setting of BDRV_O_CACHE_WB can be extended later to other formats. >> >> Hmm, no, what was I thinking... >> >> But there is a very simple thing we can do: >> >> - patch 2 can be left as is >> >> - in patch 3 bdrv_reopen_queue, always add BDRV_O_CACHE_WB to >> bs_entry->state.flags >> >> - in patch 3 bdrv_reopen_commit, always leave BDRV_O_CACHE_WB aside: >> >> reopen_state->bs->open_flags = >> (bs->open_flags & BDRV_O_CACHE_WB) | >> (reopen_state->flags & ~BDRV_O_CACHE_WB); >> >> - this patch can be dropped completely. >> > > Yes, I think that would work. The only thing I don't like is that > BDRV_O_CACHE_WB still remains a bit confusing when looking through the > code... bs->open_flags does not actually represent the open flags. > > With what I proposed above, here are the steps needed: > > - bdrv_parse_cache_flags() sets BDRV_O_CACHE_WCE instead of > BDRV_O_CACHE_WB > > - BDRV_O_CACHE_WCE is used everywhere enable_write_cache was used > > - bs->enable_write_cache is removed > > - this patch is dropped > > - bdrv_open_common() sets bs->open_flags to have BDRV_O_CACHE_WB enabled > by default. > > - The only way BDRV_O_CACHE_WB would get cleared from bs->open_flags now > would be if someone explicitly cleared it during a bdrv_reopen(). This feels totally wrong. Your BDRV_O_CACHE_WCE is what BDRV_O_CACHE_WB should be; removing bs->enable_write_cache in favour of a fix BDRV_O_CACHE_WB makes sense, though (maybe we need to consider renaming bs->open_flags then, it really hasn't anything to do with open and more). All block drivers, including raw-{posix,win32}, should completely ignore the flag in their .bdrv_open implementation, this flag is purely for the generic block layer. Drivers always open everything writeback and provide a flush function. They already do today, because today's broken BDRV_O_CACHE_WB is always set. Kevin
Am 14.09.2012 09:50, schrieb Paolo Bonzini: > Il 14/09/2012 09:27, Kevin Wolf ha scritto: >> I can't see how bs->file is needed here for writethrough semantics. >> bdrv_open_common() sets bs->enable_write_cache to false and >> bdrv_co_do_writev() checks it and flushes if necessary. Looks fine to me. > > You're right. > >> In fact, bdrv_open_common() even removes BDRV_O_CACHE_WB, so what Jeff >> removes here is really dead code (checked with strace: The file isn't >> opened with O_SYNC even when using -drive format=file). > > Yes, it's dead, on the other hand we still honor BDRV_O_CACHE_WB in all > the other protocols. Either we go and touch all the protocols > (effectively removing BDRV_O_CACHE_WB from the BlockDriver > specification), or treating raw-{posix,win32} specially means we leave > bugs everywhere else. Yes, touch all protocols and fix them. BDRV_O_CACHE_WB should be for block.c only. Kevin
Il 14/09/2012 09:55, Kevin Wolf ha scritto: >> Either we go and touch all the protocols >> > (effectively removing BDRV_O_CACHE_WB from the BlockDriver >> > specification), or treating raw-{posix,win32} specially means we leave >> > bugs everywhere else. > Yes, touch all protocols and fix them. BDRV_O_CACHE_WB should be for > block.c only. That's also fine, of course. Paolo
diff --git a/block/raw-posix.c b/block/raw-posix.c index 7d3ac9d..4a1047c 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -202,9 +202,6 @@ static void raw_parse_flags(int bdrv_flags, int *open_flags) if ((bdrv_flags & BDRV_O_NOCACHE)) { *open_flags |= O_DIRECT; } - if (!(bdrv_flags & BDRV_O_CACHE_WB)) { - *open_flags |= O_DSYNC; - } } static int raw_set_aio(void **aio_ctx, int *use_aio, int bdrv_flags) diff --git a/block/raw-win32.c b/block/raw-win32.c index 335c06a..78c8306 100644 --- a/block/raw-win32.c +++ b/block/raw-win32.c @@ -92,9 +92,6 @@ static void raw_parse_flags(int flags, int *access_flags, DWORD *overlapped) if (flags & BDRV_O_NOCACHE) { *overlapped |= FILE_FLAG_NO_BUFFERING; } - if (!(flags & BDRV_O_CACHE_WB)) { - *overlapped |= FILE_FLAG_WRITE_THROUGH; - } } static int raw_open(BlockDriverState *bs, const char *filename, int flags)
Block drivers should always open the files in writeback mode (see commit e1e9b0ac), so raw-posix/raw-win32 should not parse the BDRV_O_CACHE_WB flag. Signed-off-by: Jeff Cody <jcody@redhat.com> --- block/raw-posix.c | 3 --- block/raw-win32.c | 3 --- 2 files changed, 6 deletions(-)