Patchwork [21/47] block: add bdrv_ensure_backing_file

login
register
mail settings
Submitter Paolo Bonzini
Date July 24, 2012, 11:03 a.m.
Message ID <1343127865-16608-22-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/172858/
State New
Headers show

Comments

Paolo Bonzini - July 24, 2012, 11:03 a.m.
Mirroring runs without the backing file so that it can be copied outside
QEMU.  However, we need to add it at the time the job is completed and
QEMU switches to the target.  Factor out the common bits of opening an
image and completing a mirroring operation.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c |   69 ++++++++++++++++++++++++++++++++++++++++-----------------------
 block.h |    1 +
 2 files changed, 45 insertions(+), 25 deletions(-)
Kevin Wolf - Sept. 11, 2012, 1:32 p.m.
Am 24.07.2012 13:03, schrieb Paolo Bonzini:
> Mirroring runs without the backing file so that it can be copied outside
> QEMU.  However, we need to add it at the time the job is completed and
> QEMU switches to the target.  Factor out the common bits of opening an
> image and completing a mirroring operation.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Once again, combining code motion and code changes in one patch makes it
harder to review.

bdrv_ensure_backing_file() isn't a good name, after reading only the
subject line I had no idea what this function might do. It's still not
entirely clear to me what the different to a bdrv_open_backing_file()
is, except that it doesn't do anything if a backing file is already
open. In which cases do we rely on this behaviour?

> ---
>  block.c |   69 ++++++++++++++++++++++++++++++++++++++++-----------------------
>  block.h |    1 +
>  2 files changed, 45 insertions(+), 25 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 19da114..002b442 100644
> --- a/block.c
> +++ b/block.c
> @@ -730,6 +730,48 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags)
>      return 0;
>  }
>  
> +int bdrv_ensure_backing_file(BlockDriverState *bs)
> +{
> +    char backing_filename[PATH_MAX];
> +    int back_flags, ret;
> +    BlockDriver *back_drv = NULL;
> +
> +    if (bs->backing_hd != NULL) {
> +        return 0;
> +    }
> +
> +    bs->open_flags &= ~BDRV_O_NO_BACKING;

This doesn't do anything in this patch because the function is never
called with BDRV_O_NO_BACKING set. Is it in preparation for a second user?

> +    if (bs->backing_file[0] == '\0') {
> +        return 0;
> +    }
> +
> +    bs->backing_hd = bdrv_new("");
> +    bdrv_get_full_backing_filename(bs, backing_filename,
> +                                   sizeof(backing_filename));
> +
> +    if (bs->backing_format[0] != '\0') {
> +        back_drv = bdrv_find_format(bs->backing_format);
> +    }
> +
> +    /* backing files always opened read-only */
> +    back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT);
> +
> +    ret = bdrv_open(bs->backing_hd, backing_filename, back_flags, back_drv);
> +    if (ret < 0) {
> +        bdrv_close(bs);

I don't like this because it makes the invalid assumption that the
caller has just opened bs and wants to close it if opening the backing
file fails. I think this is part of the error handling that belongs in
the caller: It opened bs, so it is responsible for closing it in error
cases.

> +        bdrv_delete(bs->backing_hd);

This is a bug fix of its own, should be a separate patch.

> +        bs->backing_hd = NULL;
> +        return ret;
> +    }
> +    if (bs->is_temporary) {
> +        bs->backing_hd->keep_read_only = !(bs->open_flags & BDRV_O_RDWR);
> +    } else {
> +        /* base images use the same setting as leaf */

Huh, "parent" turned into "leaf"?

> +        bs->backing_hd->keep_read_only = bs->keep_read_only;
> +    }
> +    return 0;
> +}

Kevin
Paolo Bonzini - Sept. 11, 2012, 1:46 p.m.
> Once again, combining code motion and code changes in one patch makes
> it harder to review.

bdrv_ensure_backing_file() is a new standalone function that happens to be
usable in bdrv_open as well.  But I can separate the changes/fixes to a
separate patch.

In particular it is can be used after a file has been opened with
BDRV_O_NO_BACKING (at which point the flag does not reflect reality
anymore, hence the removal of the flag).

> bdrv_ensure_backing_file() isn't a good name, after reading only the
> subject line I had no idea what this function might do. It's still
> not entirely clear to me what the different to a bdrv_open_backing_file()
> is, except that it doesn't do anything if a backing file is already
> open. In which cases do we rely on this behaviour?

We open the mirroring target with BDRV_O_NO_BACKING usually, but require
the backing file if the cluster size is larger than the dirty block
granularity.  Later, COW is done in the mirror job, so this is not
needed anymore at the end of the series.
 
> > ---
> >  block.c |   69
> >  ++++++++++++++++++++++++++++++++++++++++-----------------------
> >  block.h |    1 +
> >  2 files changed, 45 insertions(+), 25 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 19da114..002b442 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -730,6 +730,48 @@ int bdrv_file_open(BlockDriverState **pbs,
> > const char *filename, int flags)
> >      return 0;
> >  }
> >  
> > +int bdrv_ensure_backing_file(BlockDriverState *bs)
> > +{
> > +    char backing_filename[PATH_MAX];
> > +    int back_flags, ret;
> > +    BlockDriver *back_drv = NULL;
> > +
> > +    if (bs->backing_hd != NULL) {
> > +        return 0;
> > +    }
> > +
> > +    bs->open_flags &= ~BDRV_O_NO_BACKING;
> 
> This doesn't do anything in this patch because the function is never
> called with BDRV_O_NO_BACKING set. Is it in preparation for a second
> user?

Yes, see above.

> > +    if (bs->backing_file[0] == '\0') {
> > +        return 0;
> > +    }
> > +
> > +    bs->backing_hd = bdrv_new("");
> > +    bdrv_get_full_backing_filename(bs, backing_filename,
> > +                                   sizeof(backing_filename));
> > +
> > +    if (bs->backing_format[0] != '\0') {
> > +        back_drv = bdrv_find_format(bs->backing_format);
> > +    }
> > +
> > +    /* backing files always opened read-only */
> > +    back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT);
> > +
> > +    ret = bdrv_open(bs->backing_hd, backing_filename, back_flags,
> > back_drv);
> > +    if (ret < 0) {
> > +        bdrv_close(bs);
> 
> I don't like this because it makes the invalid assumption that the
> caller has just opened bs and wants to close it if opening the
> backing file fails. I think this is part of the error handling that belongs
> in the caller: It opened bs, so it is responsible for closing it in
> error cases.

It's a bug, it should have closed bs->backing_hd.

> > +        bdrv_delete(bs->backing_hd);
> 
> This is a bug fix of its own, should be a separate patch.

Ok.

> > +        bs->backing_hd = NULL;
> > +        return ret;
> > +    }
> > +    if (bs->is_temporary) {
> > +        bs->backing_hd->keep_read_only = !(bs->open_flags &
> > BDRV_O_RDWR);
> > +    } else {
> > +        /* base images use the same setting as leaf */
> 
> Huh, "parent" turned into "leaf"?

Will move this to a separate patch, too.

Paolo

> > +        bs->backing_hd->keep_read_only = bs->keep_read_only;
> > +    }
> > +    return 0;
> > +}
> 
> Kevin
>
Kevin Wolf - Sept. 11, 2012, 1:58 p.m.
Am 11.09.2012 15:46, schrieb Paolo Bonzini:
> 
>> Once again, combining code motion and code changes in one patch makes
>> it harder to review.
> 
> bdrv_ensure_backing_file() is a new standalone function that happens to be
> usable in bdrv_open as well.  But I can separate the changes/fixes to a
> separate patch.
> 
> In particular it is can be used after a file has been opened with
> BDRV_O_NO_BACKING (at which point the flag does not reflect reality
> anymore, hence the removal of the flag).

Yes, that's what I figured eventually. Maybe some documentation for the
function couldn't hurt.

>> bdrv_ensure_backing_file() isn't a good name, after reading only the
>> subject line I had no idea what this function might do. It's still
>> not entirely clear to me what the different to a bdrv_open_backing_file()
>> is, except that it doesn't do anything if a backing file is already
>> open. In which cases do we rely on this behaviour?
> 
> We open the mirroring target with BDRV_O_NO_BACKING usually, but require
> the backing file if the cluster size is larger than the dirty block
> granularity.  Later, COW is done in the mirror job, so this is not
> needed anymore at the end of the series.

Can we then put a /* FIXME */ comment there and revert that behaviour at
the end of the series? Then we can call it bdrv_open_backing_file() and
it's meaning becomes more obvious.

>>> +    if (bs->backing_file[0] == '\0') {
>>> +        return 0;
>>> +    }
>>> +
>>> +    bs->backing_hd = bdrv_new("");
>>> +    bdrv_get_full_backing_filename(bs, backing_filename,
>>> +                                   sizeof(backing_filename));
>>> +
>>> +    if (bs->backing_format[0] != '\0') {
>>> +        back_drv = bdrv_find_format(bs->backing_format);
>>> +    }
>>> +
>>> +    /* backing files always opened read-only */
>>> +    back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT);
>>> +
>>> +    ret = bdrv_open(bs->backing_hd, backing_filename, back_flags,
>>> back_drv);
>>> +    if (ret < 0) {
>>> +        bdrv_close(bs);
>>
>> I don't like this because it makes the invalid assumption that the
>> caller has just opened bs and wants to close it if opening the
>> backing file fails. I think this is part of the error handling that belongs
>> in the caller: It opened bs, so it is responsible for closing it in
>> error cases.
> 
> It's a bug, it should have closed bs->backing_hd.

Are you sure? You removed the bdrv_close(bs) in the caller, so that it's
missing there would be a second bug.

An explicit bdrv_close(bs->backing_hd) isn't required here, it is
implicitly called in bdrv_delete(bs->backing_hd).

>>> +        bdrv_delete(bs->backing_hd);
>>
>> This is a bug fix of its own, should be a separate patch.
> 
> Ok.
> 
>>> +        bs->backing_hd = NULL;
>>> +        return ret;
>>> +    }
>>> +    if (bs->is_temporary) {
>>> +        bs->backing_hd->keep_read_only = !(bs->open_flags &
>>> BDRV_O_RDWR);
>>> +    } else {
>>> +        /* base images use the same setting as leaf */
>>
>> Huh, "parent" turned into "leaf"?
> 
> Will move this to a separate patch, too.

I don't even understand what you're trying to say in this comment. The
only tree that I can think of (so something like leaves exists) is built
by bs->file and bs->backing_hd. But in this case, the top-level image,
from which the flags are taken, is the root and not a leaf?

Kevin
Paolo Bonzini - Sept. 11, 2012, 2:10 p.m.
----- Messaggio originale -----
> Da: "Kevin Wolf" <kwolf@redhat.com>
> A: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, eblake@redhat.com, jcody@redhat.com, stefanha@linux.vnet.ibm.com
> Inviato: Martedì, 11 settembre 2012 15:58:38
> Oggetto: Re: [PATCH 21/47] block: add bdrv_ensure_backing_file
> 
> Am 11.09.2012 15:46, schrieb Paolo Bonzini:
> > 
> >> Once again, combining code motion and code changes in one patch
> >> makes
> >> it harder to review.
> > 
> > bdrv_ensure_backing_file() is a new standalone function that
> > happens to be
> > usable in bdrv_open as well.  But I can separate the changes/fixes
> > to a
> > separate patch.
> > 
> > In particular it is can be used after a file has been opened with
> > BDRV_O_NO_BACKING (at which point the flag does not reflect reality
> > anymore, hence the removal of the flag).
> 
> Yes, that's what I figured eventually. Maybe some documentation for
> the function couldn't hurt.
> 
> >> bdrv_ensure_backing_file() isn't a good name, after reading only
> >> the
> >> subject line I had no idea what this function might do. It's still
> >> not entirely clear to me what the different to a
> >> bdrv_open_backing_file()
> >> is, except that it doesn't do anything if a backing file is
> >> already
> >> open. In which cases do we rely on this behaviour?
> > 
> > We open the mirroring target with BDRV_O_NO_BACKING usually, but
> > require
> > the backing file if the cluster size is larger than the dirty block
> > granularity.  Later, COW is done in the mirror job, so this is not
> > needed anymore at the end of the series.
> 
> Can we then put a /* FIXME */ comment there and revert that behaviour
> at the end of the series? Then we can call it bdrv_open_backing_file()
> and it's meaning becomes more obvious.

Actually, now that my machine finished upgrading and I can look at the
source code, we do use the functionality even at the end of this series.
If you call block-job-complete to complete mirroring, bdrv_ensure_backing_file()
is called.  But block-job-complete can be called multiple times, because
completion is entirely asynchronous.  I can check bs->backing_hd in the
completion callback, but I think it's less clean (there is no reason in
principle why block/mirror.c should include block_int.h, and adding a
function just to use it once seems not worth).

I would like to add documentation to all functions in block.h in 1.3,
I can start from this function if that would mean keeping it as is...

> >>> +    if (bs->backing_file[0] == '\0') {
> >>> +        return 0;
> >>> +    }
> >>> +
> >>> +    bs->backing_hd = bdrv_new("");
> >>> +    bdrv_get_full_backing_filename(bs, backing_filename,
> >>> +                                   sizeof(backing_filename));
> >>> +
> >>> +    if (bs->backing_format[0] != '\0') {
> >>> +        back_drv = bdrv_find_format(bs->backing_format);
> >>> +    }
> >>> +
> >>> +    /* backing files always opened read-only */
> >>> +    back_flags = bs->open_flags & ~(BDRV_O_RDWR |
> >>> BDRV_O_SNAPSHOT);
> >>> +
> >>> +    ret = bdrv_open(bs->backing_hd, backing_filename,
> >>> back_flags,
> >>> back_drv);
> >>> +    if (ret < 0) {
> >>> +        bdrv_close(bs);
> >>
> >> I don't like this because it makes the invalid assumption that the
> >> caller has just opened bs and wants to close it if opening the
> >> backing file fails. I think this is part of the error handling
> >> that belongs
> >> in the caller: It opened bs, so it is responsible for closing it
> >> in
> >> error cases.
> > 
> > It's a bug, it should have closed bs->backing_hd.
> 
> Are you sure? You removed the bdrv_close(bs) in the caller, so that
> it's missing there would be a second bug.
> An explicit bdrv_close(bs->backing_hd) isn't required here, it is
> implicitly called in bdrv_delete(bs->backing_hd).

True.  But likely my mental process was to add the bdrv_close(bs) here
thinking that it would match the bdrv_delete below.  Note that
bdrv_close(bs) already does delete bs->backing_hd.

> >>> +        bdrv_delete(bs->backing_hd);
> >>
> >> This is a bug fix of its own, should be a separate patch.
> > 
> > Ok.
> > 
> >>> +        bs->backing_hd = NULL;
> >>> +        return ret;
> >>> +    }
> >>> +    if (bs->is_temporary) {
> >>> +        bs->backing_hd->keep_read_only = !(bs->open_flags &
> >>> BDRV_O_RDWR);
> >>> +    } else {
> >>> +        /* base images use the same setting as leaf */
> >>
> >> Huh, "parent" turned into "leaf"?
> > 
> > Will move this to a separate patch, too.
> 
> I don't even understand what you're trying to say in this comment.

Well, I couldn't understand the original comment either. :)  To me,
base image and parent is a synonym...

The images form a tree (snapshots being nodes and with each node
having a parent pointer); what we open is a path from root to leaf,
so the top-level image is a leaf.

Paolo

> The
> only tree that I can think of (so something like leaves exists) is
> built by bs->file and bs->backing_hd. But in this case, the top-level
> image, from which the flags are taken, is the root and not a leaf?
> 
> Kevin
>
Kevin Wolf - Sept. 11, 2012, 3:38 p.m.
Am 11.09.2012 16:10, schrieb Paolo Bonzini:
> 
> 
> ----- Messaggio originale -----
>> Da: "Kevin Wolf" <kwolf@redhat.com>
>> A: "Paolo Bonzini" <pbonzini@redhat.com>
>> Cc: qemu-devel@nongnu.org, eblake@redhat.com, jcody@redhat.com, stefanha@linux.vnet.ibm.com
>> Inviato: Martedì, 11 settembre 2012 15:58:38
>> Oggetto: Re: [PATCH 21/47] block: add bdrv_ensure_backing_file
>>
>> Am 11.09.2012 15:46, schrieb Paolo Bonzini:
>>>
>>>> Once again, combining code motion and code changes in one patch
>>>> makes
>>>> it harder to review.
>>>
>>> bdrv_ensure_backing_file() is a new standalone function that
>>> happens to be
>>> usable in bdrv_open as well.  But I can separate the changes/fixes
>>> to a
>>> separate patch.
>>>
>>> In particular it is can be used after a file has been opened with
>>> BDRV_O_NO_BACKING (at which point the flag does not reflect reality
>>> anymore, hence the removal of the flag).
>>
>> Yes, that's what I figured eventually. Maybe some documentation for
>> the function couldn't hurt.
>>
>>>> bdrv_ensure_backing_file() isn't a good name, after reading only
>>>> the
>>>> subject line I had no idea what this function might do. It's still
>>>> not entirely clear to me what the different to a
>>>> bdrv_open_backing_file()
>>>> is, except that it doesn't do anything if a backing file is
>>>> already
>>>> open. In which cases do we rely on this behaviour?
>>>
>>> We open the mirroring target with BDRV_O_NO_BACKING usually, but
>>> require
>>> the backing file if the cluster size is larger than the dirty block
>>> granularity.  Later, COW is done in the mirror job, so this is not
>>> needed anymore at the end of the series.
>>
>> Can we then put a /* FIXME */ comment there and revert that behaviour
>> at the end of the series? Then we can call it bdrv_open_backing_file()
>> and it's meaning becomes more obvious.
> 
> Actually, now that my machine finished upgrading and I can look at the
> source code, we do use the functionality even at the end of this series.
> If you call block-job-complete to complete mirroring, bdrv_ensure_backing_file()
> is called.  But block-job-complete can be called multiple times, because
> completion is entirely asynchronous.  I can check bs->backing_hd in the
> completion callback, but I think it's less clean (there is no reason in
> principle why block/mirror.c should include block_int.h, and adding a
> function just to use it once seems not worth).
> 
> I would like to add documentation to all functions in block.h in 1.3,
> I can start from this function if that would mean keeping it as is...

I'm not against keeping the logic, just adding documentation to new
functions would be good; even more so if their semantics isn't obvious.

I think I'd consider renaming the function anyway.

>>>>> +    if (bs->backing_file[0] == '\0') {
>>>>> +        return 0;
>>>>> +    }
>>>>> +
>>>>> +    bs->backing_hd = bdrv_new("");
>>>>> +    bdrv_get_full_backing_filename(bs, backing_filename,
>>>>> +                                   sizeof(backing_filename));
>>>>> +
>>>>> +    if (bs->backing_format[0] != '\0') {
>>>>> +        back_drv = bdrv_find_format(bs->backing_format);
>>>>> +    }
>>>>> +
>>>>> +    /* backing files always opened read-only */
>>>>> +    back_flags = bs->open_flags & ~(BDRV_O_RDWR |
>>>>> BDRV_O_SNAPSHOT);
>>>>> +
>>>>> +    ret = bdrv_open(bs->backing_hd, backing_filename,
>>>>> back_flags,
>>>>> back_drv);
>>>>> +    if (ret < 0) {
>>>>> +        bdrv_close(bs);
>>>>
>>>> I don't like this because it makes the invalid assumption that the
>>>> caller has just opened bs and wants to close it if opening the
>>>> backing file fails. I think this is part of the error handling
>>>> that belongs
>>>> in the caller: It opened bs, so it is responsible for closing it
>>>> in
>>>> error cases.
>>>
>>> It's a bug, it should have closed bs->backing_hd.
>>
>> Are you sure? You removed the bdrv_close(bs) in the caller, so that
>> it's missing there would be a second bug.
>> An explicit bdrv_close(bs->backing_hd) isn't required here, it is
>> implicitly called in bdrv_delete(bs->backing_hd).
> 
> True.  But likely my mental process was to add the bdrv_close(bs) here
> thinking that it would match the bdrv_delete below.  Note that
> bdrv_close(bs) already does delete bs->backing_hd.

Whatever, as long as the bug gets fixed... ;-)

>>>>> +        bdrv_delete(bs->backing_hd);
>>>>
>>>> This is a bug fix of its own, should be a separate patch.
>>>
>>> Ok.
>>>
>>>>> +        bs->backing_hd = NULL;
>>>>> +        return ret;
>>>>> +    }
>>>>> +    if (bs->is_temporary) {
>>>>> +        bs->backing_hd->keep_read_only = !(bs->open_flags &
>>>>> BDRV_O_RDWR);
>>>>> +    } else {
>>>>> +        /* base images use the same setting as leaf */
>>>>
>>>> Huh, "parent" turned into "leaf"?
>>>
>>> Will move this to a separate patch, too.
>>
>> I don't even understand what you're trying to say in this comment.
> 
> Well, I couldn't understand the original comment either. :)  To me,
> base image and parent is a synonym...
> 
> The images form a tree (snapshots being nodes and with each node
> having a parent pointer); what we open is a path from root to leaf,
> so the top-level image is a leaf.

We definitely need to get the terminology clarified...

Yes, you're right, the images files on the disk form a tree and your
terminology matches this. But in a running qemu, we have a tree of
BlockDriverStates (as I mentioned bs->file and bs->backing_hd create it,
along with driver specific links), and unfortunately it's direction is
exactly the opposite. This is what the terminology of the original
comment was based on and what I thought of. Both approaches are right in
a way.

I had a similar confusion when talking to Jeff recently, and there we
decided to avoid talking about children and parents at all. Maybe this
is the best in order to avoid misunderstandings. For the image files on
the disk "backing file/overlay" are good alternatives; I'm not sure if
we have any for the BDS tree.

Kevin

Patch

diff --git a/block.c b/block.c
index 19da114..002b442 100644
--- a/block.c
+++ b/block.c
@@ -730,6 +730,48 @@  int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags)
     return 0;
 }
 
+int bdrv_ensure_backing_file(BlockDriverState *bs)
+{
+    char backing_filename[PATH_MAX];
+    int back_flags, ret;
+    BlockDriver *back_drv = NULL;
+
+    if (bs->backing_hd != NULL) {
+        return 0;
+    }
+
+    bs->open_flags &= ~BDRV_O_NO_BACKING;
+    if (bs->backing_file[0] == '\0') {
+        return 0;
+    }
+
+    bs->backing_hd = bdrv_new("");
+    bdrv_get_full_backing_filename(bs, backing_filename,
+                                   sizeof(backing_filename));
+
+    if (bs->backing_format[0] != '\0') {
+        back_drv = bdrv_find_format(bs->backing_format);
+    }
+
+    /* backing files always opened read-only */
+    back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT);
+
+    ret = bdrv_open(bs->backing_hd, backing_filename, back_flags, back_drv);
+    if (ret < 0) {
+        bdrv_close(bs);
+        bdrv_delete(bs->backing_hd);
+        bs->backing_hd = NULL;
+        return ret;
+    }
+    if (bs->is_temporary) {
+        bs->backing_hd->keep_read_only = !(bs->open_flags & BDRV_O_RDWR);
+    } else {
+        /* base images use the same setting as leaf */
+        bs->backing_hd->keep_read_only = bs->keep_read_only;
+    }
+    return 0;
+}
+
 /*
  * Opens a disk image (raw, qcow2, vmdk, ...)
  */
@@ -813,34 +855,11 @@  int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
     }
 
     /* If there is a backing file, use it */
-    if ((flags & BDRV_O_NO_BACKING) == 0 && bs->backing_file[0] != '\0') {
-        char backing_filename[PATH_MAX];
-        int back_flags;
-        BlockDriver *back_drv = NULL;
-
-        bs->backing_hd = bdrv_new("");
-        bdrv_get_full_backing_filename(bs, backing_filename,
-                                       sizeof(backing_filename));
-
-        if (bs->backing_format[0] != '\0') {
-            back_drv = bdrv_find_format(bs->backing_format);
-        }
-
-        /* backing files always opened read-only */
-        back_flags =
-            flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
-
-        ret = bdrv_open(bs->backing_hd, backing_filename, back_flags, back_drv);
+    if ((flags & BDRV_O_NO_BACKING) == 0) {
+        ret = bdrv_ensure_backing_file(bs);
         if (ret < 0) {
-            bdrv_close(bs);
             return ret;
         }
-        if (bs->is_temporary) {
-            bs->backing_hd->keep_read_only = !(flags & BDRV_O_RDWR);
-        } else {
-            /* base image inherits from "parent" */
-            bs->backing_hd->keep_read_only = bs->keep_read_only;
-        }
     }
 
     if (!bdrv_key_required(bs)) {
diff --git a/block.h b/block.h
index 9bff842..8aeb7a9 100644
--- a/block.h
+++ b/block.h
@@ -122,6 +122,7 @@  void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
 void bdrv_delete(BlockDriverState *bs);
 int bdrv_parse_cache_flags(const char *mode, int *flags);
 int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
+int bdrv_ensure_backing_file(BlockDriverState *bs);
 int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
               BlockDriver *drv);
 void bdrv_close(BlockDriverState *bs);