| Message ID | 20220121170544.2049944-32-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block layer: split block APIs in global state and I/O | expand |
On 21.01.22 18:05, Emanuele Giuseppe Esposito wrote: > Introduce .pre_run() job callback. This cb will run in job_start, > before the coroutine is created and runs run() in the job aiocontext. I presume this means “before the coroutine is created that will run run() in the job aiocontext”? (The way it’s written here sounds a bit like .pre_run() will run run()) > Therefore, .pre_run() always runs in the main loop. > We can use this function together with clean() cb to replace > bdrv_child_refresh_perms in block_crypto_amend_options_generic_luks(), > since that function can also be called from an iothread via > .bdrv_co_amend(). > > In addition, doing so we check for permissions in all bdrv > in amend, not only crypto. As I’ve written in my reply to patch 30, I’m not quite sold on this part, but disregarding that part (and one typo below), this patch looks good to me. > .pre_run() and .clean() take care of calling bdrv_amend_pre_run() > and bdrv_amend_clean() respectively, to set up driver-specific flags > and allow the crypto driver to temporarly provide the WRITE > perm to qcrypto_block_amend_options(). > > .pre_run() is not yet invoked by job_start, but .clean() is. > This is not a problem, since it will just be a redundant check > and crypto will have the update->keys flag == false anyways. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > block/amend.c | 33 +++++++++++++++++++++++++++++++++ > include/qemu/job.h | 9 +++++++++ > 2 files changed, 42 insertions(+) [...] > diff --git a/include/qemu/job.h b/include/qemu/job.h > index 4ea7a4a0cd..915ceff425 100644 > --- a/include/qemu/job.h > +++ b/include/qemu/job.h > @@ -223,6 +223,15 @@ struct JobDriver { > * the GS API. > */ > > + /** > + * Called in the Main Loop context, before the job coroutine > + * is started in the job aiocontext. Useful to perform operations > + * that needs to be done under BQL (like permissions setup). s/needs/need/ > + * On success, it returns 0. Otherwise the job failed to initialize > + * and won't start. > + */ > + int (*pre_run)(Job *job, Error **errp); > + > /** > * Called when the job is resumed by the user (i.e. user_paused becomes > * false). .user_resume is called before .resume.
Am 21.01.2022 um 18:05 hat Emanuele Giuseppe Esposito geschrieben: > Introduce .pre_run() job callback. This cb will run in job_start, > before the coroutine is created and runs run() in the job aiocontext. > > Therefore, .pre_run() always runs in the main loop. > We can use this function together with clean() cb to replace > bdrv_child_refresh_perms in block_crypto_amend_options_generic_luks(), > since that function can also be called from an iothread via > .bdrv_co_amend(). How is this different from having the same code in the function that creates the job, i.e. qmp_x_blockdev_amend()? Almost all block jobs have some setup code in the function that creates the job instead of doing everything in .run(), precisely because they know this code runs in the main thread. Is amend really so different from the other block jobs in this respect that it needs a different solution? > In addition, doing so we check for permissions in all bdrv > in amend, not only crypto. > > .pre_run() and .clean() take care of calling bdrv_amend_pre_run() > and bdrv_amend_clean() respectively, to set up driver-specific flags > and allow the crypto driver to temporarly provide the WRITE > perm to qcrypto_block_amend_options(). > > .pre_run() is not yet invoked by job_start, but .clean() is. > This is not a problem, since it will just be a redundant check > and crypto will have the update->keys flag == false anyways. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> I find the way how you split the patches a bit confusing because the patches aren't self-contained, but always refer to what the code will do in the future, because after the patch it's dead code that isn't even theoretically called until the final patch comes in. Can we restructure this a bit? First a patch that adds a new JobDriver callback (if really needed) along with the actual calls for it and everything else that needs to be touched in the generic job infrastructure. Second, new BlockDriver callbacks with all of the plumbing code. Third, the amend job changes with a patch that doesn't touch anything but block/amend.c and potentially block/crypto.c (the latter could also be another separate patch). This change with three or four patches could also be a candidate to be split out into a separate smaller series. Kevin
On 07/02/2022 19:14, Kevin Wolf wrote: > Am 21.01.2022 um 18:05 hat Emanuele Giuseppe Esposito geschrieben: >> Introduce .pre_run() job callback. This cb will run in job_start, >> before the coroutine is created and runs run() in the job aiocontext. >> >> Therefore, .pre_run() always runs in the main loop. >> We can use this function together with clean() cb to replace >> bdrv_child_refresh_perms in block_crypto_amend_options_generic_luks(), >> since that function can also be called from an iothread via >> .bdrv_co_amend(). > > How is this different from having the same code in the function that > creates the job, i.e. qmp_x_blockdev_amend()? > > Almost all block jobs have some setup code in the function that creates > the job instead of doing everything in .run(), precisely because they > know this code runs in the main thread. > > Is amend really so different from the other block jobs in this respect > that it needs a different solution? > Are you suggesting to simply call .bdrv_amend_pre_run before job_start() and just leave JobDriver .clean() to call .bdrv_amend_clean? Yes, that will work too. I will delete .pre_run(). >> In addition, doing so we check for permissions in all bdrv >> in amend, not only crypto. >> >> .pre_run() and .clean() take care of calling bdrv_amend_pre_run() >> and bdrv_amend_clean() respectively, to set up driver-specific flags >> and allow the crypto driver to temporarly provide the WRITE >> perm to qcrypto_block_amend_options(). >> >> .pre_run() is not yet invoked by job_start, but .clean() is. >> This is not a problem, since it will just be a redundant check >> and crypto will have the update->keys flag == false anyways. >> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > > I find the way how you split the patches a bit confusing because the > patches aren't self-contained, but always refer to what the code will do > in the future, because after the patch it's dead code that isn't even > theoretically called until the final patch comes in. > > Can we restructure this a bit? First a patch that adds a new JobDriver > callback (if really needed) along with the actual calls for it and > everything else that needs to be touched in the generic job > infrastructure. Second, new BlockDriver callbacks with all of the > plumbing code. Third, the amend job changes with a patch that doesn't > touch anything but block/amend.c and potentially block/crypto.c (the > latter could also be another separate patch). It is more or less what also Hanna suggested, I have it for the next version. > > This change with three or four patches could also be a candidate to be > split out into a separate smaller series. Makes sense. Emanuele > > Kevin >
diff --git a/block/amend.c b/block/amend.c index 392df9ef83..1618fd05a6 100644 --- a/block/amend.c +++ b/block/amend.c @@ -53,10 +53,43 @@ static int coroutine_fn blockdev_amend_run(Job *job, Error **errp) return ret; } +static int blockdev_amend_refresh_perms(Job *job, Error **errp) +{ + BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common); + return bdrv_child_refresh_perms(s->bs, s->bs->file, errp); +} + +static int blockdev_amend_pre_run(Job *job, Error **errp) +{ + BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common); + + if (s->bs->drv->bdrv_amend_pre_run) { + s->bs->drv->bdrv_amend_pre_run(s->bs); + } + return blockdev_amend_refresh_perms(job, errp); +} + +static void blockdev_amend_clean(Job *job) +{ + BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common); + Error *errp = NULL; + + if (s->bs->drv->bdrv_amend_clean) { + s->bs->drv->bdrv_amend_clean(s->bs); + } + + blockdev_amend_refresh_perms(job, &errp); + if (errp) { + error_report_err(errp); + } +} + static const JobDriver blockdev_amend_job_driver = { .instance_size = sizeof(BlockdevAmendJob), .job_type = JOB_TYPE_AMEND, .run = blockdev_amend_run, + .pre_run = blockdev_amend_pre_run, + .clean = blockdev_amend_clean, }; void qmp_x_blockdev_amend(const char *job_id, diff --git a/include/qemu/job.h b/include/qemu/job.h index 4ea7a4a0cd..915ceff425 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -223,6 +223,15 @@ struct JobDriver { * the GS API. */ + /** + * Called in the Main Loop context, before the job coroutine + * is started in the job aiocontext. Useful to perform operations + * that needs to be done under BQL (like permissions setup). + * On success, it returns 0. Otherwise the job failed to initialize + * and won't start. + */ + int (*pre_run)(Job *job, Error **errp); + /** * Called when the job is resumed by the user (i.e. user_paused becomes * false). .user_resume is called before .resume.
Introduce .pre_run() job callback. This cb will run in job_start, before the coroutine is created and runs run() in the job aiocontext. Therefore, .pre_run() always runs in the main loop. We can use this function together with clean() cb to replace bdrv_child_refresh_perms in block_crypto_amend_options_generic_luks(), since that function can also be called from an iothread via .bdrv_co_amend(). In addition, doing so we check for permissions in all bdrv in amend, not only crypto. .pre_run() and .clean() take care of calling bdrv_amend_pre_run() and bdrv_amend_clean() respectively, to set up driver-specific flags and allow the crypto driver to temporarly provide the WRITE perm to qcrypto_block_amend_options(). .pre_run() is not yet invoked by job_start, but .clean() is. This is not a problem, since it will just be a redundant check and crypto will have the update->keys flag == false anyways. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- block/amend.c | 33 +++++++++++++++++++++++++++++++++ include/qemu/job.h | 9 +++++++++ 2 files changed, 42 insertions(+)