Patchwork [v3,09/19] block: raw-posix image file reopen

login
register
mail settings
Submitter Jeff Cody
Date Sept. 18, 2012, 6:53 p.m.
Message ID <4feab4ab212834c55707c0690d8efcb4f3662b1c.1347993885.git.jcody@redhat.com>
Download mbox | patch
Permalink /patch/184835/
State New
Headers show

Comments

Jeff Cody - Sept. 18, 2012, 6:53 p.m.
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(+)
Eric Blake - Sept. 18, 2012, 9:20 p.m.
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.
Jeff Cody - Sept. 18, 2012, 10:20 p.m.
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.
>
Kevin Wolf - Sept. 20, 2012, 2:10 p.m.
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
Jeff Cody - Sept. 20, 2012, 2:45 p.m.
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
>

Patch

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,