[PATCH] Fix bug 9412 - SMB2 server doesn't support recvfile.

[Jeremy Allison]

On Fri, Apr 05, 2013 at 09:20:08AM +0200, Stefan (metze) Metzmacher wrote:
> 
> The whole SMB2 socket handling is different from the SMB1 one,
> the SMB2 code is a lot more async and the socket is non-blocking.
> 
> Maybe we could let smbd_smb2_request_read_done() keep 'subreq'
> if state->doing_receivefile is true, we need to remember this
> in smb2req.

That's what the setting up of the 'unread_bytes' variable
does. Having a non-zero unread_bytes is the way we determine
we're in a recvfile call in both SMB1 and SMB2. I don't see
the need to add another state variable to smb2req.

> As the socket is non-blocking, we need to make sure recvfile()
> doesn't get EAGAIN. We could extent the tstream_readv_pdu code
> to handle an iovec with iov_len != 0 and iov_base == NULL.
> This could be used to wait until the whole request arrived
> in the kernel, by checking tstream_pending_bytes().
> Or we have to mark the socket blocking during for recvfile() and
> drain_socket().

The default implementation of recvfile() and drain_socket()
already cope with EAGAIN/EWOULDBLOCK (admittedly by
spinning on the call) Look at source3/lib/system.c:sys_read().

I think this can be done as an additional patch one the base
recvfile code is added as this one is going to take some care in
getting right for what is basically an optimization for
a rare case (i.e. I've never seen it, and neither has
the OEM :-). The EAGAIN case would only happen on (a)
a high latency link or (b) a network undergoing severe
congestion, neither of which is a good use case for SMB
in any case :-).

Rather than changing the tstream code the simplest case
is just to temporarily set the socket to blocking inside
vfs_write_data()/vfs_pwrite_data() (and possibly drain_socket()).
That is adding 4 extra fcntl system calls per RECVFILE call
due to the nature of the set_blocking() API not returning
the previous flags state, so I'm reluctant to do this unless
I really have to.

> Maybe you can change "caller" into "caller (smbd_smb2_request_read_done)".
> From just reading the comment it was not clear to me what 'caller' was
> referring to.

Ok, I'll fix the comment and resubmit.

> BTW: I just found the attached patch in one of my branches again,
> which should improve the splice recvfile implementation.
> But with this junkcode test:
> https://gitweb.samba.org/?p=metze/misc/junkcode.git;a=commitdiff;h=43121900e96a0f779e110ed162c1db766eace4e6
> https://gitweb.samba.org/?p=metze/misc/junkcode.git;a=blob;f=splice/splice2.c
> 
> I'm getting this on my thinkpad x230

splice() is simply broken as a RECVFILE call.

There is a good reason the OEMs who need this
have a separate kernel patch and don't rely on
splice().

I can discuss the politics of why this isn't
upstream offline :-).

Jeremy.