[PATCH] Fix bug 9412 - SMB2 server doesn't support recvfile.
Jeremy Allison
jra at samba.org
Thu Apr 4 13:28:13 MDT 2013
On Thu, Apr 04, 2013 at 09:56:16AM +0200, Stefan (metze) Metzmacher wrote:
> Hi Jeremy,
>
> > The good news is that it got a lot smaller :-).
> > I'm really pleased with how little I had to
> > change to make this work. It's only a 10 element
> > patch set, and most of these are 5-10 line micro
> > changes.
> >
> > It turns out the changes made for 4.0.x meant
> > that I had to make *no* changes in the
> > smbd/smb2_write.c code to make RECVFILE work
> > (which is pretty amazing :-), and I think proves
> > we have some good abstractions in the 4.0.x code.
> >
> > This patchset has been tested by me under valgrind,
> > and by the OEM on their box, and gives an 18%
> > improvement in SMB2 write speed which finally makes
> > SMB2 faster than SMB1 on their smaller machines.
> >
> > Please review and give feedback (feel free to push
> > if you're happy with it :-).
>
> This really looks great! Thanks!
>
> I have a few comments see below.
Ok, here's the updated patchset that includes
the changes you requested except for the one
I didn't understand, and the one I didn't
agree with :-).
Cheers,
Jeremy.
-------------- next part --------------
From bcf2e1694ec85c750cc48cf33f3f283a9cb17162 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 18 Mar 2013 12:00:25 -0700
Subject: [PATCH 01/10] Ensure we don't do an SMB2 aio write if RECVFILE is
active.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/aio.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c
index 3f553eb..e8be408 100644
--- a/source3/smbd/aio.c
+++ b/source3/smbd/aio.c
@@ -861,6 +861,11 @@ NTSTATUS schedule_aio_smb2_write(connection_struct *conn,
return NT_STATUS_RETRY;
}
+ if (smbreq->unread_bytes) {
+ /* Can't do async with recvfile. */
+ return NT_STATUS_RETRY;
+ }
+
if (!(aio_ex = create_aio_extra(smbreq->smb2req, fsp, 0))) {
return NT_STATUS_NO_MEMORY;
}
--
1.8.1.3
From cd73967baa6d8f71687adb9a0d08289861d1d422 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Tue, 19 Mar 2013 12:16:32 -0700
Subject: [PATCH 02/10] If we already have an smb1req attached to the struct
smbd_smb2_request, don't recreate it.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/smb2_glue.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/source3/smbd/smb2_glue.c b/source3/smbd/smb2_glue.c
index 1b2b4dd..9fc1e49 100644
--- a/source3/smbd/smb2_glue.c
+++ b/source3/smbd/smb2_glue.c
@@ -28,9 +28,13 @@ struct smb_request *smbd_smb2_fake_smb_request(struct smbd_smb2_request *req)
struct smb_request *smbreq;
const uint8_t *inhdr = SMBD_SMB2_IN_HDR_PTR(req);
- smbreq = talloc_zero(req, struct smb_request);
- if (smbreq == NULL) {
- return NULL;
+ if (req->smb1req) {
+ smbreq = req->smb1req;
+ } else {
+ smbreq = talloc_zero(req, struct smb_request);
+ if (smbreq == NULL) {
+ return NULL;
+ }
}
smbreq->request_time = req->request_time;
--
1.8.1.3
From d9c96c3a4a7ff913a4d529c0dfb7c201781fe825 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Tue, 19 Mar 2013 12:24:17 -0700
Subject: [PATCH 03/10] Add function smbd_smb2_unread_bytes().
Returns number of bytes left to read for recvfile. Will be
used in SMB_2_WRITE_FILE code path.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/globals.h | 1 +
source3/smbd/smb2_glue.c | 12 ++++++++++++
2 files changed, 13 insertions(+)
diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h
index 3461bc5..a0a04bb 100644
--- a/source3/smbd/globals.h
+++ b/source3/smbd/globals.h
@@ -247,6 +247,7 @@ NTSTATUS smbd_smb2_request_pending_queue(struct smbd_smb2_request *req,
uint32_t defer_time);
struct smb_request *smbd_smb2_fake_smb_request(struct smbd_smb2_request *req);
+size_t smbd_smb2_unread_bytes(struct smbd_smb2_request *req);
void remove_smb2_chained_fsp(files_struct *fsp);
NTSTATUS smbd_smb2_request_verify_creditcharge(struct smbd_smb2_request *req,
diff --git a/source3/smbd/smb2_glue.c b/source3/smbd/smb2_glue.c
index 9fc1e49..54135b5 100644
--- a/source3/smbd/smb2_glue.c
+++ b/source3/smbd/smb2_glue.c
@@ -59,6 +59,18 @@ struct smb_request *smbd_smb2_fake_smb_request(struct smbd_smb2_request *req)
}
/*********************************************************
+ Are there unread bytes for recvfile ?
+*********************************************************/
+
+size_t smbd_smb2_unread_bytes(struct smbd_smb2_request *req)
+{
+ if (req->smb1req) {
+ return req->smb1req->unread_bytes;
+ }
+ return 0;
+}
+
+/*********************************************************
Called from file_free() to remove any chained fsp pointers.
*********************************************************/
--
1.8.1.3
From 63f41a70fa26664381f9333e85af30b3115a1eaa Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Tue, 19 Mar 2013 12:36:52 -0700
Subject: [PATCH 04/10] Allow smbd_smb2_request_error_ex() to cope with unread
bytes on error.
Drain the socket if a RECVFILE write failed.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/smb2_server.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index eb7059e..7eb62fc 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -2621,11 +2621,21 @@ NTSTATUS smbd_smb2_request_error_ex(struct smbd_smb2_request *req,
{
DATA_BLOB body;
uint8_t *outhdr = SMBD_SMB2_OUT_HDR_PTR(req);
+ size_t unread_bytes = smbd_smb2_unread_bytes(req);
DEBUG(10,("smbd_smb2_request_error_ex: idx[%d] status[%s] |%s| at %s\n",
req->current_idx, nt_errstr(status), info ? " +info" : "",
location));
+ if (unread_bytes) {
+ /* Recvfile error. Drain incoming socket. */
+ size_t ret = drain_socket(req->sconn->sock, unread_bytes);
+ if (ret != unread_bytes) {
+ smbd_server_connection_terminate(req->sconn,
+ "Failed to drain SMB2 socket\n");
+ }
+ }
+
body.data = outhdr + SMB2_HDR_BODY;
body.length = 8;
SSVAL(body.data, 0, 9);
--
1.8.1.3
From ca2fe9d6591fabf641ea75e1af17b2da227e0467 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 1 Apr 2013 13:12:55 -0700
Subject: [PATCH 05/10] Add utility function get_min_receive_file_size().
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/smb2_server.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index 7eb62fc..1c622a8 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -2842,6 +2842,17 @@ static int smbd_smb2_request_next_vector(struct tstream_context *stream,
size_t *_count);
static void smbd_smb2_request_read_done(struct tevent_req *subreq);
+static size_t get_min_receive_file_size(struct smbd_smb2_request *smb2_req)
+{
+ if (smb2_req->do_signing) {
+ return 0;
+ }
+ if (smb2_req->do_encryption) {
+ return 0;
+ }
+ return (size_t)lp_min_receive_file_size();
+}
+
static struct tevent_req *smbd_smb2_request_read_send(TALLOC_CTX *mem_ctx,
struct tevent_context *ev,
struct smbd_server_connection *sconn)
--
1.8.1.3
From 22d3924a279dbbda9f7ffc8d6298d9fee5404038 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 1 Apr 2013 13:14:13 -0700
Subject: [PATCH 06/10] Add macro SMBD_SMB2_SHORT_RECEIVEFILE_WRITE_LEN.
This is the 'short' length we'll read in the SMB2_WRITE receivefile
code path.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/globals.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h
index a0a04bb..f075a6d 100644
--- a/source3/smbd/globals.h
+++ b/source3/smbd/globals.h
@@ -546,6 +546,8 @@ struct smbd_smb2_request {
#define SMBD_SMB2_OUT_DYN_PTR(req) (uint8_t *)(SMBD_SMB2_OUT_DYN_IOV(req)->iov_base)
#define SMBD_SMB2_OUT_DYN_LEN(req) (SMBD_SMB2_OUT_DYN_IOV(req)->iov_len)
+#define SMBD_SMB2_SHORT_RECEIVEFILE_WRITE_LEN (SMB2_HDR_BODY + 0x30)
+
struct {
/*
* vector[0] TRANSPORT HEADER (empty)
--
1.8.1.3
From e35cfa8a336f66c928c5b33757b8a1ff9b4f4bcf Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 1 Apr 2013 13:17:09 -0700
Subject: [PATCH 07/10] Add extra fields into struct
smbd_smb2_request_read_state to support receivefile.
Initialize min_recv_size with the size that will trigger the
receivefile write path.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/smb2_server.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index 1c622a8..e1671a7 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -2831,6 +2831,8 @@ struct smbd_smb2_request_read_state {
uint8_t nbt[NBT_HDR_SIZE];
bool done;
} hdr;
+ bool doing_receivefile;
+ size_t min_recv_size;
size_t pktlen;
uint8_t *pktbuf;
};
@@ -2874,6 +2876,7 @@ static struct tevent_req *smbd_smb2_request_read_send(TALLOC_CTX *mem_ctx,
return tevent_req_post(req, ev);
}
state->smb2_req->sconn = sconn;
+ state->min_recv_size = get_min_receive_file_size(state->smb2_req);
subreq = tstream_readv_pdu_queue_send(state->smb2_req,
state->ev,
--
1.8.1.3
From 47c53ca35fbf73bde4e8ec333274159a589044a6 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 1 Apr 2013 13:19:01 -0700
Subject: [PATCH 08/10] Add stub static function that will turn on/off
receivefile code path.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/smb2_server.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index e1671a7..a983dcd 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -2892,6 +2892,11 @@ static struct tevent_req *smbd_smb2_request_read_send(TALLOC_CTX *mem_ctx,
return req;
}
+static bool is_smb2_recvfile_write(struct smbd_smb2_request_read_state *state)
+{
+ return false;
+}
+
static int smbd_smb2_request_next_vector(struct tstream_context *stream,
void *private_data,
TALLOC_CTX *mem_ctx,
--
1.8.1.3
From 1f8a516bb625f2ea71aacc519982faa2c6e43052 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 1 Apr 2013 13:24:07 -0700
Subject: [PATCH 09/10] The guts of the receivefile code changes.
If an incoming PDU might qualify, only read
SMBD_SMB2_SHORT_RECEIVEFILE_WRITE_LEN = (SMB2_HEADER + SMB2_WRITE_BODY_LEN)
bytes rather than the whole PDU.
Next time we're called, use is_smb2_recvfile_write() to decide if
this is an SMB2_WRITE that fit the receivefile criteria, otherwise
just read the rest of the PDU.
If we did do a short receivefile read, set up the smb2_req->smb1req->unread_bytes
value to show what bytes remain in the TCP buffers.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/smb2_server.c | 64 ++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 59 insertions(+), 5 deletions(-)
diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index a983dcd..4fc2bc7 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -2906,12 +2906,37 @@ static int smbd_smb2_request_next_vector(struct tstream_context *stream,
struct smbd_smb2_request_read_state *state =
talloc_get_type_abort(private_data,
struct smbd_smb2_request_read_state);
- struct iovec *vector;
+ struct iovec *vector = NULL;
+ size_t min_recvfile_size = UINT32_MAX;
if (state->pktlen > 0) {
- /* if there're no remaining bytes, we're done */
- *_vector = NULL;
- *_count = 0;
+ if (state->doing_receivefile && !is_smb2_recvfile_write(state)) {
+ /*
+ * Not a possible receivefile write.
+ * Read the rest of the data.
+ */
+ state->doing_receivefile = false;
+ vector = talloc_array(mem_ctx, struct iovec, 1);
+ if (vector == NULL) {
+ return -1;
+ }
+ vector[0].iov_base = (void *)(state->pktbuf +
+ SMBD_SMB2_SHORT_RECEIVEFILE_WRITE_LEN);
+ vector[0].iov_len = (state->pktlen -
+ SMBD_SMB2_SHORT_RECEIVEFILE_WRITE_LEN);
+ *_vector = vector;
+ *_count = 1;
+ } else {
+ /*
+ * Either this is a receivefile write so we've
+ * done a short read, or if not we have all the data.
+ * Either way, we're done and the caller will handle
+ * and short read case by looking at the
+ * state->doing_receivefile value.
+ */
+ *_vector = NULL;
+ *_count = 0;
+ }
return 0;
}
@@ -2957,7 +2982,26 @@ static int smbd_smb2_request_next_vector(struct tstream_context *stream,
}
vector[0].iov_base = (void *)state->pktbuf;
- vector[0].iov_len = state->pktlen;
+
+ if (state->min_recv_size != 0) {
+ min_recvfile_size = SMBD_SMB2_SHORT_RECEIVEFILE_WRITE_LEN;
+ min_recvfile_size += state->min_recv_size;
+ }
+
+ if (state->pktlen > min_recvfile_size) {
+ /*
+ * Might be a receivefile write. Read the SMB2 HEADER +
+ * SMB2_WRITE header first. Set 'doing_receivefile'
+ * as we're *attempting* receivefile write. If this
+ * turns out not to be a SMB2_WRITE request or otherwise
+ * not suitable then we'll just read the rest of the data
+ * the next time this function is called.
+ */
+ vector[0].iov_len = SMBD_SMB2_SHORT_RECEIVEFILE_WRITE_LEN;
+ state->doing_receivefile = true;
+ } else {
+ vector[0].iov_len = state->pktlen;
+ }
*_vector = vector;
*_count = 1;
@@ -3020,6 +3064,16 @@ static void smbd_smb2_request_read_done(struct tevent_req *subreq)
return;
}
+ if (state->doing_receivefile) {
+ state->smb2_req->smb1req = talloc_zero(state->smb2_req,
+ struct smb_request);
+ if (tevent_req_nomem(state->smb2_req->smb1req, req)) {
+ return;
+ }
+ state->smb2_req->smb1req->unread_bytes =
+ state->pktlen - SMBD_SMB2_SHORT_RECEIVEFILE_WRITE_LEN;
+ }
+
state->smb2_req->current_idx = 1;
tevent_req_done(req);
--
1.8.1.3
From 2023019409a2a3ade628b0bde9d5419b335d885f Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 1 Apr 2013 11:16:01 -0700
Subject: [PATCH 10/10] Add the internals of is_smb2_recvfile_write.
This turns on the real receivefile detection, and completes
the receivefile code path changes.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/smb2_server.c | 38 +++++++++++++++++++++++++++++++++++++-
1 file changed, 37 insertions(+), 1 deletion(-)
diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index 4fc2bc7..f1fcb7e 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -2894,7 +2894,43 @@ static struct tevent_req *smbd_smb2_request_read_send(TALLOC_CTX *mem_ctx,
static bool is_smb2_recvfile_write(struct smbd_smb2_request_read_state *state)
{
- return false;
+ uint32_t flags;
+
+ if (IVAL(state->pktbuf, 0) == SMB2_TF_MAGIC) {
+ /* Transform header. Cannot recvfile. */
+ return false;
+ }
+ if (IVAL(state->pktbuf, 0) != SMB2_MAGIC) {
+ /* Not SMB2. Normal error path will cope. */
+ return false;
+ }
+ if (SVAL(state->pktbuf, 4) != SMB2_HDR_BODY) {
+ /* Not SMB2. Normal error path will cope. */
+ return false;
+ }
+ if (SVAL(state->pktbuf, SMB2_HDR_OPCODE) != SMB2_OP_WRITE) {
+ /* Needs to be a WRITE. */
+ return false;
+ }
+ if (IVAL(state->pktbuf, SMB2_HDR_NEXT_COMMAND) != 0) {
+ /* Chained. Cannot recvfile. */
+ return false;
+ }
+ flags = IVAL(state->pktbuf, SMB2_HDR_FLAGS);
+ if (flags & SMB2_HDR_FLAG_CHAINED) {
+ /* Chained. Cannot recvfile. */
+ return false;
+ }
+ if (flags & SMB2_HDR_FLAG_SIGNED) {
+ /* Signed. Cannot recvfile. */
+ return false;
+ }
+
+ DEBUG(10,("Doing recvfile write len = %u\n",
+ (unsigned int)(state->pktlen -
+ SMBD_SMB2_SHORT_RECEIVEFILE_WRITE_LEN)));
+
+ return true;
}
static int smbd_smb2_request_next_vector(struct tstream_context *stream,
--
1.8.1.3
More information about the samba-technical
mailing list