| Message ID | 4feab4ab212834c55707c0690d8efcb4f3662b1c.1347993885.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/18/2012 12:53 PM, Jeff Cody wrote: > This is derived from the Supriya Kannery's reopen patches. > > This contains the raw-posix driver changes for the bdrv_reopen_* > functions. All changes are staged into a temporary scratch buffer > during the prepare() stage, and copied over to the live structure > during commit(). Upon abort(), all changes are abandoned, and the > live structures are unmodified. > > The _prepare() will create an extra fd - either by means of a dup, > if possible, or opening a new fd if not (for instance, access > control changes). Upon _commit(), the original fd is closed and > the new fd is used. Upon _abort(), the duplicate/new fd is closed. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/raw-posix.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 99 insertions(+) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 6bf5480..edd1eca 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -138,6 +138,15 @@ typedef struct BDRVRawState { > #endif > } BDRVRawState; > > +typedef struct BDRVRawReopenState { > + int fd; > + int open_flags; > +#ifdef CONFIG_LINUX_AIO > + int use_aio; > + void *aio_ctx; > +#endif These members are conditional... > +static int raw_reopen_prepare(BDRVReopenState *state, > + BlockReopenQueue *queue, Error **errp) > +{ > + BDRVRawState *s; > + BDRVRawReopenState *raw_s; > + int ret = 0; > + > + assert(state != NULL); > + assert(state->bs != NULL); > + > + s = state->bs->opaque; > + > + state->opaque = g_malloc0(sizeof(BDRVRawReopenState)); > + raw_s = state->opaque; > + raw_s->use_aio = s->use_aio; > + raw_s->aio_ctx = s->aio_ctx; ...but you are unconditionally assigning into them. This will introduce compile failures on other platforms. > + if ((raw_s->open_flags & ~fcntl_flags) == (s->open_flags & ~fcntl_flags)) { > + /* dup the original fd */ > + /* TODO: use qemu fcntl wrapper */ > + raw_s->fd = fcntl(s->fd, F_DUPFD_CLOEXEC, 0); F_DUPFD_CLOEXEC is not defined everywhere yet; you still need to address your TODO. > + > + /* If we cannot use fctnl, or fcntl failed, fall back to qemu_open() */ s/fctnl/fcntl/ > +static void raw_reopen_commit(BDRVReopenState *state) > +{ > + BDRVRawReopenState *raw_s = state->opaque; > + BDRVRawState *s = state->bs->opaque; > + > + s->open_flags = raw_s->open_flags; > + > + qemu_close(s->fd); > + s->fd = raw_s->fd; > + s->use_aio = raw_s->use_aio; > + s->aio_ctx = raw_s->aio_ctx; Again, more unconditional use of conditional members.
On 09/18/2012 05:20 PM, Eric Blake wrote: > On 09/18/2012 12:53 PM, Jeff Cody wrote: >> This is derived from the Supriya Kannery's reopen patches. >> >> This contains the raw-posix driver changes for the bdrv_reopen_* >> functions. All changes are staged into a temporary scratch buffer >> during the prepare() stage, and copied over to the live structure >> during commit(). Upon abort(), all changes are abandoned, and the >> live structures are unmodified. >> >> The _prepare() will create an extra fd - either by means of a dup, >> if possible, or opening a new fd if not (for instance, access >> control changes). Upon _commit(), the original fd is closed and >> the new fd is used. Upon _abort(), the duplicate/new fd is closed. >> >> Signed-off-by: Jeff Cody <jcody@redhat.com> >> --- >> block/raw-posix.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 99 insertions(+) >> >> diff --git a/block/raw-posix.c b/block/raw-posix.c >> index 6bf5480..edd1eca 100644 >> --- a/block/raw-posix.c >> +++ b/block/raw-posix.c >> @@ -138,6 +138,15 @@ typedef struct BDRVRawState { >> #endif >> } BDRVRawState; >> >> +typedef struct BDRVRawReopenState { >> + int fd; >> + int open_flags; >> +#ifdef CONFIG_LINUX_AIO >> + int use_aio; >> + void *aio_ctx; >> +#endif > > These members are conditional... > >> +static int raw_reopen_prepare(BDRVReopenState *state, >> + BlockReopenQueue *queue, Error **errp) >> +{ >> + BDRVRawState *s; >> + BDRVRawReopenState *raw_s; >> + int ret = 0; >> + >> + assert(state != NULL); >> + assert(state->bs != NULL); >> + >> + s = state->bs->opaque; >> + >> + state->opaque = g_malloc0(sizeof(BDRVRawReopenState)); >> + raw_s = state->opaque; >> + raw_s->use_aio = s->use_aio; >> + raw_s->aio_ctx = s->aio_ctx; > > ...but you are unconditionally assigning into them. This will introduce > compile failures on other platforms. Argh. Thanks. > >> + if ((raw_s->open_flags & ~fcntl_flags) == (s->open_flags & ~fcntl_flags)) { >> + /* dup the original fd */ >> + /* TODO: use qemu fcntl wrapper */ >> + raw_s->fd = fcntl(s->fd, F_DUPFD_CLOEXEC, 0); > > F_DUPFD_CLOEXEC is not defined everywhere yet; you still need to address > your TODO. > >> + >> + /* If we cannot use fctnl, or fcntl failed, fall back to qemu_open() */ > > s/fctnl/fcntl/ > >> +static void raw_reopen_commit(BDRVReopenState *state) >> +{ >> + BDRVRawReopenState *raw_s = state->opaque; >> + BDRVRawState *s = state->bs->opaque; >> + >> + s->open_flags = raw_s->open_flags; >> + >> + qemu_close(s->fd); >> + s->fd = raw_s->fd; >> + s->use_aio = raw_s->use_aio; >> + s->aio_ctx = raw_s->aio_ctx; > > Again, more unconditional use of conditional members. >
Am 18.09.2012 20:53, schrieb Jeff Cody: > This is derived from the Supriya Kannery's reopen patches. > > This contains the raw-posix driver changes for the bdrv_reopen_* > functions. All changes are staged into a temporary scratch buffer > during the prepare() stage, and copied over to the live structure > during commit(). Upon abort(), all changes are abandoned, and the > live structures are unmodified. > > The _prepare() will create an extra fd - either by means of a dup, > if possible, or opening a new fd if not (for instance, access > control changes). Upon _commit(), the original fd is closed and > the new fd is used. Upon _abort(), the duplicate/new fd is closed. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > +static int raw_reopen_prepare(BDRVReopenState *state, > + BlockReopenQueue *queue, Error **errp) > +{ > + BDRVRawState *s; > + BDRVRawReopenState *raw_s; > + int ret = 0; > + > + assert(state != NULL); > + assert(state->bs != NULL); > + > + s = state->bs->opaque; > + > + state->opaque = g_malloc0(sizeof(BDRVRawReopenState)); > + raw_s = state->opaque; > + raw_s->use_aio = s->use_aio; > + raw_s->aio_ctx = s->aio_ctx; You can immediately set s->aio_ctx instead of going through BDRVRawReopenState with it. It seems to be valid to have it present while use_aio = 0. If it wasn't valid, you'd have to free the context when reopening without Linux AIO. > + > + raw_parse_flags(state->flags, &raw_s->open_flags); > + raw_set_aio(&raw_s->aio_ctx, &raw_s->use_aio, state->flags); At least you're consistently omitting the error check. :-) > + > + raw_s->fd = -1; > + > + int fcntl_flags = O_APPEND | O_ASYNC | O_NONBLOCK; > +#ifdef O_NOATIME > + fcntl_flags |= O_NOATIME; > +#endif > + if ((raw_s->open_flags & ~fcntl_flags) == (s->open_flags & ~fcntl_flags)) { > + /* dup the original fd */ > + /* TODO: use qemu fcntl wrapper */ Hm, still not addressed? Kevin
On 09/20/2012 10:10 AM, Kevin Wolf wrote: > Am 18.09.2012 20:53, schrieb Jeff Cody: >> This is derived from the Supriya Kannery's reopen patches. >> >> This contains the raw-posix driver changes for the bdrv_reopen_* >> functions. All changes are staged into a temporary scratch buffer >> during the prepare() stage, and copied over to the live structure >> during commit(). Upon abort(), all changes are abandoned, and the >> live structures are unmodified. >> >> The _prepare() will create an extra fd - either by means of a dup, >> if possible, or opening a new fd if not (for instance, access >> control changes). Upon _commit(), the original fd is closed and >> the new fd is used. Upon _abort(), the duplicate/new fd is closed. >> >> Signed-off-by: Jeff Cody <jcody@redhat.com> > >> +static int raw_reopen_prepare(BDRVReopenState *state, >> + BlockReopenQueue *queue, Error **errp) >> +{ >> + BDRVRawState *s; >> + BDRVRawReopenState *raw_s; >> + int ret = 0; >> + >> + assert(state != NULL); >> + assert(state->bs != NULL); >> + >> + s = state->bs->opaque; >> + >> + state->opaque = g_malloc0(sizeof(BDRVRawReopenState)); >> + raw_s = state->opaque; >> + raw_s->use_aio = s->use_aio; >> + raw_s->aio_ctx = s->aio_ctx; > > You can immediately set s->aio_ctx instead of going through > BDRVRawReopenState with it. It seems to be valid to have it present > while use_aio = 0. If it wasn't valid, you'd have to free the context > when reopening without Linux AIO. > Good catch, thanks. >> + >> + raw_parse_flags(state->flags, &raw_s->open_flags); >> + raw_set_aio(&raw_s->aio_ctx, &raw_s->use_aio, state->flags); > > At least you're consistently omitting the error check. :-) > Thanks, fixed. I guess I am an optimist at heart :) >> + >> + raw_s->fd = -1; >> + >> + int fcntl_flags = O_APPEND | O_ASYNC | O_NONBLOCK; >> +#ifdef O_NOATIME >> + fcntl_flags |= O_NOATIME; >> +#endif >> + if ((raw_s->open_flags & ~fcntl_flags) == (s->open_flags & ~fcntl_flags)) { >> + /* dup the original fd */ >> + /* TODO: use qemu fcntl wrapper */ > > Hm, still not addressed? > No. I mentioned this in the cover letter for v2. I'd rather see the qemu fcntl wrapper happen as a separate series, and then come back and update this, if that is OK. I'm afraid changes to qemu_open or qemu_dup_flags would delay getting this series in. Although Eric is right, I do need to ifdef the F_DUPFD_CLOEXEC. > Kevin >
diff --git a/block/raw-posix.c b/block/raw-posix.c index 6bf5480..edd1eca 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -138,6 +138,15 @@ typedef struct BDRVRawState { #endif } BDRVRawState; +typedef struct BDRVRawReopenState { + int fd; + int open_flags; +#ifdef CONFIG_LINUX_AIO + int use_aio; + void *aio_ctx; +#endif +} BDRVRawReopenState; + static int fd_open(BlockDriverState *bs); static int64_t raw_getlength(BlockDriverState *bs); @@ -288,6 +297,93 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags) return raw_open_common(bs, filename, flags, 0); } +static int raw_reopen_prepare(BDRVReopenState *state, + BlockReopenQueue *queue, Error **errp) +{ + BDRVRawState *s; + BDRVRawReopenState *raw_s; + int ret = 0; + + assert(state != NULL); + assert(state->bs != NULL); + + s = state->bs->opaque; + + state->opaque = g_malloc0(sizeof(BDRVRawReopenState)); + raw_s = state->opaque; + raw_s->use_aio = s->use_aio; + raw_s->aio_ctx = s->aio_ctx; + + raw_parse_flags(state->flags, &raw_s->open_flags); + raw_set_aio(&raw_s->aio_ctx, &raw_s->use_aio, state->flags); + + raw_s->fd = -1; + + int fcntl_flags = O_APPEND | O_ASYNC | O_NONBLOCK; +#ifdef O_NOATIME + fcntl_flags |= O_NOATIME; +#endif + if ((raw_s->open_flags & ~fcntl_flags) == (s->open_flags & ~fcntl_flags)) { + /* dup the original fd */ + /* TODO: use qemu fcntl wrapper */ + raw_s->fd = fcntl(s->fd, F_DUPFD_CLOEXEC, 0); + if (raw_s->fd >= 0) { + ret = fcntl_setfl(raw_s->fd, raw_s->open_flags); + if (ret) { + qemu_close(raw_s->fd); + raw_s->fd = -1; + } + } + } + + /* If we cannot use fctnl, or fcntl failed, fall back to qemu_open() */ + if (raw_s->fd == -1) { + assert(!(raw_s->open_flags & O_CREAT)); + raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags); + if (raw_s->fd == -1) { + ret = -1; + } + } + + return ret; +} + + +static void raw_reopen_commit(BDRVReopenState *state) +{ + BDRVRawReopenState *raw_s = state->opaque; + BDRVRawState *s = state->bs->opaque; + + s->open_flags = raw_s->open_flags; + + qemu_close(s->fd); + s->fd = raw_s->fd; + s->use_aio = raw_s->use_aio; + s->aio_ctx = raw_s->aio_ctx; + + g_free(state->opaque); + state->opaque = NULL; +} + + +static void raw_reopen_abort(BDRVReopenState *state) +{ + BDRVRawReopenState *raw_s = state->opaque; + + /* nothing to do if NULL, we didn't get far enough */ + if (raw_s == NULL) { + return; + } + + if (raw_s->fd >= 0) { + qemu_close(raw_s->fd); + raw_s->fd = -1; + } + g_free(state->opaque); + state->opaque = NULL; +} + + /* XXX: use host sector size if necessary with: #ifdef DIOCGSECTORSIZE { @@ -738,6 +834,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,
This is derived from the Supriya Kannery's reopen patches. This contains the raw-posix driver changes for the bdrv_reopen_* functions. All changes are staged into a temporary scratch buffer during the prepare() stage, and copied over to the live structure during commit(). Upon abort(), all changes are abandoned, and the live structures are unmodified. The _prepare() will create an extra fd - either by means of a dup, if possible, or opening a new fd if not (for instance, access control changes). Upon _commit(), the original fd is closed and the new fd is used. Upon _abort(), the duplicate/new fd is closed. Signed-off-by: Jeff Cody <jcody@redhat.com> --- block/raw-posix.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+)