| Message ID | 20180628000745.4477-3-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Fix some filename generation issues | expand |
Hi! On 06/28/2018 03:07 AM, Max Reitz wrote: > bdrv_refresh_filename() should invoke itself recursively on all > children, not just on file. > > With that change, we can remove the manual invocations in blkverify, > quorum, commit, and mirror. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > Reviewed-by: Alberto Garcia <berto@igalia.com> > Reviewed-by: Kevin Wolf <kwolf@redhat.com> > --- > block.c | 9 +++++---- > block/blkverify.c | 3 --- > block/commit.c | 1 - > block/mirror.c | 1 - > block/quorum.c | 1 - > 5 files changed, 5 insertions(+), 10 deletions(-) > > diff --git a/block.c b/block.c > index e418c97423..52247062d5 100644 > --- a/block.c > +++ b/block.c > @@ -5174,16 +5174,17 @@ static bool append_open_options(QDict *d, BlockDriverState *bs) > void bdrv_refresh_filename(BlockDriverState *bs) > { > BlockDriver *drv = bs->drv; > + BdrvChild *child; > QDict *opts; > > if (!drv) { > return; > } > > - /* This BDS's file name will most probably depend on its file's name, so > - * refresh that first */ > - if (bs->file) { > - bdrv_refresh_filename(bs->file->bs); > + /* This BDS's file name may depend on any of its children's file names, so > + * refresh those first */ > + QLIST_FOREACH(child, &bs->children, next) { > + bdrv_refresh_filename(child->bs); > } > > if (drv->bdrv_refresh_filename) { > diff --git a/block/blkverify.c b/block/blkverify.c > index da97ee5927..4a18baaf20 100644 > --- a/block/blkverify.c > +++ b/block/blkverify.c > @@ -285,9 +285,6 @@ static void blkverify_refresh_filename(BlockDriverState *bs, QDict *options) > { > BDRVBlkverifyState *s = bs->opaque; > > - /* bs->file->bs has already been refreshed */ > - bdrv_refresh_filename(s->test_file->bs); > - > if (bs->file->bs->full_open_options > && s->test_file->bs->full_open_options) > { > diff --git a/block/commit.c b/block/commit.c > index e1814d9693..26db55d800 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -234,7 +234,6 @@ static int coroutine_fn bdrv_commit_top_preadv(BlockDriverState *bs, > > static void bdrv_commit_top_refresh_filename(BlockDriverState *bs, QDict *opts) > { > - bdrv_refresh_filename(bs->backing->bs); > pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), > bs->backing->bs->filename); > } > diff --git a/block/mirror.c b/block/mirror.c > index 61bd9f3cf1..2f5ccae2b1 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1421,7 +1421,6 @@ static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts) > * bdrv_set_backing_hd */ > return; > } > - bdrv_refresh_filename(bs->backing->bs); > pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), > bs->backing->bs->filename); > } > diff --git a/block/quorum.c b/block/quorum.c > index 9152da8c58..03388590f3 100644 > --- a/block/quorum.c > +++ b/block/quorum.c > @@ -1080,7 +1080,6 @@ static void quorum_refresh_filename(BlockDriverState *bs, QDict *options) > int i; > > for (i = 0; i < s->num_children; i++) { > - bdrv_refresh_filename(s->children[i]->bs); > if (!s->children[i]->bs->full_open_options) { > return; > } > Should blklogwrites not also receive the same treatment? Best regards, Ari Sundholm ari@tuxera.com
On 2018-07-19 14:47, Ari Sundholm wrote: > Hi! > > On 06/28/2018 03:07 AM, Max Reitz wrote: >> bdrv_refresh_filename() should invoke itself recursively on all >> children, not just on file. >> >> With that change, we can remove the manual invocations in blkverify, >> quorum, commit, and mirror. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> Reviewed-by: Alberto Garcia <berto@igalia.com> >> Reviewed-by: Kevin Wolf <kwolf@redhat.com> >> --- >> block.c | 9 +++++---- >> block/blkverify.c | 3 --- >> block/commit.c | 1 - >> block/mirror.c | 1 - >> block/quorum.c | 1 - >> 5 files changed, 5 insertions(+), 10 deletions(-) >> >> diff --git a/block.c b/block.c >> index e418c97423..52247062d5 100644 >> --- a/block.c >> +++ b/block.c >> @@ -5174,16 +5174,17 @@ static bool append_open_options(QDict *d, >> BlockDriverState *bs) >> void bdrv_refresh_filename(BlockDriverState *bs) >> { >> BlockDriver *drv = bs->drv; >> + BdrvChild *child; >> QDict *opts; >> if (!drv) { >> return; >> } >> - /* This BDS's file name will most probably depend on its file's >> name, so >> - * refresh that first */ >> - if (bs->file) { >> - bdrv_refresh_filename(bs->file->bs); >> + /* This BDS's file name may depend on any of its children's file >> names, so >> + * refresh those first */ >> + QLIST_FOREACH(child, &bs->children, next) { >> + bdrv_refresh_filename(child->bs); >> } >> if (drv->bdrv_refresh_filename) { >> diff --git a/block/blkverify.c b/block/blkverify.c >> index da97ee5927..4a18baaf20 100644 >> --- a/block/blkverify.c >> +++ b/block/blkverify.c >> @@ -285,9 +285,6 @@ static void >> blkverify_refresh_filename(BlockDriverState *bs, QDict *options) >> { >> BDRVBlkverifyState *s = bs->opaque; >> - /* bs->file->bs has already been refreshed */ >> - bdrv_refresh_filename(s->test_file->bs); >> - >> if (bs->file->bs->full_open_options >> && s->test_file->bs->full_open_options) >> { >> diff --git a/block/commit.c b/block/commit.c >> index e1814d9693..26db55d800 100644 >> --- a/block/commit.c >> +++ b/block/commit.c >> @@ -234,7 +234,6 @@ static int coroutine_fn >> bdrv_commit_top_preadv(BlockDriverState *bs, >> static void bdrv_commit_top_refresh_filename(BlockDriverState *bs, >> QDict *opts) >> { >> - bdrv_refresh_filename(bs->backing->bs); >> pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), >> bs->backing->bs->filename); >> } >> diff --git a/block/mirror.c b/block/mirror.c >> index 61bd9f3cf1..2f5ccae2b1 100644 >> --- a/block/mirror.c >> +++ b/block/mirror.c >> @@ -1421,7 +1421,6 @@ static void >> bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts) >> * bdrv_set_backing_hd */ >> return; >> } >> - bdrv_refresh_filename(bs->backing->bs); >> pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), >> bs->backing->bs->filename); >> } >> diff --git a/block/quorum.c b/block/quorum.c >> index 9152da8c58..03388590f3 100644 >> --- a/block/quorum.c >> +++ b/block/quorum.c >> @@ -1080,7 +1080,6 @@ static void >> quorum_refresh_filename(BlockDriverState *bs, QDict *options) >> int i; >> for (i = 0; i < s->num_children; i++) { >> - bdrv_refresh_filename(s->children[i]->bs); >> if (!s->children[i]->bs->full_open_options) { >> return; >> } >> > > Should blklogwrites not also receive the same treatment? Probably, but how could I have known before blklogwrites was in? :-) Max
On 07/22/2018 12:14 AM, Max Reitz wrote: > On 2018-07-19 14:47, Ari Sundholm wrote: >> Hi! >> >> On 06/28/2018 03:07 AM, Max Reitz wrote: >>> bdrv_refresh_filename() should invoke itself recursively on all >>> children, not just on file. >>> >>> With that change, we can remove the manual invocations in blkverify, >>> quorum, commit, and mirror. >>> >>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>> Reviewed-by: Alberto Garcia <berto@igalia.com> >>> Reviewed-by: Kevin Wolf <kwolf@redhat.com> >>> --- >>> block.c | 9 +++++---- >>> block/blkverify.c | 3 --- >>> block/commit.c | 1 - >>> block/mirror.c | 1 - >>> block/quorum.c | 1 - >>> 5 files changed, 5 insertions(+), 10 deletions(-) >>> >>> diff --git a/block.c b/block.c >>> index e418c97423..52247062d5 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -5174,16 +5174,17 @@ static bool append_open_options(QDict *d, >>> BlockDriverState *bs) >>> void bdrv_refresh_filename(BlockDriverState *bs) >>> { >>> BlockDriver *drv = bs->drv; >>> + BdrvChild *child; >>> QDict *opts; >>> if (!drv) { >>> return; >>> } >>> - /* This BDS's file name will most probably depend on its file's >>> name, so >>> - * refresh that first */ >>> - if (bs->file) { >>> - bdrv_refresh_filename(bs->file->bs); >>> + /* This BDS's file name may depend on any of its children's file >>> names, so >>> + * refresh those first */ >>> + QLIST_FOREACH(child, &bs->children, next) { >>> + bdrv_refresh_filename(child->bs); >>> } >>> if (drv->bdrv_refresh_filename) { >>> diff --git a/block/blkverify.c b/block/blkverify.c >>> index da97ee5927..4a18baaf20 100644 >>> --- a/block/blkverify.c >>> +++ b/block/blkverify.c >>> @@ -285,9 +285,6 @@ static void >>> blkverify_refresh_filename(BlockDriverState *bs, QDict *options) >>> { >>> BDRVBlkverifyState *s = bs->opaque; >>> - /* bs->file->bs has already been refreshed */ >>> - bdrv_refresh_filename(s->test_file->bs); >>> - >>> if (bs->file->bs->full_open_options >>> && s->test_file->bs->full_open_options) >>> { >>> diff --git a/block/commit.c b/block/commit.c >>> index e1814d9693..26db55d800 100644 >>> --- a/block/commit.c >>> +++ b/block/commit.c >>> @@ -234,7 +234,6 @@ static int coroutine_fn >>> bdrv_commit_top_preadv(BlockDriverState *bs, >>> static void bdrv_commit_top_refresh_filename(BlockDriverState *bs, >>> QDict *opts) >>> { >>> - bdrv_refresh_filename(bs->backing->bs); >>> pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), >>> bs->backing->bs->filename); >>> } >>> diff --git a/block/mirror.c b/block/mirror.c >>> index 61bd9f3cf1..2f5ccae2b1 100644 >>> --- a/block/mirror.c >>> +++ b/block/mirror.c >>> @@ -1421,7 +1421,6 @@ static void >>> bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts) >>> * bdrv_set_backing_hd */ >>> return; >>> } >>> - bdrv_refresh_filename(bs->backing->bs); >>> pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), >>> bs->backing->bs->filename); >>> } >>> diff --git a/block/quorum.c b/block/quorum.c >>> index 9152da8c58..03388590f3 100644 >>> --- a/block/quorum.c >>> +++ b/block/quorum.c >>> @@ -1080,7 +1080,6 @@ static void >>> quorum_refresh_filename(BlockDriverState *bs, QDict *options) >>> int i; >>> for (i = 0; i < s->num_children; i++) { >>> - bdrv_refresh_filename(s->children[i]->bs); >>> if (!s->children[i]->bs->full_open_options) { >>> return; >>> } >>> >> >> Should blklogwrites not also receive the same treatment? > > Probably, but how could I have known before blklogwrites was in? :-) > Sorry about that. I realized my mistake soon after sending my message. :) My excuse is that the threaded view in my Thunderbird showed the series among new ones (due to a recent "ping" message in the thread), and I didn't immediately notice from the timestamps that the series itself had been sent much earlier. Best regards, Ari Sundholm ari@tuxera.com
diff --git a/block.c b/block.c index e418c97423..52247062d5 100644 --- a/block.c +++ b/block.c @@ -5174,16 +5174,17 @@ static bool append_open_options(QDict *d, BlockDriverState *bs) void bdrv_refresh_filename(BlockDriverState *bs) { BlockDriver *drv = bs->drv; + BdrvChild *child; QDict *opts; if (!drv) { return; } - /* This BDS's file name will most probably depend on its file's name, so - * refresh that first */ - if (bs->file) { - bdrv_refresh_filename(bs->file->bs); + /* This BDS's file name may depend on any of its children's file names, so + * refresh those first */ + QLIST_FOREACH(child, &bs->children, next) { + bdrv_refresh_filename(child->bs); } if (drv->bdrv_refresh_filename) { diff --git a/block/blkverify.c b/block/blkverify.c index da97ee5927..4a18baaf20 100644 --- a/block/blkverify.c +++ b/block/blkverify.c @@ -285,9 +285,6 @@ static void blkverify_refresh_filename(BlockDriverState *bs, QDict *options) { BDRVBlkverifyState *s = bs->opaque; - /* bs->file->bs has already been refreshed */ - bdrv_refresh_filename(s->test_file->bs); - if (bs->file->bs->full_open_options && s->test_file->bs->full_open_options) { diff --git a/block/commit.c b/block/commit.c index e1814d9693..26db55d800 100644 --- a/block/commit.c +++ b/block/commit.c @@ -234,7 +234,6 @@ static int coroutine_fn bdrv_commit_top_preadv(BlockDriverState *bs, static void bdrv_commit_top_refresh_filename(BlockDriverState *bs, QDict *opts) { - bdrv_refresh_filename(bs->backing->bs); pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), bs->backing->bs->filename); } diff --git a/block/mirror.c b/block/mirror.c index 61bd9f3cf1..2f5ccae2b1 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1421,7 +1421,6 @@ static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts) * bdrv_set_backing_hd */ return; } - bdrv_refresh_filename(bs->backing->bs); pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), bs->backing->bs->filename); } diff --git a/block/quorum.c b/block/quorum.c index 9152da8c58..03388590f3 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -1080,7 +1080,6 @@ static void quorum_refresh_filename(BlockDriverState *bs, QDict *options) int i; for (i = 0; i < s->num_children; i++) { - bdrv_refresh_filename(s->children[i]->bs); if (!s->children[i]->bs->full_open_options) { return; }