SMB2/3 Performance

[David Disseldorp]

Hi Metze,

I've a few minor comments on the following change...

On Fri, 22 Nov 2013 15:18:46 +0100
"Stefan (metze) Metzmacher" <metze at samba.org> wrote:

> From d8f784dbb55fa56f83dcea605a9fa5e38828063b Mon Sep 17 00:00:00 2001
> From: Stefan Metzmacher <metze at samba.org>
> Date: Mon, 14 Oct 2013 10:33:57 +0200
> Subject: [PATCH 2/6] s3:smb2_server: for performance reasons we use tevent_fd
>  and readv/writev directly

...

> +
> +static NTSTATUS smbd_smb2_io_handler(struct smbd_server_connection *sconn,
> +				     uint16_t fde_flags)
> +{

This function's a doozy. Please split it into separate TEVENT_FD_WRITE
/ TEVENT_FD_READ subfunctions.


> +	struct smbd_smb2_request_read_state *state = &sconn->smb2.request_read_state;
>  	struct smbd_smb2_request *req = NULL;
> +	size_t min_recvfile_size = UINT32_MAX;
> +	int ret;
> +	int err;
> +	bool retry;
> +	NTSTATUS status;
> +	NTTIME now;
> +
> +	if (fde_flags & TEVENT_FD_WRITE) {
...
> +	}
> +
> +may_read:
> +	if (!(fde_flags & TEVENT_FD_READ)) {
> +		return NT_STATUS_OK;
> +	}
> +
> +	if (state->req == NULL) {
> +		TEVENT_FD_NOT_READABLE(sconn->smb2.fde);
> +		return NT_STATUS_OK;
> +	}
> +
> +again:
> +	if (!state->hdr.done) {
> +		state->hdr.done = true;
> +
> +		state->vector.iov_base = (void *)state->hdr.nbt;
> +		state->vector.iov_len = NBT_HDR_SIZE;
> +	}
> +
> +	ret = readv(sconn->sock, &state->vector, 1);
> +	if (ret == 0) {
> +		/* propagate end of file */
> +		return NT_STATUS_END_OF_FILE;
> +	}
> +	err = socket_error_from_errno(ret, errno, &retry);
> +	if (retry) {
> +		/* retry later */
> +		TEVENT_FD_READABLE(sconn->smb2.fde);
> +		return NT_STATUS_OK;
> +	}
> +	if (err != 0) {
> +		return map_nt_error_from_unix_common(err);
> +	}
> +
> +	if (ret < state->vector.iov_len) {
> +		uint8_t *base;
> +		base = (uint8_t *)state->vector.iov_base;
> +		base += ret;
> +		state->vector.iov_base = (void *)base;
> +		state->vector.iov_len -= ret;
> +		/* we have more to read */
> +		TEVENT_FD_READABLE(sconn->smb2.fde);
> +		return NT_STATUS_OK;
> +	}
> +
> +	if (state->pktlen > 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;
> +			state->vector.iov_base = (void *)(state->pktbuf +
> +				SMBD_SMB2_SHORT_RECEIVEFILE_WRITE_LEN);
> +			state->vector.iov_len = (state->pktlen -
> +				SMBD_SMB2_SHORT_RECEIVEFILE_WRITE_LEN);
> +			goto again;
> +		}
>  
> -	status = smbd_smb2_request_read_recv(subreq, sconn, &req);
> -	TALLOC_FREE(subreq);
> +		/*
> +		 * Either this is a receivefile write so we've
> +		 * done a short read, or if not we have all the data.
> +		 */
> +		goto got_full;
> +	}
> +
> +	/*
> +	 * Now we analyze the NBT header
> +	 */
> +	state->pktlen = smb2_len(state->hdr.nbt);
> +	if (state->pktlen == 0) {
> +		goto got_full;
> +	}
> +
> +	state->pktbuf = talloc_array(state->req, uint8_t, state->pktlen);
> +	if (state->pktbuf == NULL) {
> +		return NT_STATUS_NO_MEMORY;
> +	}
> +
> +	state->vector.iov_base = (void *)state->pktbuf;
> +
> +	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.
> +		 */
> +		state->vector.iov_len = SMBD_SMB2_SHORT_RECEIVEFILE_WRITE_LEN;
> +		state->doing_receivefile = true;
> +	} else {
> +		state->vector.iov_len = state->pktlen;
> +	}
> +
> +	goto again;
> +
> +got_full:
> +
> +	if (state->hdr.nbt[0] != 0x00) {
> +		DEBUG(1,("smbd_smb2_connection_handler: ignore NBT[0x%02X] msg\n",
> +			 state->hdr.nbt[1]));

Function name prefix is wrong here, I'd prefer to drop it completely.

> +
> +		req = state->req;
> +		ZERO_STRUCTP(state);
> +		state->req = req;
> +		state->min_recv_size = get_min_receive_file_size(state->req);
> +		req = NULL;
> +		goto again;
> +	}
> +
> +	req = state->req;
> +	state->req = NULL;
> +
> +	req->request_time = timeval_current();
> +	now = timeval_to_nttime(&req->request_time);
> +
> +	status = smbd_smb2_inbuf_parse_compound(req->sconn->conn,
> +						now,
> +						state->pktbuf,
> +						state->pktlen,
> +						req,
> +						&req->in.vector,
> +						&req->in.vector_count);
>  	if (!NT_STATUS_IS_OK(status)) {
> -		DEBUG(2,("smbd_smb2_request_incoming: client read error %s\n",
> -			nt_errstr(status)));
> -		smbd_server_connection_terminate(sconn, nt_errstr(status));
> -		return;
> +		return status;
> +	}
> +
> +	if (state->doing_receivefile) {
> +		req->smb1req = talloc_zero(req, struct smb_request);
> +		if (req->smb1req == NULL) {
> +			return NT_STATUS_NO_MEMORY;
> +		}
> +		req->smb1req->unread_bytes =
> +			state->pktlen - SMBD_SMB2_SHORT_RECEIVEFILE_WRITE_LEN;
>  	}
>  
> +	ZERO_STRUCTP(state);
> +
> +	req->current_idx = 1;
> +
>  	DEBUG(10,("smbd_smb2_request_incoming: idx[%d] of %d vectors\n",

Incorrect prefix here too.

>  		 req->current_idx, req->in.vector_count));
>  
>  	status = smbd_smb2_request_validate(req);
>  	if (!NT_STATUS_IS_OK(status)) {
> -		smbd_server_connection_terminate(sconn, nt_errstr(status));
> -		return;
> +		return status;
>  	}
>  
>  	status = smbd_smb2_request_setup_out(req);
>  	if (!NT_STATUS_IS_OK(status)) {
> -		smbd_server_connection_terminate(sconn, nt_errstr(status));
> -		return;
> +		return status;
>  	}
>  
>  	status = smbd_smb2_request_dispatch(req);
>  	if (!NT_STATUS_IS_OK(status)) {
> -		smbd_server_connection_terminate(sconn, nt_errstr(status));
> -		return;
> -	}
> -
> -	status = smbd_smb2_request_next_incoming(sconn);
> -	if (!NT_STATUS_IS_OK(status)) {
> -		smbd_server_connection_terminate(sconn, nt_errstr(status));
> -		return;
> +		return status;
>  	}
>  
>  	sconn->num_requests++;
> @@ -3307,4 +3212,27 @@ static void smbd_smb2_request_incoming(struct tevent_req *subreq)
>  		change_to_root_user();
>  		check_log_size();
>  	}
> +
> +	status = smbd_smb2_request_next_incoming(sconn);
> +	if (!NT_STATUS_IS_OK(status)) {
> +		return status;
> +	}
> +
> +	return NT_STATUS_OK;
> +}
> +
> +static void smbd_smb2_connection_handler(struct tevent_context *ev,
> +					 struct tevent_fd *fde,
> +					 uint16_t flags,
> +					 void *private_data)
> +{
> +	struct smbd_server_connection *sconn = talloc_get_type(private_data,
> +					      struct smbd_server_connection);

Should be talloc_get_type_abort().

> +	NTSTATUS status;
> +
> +	status = smbd_smb2_io_handler(sconn, flags);
> +	if (!NT_STATUS_IS_OK(status)) {
> +		smbd_server_connection_terminate(sconn, nt_errstr(status));
> +		return;
> +	}
>  }