| Message ID | 20211005143215.29500-12-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:01AM -0400, Emanuele Giuseppe Esposito wrote: > blockjob functions run always under the BQL lock. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > include/block/blockjob.h | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/include/block/blockjob.h b/include/block/blockjob.h > index d200f33c10..3bf384f8bf 100644 > --- a/include/block/blockjob.h > +++ b/include/block/blockjob.h > @@ -77,6 +77,27 @@ typedef struct BlockJob { > GSList *nodes; > } BlockJob; > > +/* > + * 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 functions below must use this assertion: > + * g_assert(qemu_in_main_thread()); > + * to catch when they are accidentally called without the BQL. > + */ This is comment is duplicated in many places. I suggest explaining it in one place and using references in the other files: /* * Global state (GS) API. These functions run under the BQL lock. * * See include/block/block.h for more information about the GS API. */
>> +/* >> + * 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 functions below must use this assertion: >> + * g_assert(qemu_in_main_thread()); >> + * to catch when they are accidentally called without the BQL. >> + */ > > This is comment is duplicated in many places. I suggest explaining it in > one place and using references in the other files: > > /* > * Global state (GS) API. These functions run under the BQL lock. > * > * See include/block/block.h for more information about the GS API. > */ > Good idea. Should I also do that for I/O, or it's not worth for very few lines? Thank you, Emanuele
On Fri, Oct 08, 2021 at 09:20:35AM +0200, Emanuele Giuseppe Esposito wrote: > > > > +/* > > > + * 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 functions below must use this assertion: > > > + * g_assert(qemu_in_main_thread()); > > > + * to catch when they are accidentally called without the BQL. > > > + */ > > > > This is comment is duplicated in many places. I suggest explaining it in > > one place and using references in the other files: > > > > /* > > * Global state (GS) API. These functions run under the BQL lock. > > * > > * See include/block/block.h for more information about the GS API. > > */ > > > > Good idea. Should I also do that for I/O, or it's not worth for very few > lines? Up to you, but I think it makes sense to have a minimal comment for both the GS and I/O API. Stefan
diff --git a/include/block/blockjob.h b/include/block/blockjob.h index d200f33c10..3bf384f8bf 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -77,6 +77,27 @@ typedef struct BlockJob { GSList *nodes; } BlockJob; +/* + * 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 functions below must use this assertion: + * g_assert(qemu_in_main_thread()); + * to catch when they are accidentally called without the BQL. + */ + /** * block_job_next: * @job: A block job, or %NULL. @@ -158,6 +179,8 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp); */ void block_job_iostatus_reset(BlockJob *job); +/* Common functions that are neither I/O nor Global State */ + /** * block_job_is_internal: * @job: The job to determine if it is user-visible or not.
blockjob functions run always under the BQL lock. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- include/block/blockjob.h | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)