Re: [Qemu-devel] [PATCH 3/3] Add helper functions to enable virtio-9p make use of the threadlets

[Stefan Hajnoczi]

On Wed, Nov 10, 2010 at 1:41 AM, Venkateswararao Jujjuri (JV)
<address@hidden> wrote:
> On 11/9/2010 1:06 AM, Stefan Hajnoczi wrote:
>> On Mon, Nov 8, 2010 at 10:47 AM, Arun R Bharadwaj
>> <address@hidden> wrote:
>>> +
>>> +static struct {
>>> +    int rfd;
>>> +    int wfd;
>>> +    QemuMutex lock;
>>> +    QTAILQ_HEAD(, v9fs_post_op) post_op_list;
>>> +} v9fs_async_struct;
>>> +
>>> +static void die2(int err, const char *what)
>>> +{
>>> +    fprintf(stderr, "%s failed: %s\n", what, strerror(err));
>>> +    abort();
>>> +}
>>> +
>>> +static void die(const char *what)
>>> +{
>>> +    die2(errno, what);
>>> +}
>>> +
>>> +#define ASYNC_MAX_PROCESS   5
>>> +
>>> +/**
>>> + * v9fs_process_post_ops: Process any pending v9fs_post_posix_operation
>>> + * @arg: Not used.
>>> + *
>>> + * This function serves as a callback to the iothread to be called into 
>>> whenever
>>> + * the v9fs_async_struct.wfd is written into. This thread goes through the 
>>> list
>>> + * of v9fs_post_posix_operations() and executes them. In the process, it 
>>> might
>>> + * queue more job on the asynchronous thread pool.
>>> + */
>>> +static void v9fs_process_post_ops(void *arg)
>>> +{
>>> +    int count = 0;
>>> +    struct v9fs_post_op *post_op;
>>> +    int ret;
>>> +    char byte;
>>> +
>>> +    qemu_mutex_lock(&v9fs_async_struct.lock);
>>> +    do {
>>> +        ret = read(v9fs_async_struct.rfd, &byte, sizeof(byte));
>>> +    } while (ret >= 0 && errno != EAGAIN);
>>
>> } while (ret > 1 || (ret == -1 && errno == EINTR)); ?
>
> Not sure why we need to retry if ret > 1; all we need is
>
> } while (ret == -1 && errno == EINTR);

We've been notified.  Now we want to drain the pipe fully in case we
were notified multiple times and bytes have been buffered up before we
got around to processing pending post_ops.

>>> diff --git a/posix-aio-compat.c b/posix-aio-compat.c
>>> index 9e02d18..8019be7 100644
>>> --- a/posix-aio-compat.c
>>> +++ b/posix-aio-compat.c
>>> @@ -260,6 +260,8 @@ static ssize_t handle_aiocb_rw(struct qemu_paiocb 
>>> *aiocb)
>>>     return nbytes;
>>>  }
>>>
>>> +static PosixAioState */;
>>> +
>>>  static void aio_thread(ThreadletWork *work)
>>>  {
>>>     pid_t pid;
>>> @@ -290,6 +292,15 @@ static void aio_thread(ThreadletWork *work)
>>>     aiocb->ret = ret;
>>>     qemu_mutex_unlock(&aiocb_mutex);
>>>
>>> +    if (posix_aio_state) {
>>
>> This test is not needed because posix_aio_state must be non-NULL here.
>
> I see similar check in the old code also..

The old code was in a signal handler, which means it could be called
at an inconvenient moment - before posix_aio_state has been
initialized.  But now we're guaranteed that posix_aio_state is
initialized because we'd never get to aio_thread() otherwise.

Stefan