| Message ID | 20211025101735.2060852-25-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 25.10.21 12:17, Emanuele Giuseppe Esposito wrote: > The job API will be handled separately in another serie. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > include/qemu/job.h | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/include/qemu/job.h b/include/qemu/job.h > index 6e67b6977f..7e9e59f4b8 100644 > --- a/include/qemu/job.h > +++ b/include/qemu/job.h > @@ -169,12 +169,21 @@ typedef struct Job { > * Callbacks and other information about a Job driver. > */ > struct JobDriver { > + > + /* Fields initialized in struct definition and never changed. */ Like in patch 19, I’d prefer a slightly more verbose comment that I’d find more easily readable. > + > /** Derived Job struct size */ > size_t instance_size; > > /** Enum describing the operation */ > JobType job_type; > > + /* > + * Functions run without regard to the BQL and may run in any s/and/that/? > + * arbitrary thread. These functions do not need to be thread-safe > + * because the caller ensures that are invoked from one thread at time. s/that/they/ (or “that they”) I believe .run() must be run in the job’s context, though. Not sure if that’s necessary to note, but it isn’t really an arbitrary thread, and block jobs certainly require this (because they run in the block device’s context). Or is that something that’s going to change with I/O threading? Hanna
On 15/11/2021 16:11, Hanna Reitz wrote: > On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote: >> The job API will be handled separately in another serie. >> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >> --- >> include/qemu/job.h | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/include/qemu/job.h b/include/qemu/job.h >> index 6e67b6977f..7e9e59f4b8 100644 >> --- a/include/qemu/job.h >> +++ b/include/qemu/job.h >> @@ -169,12 +169,21 @@ typedef struct Job { >> * Callbacks and other information about a Job driver. >> */ >> struct JobDriver { >> + >> + /* Fields initialized in struct definition and never changed. */ > > Like in patch 19, I’d prefer a slightly more verbose comment that I’d > find more easily readable. > >> + >> /** Derived Job struct size */ >> size_t instance_size; >> /** Enum describing the operation */ >> JobType job_type; >> + /* >> + * Functions run without regard to the BQL and may run in any > > s/and/that/? > >> + * arbitrary thread. These functions do not need to be thread-safe >> + * because the caller ensures that are invoked from one thread at >> time. > > s/that/they/ (or “that they”) > > I believe .run() must be run in the job’s context, though. Not sure if > that’s necessary to note, but it isn’t really an arbitrary thread, and > block jobs certainly require this (because they run in the block > device’s context). Or is that something that’s going to change with I/O > threading? > What about moving .run() before the comment and add "Must be run in the job's context" to its comment description? maybe also add the following assertion in job_co_entry (that calls job->run())? assert(job->aio_context == qemu_get_current_aio_context()); Emanuele
On 17.11.21 14:43, Emanuele Giuseppe Esposito wrote: > > > On 15/11/2021 16:11, Hanna Reitz wrote: >> On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote: >>> The job API will be handled separately in another serie. >>> >>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >>> --- >>> include/qemu/job.h | 16 ++++++++++++++++ >>> 1 file changed, 16 insertions(+) >>> >>> diff --git a/include/qemu/job.h b/include/qemu/job.h >>> index 6e67b6977f..7e9e59f4b8 100644 >>> --- a/include/qemu/job.h >>> +++ b/include/qemu/job.h >>> @@ -169,12 +169,21 @@ typedef struct Job { >>> * Callbacks and other information about a Job driver. >>> */ >>> struct JobDriver { >>> + >>> + /* Fields initialized in struct definition and never changed. */ >> >> Like in patch 19, I’d prefer a slightly more verbose comment that I’d >> find more easily readable. >> >>> + >>> /** Derived Job struct size */ >>> size_t instance_size; >>> /** Enum describing the operation */ >>> JobType job_type; >>> + /* >>> + * Functions run without regard to the BQL and may run in any >> >> s/and/that/? >> >>> + * arbitrary thread. These functions do not need to be thread-safe >>> + * because the caller ensures that are invoked from one thread >>> at time. >> >> s/that/they/ (or “that they”) >> >> I believe .run() must be run in the job’s context, though. Not sure >> if that’s necessary to note, but it isn’t really an arbitrary thread, >> and block jobs certainly require this (because they run in the block >> device’s context). Or is that something that’s going to change with >> I/O threading? >> > > What about moving .run() before the comment and add "Must be run in > the job's context" to its comment description? Sure, works for me. > maybe also add the following assertion in job_co_entry (that calls > job->run())? > > assert(job->aio_context == qemu_get_current_aio_context()); Sounds good! Hanna
diff --git a/include/qemu/job.h b/include/qemu/job.h index 6e67b6977f..7e9e59f4b8 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -169,12 +169,21 @@ typedef struct Job { * Callbacks and other information about a Job driver. */ struct JobDriver { + + /* Fields initialized in struct definition and never changed. */ + /** Derived Job struct size */ size_t instance_size; /** Enum describing the operation */ JobType job_type; + /* + * Functions run without regard to the BQL and may run in any + * arbitrary thread. These functions do not need to be thread-safe + * because the caller ensures that are invoked from one thread at time. + */ + /** * Mandatory: Entrypoint for the Coroutine. * @@ -201,6 +210,13 @@ struct JobDriver { */ void coroutine_fn (*resume)(Job *job); + /* + * Global state (GS) API. These functions run under the BQL lock. + * + * See include/block/block-global-state.h for more information about + * the GS API. + */ + /** * Called when the job is resumed by the user (i.e. user_paused becomes * false). .user_resume is called before .resume.