| Message ID | 20200428132629.796753-5-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Do not call BlockDriver.bdrv_make_empty() directly | expand |
On 4/28/20 8:26 AM, Max Reitz wrote: > bdrv_commit() already has a BlockBackend pointing to the BDS that we > want to empty, it just has the wrong permissions. > > qemu-img commit has no BlockBackend pointing to the old backing file > yet, but introducing one is simple. > > After this commit, bdrv_make_empty() is the only remaining caller of > BlockDriver.bdrv_make_empty(). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/commit.c | 8 +++++++- > qemu-img.c | 19 ++++++++++++++----- > 2 files changed, 21 insertions(+), 6 deletions(-) > > diff --git a/block/commit.c b/block/commit.c > index 8e672799af..24720ba67d 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs) > } > > if (drv->bdrv_make_empty) { This 'if' is still a bit awkward. Do all filter drivers set this function, or will bdrv_make_empty() automatically take care of looking through filters? Should this be a check of a supported_ variable instead (similar to how Kevin just added supported_truncate_flags)? > - ret = drv->bdrv_make_empty(bs); > + ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL); > if (ret < 0) { > goto ro_cleanup; > } > + > + ret = blk_make_empty(src, NULL); So, if the driver lacks the callback, you miss calling blk_make_empty() even if it would have done something. > + if (ret < 0) { > + goto ro_cleanup; > + } > + > blk_flush(src); > } > > diff --git a/qemu-img.c b/qemu-img.c > index 821cbf610e..a5e8659867 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv) > goto unref_backing; > } > > - if (!drop && bs->drv->bdrv_make_empty) { > - ret = bs->drv->bdrv_make_empty(bs); Old code: if the driver had the callback, use it. > - if (ret) { > - error_setg_errno(&local_err, -ret, "Could not empty %s", > - filename); > + if (!drop) { > + BlockBackend *old_backing_blk; > + > + old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL, > + &local_err); > + if (!old_backing_blk) { > + goto unref_backing; > + } > + ret = blk_make_empty(old_backing_blk, &local_err); New code: Call blk_make_empty() whether or not driver has the callback, then deal with the fallout. > + blk_unref(old_backing_blk); > + if (ret == -ENOTSUP) { > + error_free(local_err); > + local_err = NULL; > + } else if (ret < 0) { > goto unref_backing; > } But this actually looks smarter than the commit case.
Am 28.04.2020 um 15:26 hat Max Reitz geschrieben: > bdrv_commit() already has a BlockBackend pointing to the BDS that we > want to empty, it just has the wrong permissions. > > qemu-img commit has no BlockBackend pointing to the old backing file > yet, but introducing one is simple. > > After this commit, bdrv_make_empty() is the only remaining caller of > BlockDriver.bdrv_make_empty(). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/commit.c | 8 +++++++- > qemu-img.c | 19 ++++++++++++++----- > 2 files changed, 21 insertions(+), 6 deletions(-) > > diff --git a/block/commit.c b/block/commit.c > index 8e672799af..24720ba67d 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs) > } > > if (drv->bdrv_make_empty) { > - ret = drv->bdrv_make_empty(bs); > + ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL); This is very likely to fail because the common case is that the source node is attached to a guest device that doesn't share writes. (qemu-iotests 131 and 274 catch this.) So I think after my theoretical comment in patch 1, this is the practical reason why we need WRITE_UNCHANGED rather than WRITE. Also, why don't you take this permission from the start so that we would error out right away rather than failing after waiting for the all the data to be copied? > if (ret < 0) { > goto ro_cleanup; > } > + > + ret = blk_make_empty(src, NULL); > + if (ret < 0) { > + goto ro_cleanup; > + } > + > blk_flush(src); > } > > diff --git a/qemu-img.c b/qemu-img.c > index 821cbf610e..a5e8659867 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv) > goto unref_backing; > } > > - if (!drop && bs->drv->bdrv_make_empty) { > - ret = bs->drv->bdrv_make_empty(bs); > - if (ret) { > - error_setg_errno(&local_err, -ret, "Could not empty %s", > - filename); > + if (!drop) { > + BlockBackend *old_backing_blk; > + > + old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL, > + &local_err); Oh, you actually depend on another series that you didn't mention in the cover letter. > + if (!old_backing_blk) { > + goto unref_backing; > + } > + ret = blk_make_empty(old_backing_blk, &local_err); > + blk_unref(old_backing_blk); > + if (ret == -ENOTSUP) { > + error_free(local_err); > + local_err = NULL; > + } else if (ret < 0) { > goto unref_backing; > } > } Kevin
On 28.04.20 16:07, Eric Blake wrote: > On 4/28/20 8:26 AM, Max Reitz wrote: >> bdrv_commit() already has a BlockBackend pointing to the BDS that we >> want to empty, it just has the wrong permissions. >> >> qemu-img commit has no BlockBackend pointing to the old backing file >> yet, but introducing one is simple. >> >> After this commit, bdrv_make_empty() is the only remaining caller of >> BlockDriver.bdrv_make_empty(). >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block/commit.c | 8 +++++++- >> qemu-img.c | 19 ++++++++++++++----- >> 2 files changed, 21 insertions(+), 6 deletions(-) >> >> diff --git a/block/commit.c b/block/commit.c >> index 8e672799af..24720ba67d 100644 >> --- a/block/commit.c >> +++ b/block/commit.c >> @@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs) >> } >> if (drv->bdrv_make_empty) { > > This 'if' is still a bit awkward. Do all filter drivers set this > function, or will bdrv_make_empty() automatically take care of looking > through filters? Should this be a check of a supported_ variable > instead (similar to how Kevin just added supported_truncate_flags)? > >> - ret = drv->bdrv_make_empty(bs); >> + ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL); >> if (ret < 0) { >> goto ro_cleanup; >> } >> + >> + ret = blk_make_empty(src, NULL); > > So, if the driver lacks the callback, you miss calling blk_make_empty() > even if it would have done something. We can’t just call blk_make_empty() and then special case -ENOTSUP here, though, because the BB doesn’t have a WRITE permission beforehand. So we have to take that permission before we can call blk_make_empty(). But taking the permission can fail, and then we kind of have to report the -EPERM, even though the BlockDriver may not support emptying anyway. I suppose if we just have to take the WRITE_UNCHANGED permission, though, we can assume that that’s basically always allowed, so it shouldn’t be that much of a problem there. Max >> + if (ret < 0) { >> + goto ro_cleanup; >> + } >> + >> blk_flush(src); >> } >> diff --git a/qemu-img.c b/qemu-img.c >> index 821cbf610e..a5e8659867 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv) >> goto unref_backing; >> } >> - if (!drop && bs->drv->bdrv_make_empty) { >> - ret = bs->drv->bdrv_make_empty(bs); > > Old code: if the driver had the callback, use it. > >> - if (ret) { >> - error_setg_errno(&local_err, -ret, "Could not empty %s", >> - filename); >> + if (!drop) { >> + BlockBackend *old_backing_blk; >> + >> + old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, >> BLK_PERM_ALL, >> + &local_err); >> + if (!old_backing_blk) { >> + goto unref_backing; >> + } >> + ret = blk_make_empty(old_backing_blk, &local_err); > > New code: Call blk_make_empty() whether or not driver has the callback, > then deal with the fallout. > >> + blk_unref(old_backing_blk); >> + if (ret == -ENOTSUP) { >> + error_free(local_err); >> + local_err = NULL; >> + } else if (ret < 0) { >> goto unref_backing; >> } > > But this actually looks smarter than the commit case. >
On 28.04.20 17:03, Kevin Wolf wrote: > Am 28.04.2020 um 15:26 hat Max Reitz geschrieben: >> bdrv_commit() already has a BlockBackend pointing to the BDS that we >> want to empty, it just has the wrong permissions. >> >> qemu-img commit has no BlockBackend pointing to the old backing file >> yet, but introducing one is simple. >> >> After this commit, bdrv_make_empty() is the only remaining caller of >> BlockDriver.bdrv_make_empty(). >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block/commit.c | 8 +++++++- >> qemu-img.c | 19 ++++++++++++++----- >> 2 files changed, 21 insertions(+), 6 deletions(-) >> >> diff --git a/block/commit.c b/block/commit.c >> index 8e672799af..24720ba67d 100644 >> --- a/block/commit.c >> +++ b/block/commit.c >> @@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs) >> } >> >> if (drv->bdrv_make_empty) { >> - ret = drv->bdrv_make_empty(bs); >> + ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL); > > This is very likely to fail because the common case is that the source > node is attached to a guest device that doesn't share writes. > (qemu-iotests 131 and 274 catch this.) > > So I think after my theoretical comment in patch 1, this is the > practical reason why we need WRITE_UNCHANGED rather than WRITE. > > Also, why don't you take this permission from the start so that we would > error out right away rather than failing after waiting for the all the > data to be copied? Because we only need to take it when the BlockDriver actually supports bdrv_make_empty(), so I thought I’d put it here where we have the check anyway. However, yes, when we take WRITE_UNCHANGED, we might as well take it unconditionally from the start. (And then call blk_make_empty() unconditionally here, too, and let it figure out -ENOTSUP, like Eric noted.) >> if (ret < 0) { >> goto ro_cleanup; >> } >> + >> + ret = blk_make_empty(src, NULL); >> + if (ret < 0) { >> + goto ro_cleanup; >> + } >> + >> blk_flush(src); >> } >> >> diff --git a/qemu-img.c b/qemu-img.c >> index 821cbf610e..a5e8659867 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv) >> goto unref_backing; >> } >> >> - if (!drop && bs->drv->bdrv_make_empty) { >> - ret = bs->drv->bdrv_make_empty(bs); >> - if (ret) { >> - error_setg_errno(&local_err, -ret, "Could not empty %s", >> - filename); >> + if (!drop) { >> + BlockBackend *old_backing_blk; >> + >> + old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL, >> + &local_err); > > Oh, you actually depend on another series that you didn't mention in > the cover letter. Well, yes. I didn’t really realize because I just based it on my block-next... Max
diff --git a/block/commit.c b/block/commit.c index 8e672799af..24720ba67d 100644 --- a/block/commit.c +++ b/block/commit.c @@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs) } if (drv->bdrv_make_empty) { - ret = drv->bdrv_make_empty(bs); + ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL); if (ret < 0) { goto ro_cleanup; } + + ret = blk_make_empty(src, NULL); + if (ret < 0) { + goto ro_cleanup; + } + blk_flush(src); } diff --git a/qemu-img.c b/qemu-img.c index 821cbf610e..a5e8659867 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv) goto unref_backing; } - if (!drop && bs->drv->bdrv_make_empty) { - ret = bs->drv->bdrv_make_empty(bs); - if (ret) { - error_setg_errno(&local_err, -ret, "Could not empty %s", - filename); + if (!drop) { + BlockBackend *old_backing_blk; + + old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL, + &local_err); + if (!old_backing_blk) { + goto unref_backing; + } + ret = blk_make_empty(old_backing_blk, &local_err); + blk_unref(old_backing_blk); + if (ret == -ENOTSUP) { + error_free(local_err); + local_err = NULL; + } else if (ret < 0) { goto unref_backing; } }
bdrv_commit() already has a BlockBackend pointing to the BDS that we want to empty, it just has the wrong permissions. qemu-img commit has no BlockBackend pointing to the old backing file yet, but introducing one is simple. After this commit, bdrv_make_empty() is the only remaining caller of BlockDriver.bdrv_make_empty(). Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/commit.c | 8 +++++++- qemu-img.c | 19 ++++++++++++++----- 2 files changed, 21 insertions(+), 6 deletions(-)