Re: [Qemu-devel] [PATCH v4 04/12] migration: avoid concurrent invoke channel_close by different threads

[858585 jemmy]

On Wed, May 30, 2018 at 10:45 PM, Dr. David Alan Gilbert
<address@hidden> wrote:
> * Lidong Chen (address@hidden) wrote:
>> From: Lidong Chen <address@hidden>
>>
>> The channel_close maybe invoked by different threads. For example, source
>> qemu invokes qemu_fclose in main thread, migration thread and return path
>> thread. Destination qemu invokes qemu_fclose in main thread, listen thread
>> and COLO incoming thread.
>>
>> Add a mutex in QEMUFile struct to avoid concurrent invoke channel_close.
>>
>> Signed-off-by: Lidong Chen <address@hidden>
>> ---
>>  migration/qemu-file.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
>> index 977b9ae..87d0f05 100644
>> --- a/migration/qemu-file.c
>> +++ b/migration/qemu-file.c
>> @@ -52,6 +52,7 @@ struct QEMUFile {
>>      unsigned int iovcnt;
>>
>>      int last_error;
>> +    QemuMutex lock;
>
> That could do with a comment saying what you're protecting
>
>>  };
>>
>>  /*
>> @@ -96,6 +97,7 @@ QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps 
>> *ops)
>>
>>      f = g_new0(QEMUFile, 1);
>>
>> +    qemu_mutex_init(&f->lock);
>>      f->opaque = opaque;
>>      f->ops = ops;
>>      return f;
>> @@ -328,7 +330,9 @@ int qemu_fclose(QEMUFile *f)
>>      ret = qemu_file_get_error(f);
>>
>>      if (f->ops->close) {
>> +        qemu_mutex_lock(&f->lock);
>>          int ret2 = f->ops->close(f->opaque);
>> +        qemu_mutex_unlock(&f->lock);
>
> OK, and at least for the RDMA code, if it calls
> close a 2nd time, rioc->rdma is checked so it wont actually free stuff a
> 2nd time.
>
>>          if (ret >= 0) {
>>              ret = ret2;
>>          }
>> @@ -339,6 +343,7 @@ int qemu_fclose(QEMUFile *f)
>>      if (f->last_error) {
>>          ret = f->last_error;
>>      }
>> +    qemu_mutex_destroy(&f->lock);
>>      g_free(f);
>
> Hmm but that's not safe; if two things really do call qemu_fclose()
> on the same structure they race here and can end up destroying the lock
> twice, or doing f->lock  after the 1st one has already g_free(f).

>
>
> So lets go back a step.
> I think:
>   a) There should always be a separate QEMUFile* for
>      to_src_file and from_src_file - I don't see where you open
>      the 2nd one; I don't see your implementation of
>      f->ops->get_return_path.

yes, current qemu version use a separate QEMUFile* for to_src_file and
from_src_file.
and the two QEMUFile point to one QIOChannelRDMA.

the f->ops->get_return_path is implemented by channel_output_ops or
channel_input_ops.

>   b) I *think* that while the different threads might all call
>      fclose(), I think there should only ever be one qemu_fclose
>      call for each direction on the QEMUFile.
>
> But now we have two problems:
>   If (a) is true then f->lock  is separate on each one so
>    doesn't really protect if the two directions are closed
>    at once. (Assuming (b) is true)

yes, you are right.  so I should add a QemuMutex in QIOChannel structure, not
QEMUFile structure. and qemu_mutex_destroy the QemuMutex in
qio_channel_finalize.

Thank you.

>
>   If (a) is false and we actually share a single QEMUFile then
>  that race at the end happens.
>
> Dave
>
>
>>      trace_qemu_file_fclose();
>>      return ret;
>> --
>> 1.8.3.1
>>
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK