| Message ID | 20120201030712.2990.41527.sendpatchset@skannery.in.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 01/31/2012 09:07 PM, Supriya Kannery wrote: > raw-posix driver changes for bdrv_reopen_xx functions to > safely reopen image files. Reopening of image files while > changing hostcache dynamically is handled here. > > Signed-off-by: Supriya Kannery<supriyak@linux.vnet.ibm.com> > > Index: qemu/block/raw.c > =================================================================== > --- qemu.orig/block/raw.c > +++ qemu/block/raw.c > @@ -9,6 +9,22 @@ static int raw_open(BlockDriverState *bs > return 0; > } > > +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, > + int flags) > +{ > + return bdrv_reopen_prepare(bs->file, prs, flags); > +} > + > +static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs) > +{ > + bdrv_reopen_commit(bs->file, rs); > +} > + > +static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs) > +{ > + bdrv_reopen_abort(bs->file, rs); > +} > + > static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num, > int nb_sectors, QEMUIOVector *qiov) > { > @@ -109,6 +125,10 @@ static BlockDriver bdrv_raw = { > .instance_size = 1, > > .bdrv_open = raw_open, > + .bdrv_reopen_prepare > + = raw_reopen_prepare, > + .bdrv_reopen_commit = raw_reopen_commit, > + .bdrv_reopen_abort = raw_reopen_abort, > .bdrv_close = raw_close, > > .bdrv_co_readv = raw_co_readv, > Index: qemu/block/raw-posix.c > =================================================================== > --- qemu.orig/block/raw-posix.c > +++ qemu/block/raw-posix.c > @@ -136,6 +136,11 @@ typedef struct BDRVRawState { > #endif > } BDRVRawState; > > +typedef struct BDRVRawReopenState { > + BDRVReopenState reopen_state; > + BDRVRawState *stash_s; > +} BDRVRawReopenState; > + > static int fd_open(BlockDriverState *bs); > static int64_t raw_getlength(BlockDriverState *bs); > > @@ -279,6 +284,71 @@ static int raw_open(BlockDriverState *bs > return raw_open_common(bs, filename, flags, 0); > } > > +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, > + int flags) > +{ > + BDRVRawReopenState *raw_rs = g_malloc0(sizeof(BDRVRawReopenState)); > + BDRVRawState *s = bs->opaque; > + int ret = 0; > + > + raw_rs->reopen_state.bs = bs; > + > + /* stash state before reopen */ > + raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState)); > + memcpy(raw_rs->stash_s, s, sizeof(BDRVRawState)); > + s->fd = dup(raw_rs->stash_s->fd); > + > + *prs =&(raw_rs->reopen_state); > + > + /* Flags that can be set using fcntl */ > + int fcntl_flags = BDRV_O_NOCACHE; > + > + if ((bs->open_flags& ~fcntl_flags) == (flags& ~fcntl_flags)) { > + if ((flags& BDRV_O_NOCACHE)) { > + s->open_flags |= O_DIRECT; > + } else { > + s->open_flags&= ~O_DIRECT; > + } > + printf("O_DIRECT flag\n"); > + ret = fcntl_setfl(s->fd, s->open_flags); raw-posix.c:raw_aio_submit() does some extra alignment work if s->aligned_buf was set due to the image being opened O_DIRECT initially, not sure what the impact is but probably want to clean that up here. > + } else { > + > + printf("close and open with new flags\n"); > + /* close and reopen using new flags */ > + bs->drv->bdrv_close(bs); > + ret = bs->drv->bdrv_file_open(bs, bs->filename, flags); > + } > + return ret; > +} > + > +static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs) > +{ > + BDRVRawReopenState *raw_rs; > + > + raw_rs = container_of(rs, BDRVRawReopenState, reopen_state); > + > + /* clean up stashed state */ > + close(raw_rs->stash_s->fd); > + g_free(raw_rs->stash_s); > + g_free(raw_rs); > +} > + > +static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs) > +{ > + BDRVRawReopenState *raw_rs; > + BDRVRawState *s = bs->opaque; > + > + raw_rs = container_of(rs, BDRVRawReopenState, reopen_state); > + > + /* revert to stashed state */ > + if (s->fd != -1) { > + close(s->fd); > + } > + memcpy(s, raw_rs->stash_s, sizeof(BDRVRawState)); > + g_free(raw_rs->stash_s); > + g_free(raw_rs); > +} > + > /* XXX: use host sector size if necessary with: > #ifdef DIOCGSECTORSIZE > { > @@ -631,6 +701,9 @@ static BlockDriver bdrv_file = { > .instance_size = sizeof(BDRVRawState), > .bdrv_probe = NULL, /* no probe for protocols */ > .bdrv_file_open = raw_open, > + .bdrv_reopen_prepare = raw_reopen_prepare, > + .bdrv_reopen_commit = raw_reopen_commit, > + .bdrv_reopen_abort = raw_reopen_abort, > .bdrv_close = raw_close, > .bdrv_create = raw_create, > .bdrv_co_discard = raw_co_discard, > >
On Wed, Feb 01, 2012 at 08:37:12AM +0530, Supriya Kannery wrote: > + /* stash state before reopen */ > + raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState)); > + memcpy(raw_rs->stash_s, s, sizeof(BDRVRawState)); Copying a struct is fragile, Mike Roth pointed out the potential issue with aligned_buf. If raw-posix could open from a given file descriptor as an alternative to opening a filename, then it would be clean and natural to simply re-initialize from the dup'd file descriptor in the abort case. That's the approach I would try instead of stashing the whole struct. Stefan
Am 01.02.2012 04:07, schrieb Supriya Kannery: > raw-posix driver changes for bdrv_reopen_xx functions to > safely reopen image files. Reopening of image files while > changing hostcache dynamically is handled here. > > Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com> > > Index: qemu/block/raw.c > =================================================================== > --- qemu.orig/block/raw.c > +++ qemu/block/raw.c > @@ -9,6 +9,22 @@ static int raw_open(BlockDriverState *bs > return 0; > } > > +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, > + int flags) > +{ > + return bdrv_reopen_prepare(bs->file, prs, flags); > +} > + > +static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs) > +{ > + bdrv_reopen_commit(bs->file, rs); > +} > + > +static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs) > +{ > + bdrv_reopen_abort(bs->file, rs); > +} > + > static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num, > int nb_sectors, QEMUIOVector *qiov) > { > @@ -109,6 +125,10 @@ static BlockDriver bdrv_raw = { > .instance_size = 1, > > .bdrv_open = raw_open, > + .bdrv_reopen_prepare > + = raw_reopen_prepare, You can just indent to the next level instead of line wrapping. > + .bdrv_reopen_commit = raw_reopen_commit, > + .bdrv_reopen_abort = raw_reopen_abort, > .bdrv_close = raw_close, > > .bdrv_co_readv = raw_co_readv, > Index: qemu/block/raw-posix.c > =================================================================== > --- qemu.orig/block/raw-posix.c > +++ qemu/block/raw-posix.c > @@ -136,6 +136,11 @@ typedef struct BDRVRawState { > #endif > } BDRVRawState; > > +typedef struct BDRVRawReopenState { > + BDRVReopenState reopen_state; > + BDRVRawState *stash_s; > +} BDRVRawReopenState; See Stefan's comment. If it's possible to save only the fd and maybe one or two other fields, then we should do that. > static int fd_open(BlockDriverState *bs); > static int64_t raw_getlength(BlockDriverState *bs); > > @@ -279,6 +284,71 @@ static int raw_open(BlockDriverState *bs > return raw_open_common(bs, filename, flags, 0); > } > > +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, > + int flags) > +{ > + BDRVRawReopenState *raw_rs = g_malloc0(sizeof(BDRVRawReopenState)); > + BDRVRawState *s = bs->opaque; > + int ret = 0; > + > + raw_rs->reopen_state.bs = bs; > + > + /* stash state before reopen */ > + raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState)); > + memcpy(raw_rs->stash_s, s, sizeof(BDRVRawState)); > + s->fd = dup(raw_rs->stash_s->fd); > + > + *prs = &(raw_rs->reopen_state); > + > + /* Flags that can be set using fcntl */ > + int fcntl_flags = BDRV_O_NOCACHE; > + > + if ((bs->open_flags & ~fcntl_flags) == (flags & ~fcntl_flags)) { > + if ((flags & BDRV_O_NOCACHE)) { > + s->open_flags |= O_DIRECT; > + } else { > + s->open_flags &= ~O_DIRECT; > + } > + printf("O_DIRECT flag\n"); Debugging leftover? > + ret = fcntl_setfl(s->fd, s->open_flags); > + } else { > + > + printf("close and open with new flags\n"); Same here. Kevin
On 02/02/2012 05:45 AM, Michael Roth wrote: > On 01/31/2012 09:07 PM, Supriya Kannery wrote: >> raw-posix driver changes for bdrv_reopen_xx functions to >> safely reopen image files. Reopening of image files while >> changing hostcache dynamically is handled here. >> >> + >> + /* Flags that can be set using fcntl */ >> + int fcntl_flags = BDRV_O_NOCACHE; >> + >> + if ((bs->open_flags& ~fcntl_flags) == (flags& ~fcntl_flags)) { >> + if ((flags& BDRV_O_NOCACHE)) { >> + s->open_flags |= O_DIRECT; >> + } else { >> + s->open_flags&= ~O_DIRECT; >> + } >> + printf("O_DIRECT flag\n"); >> + ret = fcntl_setfl(s->fd, s->open_flags); > > raw-posix.c:raw_aio_submit() does some extra alignment work if > s->aligned_buf was set due to the image being opened O_DIRECT initially, > not sure what the impact is but probably want to clean that up here. > ok, will check on this thanks! for reviewing Supriya
On 02/08/2012 08:24 PM, Kevin Wolf wrote: > Am 01.02.2012 04:07, schrieb Supriya Kannery: >> raw-posix driver changes for bdrv_reopen_xx functions to >> safely reopen image files. Reopening of image files while >> changing hostcache dynamically is handled here. >> >> >> +typedef struct BDRVRawReopenState { >> + BDRVReopenState reopen_state; >> + BDRVRawState *stash_s; >> +} BDRVRawReopenState; > > See Stefan's comment. If it's possible to save only the fd and maybe one > or two other fields, then we should do that. > Yes, for V1 of this patchset, will look for stashing only those relevant fields of a driver state wherever possible >> + >> + if ((bs->open_flags& ~fcntl_flags) == (flags& ~fcntl_flags)) { >> + if ((flags& BDRV_O_NOCACHE)) { >> + s->open_flags |= O_DIRECT; >> + } else { >> + s->open_flags&= ~O_DIRECT; >> + } >> + printf("O_DIRECT flag\n"); > > Debugging leftover? > yes :-), didn't do a proper cleanup as this is RFC for the stashing approach. >> + ret = fcntl_setfl(s->fd, s->open_flags); >> + } else { >> + >> + printf("close and open with new flags\n"); > > Same here. > V1 will be a clean one ! > Kevin >
On 02/07/2012 03:47 PM, Stefan Hajnoczi wrote: > On Wed, Feb 01, 2012 at 08:37:12AM +0530, Supriya Kannery wrote: >> + /* stash state before reopen */ >> + raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState)); >> + memcpy(raw_rs->stash_s, s, sizeof(BDRVRawState)); > > Copying a struct is fragile, Mike Roth pointed out the potential issue > with aligned_buf. > > If raw-posix could open from a given file descriptor as an alternative > to opening a filename, then it would be clean and natural to simply > re-initialize from the dup'd file descriptor in the abort case. That's > the approach I would try instead of stashing the whole struct. > > Stefan > fine, will get V1 with stashing of only required fields wherever possible instead of stashing the full struct.
Index: qemu/block/raw.c =================================================================== --- qemu.orig/block/raw.c +++ qemu/block/raw.c @@ -9,6 +9,22 @@ static int raw_open(BlockDriverState *bs return 0; } +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, + int flags) +{ + return bdrv_reopen_prepare(bs->file, prs, flags); +} + +static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs) +{ + bdrv_reopen_commit(bs->file, rs); +} + +static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs) +{ + bdrv_reopen_abort(bs->file, rs); +} + static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num, int nb_sectors, QEMUIOVector *qiov) { @@ -109,6 +125,10 @@ static BlockDriver bdrv_raw = { .instance_size = 1, .bdrv_open = raw_open, + .bdrv_reopen_prepare + = raw_reopen_prepare, + .bdrv_reopen_commit = raw_reopen_commit, + .bdrv_reopen_abort = raw_reopen_abort, .bdrv_close = raw_close, .bdrv_co_readv = raw_co_readv, Index: qemu/block/raw-posix.c =================================================================== --- qemu.orig/block/raw-posix.c +++ qemu/block/raw-posix.c @@ -136,6 +136,11 @@ typedef struct BDRVRawState { #endif } BDRVRawState; +typedef struct BDRVRawReopenState { + BDRVReopenState reopen_state; + BDRVRawState *stash_s; +} BDRVRawReopenState; + static int fd_open(BlockDriverState *bs); static int64_t raw_getlength(BlockDriverState *bs); @@ -279,6 +284,71 @@ static int raw_open(BlockDriverState *bs return raw_open_common(bs, filename, flags, 0); } +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, + int flags) +{ + BDRVRawReopenState *raw_rs = g_malloc0(sizeof(BDRVRawReopenState)); + BDRVRawState *s = bs->opaque; + int ret = 0; + + raw_rs->reopen_state.bs = bs; + + /* stash state before reopen */ + raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState)); + memcpy(raw_rs->stash_s, s, sizeof(BDRVRawState)); + s->fd = dup(raw_rs->stash_s->fd); + + *prs = &(raw_rs->reopen_state); + + /* Flags that can be set using fcntl */ + int fcntl_flags = BDRV_O_NOCACHE; + + if ((bs->open_flags & ~fcntl_flags) == (flags & ~fcntl_flags)) { + if ((flags & BDRV_O_NOCACHE)) { + s->open_flags |= O_DIRECT; + } else { + s->open_flags &= ~O_DIRECT; + } + printf("O_DIRECT flag\n"); + ret = fcntl_setfl(s->fd, s->open_flags); + } else { + + printf("close and open with new flags\n"); + /* close and reopen using new flags */ + bs->drv->bdrv_close(bs); + ret = bs->drv->bdrv_file_open(bs, bs->filename, flags); + } + return ret; +} + +static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs) +{ + BDRVRawReopenState *raw_rs; + + raw_rs = container_of(rs, BDRVRawReopenState, reopen_state); + + /* clean up stashed state */ + close(raw_rs->stash_s->fd); + g_free(raw_rs->stash_s); + g_free(raw_rs); +} + +static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs) +{ + BDRVRawReopenState *raw_rs; + BDRVRawState *s = bs->opaque; + + raw_rs = container_of(rs, BDRVRawReopenState, reopen_state); + + /* revert to stashed state */ + if (s->fd != -1) { + close(s->fd); + } + memcpy(s, raw_rs->stash_s, sizeof(BDRVRawState)); + g_free(raw_rs->stash_s); + g_free(raw_rs); +} + /* XXX: use host sector size if necessary with: #ifdef DIOCGSECTORSIZE { @@ -631,6 +701,9 @@ static BlockDriver bdrv_file = { .instance_size = sizeof(BDRVRawState), .bdrv_probe = NULL, /* no probe for protocols */ .bdrv_file_open = raw_open, + .bdrv_reopen_prepare = raw_reopen_prepare, + .bdrv_reopen_commit = raw_reopen_commit, + .bdrv_reopen_abort = raw_reopen_abort, .bdrv_close = raw_close, .bdrv_create = raw_create, .bdrv_co_discard = raw_co_discard,
raw-posix driver changes for bdrv_reopen_xx functions to safely reopen image files. Reopening of image files while changing hostcache dynamically is handled here. Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>