Patchset to add asynchronous open/close to master

[Jeremy Allison]

On Wed, Jun 13, 2012 at 11:54:33AM +0200, Stefan (metze) Metzmacher wrote:
> 
> It may look simple, but it isn't!

Yes it is. Read the code :-).

> From the manpage "These are per-share parameters", this is wrong!

No, they are per-share parameters. In what way are they not ?

> The usage of openat() let the module skip the next SMB_VFS module!

Yes. This isn't a stackable implementation.

> There's no infrastructure to cancel async opens! I think it can happen
> that the client believes the open was canceled, but the file gets created.

There is an inherant race condition here. Whenever
you invoke an asynchronous open you may not get the
cancel before the file is created.

How could you possibly make this race free ? It simply
isn't possible with an asynchronous network protocol.

> find_completed_open() may checks !(opd->flags & O_EXCL), shouldn't
> check O_CREATE? as aio_open_fn()

No. If you read the code you'll discover that if O_CREAT is
not set then you just do a normal sync open. So this can
never be the case in find_completed_open() - it'll never
get here.

> aio_close_worker() uses 'close' and skips the next SMB_VFS module!

Yes, you're not paying attention. aio_pthread is *NOT* a stackable
module. Never has been. It uses pread and pwrite directly. That
has been in the tree a long time. This is unavoidable as the VFS
is not thread safe. We can only call the raw syscalls from the
worker functions. Come on Metze, you should know this !

> There's no infrastructure to defer async close calls,
> so the smb layer calls file_free(), which means we return the result to
> the client before the close is done.

Yes.

> adding 'uint64_t mid' to files_struct is an incompatible change.

Incompatible with what, exactly ? What is the problem
you're trying to solve by freezing the definition of
files_struct ?

> Just checking if (st.st_uid == opd->uid) isn't enough,
> the global context might be switched to a force group context.

This is valid. I'll fix this.

> Instead of extending the usage of the deferred_open_queue,
> we should really create a generic infrastructure based on
> tevent_req based _send/_recv function pairs.

Not going to happen in the time we have for getting
4.0.0 out of the door. When you replace the deferred_open_queue,
then you can replace this code too.

In the meantime I can see only one valid claim against
this patch, which I'll fix. If you think the other problems
are valid, feel free to follow up with further explainations.

Thanks for the review :-).

Cheers,

	Jeremy.