| Message ID | 1343127865-16608-22-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
> 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 >
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
----- 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 >
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
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);
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(-)