Re: [PATCH v4 24/25] job.h: split function pointers in JobDriver
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
Reviewed-by: Stefan Hajnoczi
---
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
Re: [PATCH v4 24/25] job.h: split function pointers in JobDriver
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
Reviewed-by: Stefan Hajnoczi
---
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
Re: [PATCH v4 24/25] job.h: split function pointers in JobDriver
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
Reviewed-by: Stefan Hajnoczi
---
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
[PATCH v4 24/25] job.h: split function pointers in JobDriver
The job API will be handled separately in another serie.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Stefan Hajnoczi
---
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. */
+
/** 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.
--
2.27.0
