| Message ID | 20211005143215.29500-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 Tue, Oct 05, 2021 at 10:32:14AM -0400, Emanuele Giuseppe Esposito wrote: > The job API will be handled separately in another serie. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > include/qemu/job.h | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/include/qemu/job.h b/include/qemu/job.h > index 41162ed494..c236c43026 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; > > + /* > + * I/O API functions. These functions are thread-safe, and therefore > + * can run in any thread as long as they have called > + * aio_context_acquire/release(). > + */ This comment described the block layer well but job.h is more generic. I don't think these functions are necessarily thread-safe (they shouldn't be invoked from multiple threads at the same time). It's just that they are run without regard to the BQL and may run in an arbitrary thread. Other than that: Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On 07/10/2021 16:54, Stefan Hajnoczi wrote: > On Tue, Oct 05, 2021 at 10:32:14AM -0400, Emanuele Giuseppe Esposito wrote: >> The job API will be handled separately in another serie. >> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> --- >> include/qemu/job.h | 31 +++++++++++++++++++++++++++++++ >> 1 file changed, 31 insertions(+) >> >> diff --git a/include/qemu/job.h b/include/qemu/job.h >> index 41162ed494..c236c43026 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; >> >> + /* >> + * I/O API functions. These functions are thread-safe, and therefore >> + * can run in any thread as long as they have called >> + * aio_context_acquire/release(). >> + */ > > This comment described the block layer well but job.h is more generic. I > don't think these functions are necessarily thread-safe (they shouldn't > be invoked from multiple threads at the same time). It's just that they > are run without regard to the BQL and may run in an arbitrary thread. Ok, in this instance I will change it to: /* * Functions run without regard to the BQL and may run in any * arbitrary thread. */ Thank you, Emanuele > > Other than that: > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >
On Fri, Oct 08, 2021 at 12:48:26PM +0200, Emanuele Giuseppe Esposito wrote: > > > On 07/10/2021 16:54, Stefan Hajnoczi wrote: > > On Tue, Oct 05, 2021 at 10:32:14AM -0400, Emanuele Giuseppe Esposito wrote: > > > The job API will be handled separately in another serie. > > > > > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > > > --- > > > include/qemu/job.h | 31 +++++++++++++++++++++++++++++++ > > > 1 file changed, 31 insertions(+) > > > > > > diff --git a/include/qemu/job.h b/include/qemu/job.h > > > index 41162ed494..c236c43026 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; > > > + /* > > > + * I/O API functions. These functions are thread-safe, and therefore > > > + * can run in any thread as long as they have called > > > + * aio_context_acquire/release(). > > > + */ > > > > This comment described the block layer well but job.h is more generic. I > > don't think these functions are necessarily thread-safe (they shouldn't > > be invoked from multiple threads at the same time). It's just that they > > are run without regard to the BQL and may run in an arbitrary thread. > > Ok, in this instance I will change it to: > > /* > * Functions run without regard to the BQL and may run in any > * arbitrary thread. > */ It's not clear from that comment whether the functions need to be thread-safe (re-entrancy safe) or not. I think the answer is "no" since the caller ensures they are called from one thread at a time. Stefan
diff --git a/include/qemu/job.h b/include/qemu/job.h index 41162ed494..c236c43026 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; + /* + * I/O API functions. These functions are thread-safe, and therefore + * can run in any thread as long as they have called + * aio_context_acquire/release(). + */ + /** * Mandatory: Entrypoint for the Coroutine. * @@ -201,6 +210,28 @@ struct JobDriver { */ void coroutine_fn (*resume)(Job *job); + /* + * Global state (GS) API. These functions run under the BQL lock. + * + * If a function modifies the graph, it also uses drain and/or + * aio_context_acquire/release to be sure it has unique access. + * aio_context locking is needed together with BQL because of + * the thread-safe I/O API that concurrently runs and accesses + * the graph without the BQL. + * + * It is important to note that not all of these functions are + * necessarily limited to running under the BQL, but they would + * require additional auditing and may small thread-safety changes + * to move them into the I/O API. Often it's not worth doing that + * work since the APIs are only used with the BQL held at the + * moment, so they have been placed in the GS API (for now). + * + * All callers that use these function pointers must + * use this assertion: + * g_assert(qemu_in_main_thread()); + * to catch when they are accidentally called without the BQL. + */ + /** * Called when the job is resumed by the user (i.e. user_paused becomes * false). .user_resume is called before .resume.
The job API will be handled separately in another serie. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- include/qemu/job.h | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)