Hi Sebastian,
On 27.02.2017 19:42, Sebastian Lackner wrote:
On 22.02.2017 14:51, Jacek Caban wrote:
Signed-off-by: Jacek Caban jacek@codeweavers.com
dlls/kernel32/tests/pipe.c | 28 ++- dlls/ntdll/file.c | 7 +- dlls/ntdll/tests/file.c | 2 +- server/async.c | 9 + server/file.h | 1 + server/named_pipe.c | 446 ++++++++++++++++++++++++++++++++++++++++++--- 6 files changed, 447 insertions(+), 46 deletions(-)
As I see this updated patchset allows to return the result immediately, but still uses a USR1 signal to transfer back the IO completion info. Wouldn't it be useful to get rid of that aswell?
I believe it would be useful, but I assumed it does not block this patch. It's tricky, but I gave it some thoughts before sending patches and intend to look at that as some point. Here are some thoughts:
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).
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: - Use write_process_memory() in async_terminate() when we know we can do that for particular async - Since above will complicate wait handle logic, it would be nice to move this logic to read/write/ioctl/flush request handlers instead of duplicating that in fd ops. - The above will need invasive change for device IRP-based calls, which currently use irp_call, not async, for wait handles. - The above has a side quest: it would be nice to consider making device calls blocking more compatible. I believe that we should always wait until real driver completes request (even for non-blocking calls) and return STATUS_PENDING only if driver returned STATUS_PENDING. That's something to keep in mind while doing design decision, not strictly related. - Right now, user passed APCs are stored in client and transferred as APC_ASYNC_IO. This would need to be changed.
I'm sure some of those tasks will need more smaller changes. Those would be much nicer to work on having named pipes in the tree first, so that we can at least test the code. And, since I think we will need APC+blocking as a fallback anyway, which we can use blocking as the main solution for the time being and have it well tested.
That said, I agree it would be useful, I just think it shouldn't block the patch.
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?
- iosb = async_get_iosb( async );
- if ((blocking || iosb->status != STATUS_PENDING)
&& !(handle = alloc_handle( current->process, async, SYNCHRONIZE, 0 )))async_terminate( async, get_error() );I don't think this will work as expected. async_terminate might queue an APC, but the caller will immediately deallocate the async struct in server_write_file because status != STATUS_PENDING.
Good point, I will send an updated patch.
Thanks for the review,
Jacek
[1] https://www.winehq.org/pipermail/wine-patches/2017-January/157013.html
[2] https://www.winehq.org/pipermail/wine-devel/2017-February/116060.html