Patchset to add asynchronous open/close to master

[Stefan (metze) Metzmacher]

Hi Jeremy,

> 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 :-).

I have and it's really tricky code with a lot of possible side effects,
I wouldn't say it's simple, maybe as simple as SPNEGO or SASL:-)

>> From the manpage "These are per-share parameters", this is wrong!
> 
> No, they are per-share parameters. In what way are they not ?

aio_pthread_connect() sets global variables!
actually it's really a bug to set global variables from a per share option,
as a new tree connect changes the behavior of another one.

Please fix the code and make them global options.
Or implement the per share option correct.

>> The usage of openat() let the module skip the next SMB_VFS module!
> 
> Yes. This isn't a stackable implementation.

Why does it call SMB_VFS_NEXT* then, when not doing async stuff,
I think it should always or never call raw syscalls.

Otherwise it's confusing, at least add some comments.

>> 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.

I know, but as far as I can see the cancel engine would just call
smbd_smb2_create_cancel and will return NT_STATUS_CANCELLED to the client
while the thread still hangs in the syscall.

I think we should not return NT_STATUS_CANCELLED in that case.

>> 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.

Ok, got it.

>> 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 !

Sure, but reading SMB_VFS_NEXT* calls also in the new code is confusing!
Please avoid it or at least add comments.

>> 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.

Ok, that is fine, but we should call close() not SMB_VFS_NEXT_CLOSE(),
even if we're not async.

>> 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 ?

You said you want this in 3.6 and for 3.6 this is an incompatible change!
For master it's fine to add it.

The only option that might work for 3.6 is to add it to the end of the
structure,
but I still won't be very happy with changing it.

>> 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.

Ok, that sounds good.

> 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.

I think there're more, see above:-)
But nothing that cannot be resolved (at least) for master...

> Thanks for the review :-).

The reason for beeing to critical is that you want this in 3.6:-)

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20120614/37a7ab21/attachment.pgp>