| Message ID | 1452558972-20316-5-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 12/01/2016 01:36, John Snow wrote: > Instead of relying on peeking at bs->job, we want to explicitly get > a reference to the job that was involved in this notifier callback. > > Extend the Notifier to include a job pointer, and include a reference > to the job registering the callback. This cuts out a few more cases > where we have to rely on bs->job. > > Signed-off-by: John Snow <jsnow@redhat.com> Why don't you just put the NotifierWithReturn inside the BackupBlockJob struct, and use container_of to get from NWR to BackupBlockJob? Paolo
On 01/12/2016 03:46 AM, Paolo Bonzini wrote: > > > On 12/01/2016 01:36, John Snow wrote: >> Instead of relying on peeking at bs->job, we want to explicitly get >> a reference to the job that was involved in this notifier callback. >> >> Extend the Notifier to include a job pointer, and include a reference >> to the job registering the callback. This cuts out a few more cases >> where we have to rely on bs->job. >> >> Signed-off-by: John Snow <jsnow@redhat.com> > > Why don't you just put the NotifierWithReturn inside the BackupBlockJob > struct, and use container_of to get from NWR to BackupBlockJob? > > Paolo > That's another way (including the notifier within the job vs. including the job within the notifier.) This one simply occurred to me first. Any strong benefit to that method, or just a matter of style? --js
On 12/01/2016 18:57, John Snow wrote: > > > On 01/12/2016 03:46 AM, Paolo Bonzini wrote: >> >> >> On 12/01/2016 01:36, John Snow wrote: >>> Instead of relying on peeking at bs->job, we want to explicitly get >>> a reference to the job that was involved in this notifier callback. >>> >>> Extend the Notifier to include a job pointer, and include a reference >>> to the job registering the callback. This cuts out a few more cases >>> where we have to rely on bs->job. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >> >> Why don't you just put the NotifierWithReturn inside the BackupBlockJob >> struct, and use container_of to get from NWR to BackupBlockJob? >> >> Paolo >> > > That's another way (including the notifier within the job vs. including > the job within the notifier.) This one simply occurred to me first. Any > strong benefit to that method, or just a matter of style? It's usually the one that is used with notifiers, no other reason. Paolo
On 01/12/2016 01:01 PM, Paolo Bonzini wrote: > > > On 12/01/2016 18:57, John Snow wrote: >> >> >> On 01/12/2016 03:46 AM, Paolo Bonzini wrote: >>> >>> >>> On 12/01/2016 01:36, John Snow wrote: >>>> Instead of relying on peeking at bs->job, we want to explicitly get >>>> a reference to the job that was involved in this notifier callback. >>>> >>>> Extend the Notifier to include a job pointer, and include a reference >>>> to the job registering the callback. This cuts out a few more cases >>>> where we have to rely on bs->job. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>> >>> Why don't you just put the NotifierWithReturn inside the BackupBlockJob >>> struct, and use container_of to get from NWR to BackupBlockJob? >>> >>> Paolo >>> >> >> That's another way (including the notifier within the job vs. including >> the job within the notifier.) This one simply occurred to me first. Any >> strong benefit to that method, or just a matter of style? > > It's usually the one that is used with notifiers, no other reason. > > > Paolo > I'll follow convention, I just didn't bump into an example to model. --js
Am 12.01.2016 um 19:02 hat John Snow geschrieben: > > > On 01/12/2016 01:01 PM, Paolo Bonzini wrote: > > > > > > On 12/01/2016 18:57, John Snow wrote: > >> > >> > >> On 01/12/2016 03:46 AM, Paolo Bonzini wrote: > >>> > >>> > >>> On 12/01/2016 01:36, John Snow wrote: > >>>> Instead of relying on peeking at bs->job, we want to explicitly get > >>>> a reference to the job that was involved in this notifier callback. > >>>> > >>>> Extend the Notifier to include a job pointer, and include a reference > >>>> to the job registering the callback. This cuts out a few more cases > >>>> where we have to rely on bs->job. > >>>> > >>>> Signed-off-by: John Snow <jsnow@redhat.com> > >>> > >>> Why don't you just put the NotifierWithReturn inside the BackupBlockJob > >>> struct, and use container_of to get from NWR to BackupBlockJob? > >>> > >>> Paolo > >>> > >> > >> That's another way (including the notifier within the job vs. including > >> the job within the notifier.) This one simply occurred to me first. Any > >> strong benefit to that method, or just a matter of style? > > > > It's usually the one that is used with notifiers, no other reason. > > I'll follow convention, I just didn't bump into an example to model. This means that I should wait for a v2? (Hm, or is this Markus' area, actually? Or Jeff's?) Otherwise, this series is: Reviewed-by: Kevin Wolf <kwolf@redhat.com>
On 01/18/2016 09:29 AM, Kevin Wolf wrote: > Am 12.01.2016 um 19:02 hat John Snow geschrieben: >> >> >> On 01/12/2016 01:01 PM, Paolo Bonzini wrote: >>> >>> >>> On 12/01/2016 18:57, John Snow wrote: >>>> >>>> >>>> On 01/12/2016 03:46 AM, Paolo Bonzini wrote: >>>>> >>>>> >>>>> On 12/01/2016 01:36, John Snow wrote: >>>>>> Instead of relying on peeking at bs->job, we want to explicitly get >>>>>> a reference to the job that was involved in this notifier callback. >>>>>> >>>>>> Extend the Notifier to include a job pointer, and include a reference >>>>>> to the job registering the callback. This cuts out a few more cases >>>>>> where we have to rely on bs->job. >>>>>> >>>>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>>> >>>>> Why don't you just put the NotifierWithReturn inside the BackupBlockJob >>>>> struct, and use container_of to get from NWR to BackupBlockJob? >>>>> >>>>> Paolo >>>>> >>>> >>>> That's another way (including the notifier within the job vs. including >>>> the job within the notifier.) This one simply occurred to me first. Any >>>> strong benefit to that method, or just a matter of style? >>> >>> It's usually the one that is used with notifiers, no other reason. >> >> I'll follow convention, I just didn't bump into an example to model. > > This means that I should wait for a v2? (Hm, or is this Markus' area, > actually? Or Jeff's?) > > Otherwise, this series is: > Reviewed-by: Kevin Wolf <kwolf@redhat.com> > I hadn't re-rolled just yet, it seems like a matter of taste but I usually defer to convention for predictability's sake. Waiting for Jeff, primarily. --js
diff --git a/block/backup.c b/block/backup.c index 325e247..58c76be 100644 --- a/block/backup.c +++ b/block/backup.c @@ -89,11 +89,11 @@ static void cow_request_end(CowRequest *req) } static int coroutine_fn backup_do_cow(BlockDriverState *bs, + BackupBlockJob *job, int64_t sector_num, int nb_sectors, bool *error_is_read, bool is_write_notifier) { - BackupBlockJob *job = (BackupBlockJob *)bs->job; CowRequest cow_request; struct iovec iov; QEMUIOVector bounce_qiov; @@ -187,10 +187,17 @@ out: return ret; } +/* Extend the generic Notifier interface */ +typedef struct BackupNotifier { + NotifierWithReturn common; + BackupBlockJob *job; +} BackupNotifier; + static int coroutine_fn backup_before_write_notify( NotifierWithReturn *notifier, void *opaque) { + BackupNotifier *bnotifier = (BackupNotifier *)notifier; BdrvTrackedRequest *req = opaque; int64_t sector_num = req->offset >> BDRV_SECTOR_BITS; int nb_sectors = req->bytes >> BDRV_SECTOR_BITS; @@ -198,7 +205,8 @@ static int coroutine_fn backup_before_write_notify( assert((req->offset & (BDRV_SECTOR_SIZE - 1)) == 0); assert((req->bytes & (BDRV_SECTOR_SIZE - 1)) == 0); - return backup_do_cow(req->bs, sector_num, nb_sectors, NULL, true); + return backup_do_cow(req->bs, bnotifier->job, sector_num, + nb_sectors, NULL, true); } static void backup_set_speed(BlockJob *job, int64_t speed, Error **errp) @@ -346,7 +354,8 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job) if (yield_and_check(job)) { return ret; } - ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER, + ret = backup_do_cow(bs, job, + cluster * BACKUP_SECTORS_PER_CLUSTER, BACKUP_SECTORS_PER_CLUSTER, &error_is_read, false); if ((ret < 0) && @@ -382,8 +391,11 @@ static void coroutine_fn backup_run(void *opaque) BlockDriverState *bs = job->common.bs; BlockDriverState *target = job->target; BlockdevOnError on_target_error = job->on_target_error; - NotifierWithReturn before_write = { - .notify = backup_before_write_notify, + BackupNotifier before_write = { + .common = { + .notify = backup_before_write_notify, + }, + .job = job, }; int64_t start, end; int ret = 0; @@ -402,7 +414,8 @@ static void coroutine_fn backup_run(void *opaque) blk_iostatus_enable(target->blk); } - bdrv_add_before_write_notifier(bs, &before_write); + block_job_ref(&job->common); + bdrv_add_before_write_notifier(bs, (NotifierWithReturn *)&before_write); if (job->sync_mode == MIRROR_SYNC_MODE_NONE) { while (!block_job_is_cancelled(&job->common)) { @@ -454,7 +467,7 @@ static void coroutine_fn backup_run(void *opaque) } } /* FULL sync mode we copy the whole drive. */ - ret = backup_do_cow(bs, start * BACKUP_SECTORS_PER_CLUSTER, + ret = backup_do_cow(bs, job, start * BACKUP_SECTORS_PER_CLUSTER, BACKUP_SECTORS_PER_CLUSTER, &error_is_read, false); if (ret < 0) { /* Depending on error action, fail now or retry cluster */ @@ -470,7 +483,8 @@ static void coroutine_fn backup_run(void *opaque) } } - notifier_with_return_remove(&before_write); + notifier_with_return_remove((NotifierWithReturn *)&before_write); + block_job_unref(&job->common); /* wait until pending backup_do_cow() calls have completed */ qemu_co_rwlock_wrlock(&job->flush_rwlock);
Instead of relying on peeking at bs->job, we want to explicitly get a reference to the job that was involved in this notifier callback. Extend the Notifier to include a job pointer, and include a reference to the job registering the callback. This cuts out a few more cases where we have to rely on bs->job. Signed-off-by: John Snow <jsnow@redhat.com> --- block/backup.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)