| Message ID | 1455646106-2047-10-git-send-email-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 16.02.2016 um 19:08 hat Max Reitz geschrieben: > Move bdrv_commit_all() and bdrv_flush_all() to the BlockBackend level. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 20 ------------ > block/block-backend.c | 44 +++++++++++++++++++++++---- > block/io.c | 20 ------------ > include/block/block.h | 2 -- > stubs/Makefile.objs | 2 +- > stubs/{bdrv-commit-all.c => blk-commit-all.c} | 4 +-- > 6 files changed, 41 insertions(+), 51 deletions(-) > rename stubs/{bdrv-commit-all.c => blk-commit-all.c} (53%) > > diff --git a/block.c b/block.c > index a119840..5eac9a3 100644 > --- a/block.c > +++ b/block.c > @@ -2531,26 +2531,6 @@ ro_cleanup: > return ret; > } > > -int bdrv_commit_all(void) > -{ > - BlockDriverState *bs; > - > - QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > - AioContext *aio_context = bdrv_get_aio_context(bs); > - > - aio_context_acquire(aio_context); > - if (bs->drv && bs->backing) { > - int ret = bdrv_commit(bs); > - if (ret < 0) { > - aio_context_release(aio_context); > - return ret; > - } > - } > - aio_context_release(aio_context); > - } > - return 0; > -} > - > /* > * Return values: > * 0 - success > diff --git a/block/block-backend.c b/block/block-backend.c > index a10fe44..a918c35 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -908,11 +908,6 @@ int blk_flush(BlockBackend *blk) > return bdrv_flush(blk->bs); > } > > -int blk_flush_all(void) > -{ > - return bdrv_flush_all(); > -} > - > void blk_drain(BlockBackend *blk) > { > if (blk->bs) { > @@ -1361,5 +1356,42 @@ BlockBackendRootState *blk_get_root_state(BlockBackend *blk) > > int blk_commit_all(void) > { > - return bdrv_commit_all(); > + BlockBackend *blk = NULL; > + > + while ((blk = blk_all_next(blk)) != NULL) { > + AioContext *aio_context = blk_get_aio_context(blk); > + > + aio_context_acquire(aio_context); > + if (blk_is_available(blk) && blk->bs->drv && blk->bs->backing) { blk->bs->drv is already implied by blk_is_available(), so it's unnecessary, even though it doesn't actively hurt. Also, using blk_is_available() instead of blk_is_inserted() as in blk_flush_all() means that a drive with an open tray, but inserted medium isn't committed. I assume you did this on purpose because you're using two different functions in this patch, but isn't this a change in behaviour? If so, and we really want it, it should at least be mentioned in the commit message. > + int ret = bdrv_commit(blk->bs); > + if (ret < 0) { > + aio_context_release(aio_context); > + return ret; > + } > + } > + aio_context_release(aio_context); > + } > + return 0; > +} > + > +int blk_flush_all(void) > +{ > + BlockBackend *blk = NULL; > + int result = 0; > + > + while ((blk = blk_all_next(blk)) != NULL) { > + AioContext *aio_context = blk_get_aio_context(blk); > + int ret; > + > + aio_context_acquire(aio_context); > + if (blk_is_inserted(blk)) { > + ret = blk_flush(blk); > + if (ret < 0 && !result) { > + result = ret; > + } > + } > + aio_context_release(aio_context); > + } > + > + return result; > } Kevin
On 17.02.2016 16:51, Kevin Wolf wrote: > Am 16.02.2016 um 19:08 hat Max Reitz geschrieben: >> Move bdrv_commit_all() and bdrv_flush_all() to the BlockBackend level. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block.c | 20 ------------ >> block/block-backend.c | 44 +++++++++++++++++++++++---- >> block/io.c | 20 ------------ >> include/block/block.h | 2 -- >> stubs/Makefile.objs | 2 +- >> stubs/{bdrv-commit-all.c => blk-commit-all.c} | 4 +-- >> 6 files changed, 41 insertions(+), 51 deletions(-) >> rename stubs/{bdrv-commit-all.c => blk-commit-all.c} (53%) >> [...] >> diff --git a/block/block-backend.c b/block/block-backend.c >> index a10fe44..a918c35 100644 >> --- a/block/block-backend.c >> +++ b/block/block-backend.c [...] >> @@ -1361,5 +1356,42 @@ BlockBackendRootState *blk_get_root_state(BlockBackend *blk) >> >> int blk_commit_all(void) >> { >> - return bdrv_commit_all(); >> + BlockBackend *blk = NULL; >> + >> + while ((blk = blk_all_next(blk)) != NULL) { >> + AioContext *aio_context = blk_get_aio_context(blk); >> + >> + aio_context_acquire(aio_context); >> + if (blk_is_available(blk) && blk->bs->drv && blk->bs->backing) { > > blk->bs->drv is already implied by blk_is_available(), so it's > unnecessary, even though it doesn't actively hurt. > > Also, using blk_is_available() instead of blk_is_inserted() as in > blk_flush_all() means that a drive with an open tray, but inserted > medium isn't committed. I assume you did this on purpose because you're > using two different functions in this patch, but isn't this a change in > behaviour? If so, and we really want it, it should at least be mentioned > in the commit message. Drives with open trays are strange. I found it reasonable that every medium in the system should be flushed on blk_flush_all(), because flushing data should only bring the on-disk state to a state it is supposed to be in anyway. On the other hand, the commit operation actively changes data. Therefore, I decided to treat it like a guest write operation. However, considering that blk_commit_all() is basically only available through HMP and is thus more of a legacy operation, I don't think its behavior should be changed here. I'm therefore going to change this to blk_is_inserted(), thanks. Max
diff --git a/block.c b/block.c index a119840..5eac9a3 100644 --- a/block.c +++ b/block.c @@ -2531,26 +2531,6 @@ ro_cleanup: return ret; } -int bdrv_commit_all(void) -{ - BlockDriverState *bs; - - QTAILQ_FOREACH(bs, &bdrv_states, device_list) { - AioContext *aio_context = bdrv_get_aio_context(bs); - - aio_context_acquire(aio_context); - if (bs->drv && bs->backing) { - int ret = bdrv_commit(bs); - if (ret < 0) { - aio_context_release(aio_context); - return ret; - } - } - aio_context_release(aio_context); - } - return 0; -} - /* * Return values: * 0 - success diff --git a/block/block-backend.c b/block/block-backend.c index a10fe44..a918c35 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -908,11 +908,6 @@ int blk_flush(BlockBackend *blk) return bdrv_flush(blk->bs); } -int blk_flush_all(void) -{ - return bdrv_flush_all(); -} - void blk_drain(BlockBackend *blk) { if (blk->bs) { @@ -1361,5 +1356,42 @@ BlockBackendRootState *blk_get_root_state(BlockBackend *blk) int blk_commit_all(void) { - return bdrv_commit_all(); + BlockBackend *blk = NULL; + + while ((blk = blk_all_next(blk)) != NULL) { + AioContext *aio_context = blk_get_aio_context(blk); + + aio_context_acquire(aio_context); + if (blk_is_available(blk) && blk->bs->drv && blk->bs->backing) { + int ret = bdrv_commit(blk->bs); + if (ret < 0) { + aio_context_release(aio_context); + return ret; + } + } + aio_context_release(aio_context); + } + return 0; +} + +int blk_flush_all(void) +{ + BlockBackend *blk = NULL; + int result = 0; + + while ((blk = blk_all_next(blk)) != NULL) { + AioContext *aio_context = blk_get_aio_context(blk); + int ret; + + aio_context_acquire(aio_context); + if (blk_is_inserted(blk)) { + ret = blk_flush(blk); + if (ret < 0 && !result) { + result = ret; + } + } + aio_context_release(aio_context); + } + + return result; } diff --git a/block/io.c b/block/io.c index a69bfc4..5f9b6d6 100644 --- a/block/io.c +++ b/block/io.c @@ -1445,26 +1445,6 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs, BDRV_REQ_ZERO_WRITE | flags); } -int bdrv_flush_all(void) -{ - BlockDriverState *bs = NULL; - int result = 0; - - while ((bs = bdrv_next(bs))) { - AioContext *aio_context = bdrv_get_aio_context(bs); - int ret; - - aio_context_acquire(aio_context); - ret = bdrv_flush(bs); - if (ret < 0 && !result) { - result = ret; - } - aio_context_release(aio_context); - } - - return result; -} - typedef struct BdrvCoGetBlockStatusData { BlockDriverState *bs; BlockDriverState *base; diff --git a/include/block/block.h b/include/block/block.h index 1c4f4d8..db9e0f5 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -273,7 +273,6 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs); void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr); void bdrv_refresh_limits(BlockDriverState *bs, Error **errp); int bdrv_commit(BlockDriverState *bs); -int bdrv_commit_all(void); int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file, const char *backing_fmt); void bdrv_register(BlockDriver *bdrv); @@ -374,7 +373,6 @@ int bdrv_inactivate_all(void); /* Ensure contents are flushed to disk. */ int bdrv_flush(BlockDriverState *bs); int coroutine_fn bdrv_co_flush(BlockDriverState *bs); -int bdrv_flush_all(void); void bdrv_close_all(void); void bdrv_drain(BlockDriverState *bs); void bdrv_drain_all(void); diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs index e922de9..9d9f1d0 100644 --- a/stubs/Makefile.objs +++ b/stubs/Makefile.objs @@ -1,5 +1,5 @@ stub-obj-y += arch-query-cpu-def.o -stub-obj-y += bdrv-commit-all.o +stub-obj-y += blk-commit-all.o stub-obj-y += blockdev-close-all-bdrv-states.o stub-obj-y += clock-warp.o stub-obj-y += cpu-get-clock.o diff --git a/stubs/bdrv-commit-all.c b/stubs/blk-commit-all.c similarity index 53% rename from stubs/bdrv-commit-all.c rename to stubs/blk-commit-all.c index bf84a1d..c82fb7f 100644 --- a/stubs/bdrv-commit-all.c +++ b/stubs/blk-commit-all.c @@ -1,8 +1,8 @@ #include "qemu/osdep.h" #include "qemu-common.h" -#include "block/block.h" +#include "sysemu/block-backend.h" -int bdrv_commit_all(void) +int blk_commit_all(void) { return 0; }
Move bdrv_commit_all() and bdrv_flush_all() to the BlockBackend level. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 20 ------------ block/block-backend.c | 44 +++++++++++++++++++++++---- block/io.c | 20 ------------ include/block/block.h | 2 -- stubs/Makefile.objs | 2 +- stubs/{bdrv-commit-all.c => blk-commit-all.c} | 4 +-- 6 files changed, 41 insertions(+), 51 deletions(-) rename stubs/{bdrv-commit-all.c => blk-commit-all.c} (53%)