What is bs->reqs_lock for?

What is bs->reqs_lock for?

[Vladimir Sementsov-Ogievskiy]

Hi!

Sorry my stupid question, but which kind of concurrent access bs->reqs_lock prevents?

In my understanding the whole logic of request tracking for the bs is going in the coroutine, so, we don't have parallel access anyway? How can parallel access to bs->tracked_requests happen?


-- 
Best regards,
Vladimir

Re: What is bs->reqs_lock for?

[Paolo Bonzini]

On 13/08/20 16:57, Vladimir Sementsov-Ogievskiy wrote:
> Hi!
> 
> Sorry my stupid question, but which kind of concurrent access
> bs->reqs_lock prevents?
> 
> In my understanding the whole logic of request tracking for the bs is
> going in the coroutine, so, we don't have parallel access anyway? How
> can parallel access to bs->tracked_requests happen?

Different iothreads can access the same BlockDriverState, and block/io.c
is not protected by the AioContext lock (in fact almost nothing, or
nothing, needs it in the I/O path).

Paolo

Re: What is bs->reqs_lock for?

[Vladimir Sementsov-Ogievskiy]

13.08.2020 18:54, Paolo Bonzini wrote:
> On 13/08/20 16:57, Vladimir Sementsov-Ogievskiy wrote:
>> Hi!
>>
>> Sorry my stupid question, but which kind of concurrent access
>> bs->reqs_lock prevents?
>>
>> In my understanding the whole logic of request tracking for the bs is
>> going in the coroutine, so, we don't have parallel access anyway? How
>> can parallel access to bs->tracked_requests happen?
> 
> Different iothreads can access the same BlockDriverState, and block/io.c
> is not protected by the AioContext lock (in fact almost nothing, or
> nothing, needs it in the I/O path).
> 

I thought bs is attached to one aio context and aio context attached to one iothread.
And all normal request processing of the bs is done in this one iothread.
And when we need to access bs externally, we do it in
aio_context_acquire / aio_context_release, which protects from parallel access to
BlockDriverState fields...

But you say, that block/io.c is not protected by AioContext lock..

Does it mean that everything must be thread-safe in block/io.c and all block drivers?

Are tracked_requests different from other fields? A lot of other BlockDriverState
fields are not protected by any mutex.. For example: total_sectors, file, backing..

Could you give an example of parallel access to tracked_requests?

-- 
Best regards,
Vladimir

Re: What is bs->reqs_lock for?

[Paolo Bonzini]

On 13/08/20 18:34, Vladimir Sementsov-Ogievskiy wrote:
> I thought bs is attached to one aio context and aio context attached to
> one iothread.

For now yes, but with multiqueue there would be many iothreads sending
requests to the AioContext.  The BDS would still have a "home"
aiocontext to request socket readiness events, but
io_uring/linux_aio/threadpool requests could be issued from any iothread.

> And all normal request processing of the bs is done in this one iothread.
> And when we need to access bs externally, we do it in
> aio_context_acquire / aio_context_release, which protects from parallel
> access to BlockDriverState fields...
> 
> But you say, that block/io.c is not protected by AioContext lock..
> Does it mean that everything must be thread-safe in block/io.c and all
> block drivers?

Yes.

> 
> Are tracked_requests different from other fields? A lot of other
> BlockDriverState
> fields are not protected by any mutex.. For example: total_sectors,
> file, backing..

Rules are documented in include/block/block_int.h.  It seems however
that never_freeze was blindly added at the end.

Paolo

> Could you give an example of parallel access to tracked_requests?
> 

Re: What is bs->reqs_lock for?

[Vladimir Sementsov-Ogievskiy]

18.08.2020 09:16, Paolo Bonzini wrote:
> On 13/08/20 18:34, Vladimir Sementsov-Ogievskiy wrote:
>> I thought bs is attached to one aio context and aio context attached to
>> one iothread.
> 
> For now yes, but with multiqueue there would be many iothreads sending
> requests to the AioContext.  The BDS would still have a "home"
> aiocontext to request socket readiness events, but
> io_uring/linux_aio/threadpool requests could be issued from any iothread.
> 
>> And all normal request processing of the bs is done in this one iothread.
>> And when we need to access bs externally, we do it in
>> aio_context_acquire / aio_context_release, which protects from parallel
>> access to BlockDriverState fields...
>>
>> But you say, that block/io.c is not protected by AioContext lock..
>> Does it mean that everything must be thread-safe in block/io.c and all
>> block drivers?
> 
> Yes.
> 
>>
>> Are tracked_requests different from other fields? A lot of other
>> BlockDriverState
>> fields are not protected by any mutex.. For example: total_sectors,
>> file, backing..
> 
> Rules are documented in include/block/block_int.h. 

I should have guessed on my own..

> It seems however
> that never_freeze was blindly added at the end.
> 

Thanks for your answers!


-- 
Best regards,
Vladimir