| Message ID | 20180831222907.16257-6-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | jobs: Job Exit Refactoring Pt 2 | expand |
On 2018-09-01 00:28, John Snow wrote: > In cases where we abort the block/mirror job, there's no point in > installing the new backing chain before we finish aborting. > > Move this to the "success" portion of mirror_exit. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/mirror.c | 27 ++++++++++++++------------- > 1 file changed, 14 insertions(+), 13 deletions(-) > > diff --git a/block/mirror.c b/block/mirror.c > index cba555b4ef..c164fee883 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -642,16 +642,6 @@ static void mirror_exit(Job *job) > * required before it could become a backing file of target_bs. */ > bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL, > &error_abort); > - if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) { > - BlockDriverState *backing = s->is_none_mode ? src : s->base; > - if (backing_bs(target_bs) != backing) { > - bdrv_set_backing_hd(target_bs, backing, &local_err); > - if (local_err) { > - error_report_err(local_err); > - ret = -EPERM; > - } > - } > - } > > if (s->to_replace) { > replace_aio_context = bdrv_get_aio_context(s->to_replace); > @@ -659,9 +649,18 @@ static void mirror_exit(Job *job) > } > > if (s->should_complete && ret == 0) { > - BlockDriverState *to_replace = src; > - if (s->to_replace) { > - to_replace = s->to_replace; > + BlockDriverState *to_replace = s->to_replace ? s->to_replace : src; > + > + if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) { > + BlockDriverState *backing = s->is_none_mode ? src : s->base; > + if (backing_bs(target_bs) != backing) { > + bdrv_set_backing_hd(target_bs, backing, &local_err); > + if (local_err) { > + error_report_err(local_err); > + ret = -EPERM; > + goto clean; > + } > + } > } > > if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) { Testing shows that on post-READY cancel, s->should_complete is 0 (which makes sense, because all the rest in this path is about replacing to_replace by target_bs, which we don't want to do on cancel), so this would not be executed. However, I think we do want to give the target the correct backing chain when the job is canceled this way (as I suppose there are ways to keep the target around even with drive-mirror). So we should attach the backing chain whenever ret == 0, not just when s->should_complete is true. Max > @@ -678,6 +677,8 @@ static void mirror_exit(Job *job) > ret = -EPERM; > } > } > + > + clean: > if (s->to_replace) { > bdrv_op_unblock_all(s->to_replace, s->replace_blocker); > error_free(s->replace_blocker); >
On 09/03/2018 05:24 AM, Max Reitz wrote: > On 2018-09-01 00:28, John Snow wrote: >> In cases where we abort the block/mirror job, there's no point in >> installing the new backing chain before we finish aborting. >> >> Move this to the "success" portion of mirror_exit. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block/mirror.c | 27 ++++++++++++++------------- >> 1 file changed, 14 insertions(+), 13 deletions(-) >> >> diff --git a/block/mirror.c b/block/mirror.c >> index cba555b4ef..c164fee883 100644 >> --- a/block/mirror.c >> +++ b/block/mirror.c >> @@ -642,16 +642,6 @@ static void mirror_exit(Job *job) >> * required before it could become a backing file of target_bs. */ >> bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL, >> &error_abort); >> - if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) { >> - BlockDriverState *backing = s->is_none_mode ? src : s->base; >> - if (backing_bs(target_bs) != backing) { >> - bdrv_set_backing_hd(target_bs, backing, &local_err); >> - if (local_err) { >> - error_report_err(local_err); >> - ret = -EPERM; >> - } >> - } >> - } >> >> if (s->to_replace) { >> replace_aio_context = bdrv_get_aio_context(s->to_replace); >> @@ -659,9 +649,18 @@ static void mirror_exit(Job *job) >> } >> >> if (s->should_complete && ret == 0) { >> - BlockDriverState *to_replace = src; >> - if (s->to_replace) { >> - to_replace = s->to_replace; >> + BlockDriverState *to_replace = s->to_replace ? s->to_replace : src; >> + >> + if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) { >> + BlockDriverState *backing = s->is_none_mode ? src : s->base; >> + if (backing_bs(target_bs) != backing) { >> + bdrv_set_backing_hd(target_bs, backing, &local_err); >> + if (local_err) { >> + error_report_err(local_err); >> + ret = -EPERM; >> + goto clean; >> + } >> + } >> } >> >> if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) { > > Testing shows that on post-READY cancel, s->should_complete is 0 (which > makes sense, because all the rest in this path is about replacing > to_replace by target_bs, which we don't want to do on cancel), so this > would not be executed. > > However, I think we do want to give the target the correct backing chain > when the job is canceled this way (as I suppose there are ways to keep > the target around even with drive-mirror). > > So we should attach the backing chain whenever ret == 0, not just when > s->should_complete is true. > > Max > Ah, thank you for saving me from myself. I got too excited to have everything under that one conditional. post-script: I really, really hate the "fake cancel" we've implemented for mirror. It makes the job logic so much worse. --js >> @@ -678,6 +677,8 @@ static void mirror_exit(Job *job) >> ret = -EPERM; >> } >> } >> + >> + clean: >> if (s->to_replace) { >> bdrv_op_unblock_all(s->to_replace, s->replace_blocker); >> error_free(s->replace_blocker); >> > >
On 09/04/2018 02:15 PM, John Snow wrote: > > post-script: I really, really hate the "fake cancel" we've implemented > for mirror. It makes the job logic so much worse. Mirror really does have a tri-state way to end the job (and accessible only after the job has moved into the sync state): cancel (gracefully end the job by detaching the mirror, so that the original volume remains active - your "fake cancel") complete (gracefully end the job by pivoting to the mirror, so that the original volume is now the backup) cancel --force (end the job now, even though it means you did not actually create a mirrored copy) - only useful since commit b76e4458 (2.12) The first two are conceptually similar - the job succeeds, and one of the two files used in the mirroring is now the state of the other at the time the job concluded. Back-compat says we can't really change the block-job terminology without an adequate deprecation cycle (and I don't know if libvirt has even been taught about cancel --force yet); but for the newer 'job' functionality (which we tried to shoehorn existing block jobs into), it does seem like it would be nicer to have 'cancel' mean what blockjob cancel --force implies, and instead focus on teaching 'complete' to have a way to select which of the two completion modes are desired (complete by return to original, or complete by pivot to mirror). It might even be nice to have a way to specify which completion mode to default to at job creation time, and/or to change that default as the job is running (which also implies being able to query which completion mode is currently tied to the job, if you can complete without requesting either of the two modes explicitly).
On 09/04/2018 03:30 PM, Eric Blake wrote: > On 09/04/2018 02:15 PM, John Snow wrote: > >> >> post-script: I really, really hate the "fake cancel" we've implemented >> for mirror. It makes the job logic so much worse. > > Mirror really does have a tri-state way to end the job (and accessible > only after the job has moved into the sync state): > > cancel (gracefully end the job by detaching the mirror, so that the > original volume remains active - your "fake cancel") > complete (gracefully end the job by pivoting to the mirror, so that the > original volume is now the backup) > cancel --force (end the job now, even though it means you did not > actually create a mirrored copy) - only useful since commit b76e4458 (2.12) > > The first two are conceptually similar - the job succeeds, and one of > the two files used in the mirroring is now the state of the other at the > time the job concluded. > Sure, but I dislike how we implemented it as cancel, which mucks up the job mechanisms. I recognize there are two fully valid ways in which we want to successfully complete a mirror job. > Back-compat says we can't really change the block-job terminology > without an adequate deprecation cycle (and I don't know if libvirt has > even been taught about cancel --force yet); but for the newer 'job' > functionality (which we tried to shoehorn existing block jobs into), it > does seem like it would be nicer to have 'cancel' mean what blockjob > cancel --force implies, and instead focus on teaching 'complete' to have > a way to select which of the two completion modes are desired (complete > by return to original, or complete by pivot to mirror). It might even > be nice to have a way to specify which completion mode to default to at > job creation time, and/or to change that default as the job is running > (which also implies being able to query which completion mode is > currently tied to the job, if you can complete without requesting either > of the two modes explicitly). > Yup, that's my preference!
Am 04.09.2018 um 21:30 hat Eric Blake geschrieben: > On 09/04/2018 02:15 PM, John Snow wrote: > > > > > post-script: I really, really hate the "fake cancel" we've implemented > > for mirror. It makes the job logic so much worse. > > Mirror really does have a tri-state way to end the job (and accessible only > after the job has moved into the sync state): > > cancel (gracefully end the job by detaching the mirror, so that the original > volume remains active - your "fake cancel") > complete (gracefully end the job by pivoting to the mirror, so that the > original volume is now the backup) > cancel --force (end the job now, even though it means you did not actually > create a mirrored copy) - only useful since commit b76e4458 (2.12) > > The first two are conceptually similar - the job succeeds, and one of the > two files used in the mirroring is now the state of the other at the time > the job concluded. > > Back-compat says we can't really change the block-job terminology without an > adequate deprecation cycle (and I don't know if libvirt has even been taught > about cancel --force yet); but for the newer 'job' functionality (which we > tried to shoehorn existing block jobs into), it does seem like it would be > nicer to have 'cancel' mean what blockjob cancel --force implies, and > instead focus on teaching 'complete' to have a way to select which of the > two completion modes are desired (complete by return to original, or > complete by pivot to mirror). It might even be nice to have a way to > specify which completion mode to default to at job creation time, and/or to > change that default as the job is running (which also implies being able to > query which completion mode is currently tied to the job, if you can > complete without requesting either of the two modes explicitly). I think it's better to select the completion mode in the job-complete command than having to specify it from the start. Once we have support multiple completion modes (even just internally), nothing stops us from moving the "fake cancel" logic to the QMP layer (i.e. the drive-mirror/blockdev-mirror implementations) and get rid of it in the actual job. Kevin
On 09/05/2018 05:54 AM, Kevin Wolf wrote: > Am 04.09.2018 um 21:30 hat Eric Blake geschrieben: >> On 09/04/2018 02:15 PM, John Snow wrote: >> >>> >>> post-script: I really, really hate the "fake cancel" we've implemented >>> for mirror. It makes the job logic so much worse. >> >> Mirror really does have a tri-state way to end the job (and accessible only >> after the job has moved into the sync state): >> >> cancel (gracefully end the job by detaching the mirror, so that the original >> volume remains active - your "fake cancel") >> complete (gracefully end the job by pivoting to the mirror, so that the >> original volume is now the backup) >> cancel --force (end the job now, even though it means you did not actually >> create a mirrored copy) - only useful since commit b76e4458 (2.12) >> >> The first two are conceptually similar - the job succeeds, and one of the >> two files used in the mirroring is now the state of the other at the time >> the job concluded. >> >> Back-compat says we can't really change the block-job terminology without an >> adequate deprecation cycle (and I don't know if libvirt has even been taught >> about cancel --force yet); but for the newer 'job' functionality (which we >> tried to shoehorn existing block jobs into), it does seem like it would be >> nicer to have 'cancel' mean what blockjob cancel --force implies, and >> instead focus on teaching 'complete' to have a way to select which of the >> two completion modes are desired (complete by return to original, or >> complete by pivot to mirror). It might even be nice to have a way to >> specify which completion mode to default to at job creation time, and/or to >> change that default as the job is running (which also implies being able to >> query which completion mode is currently tied to the job, if you can >> complete without requesting either of the two modes explicitly). > > I think it's better to select the completion mode in the job-complete > command than having to specify it from the start. > > Once we have support multiple completion modes (even just internally), > nothing stops us from moving the "fake cancel" logic to the QMP layer > (i.e. the drive-mirror/blockdev-mirror implementations) and get rid of > it in the actual job. > Oh! I hadn't considered that specific tactic before. That's a very good idea! Thanks! --js
diff --git a/block/mirror.c b/block/mirror.c index cba555b4ef..c164fee883 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -642,16 +642,6 @@ static void mirror_exit(Job *job) * required before it could become a backing file of target_bs. */ bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL, &error_abort); - if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) { - BlockDriverState *backing = s->is_none_mode ? src : s->base; - if (backing_bs(target_bs) != backing) { - bdrv_set_backing_hd(target_bs, backing, &local_err); - if (local_err) { - error_report_err(local_err); - ret = -EPERM; - } - } - } if (s->to_replace) { replace_aio_context = bdrv_get_aio_context(s->to_replace); @@ -659,9 +649,18 @@ static void mirror_exit(Job *job) } if (s->should_complete && ret == 0) { - BlockDriverState *to_replace = src; - if (s->to_replace) { - to_replace = s->to_replace; + BlockDriverState *to_replace = s->to_replace ? s->to_replace : src; + + if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) { + BlockDriverState *backing = s->is_none_mode ? src : s->base; + if (backing_bs(target_bs) != backing) { + bdrv_set_backing_hd(target_bs, backing, &local_err); + if (local_err) { + error_report_err(local_err); + ret = -EPERM; + goto clean; + } + } } if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) { @@ -678,6 +677,8 @@ static void mirror_exit(Job *job) ret = -EPERM; } } + + clean: if (s->to_replace) { bdrv_op_unblock_all(s->to_replace, s->replace_blocker); error_free(s->replace_blocker);
In cases where we abort the block/mirror job, there's no point in installing the new backing chain before we finish aborting. Move this to the "success" portion of mirror_exit. Signed-off-by: John Snow <jsnow@redhat.com> --- block/mirror.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)