On 28.02.2017 14:40, Sebastian Lackner wrote:
On 28.02.2017 14:03, Jacek Caban wrote:
As long as we transfer result over server socket, we don't have control in server over when iosb and result buffer are filled, so we need a signal from client when we can signal async completion (signal event/object, queue completion). I sent a patch that was more inline with how we do that for calls where I/O is implemented on client side [1], but Alexandre preferred that to be in server [2]. Having that in server is not only cleaner, but also gives us more control, like in which case to fill iosb or signal event. It will also make it possible to implement other solution (bellow).
I assume Alexandre was mainly referring to the event / APC stuff. I assume he also would be fine with a code path to return the result immediately when available. This could for example be done by marking the async object as "in progress" to avoid queing unnecessary APCs. If a real result is available at the end of the call, it could be transferred right away. This would also help to fix the problem with "immediate returns" below.
What about the problem of client having to signal server when it received and processed the data? When would you set the event in that case? We can transfer data immediately, but we still need to know when we can set event / queue APC. Currently we know that we can do that when APC_ASYNC_IO returns.
As long as we need a way for client to signal that data is transferred, we need some sort of notification from client. We could add a new server call, that client would be responsible to call after receiving data, that we could use instead of wait handle and APCs, but I think that moving more logic to client is not the right solution.
An other solution is not to transfer result over server socket, but use write_process_memory() instead. This would allow us to do everything in a single server call. Unfortunately, it seems to me that there will be cases (like writing to memory what has write watch), when we will need APC-based fallback anyway. This is a huge change on its own, it needs at least: [...]
I don't think write_process_memory is the right way to do it. This function also has a lot of overhead, like suspending the thread, or if /proc is not available reading/writing is done in chunks of sizeof(long).
That's another reason for a good fallback. I imagine we could use APC fallback if /proc is not available. Suspending thread doesn't sound too bad, esp. given that the alternative is an extra server call.
I'm pretty sure it would be faster to just transfer back the result with the same wineserver call. Also please note that the client side already adds a reply buffer, so the changes would mostly be on the server side.
Yes, data transfer is probably faster this way, but, as I said, the problem is not really about transfer speed itself, but the fact that we need server round-trips to know when data is transferred.
Also please note that we need to distinguish "immediate returns" and "async returns" to properly implement FileIoCompletionNotificationInformation. This especially means that waiting internally is not an option.
It's easy to distinguish and still have internal waits. I can't point the right solution without looking deeper at FileIoCompletionNotificationInformation, but just as an example, you could do:
if (iosb->status != STATUS_PENDING) mark_async_as_immediate_result();
inside request handlers just after calling fd ops. Then you'd have all the info you need inside async object. Am I missing something?
With the currently proposed solution this would not work. The fd ops already call async_terminate -> async_set_result -> add_async_completion -> add_completion, but depending on the completion flags this should not be done if it is an immediate result. There is no way to know if its supposed to be an "immediate return" inside of the async_*() helpers, and checking afterwards is too late (unless we introduce a flag to block async objects from doing actions right away). Did you have a different idea how it should be implemented?
In cases covered by my patch, async_terminate doesn't call async_set_result, but queues APC instead, so that's not a problem.
More generally speaking, if we had a code path like you described, the solution I proposed would still work, just reverse the logic. Mark all created asyncs as immediate returns, and change it to async where appropriate, something like:
if (iosb->status == STATUS_PENDING) mark_async_as_async_result();
in read/write/ioctl/flush should do. register_async request could do that unconditionally.
Thanks, Jacek