[01/19] cifs: Add SendReceive3

Message ID 20171102070312.18903-2-lsahlber@redhat.com
State New
Headers show
Series
  • cifs: remove rfc1002 header from all smb2 requests
Related show

Commit Message

Leif Sahlberg Nov. 2, 2017, 7:02 a.m.
This function is similar to SendReceive2 except it does not expect
a 4 byte rfc1002 length header in the first io vector.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/cifsproto.h |  4 ++++
 fs/cifs/transport.c | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

Comments

Aurélien Aptel Nov. 2, 2017, 2:14 p.m. | #1
Ronnie Sahlberg <lsahlber@redhat.com> writes:
> +
> +	new_iov = kmalloc(sizeof(struct kvec) * (n_vec + 1), GFP_KERNEL);
> +	if (!new_iov)
> +		return -ENOMEM;
> +
> +	/* 1st iov is an RFC1002 Session Message length */
> +	memcpy(new_iov + 1, iov, (sizeof(struct kvec) * n_vec));
> +
> +	count = 0;
> +	for (i = 1; i < n_vec + 1; i++)
> +		count += new_iov[i].iov_len;
> +
> +	rfc1002_marker = cpu_to_be32(count);
> +
> +	new_iov[0].iov_base = &rfc1002_marker;
> +	new_iov[0].iov_len = 4;
> +
> +	memset(&rqst, 0, sizeof(struct smb_rqst));
> +	rqst.rq_iov = new_iov;
> +	rqst.rq_nvec = n_vec + 1;
> +
> +	rc = cifs_send_recv(xid, ses, &rqst, resp_buf_type, flags, resp_iov);
> +	kfree(new_iov);
> +	return rc;
> +}
> +

I know this is kind of unrelated but I've been thinking to myself we
should try to get rid of this dynamic allocation at some point.

IIUC the iovec never has more than a couple of elements, so we could
have something like a fixed sized stack allocated iovec array +
MAX_IOVEC_LENGTH macro.

Doing a kmalloc() for every packet defeats the purpose of the memory
pool optimization we use for small/large buffers.
Christoph Hellwig Nov. 3, 2017, 6:25 a.m. | #2
On Thu, Nov 02, 2017 at 03:14:26PM +0100, Aurélien Aptel wrote:
> I know this is kind of unrelated but I've been thinking to myself we
> should try to get rid of this dynamic allocation at some point.
> 
> IIUC the iovec never has more than a couple of elements, so we could
> have something like a fixed sized stack allocated iovec array +
> MAX_IOVEC_LENGTH macro.
> 
> Doing a kmalloc() for every packet defeats the purpose of the memory
> pool optimization we use for small/large buffers.

Yes.  You probably just want everyone to allocate an additional
entry in the vector for the header if needed, and just leave the
slot empty and start processing at offset for cases like SMB direct.
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig Nov. 3, 2017, 6:27 a.m. | #3
On Thu, Nov 02, 2017 at 06:02:54PM +1100, Ronnie Sahlberg wrote:
> This function is similar to SendReceive2 except it does not expect
> a 4 byte rfc1002 length header in the first io vector.

Can you give it a proper name?  cifs_ prefix, lower case with _
as the word delimiter.

Also please explain the strategy for phasing out the old API(s) so
that we won't have three around forever.
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
ronnie sahlberg Nov. 3, 2017, 6:37 a.m. | #4
On Fri, Nov 3, 2017 at 4:27 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Thu, Nov 02, 2017 at 06:02:54PM +1100, Ronnie Sahlberg wrote:
>> This function is similar to SendReceive2 except it does not expect
>> a 4 byte rfc1002 length header in the first io vector.
>
> Can you give it a proper name?  cifs_ prefix, lower case with _
> as the word delimiter.

Since this will now be the _only_ send/receive function SMB2 uses
(except for the two async calls)
I can rename it s/SendReceive3/SMB2SendReceive/  ?

>
> Also please explain the strategy for phasing out the old API(s) so
> that we won't have three around forever.

That is harder and I don't plan to do so.
SendReceive2 and SendReceiveNoRsp is after this series no longer used by
smb2, but they are still used by SMB1.
SMB1 uses a LOT of different send/receive functions.


I don't want to touch SMB1. I mostly want it to just go away :-)


> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig Nov. 3, 2017, 1:36 p.m. | #5
On Fri, Nov 03, 2017 at 04:37:59PM +1000, ronnie sahlberg wrote:
> Since this will now be the _only_ send/receive function SMB2 uses
> (except for the two async calls)
> I can rename it s/SendReceive3/SMB2SendReceive/  ?

smb2_send_recv, please.

> > Also please explain the strategy for phasing out the old API(s) so
> > that we won't have three around forever.
> 
> That is harder and I don't plan to do so.
> SendReceive2 and SendReceiveNoRsp is after this series no longer used by
> smb2, but they are still used by SMB1.
> SMB1 uses a LOT of different send/receive functions.
> 
> 
> I don't want to touch SMB1. I mostly want it to just go away :-)

Oh well.  I guess it's just going to slowly bitrot then..
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 4143c9dec463..a4e3c6c5aa64 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -106,6 +106,10 @@  extern int SendReceive2(const unsigned int /* xid */ , struct cifs_ses *,
 			struct kvec *, int /* nvec to send */,
 			int * /* type of buf returned */, const int flags,
 			struct kvec * /* resp vec */);
+extern int SendReceive3(const unsigned int /* xid */ , struct cifs_ses *,
+			struct kvec *, int /* nvec to send */,
+			int * /* type of buf returned */, const int flags,
+			struct kvec * /* resp vec */);
 extern int SendReceiveBlockingLock(const unsigned int xid,
 			struct cifs_tcon *ptcon,
 			struct smb_hdr *in_buf ,
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 7efbab013957..3f16adec3df5 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -827,6 +827,44 @@  SendReceive2(const unsigned int xid, struct cifs_ses *ses,
 	return rc;
 }
 
+/* Like SendReceive2 but iov[0] does not contain an rfc1002 header */
+int
+SendReceive3(const unsigned int xid, struct cifs_ses *ses,
+	     struct kvec *iov, int n_vec, int *resp_buf_type /* ret */,
+	     const int flags, struct kvec *resp_iov)
+{
+	struct smb_rqst rqst;
+	struct kvec *new_iov;
+	int rc;
+	int i;
+	__u32 count;
+	__be32 rfc1002_marker;
+
+	new_iov = kmalloc(sizeof(struct kvec) * (n_vec + 1), GFP_KERNEL);
+	if (!new_iov)
+		return -ENOMEM;
+
+	/* 1st iov is an RFC1002 Session Message length */
+	memcpy(new_iov + 1, iov, (sizeof(struct kvec) * n_vec));
+
+	count = 0;
+	for (i = 1; i < n_vec + 1; i++)
+		count += new_iov[i].iov_len;
+
+	rfc1002_marker = cpu_to_be32(count);
+
+	new_iov[0].iov_base = &rfc1002_marker;
+	new_iov[0].iov_len = 4;
+
+	memset(&rqst, 0, sizeof(struct smb_rqst));
+	rqst.rq_iov = new_iov;
+	rqst.rq_nvec = n_vec + 1;
+
+	rc = cifs_send_recv(xid, ses, &rqst, resp_buf_type, flags, resp_iov);
+	kfree(new_iov);
+	return rc;
+}
+
 int
 SendReceive(const unsigned int xid, struct cifs_ses *ses,
 	    struct smb_hdr *in_buf, struct smb_hdr *out_buf,