[Qemu-devel] [PATCH] posix-aio: don't set aiocb->ret/active outside critical section

[Qemu-devel] [PATCH] posix-aio: don't set aiocb->ret/active outside critical section

[Jim Meyering]

Move code that sets aiocb->ret and aiocb->active into critical section.
All other accesses are lock-guarded.  Spotted by coverity.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
I've included enough context to show one of the
guarded uses in the following function.

 posix-aio-compat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/posix-aio-compat.c b/posix-aio-compat.c
index 68361f5..45511d4 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -421,37 +421,37 @@ static void spawn_thread(void)
 {
     cur_threads++;
     new_threads++;
     /* If there are threads being created, they will spawn new workers, so
      * we don't spend time creating many threads in a loop holding a mutex or
      * starving the current vcpu.
      *
      * If there are no idle threads, ask the main thread to create one, so we
      * inherit the correct affinity instead of the vcpu affinity.
      */
     if (!pending_threads) {
         qemu_bh_schedule(new_thread_bh);
     }
 }

 static void qemu_paio_submit(struct qemu_paiocb *aiocb)
 {
+    mutex_lock(&lock);
     aiocb->ret = -EINPROGRESS;
     aiocb->active = 0;
-    mutex_lock(&lock);
     if (idle_threads == 0 && cur_threads < max_threads)
         spawn_thread();
     QTAILQ_INSERT_TAIL(&request_list, aiocb, node);
     mutex_unlock(&lock);
     cond_signal(&cond);
 }

 static ssize_t qemu_paio_return(struct qemu_paiocb *aiocb)
 {
     ssize_t ret;

     mutex_lock(&lock);
     ret = aiocb->ret;
     mutex_unlock(&lock);

     return ret;
 }
--
1.7.10.2.484.gcd07cc5

Re: [Qemu-devel] [PATCH] posix-aio: don't set aiocb->ret/active outside critical section

[Kevin Wolf]

Am 15.05.2012 13:27, schrieb Jim Meyering:
> 
> Move code that sets aiocb->ret and aiocb->active into critical section.
> All other accesses are lock-guarded.  Spotted by coverity.
> 
> Signed-off-by: Jim Meyering <meyering@redhat.com>
> ---
> I've included enough context to show one of the
> guarded uses in the following function.
> 
>  posix-aio-compat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/posix-aio-compat.c b/posix-aio-compat.c
> index 68361f5..45511d4 100644
> --- a/posix-aio-compat.c
> +++ b/posix-aio-compat.c
> @@ -421,37 +421,37 @@ static void spawn_thread(void)
>  {
>      cur_threads++;
>      new_threads++;
>      /* If there are threads being created, they will spawn new workers, so
>       * we don't spend time creating many threads in a loop holding a mutex or
>       * starving the current vcpu.
>       *
>       * If there are no idle threads, ask the main thread to create one, so we
>       * inherit the correct affinity instead of the vcpu affinity.
>       */
>      if (!pending_threads) {
>          qemu_bh_schedule(new_thread_bh);
>      }
>  }
> 
>  static void qemu_paio_submit(struct qemu_paiocb *aiocb)
>  {
> +    mutex_lock(&lock);
>      aiocb->ret = -EINPROGRESS;
>      aiocb->active = 0;
> -    mutex_lock(&lock);
>      if (idle_threads == 0 && cur_threads < max_threads)
>          spawn_thread();
>      QTAILQ_INSERT_TAIL(&request_list, aiocb, node);
>      mutex_unlock(&lock);
>      cond_signal(&cond);
>  }

This is just silencing coverity, not really fixing a bug, right? aiocb
is inserted into request_lists only afterwards, and before it is in the
list, other threads can't find it.

I'm just wondering why coverity doesn't complain about the callers of
qemu_paio_submit(), they access aiocb as well...

Kevin

Re: [Qemu-devel] [PATCH] posix-aio: don't set aiocb->ret/active outside critical section

[Paolo Bonzini]

Il 15/05/2012 13:34, Kevin Wolf ha scritto:
>>  static void qemu_paio_submit(struct qemu_paiocb *aiocb)
>>  {
>> +    mutex_lock(&lock);
>>      aiocb->ret = -EINPROGRESS;
>>      aiocb->active = 0;
>> -    mutex_lock(&lock);
>>      if (idle_threads == 0 && cur_threads < max_threads)
>>          spawn_thread();
>>      QTAILQ_INSERT_TAIL(&request_list, aiocb, node);
>>      mutex_unlock(&lock);
>>      cond_signal(&cond);
>>  }
> 
> This is just silencing coverity, not really fixing a bug, right? aiocb
> is inserted into request_lists only afterwards, and before it is in the
> list, other threads can't find it.

Indeed... I don't like the patch, the access is out of the lock for a
reason.

Paolo

Re: [Qemu-devel] [PATCH] posix-aio: don't set aiocb->ret/active outside critical section

[Jim Meyering]

Kevin Wolf wrote:
> Am 15.05.2012 13:27, schrieb Jim Meyering:
>>
>> Move code that sets aiocb->ret and aiocb->active into critical section.
>> All other accesses are lock-guarded.  Spotted by coverity.
>>
>> Signed-off-by: Jim Meyering <meyering@redhat.com>
>> ---
>> I've included enough context to show one of the
>> guarded uses in the following function.
>>
>>  posix-aio-compat.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/posix-aio-compat.c b/posix-aio-compat.c
>> index 68361f5..45511d4 100644
>> --- a/posix-aio-compat.c
>> +++ b/posix-aio-compat.c
>> @@ -421,37 +421,37 @@ static void spawn_thread(void)
>>  {
>>      cur_threads++;
>>      new_threads++;
>>      /* If there are threads being created, they will spawn new workers, so
>>       * we don't spend time creating many threads in a loop holding a mutex or
>>       * starving the current vcpu.
>>       *
>>       * If there are no idle threads, ask the main thread to create one, so we
>>       * inherit the correct affinity instead of the vcpu affinity.
>>       */
>>      if (!pending_threads) {
>>          qemu_bh_schedule(new_thread_bh);
>>      }
>>  }
>>
>>  static void qemu_paio_submit(struct qemu_paiocb *aiocb)
>>  {
>> +    mutex_lock(&lock);
>>      aiocb->ret = -EINPROGRESS;
>>      aiocb->active = 0;
>> -    mutex_lock(&lock);
>>      if (idle_threads == 0 && cur_threads < max_threads)
>>          spawn_thread();
>>      QTAILQ_INSERT_TAIL(&request_list, aiocb, node);
>>      mutex_unlock(&lock);
>>      cond_signal(&cond);
>>  }
>
> This is just silencing coverity, not really fixing a bug, right? aiocb

Yes.  Given your explanation, it's obvious, now.

> is inserted into request_lists only afterwards, and before it is in the
> list, other threads can't find it.
>
> I'm just wondering why coverity doesn't complain about the callers of
> qemu_paio_submit(), they access aiocb as well...

It warned only about the setting of aiocb->ret (not about ->active),
apparently because it failed to deduce that the lock is not actually
required here, while several other ->ret accesses *are* guarded.