Bug 9412 - SMB2 server doesn't support recvfile.
SMB2 server doesn't support recvfile.
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: SMB2
unspecified
All All
: P5 normal
: ---
Assigned To: Jeremy Allison
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-21 04:38 UTC by Jeremy Allison
Modified: 2014-11-01 04:38 UTC (History)
3 users (show)

See Also:


Attachments
test git-am fix for 3.6.x (13.42 KB, patch)
2012-11-21 06:18 UTC, Jeremy Allison
no flags Details
samba4-smb2-recvfile-v4.patch (10.86 KB, patch)
2013-03-30 17:55 UTC, Jose M. Prieto
no flags Details
Raw patch for 4.0.next/master (9.02 KB, patch)
2013-04-01 18:32 UTC, Jeremy Allison
no flags Details
Fixed raw-patch for 4.0.x / master (9.17 KB, patch)
2013-04-01 21:52 UTC, Jeremy Allison
no flags Details
git-am fix for master and 4.0.x (14.09 KB, patch)
2013-04-03 00:33 UTC, Jeremy Allison
no flags Details
git-am fix for master and 4.0.x (14.29 KB, patch)
2013-04-04 19:30 UTC, Jeremy Allison
no flags Details
git-am fix for master and 4.0.next (18.48 KB, patch)
2013-04-08 19:45 UTC, Jeremy Allison
no flags Details
git-am patchset that went into master. (19.33 KB, patch)
2013-04-22 17:38 UTC, Jeremy Allison
no flags Details
Patches for v4-0-test (20.23 KB, patch)
2013-04-22 17:48 UTC, Stefan Metzmacher
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2012-11-21 04:38:22 UTC
Currently the parameter "min receivefile size" has no effect on the SMB2 server, only the SMB1 server.

Patch for 3.6.x (and 4.0.x and master) to follow to fix this.

Jeremy.
Comment 1 Jeremy Allison 2012-11-21 06:18:34 UTC
Created attachment 8216 [details]
test git-am fix for 3.6.x

Compiles, but not yet tested. Not asking for review yet.

Jeremy.
Comment 2 Jeremy Allison 2012-11-27 00:48:48 UTC
W00t. Tested patch and it works. I'll work on getting this into 3.6.next, 4.0.0 and master.

Jeremy.
Comment 3 Jeremy Allison 2012-11-27 18:34:24 UTC
Comment on attachment 8216 [details]
test git-am fix for 3.6.x

Asking Metze to take a look at this.

Metze, I'll create a forward port for master and 4.0.0 and submit for review also.

Cheers,

Jeremy.
Comment 4 Jeremy Allison 2012-11-27 18:34:45 UTC
Comment on attachment 8216 [details]
test git-am fix for 3.6.x

Michael should also have a look :-).
Comment 5 Jose M. Prieto 2013-03-01 08:38:04 UTC
Hello Jeremy,

I'm a crazy guy ;) who is very interested in getting this patch working on samba 4 master trunk so that I can start working on a write optimization for a Samba server based on openWRT. This why I'm putting some effort right now on it. However, I noted that samba 4 SMB2 server code is sligthly different than samba 3 one. So let me ask you whether you did get the chance to create such as port as mentioned on comment #3? And, if so, whether you can share a link to the repository?

Thanks,
Jose M. Prieto

(In reply to comment #3)
> Comment on attachment 8216 [details]
> test git-am fix for 3.6.x
> 
> Asking Metze to take a look at this.
> 
> Metze, I'll create a forward port for master and 4.0.0 and submit for review
> also.
> 
> Cheers,
> 
> Jeremy.
Comment 6 Jeremy Allison 2013-03-01 17:08:54 UTC
I haven't moved this to master yet as I'm waiting for confirmation from an OEM that this actually does help and improve performance in real world conditions.

If you work for such an OEM and are willing to do such testing please email me directly at jra@samba.org and let's discuss it.

Jeremy.
Comment 7 Justin Maggard 2013-03-15 00:07:14 UTC
I am such an OEM. :-)  I can verify that we consistently get ~18% write speed improvement on a slow-ish platform (1.2GHz ARMv7) with this patch.  Our kernel does have a receivefile (splice from socket to file) implementation.

I'm happy to test again once a 4.0.x patch is ready; but the results on 3.6 are very good.  I'd love to see this change get pulled in.
Comment 8 Jeremy Allison 2013-03-15 23:44:18 UTC
Ok I'll forward port to master/4.0.x. Not until Monday though :-).

Jeremy.
Comment 9 Jeremy Allison 2013-03-19 22:54:03 UTC
Ouch. The vector reading code has changed significantly between 3.6.x and 4.0.x (for the better, so we aren't doing as many separate read requests and also to support the SMB3 transport level encryption) to make it much harder to do the RECVFILE port to 4.0.x. I'm still looking at what I'd need to change to do this.

Jeremy.
Comment 10 Stefan Metzmacher 2013-03-19 23:48:19 UTC
Am 19.03.2013 23:54, schrieb samba-bugs@samba.org:
> https://bugzilla.samba.org/show_bug.cgi?id=9412
> 
> --- Comment #9 from Jeremy Allison <jra@samba.org> 2013-03-19 22:54:03 UTC ---
> Ouch. The vector reading code has changed significantly between 3.6.x and 4.0.x
> (for the better, so we aren't doing as many separate read requests and also to
> support the SMB3 transport level encryption) to make it much harder to do the
> RECVFILE port to 4.0.x. I'm still looking at what I'd need to change to do
> this.

That's why I were so sceptical about the 3.6 patch,
we can't have that in 3.6 but not in newer releases...

If you can come with a similar small patch for master and 4.0
we might be able to bring it upstream, but it might cause some more work
later when we try to implement SMB-Direct (RDMA).

metze
Comment 11 Jeremy Allison 2013-03-20 16:19:04 UTC
I'm assuming in the previous comment "we can't have that in 3.6 but not in newer releases"

you mean "we *can* have that in 3.6 but not in newer releases", yes ?

I agree. I'm going to look at getting a minimal patch working in master/4.0.x. I don't think it'll cause trouble for RDMA, as it should be orthogonal to that logic.

Jeremy.
Comment 12 Stefan Metzmacher 2013-03-22 07:32:21 UTC
(In reply to comment #11)
> I'm assuming in the previous comment "we can't have that in 3.6 but not in
> newer releases"
> 
> you mean "we *can* have that in 3.6 but not in newer releases", yes ?
> 
> I agree. I'm going to look at getting a minimal patch working in master/4.0.x.
> I don't think it'll cause trouble for RDMA, as it should be orthogonal to that
> logic.

I mean if we add that at all, we need it in master and maybe 4.0.
If that's done, we can start to think about 3.6.

metze
Comment 13 Jeremy Allison 2013-03-22 16:20:00 UTC
I have hard numbers from Justin that shows he needs this for their low-end platforms, so I'm planning to code this up in the next week or so.

Jeremy.
Comment 14 Jeremy Allison 2013-03-29 23:38:22 UTC
Finally figured out how to do this (relatively, ha ha) cleanly in 4.0.x, master.

Patch should be ready for test early next week.

Jeremy.
Comment 15 Jose M. Prieto 2013-03-30 14:40:02 UTC
Hi,

I got RECVFILE working on samba 4 and SMB2 based on your patch for 3.6.x. As you said the new way the next vector function is working as of 4.x was the most challenging for me. If you want I can share the patch with you.

Regards,
Jose M. Prieto

(In reply to comment #14)
> Finally figured out how to do this (relatively, ha ha) cleanly in 4.0.x,
> master.
> 
> Patch should be ready for test early next week.
> 
> Jeremy.
Comment 16 Jeremy Allison 2013-03-30 17:17:05 UTC
Sure I'll take a look - thanks ! I already have that part working though :-). It'll be good to compare the two to see if there's anything I might have missed.

Cheers,

Jeremy.
Comment 17 Jose M. Prieto 2013-03-30 17:55:26 UTC
Created attachment 8702 [details]
samba4-smb2-recvfile-v4.patch
Comment 18 Jose M. Prieto 2013-03-30 17:58:49 UTC
Hi,

You can find the patch attached to the this thread. My work is a fork from branch v4-0-stable. 

Regards,
Jose

(In reply to comment #16)
> Sure I'll take a look - thanks ! I already have that part working though :-).
> It'll be good to compare the two to see if there's anything I might have
> missed.
> 
> Cheers,
> 
> Jeremy.
Comment 19 Jeremy Allison 2013-04-01 18:32:26 UTC
Created attachment 8706 [details]
Raw patch for 4.0.next/master

The changes to smbd_smb2_request_next_vector() in master actually make it easier to detect a recvfile write, without having to check the tid/fid explicitly (the smbd_smb2_inbuf_parse_compound() takes care of errors in that nicely).

Justin, can you test with this raw patch, and if it works for you I'll split up into smaller commits we can try and get into 4.0.next.

Cheers,

Jeremy.
Comment 20 Jeremy Allison 2013-04-01 21:38:44 UTC
Found the bug....
doing a check on IVAL(state->pktbuf, SMB2_HDR_OPCODE), this should be an SVAL instead. New patch in a min or so...

Jeremy.
Comment 21 Jeremy Allison 2013-04-01 21:52:55 UTC
Created attachment 8707 [details]
Fixed raw-patch for 4.0.x / master

Fixed the opcode read IVAL -> SVAL.

Jeremy.
Comment 22 Jeremy Allison 2013-04-03 00:33:21 UTC
Created attachment 8716 [details]
git-am fix for master and 4.0.x

Patch for 4.0.x and master currently undergoing test.
Comment 23 Justin Maggard 2013-04-03 00:47:32 UTC
This latest patch is working well for me so far.  I'm seeing that nice 18% write speed improvement on my ARM machine, which finally makes SMB2 actually faster than SMB1. :-)
Comment 24 Jeremy Allison 2013-04-04 19:30:51 UTC
Created attachment 8728 [details]
git-am fix for master and 4.0.x

Updated with Metze's change requests.
Jeremy.
Comment 25 Jeremy Allison 2013-04-08 19:45:40 UTC
Created attachment 8737 [details]
git-am fix for master and 4.0.next

Updated with Metze's comments. 3 new patches added at the end to ensure the socket is in blocking state inside drain_socket() and before any RECVFILE call.

(It's possible these last 2 might be moved into the default recvfile() code, if the kernel recvfile patch already correctly copes with the non-blocking socket reads).

Justin if you could test this I'd really appreciate it.

Cheers,

Jeremy.
Comment 26 Jeremy Allison 2013-04-08 23:23:57 UTC
Update via email from Justin:

Fortunately, the updated patch set doesn't appear to tangibly affect
write performance in my testing today.  So everything looks good from
my end.
Comment 27 Stefan Metzmacher 2013-04-20 09:26:17 UTC
Jeremy, can you upload new patches with cherry-pick information from master?
Then I can give my review+ for the releases.

Thanks!
Comment 28 Jeremy Allison 2013-04-22 17:38:13 UTC
Created attachment 8803 [details]
git-am patchset that went into master.

Here you go Metze, thanks !

Jeremy.
Comment 29 Stefan Metzmacher 2013-04-22 17:47:12 UTC
(In reply to comment #28)
> Created attachment 8803 [details]
> git-am patchset that went into master.
> 
> Here you go Metze, thanks !

I don't see (cherry picked from commit .... lines in the attachment.

in v4-0-test
git cherry-pick -x -13 95f7fc83b251efefcc2a603b936b55e2f0308a72
git format-patch --stdout -13 > tmp40.diff
should do it, I'll upload that and ask you for the review flag.
Comment 30 Jeremy Allison 2013-04-22 17:48:35 UTC
Oh sorry, I just did the 'git format-patch --stdout <start ref>..<end ref>" from master. Thought that's what you required.
Comment 31 Stefan Metzmacher 2013-04-22 17:48:43 UTC
Created attachment 8804 [details]
Patches for v4-0-test
Comment 32 Stefan Metzmacher 2013-04-22 17:51:58 UTC
(In reply to comment #30)
> Oh sorry, I just did the 'git format-patch --stdout <start ref>..<end ref>"
> from master. Thought that's what you required.

No, problem it would be cool to have a
git format-patch -x --stdout
but on the other hand it's not the worst thing,
if it's required to really apply the patches to the target branch
first, then it's less likely to generate conflicts for Karolin...
Comment 33 Stefan Metzmacher 2013-04-22 17:52:40 UTC
Comment on attachment 8216 [details]
test git-am fix for 3.6.x

I guess this patch needs to be updated, correct?
Comment 34 Jeremy Allison 2013-04-22 18:21:17 UTC
Yes the 3.6.x patch needs to be re-done. Let's ignore it for now and just leave SMB2 recvfile as 4.0.x.
Jeremy.
Comment 35 Jeremy Allison 2013-04-22 18:30:51 UTC
Comment on attachment 8804 [details]
Patches for v4-0-test

LGTM.
Comment 36 Jeremy Allison 2013-04-22 18:31:12 UTC
Re-assigning to Karolin for inclusion in 4.0.next.
Jeremy.
Comment 37 Karolin Seeger 2013-04-25 10:51:46 UTC
(In reply to comment #36)
> Re-assigning to Karolin for inclusion in 4.0.next.
> Jeremy.

Pushed to autobuild-v4-0-test.
Comment 38 Karolin Seeger 2013-04-25 10:52:24 UTC
(In reply to comment #34)
> Yes the 3.6.x patch needs to be re-done. Let's ignore it for now and just leave
> SMB2 recvfile as 4.0.x.
> Jeremy.

Re-assigning to Jeremy to re-do the patch for 3.6.
Comment 39 Karolin Seeger 2013-04-26 08:26:31 UTC
(In reply to comment #37)
> (In reply to comment #36)
> > Re-assigning to Karolin for inclusion in 4.0.next.
> > Jeremy.
> 
> Pushed to autobuild-v4-0-test.

Pushed to v4-0-test.
Comment 40 Jeremy Allison 2014-11-01 04:38:59 UTC
SMB2 recvfile is a 4.x only feature.