Skip to contentSkip to conversation

reduce messing with iovecs in cifs

RFC PATCH In review Al Viro 4 participants 17 messages Apr 9, 2016 – Apr 25, 2016

Subsystems (5)
lore
Share & tools
lore

Al Viro

author
#0 lore raw

RFC PATCH 1/1 reduce messing with iovecs in cifs

Now that sendmsg/recvmsg do not mangle iovecs and are capable of
handling bvec-based ->msg_iter, we can seriously reduce the amount of PITA in cifs. The series below is completely untested, and I would appreciate comments/review/testing/etc.

I'll post the individual patches in followups; for those who prefer to use git it can be found in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git sendmsg.cifs

1/6: [net] drop 'size' argument of sock_recvmsg()
should go via net-next; does what it says.

2/6: cifs: merge the hash calculation helpers
takes the common parts of {cifs,smb2,smb3}_calc_signature() into a
common helper.

3/6: cifs: quit playing games with draining iovecs
Switch smb_send_kvec() to passing msghdr (and thus iov_iter) and
make it use sock_sendmsg() - that allows to avoid draining iovecs, since ->msg_iter will be advanced properly and all we need is to keep it around between the calls of sock_sendmsg(), rather than reinitializing it on each loop iteration. The same thing allows to get rid of messing with kmap() when sending the stuff in ->rq_pages[] - ITER_BVEC will do the right thing.

4/6: cifs: no need to wank with copying and advancing iovec on recvmsg side either
Similar to the previous - use sock_recvmsg() in cifs_readv_from_socket()
and there's no need to modify iovecs, or allocate a copy especially for such modifications, etc.

5/6: cifs_readv_receive: use cifs_read_from_socket()
building a 1-element iovec array for cifs_readv_from_socket() is
an overkill - simple cifs_read_from_socket() will do just fine.

6/6: cifs: don't bother with kmap on read_pages side
Similar to the other half of 3/6: we can use ITER_BVEC for
read-into-page case. Just make cifs_readv_from_socket() take msghdr from caller and use a helper that would feed it a bvec-backed ->msg_iter.

author
#1 lore raw

PATCH 1/6 drop 'size' argument of sock_recvmsg()

all callers have it equal to msg_data_left(msg).

---
 drivers/target/iscsi/iscsi_target_util.c |  5 ++---
 include/linux/net.h                      |  3 +--
 net/socket.c                             | 23 ++++++++++-------------
 3 files changed, 13 insertions(+), 18 deletions(-)
drivers/target/iscsi/iscsi_target_util.c +2 -3
index 428b0d9..5772038 100644
@@ -1283,9 +1283,8 @@ static int iscsit_do_rx_data(
 	iov_iter_kvec(&msg.msg_iter, READ | ITER_KVEC,
 		      count->iov, count->iov_count, data);
 
-	while (total_rx < data) {
-		rx_loop = sock_recvmsg(conn->sock, &msg,
-				      (data - total_rx), MSG_WAITALL);
+	while (msg_data_left(&msg)) {
+		rx_loop = sock_recvmsg(conn->sock, &msg, MSG_WAITALL);
 		if (rx_loop <= 0) {
 			pr_debug("rx_loop: %d total_rx: %d\n",
 				rx_loop, total_rx);
include/linux/net.h +1 -2
index 49175e4..72c1e06 100644
@@ -218,8 +218,7 @@ int sock_create_lite(int family, int type, int proto, struct socket **res);
 struct socket *sock_alloc(void);
 void sock_release(struct socket *sock);
 int sock_sendmsg(struct socket *sock, struct msghdr *msg);
-int sock_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
-		 int flags);
+int sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags);
 struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname);
 struct socket *sockfd_lookup(int fd, int *err);
 struct socket *sock_from_file(struct file *file, int *err);
net/socket.c +10 -13
index 5f77a8e..956426e3 100644
@@ -709,17 +709,16 @@ void __sock_recv_ts_and_drops(struct msghdr *msg, struct sock *sk,
 EXPORT_SYMBOL_GPL(__sock_recv_ts_and_drops);
 
 static inline int sock_recvmsg_nosec(struct socket *sock, struct msghdr *msg,
-				     size_t size, int flags)
+				     int flags)
 {
-	return sock->ops->recvmsg(sock, msg, size, flags);
+	return sock->ops->recvmsg(sock, msg, msg_data_left(msg), flags);
 }
 
-int sock_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
-		 int flags)
+int sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags)
 {
-	int err = security_socket_recvmsg(sock, msg, size, flags);
+	int err = security_socket_recvmsg(sock, msg, msg_data_left(msg), flags);
 
-	return err ?: sock_recvmsg_nosec(sock, msg, size, flags);
+	return err ?: sock_recvmsg_nosec(sock, msg, flags);
 }
 EXPORT_SYMBOL(sock_recvmsg);
 
@@ -746,7 +745,7 @@ int kernel_recvmsg(struct socket *sock, struct msghdr *msg,
 
 	iov_iter_kvec(&msg->msg_iter, READ | ITER_KVEC, vec, num, size);
 	set_fs(KERNEL_DS);
-	result = sock_recvmsg(sock, msg, size, flags);
+	result = sock_recvmsg(sock, msg, flags);
 	set_fs(oldfs);
 	return result;
 }
@@ -796,7 +795,7 @@ static ssize_t sock_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	if (!iov_iter_count(to))	/* Match SYS5 behaviour */
 		return 0;
 
-	res = sock_recvmsg(sock, &msg, iov_iter_count(to), msg.msg_flags);
+	res = sock_recvmsg(sock, &msg, msg.msg_flags);
 	*to = msg.msg_iter;
 	return res;
 }
@@ -1696,7 +1695,7 @@ SYSCALL_DEFINE6(recvfrom, int, fd, void __user *, ubuf, size_t, size,
 	msg.msg_iocb = NULL;
 	if (sock->file->f_flags & O_NONBLOCK)
 		flags |= MSG_DONTWAIT;
-	err = sock_recvmsg(sock, &msg, iov_iter_count(&msg.msg_iter), flags);
+	err = sock_recvmsg(sock, &msg, flags);
 
 	if (err >= 0 && addr != NULL) {
 		err2 = move_addr_to_user(&address,
@@ -2073,7 +2072,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
 	struct iovec iovstack[UIO_FASTIOV];
 	struct iovec *iov = iovstack;
 	unsigned long cmsg_ptr;
-	int total_len, len;
+	int len;
 	ssize_t err;
 
 	/* kernel mode address */
@@ -2091,7 +2090,6 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
 		err = copy_msghdr_from_user(msg_sys, msg, &uaddr, &iov);
 	if (err < 0)
 		return err;
-	total_len = iov_iter_count(&msg_sys->msg_iter);
 
 	cmsg_ptr = (unsigned long)msg_sys->msg_control;
 	msg_sys->msg_flags = flags & (MSG_CMSG_CLOEXEC|MSG_CMSG_COMPAT);
@@ -2101,8 +2099,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
 
 	if (sock->file->f_flags & O_NONBLOCK)
 		flags |= MSG_DONTWAIT;
-	err = (nosec ? sock_recvmsg_nosec : sock_recvmsg)(sock, msg_sys,
-							  total_len, flags);
+	err = (nosec ? sock_recvmsg_nosec : sock_recvmsg)(sock, msg_sys, flags);
 	if (err < 0)
 		goto out_freeiov;
 	len = err;
Signature · 1 line
2.8.0.rc3
Reviewed-by on v1 1/6
#2 lore raw

Re: [PATCH 1/6] [net] drop 'size' argument of sock_recvmsg()

Quoted 117 lines · Al Viro

all callers have it equal to msg_data_left(msg).

---
 drivers/target/iscsi/iscsi_target_util.c |  5 ++--- include/linux/net.h                      |  3 +-- net/socket.c                             | 23 ++++++++++-------------

 3 files changed, 13 insertions(+), 18 deletions(-)
drivers/target/iscsi/iscsi_target_util.c +0 -0
index 428b0d9..5772038 100644
@@ -1283,9 +1283,8 @@ static int iscsit_do_rx_data(
  	iov_iter_kvec(&msg.msg_iter, READ | ITER_KVEC,
  		      count->iov, count->iov_count, data);

drivers/target/iscsi/iscsi_target_util.c +2 -3
-	while (total_rx < data) {
-		rx_loop = sock_recvmsg(conn->sock, &msg,
-				      (data - total_rx), MSG_WAITALL);
+	while (msg_data_left(&msg)) {
+		rx_loop = sock_recvmsg(conn->sock, &msg, MSG_WAITALL);

  if (rx_loop <= 0) { pr_debug("rx_loop: %d total_rx: %d\n", rx_loop, total_rx);

include/linux/net.h +1 -2
index 49175e4..72c1e06 100644
@@ -218,8 +218,7 @@ int sock_create_lite(int family, int type, int proto, struct socket **res);
  struct socket *sock_alloc(void);
  void sock_release(struct socket *sock);
  int sock_sendmsg(struct socket *sock, struct msghdr *msg);
-int sock_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
-		 int flags);
+int sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags);
  struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname);
  struct socket *sockfd_lookup(int fd, int *err);
  struct socket *sock_from_file(struct file *file, int *err);
net/socket.c +0 -0
index 5f77a8e..956426e3 100644
@@ -709,17 +709,16 @@ void __sock_recv_ts_and_drops(struct msghdr *msg, struct sock *sk,
  EXPORT_SYMBOL_GPL(__sock_recv_ts_and_drops);


 static inline int sock_recvmsg_nosec(struct socket *sock, struct msghdr *msg,

net/socket.c +1 -1
-				     size_t size, int flags)
+				     int flags)

 {

net/socket.c +1 -1
-	return sock->ops->recvmsg(sock, msg, size, flags);
+	return sock->ops->recvmsg(sock, msg, msg_data_left(msg), flags);

 }

net/socket.c +1 -2
-int sock_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
-		 int flags)
+int sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags)

 {

net/socket.c +1 -1
-	int err = security_socket_recvmsg(sock, msg, size, flags);
+	int err = security_socket_recvmsg(sock, msg, msg_data_left(msg), flags);

net/socket.c +1 -1
-	return err ?: sock_recvmsg_nosec(sock, msg, size, flags);
+	return err ?: sock_recvmsg_nosec(sock, msg, flags);

 }
 EXPORT_SYMBOL(sock_recvmsg);

net/socket.c +0 -0
@@ -746,7 +745,7 @@ int kernel_recvmsg(struct socket *sock, struct msghdr *msg,


  iov_iter_kvec(&msg->msg_iter, READ | ITER_KVEC, vec, num, size); set_fs(KERNEL_DS);

net/socket.c +1 -1
-	result = sock_recvmsg(sock, msg, size, flags);
+	result = sock_recvmsg(sock, msg, flags);

  set_fs(oldfs);
  return result;
 }

net/socket.c +0 -0
@@ -796,7 +795,7 @@ static ssize_t sock_read_iter(struct kiocb *iocb, struct iov_iter *to)

  if (!iov_iter_count(to)) /* Match SYS5 behaviour */
  return 0;

net/socket.c +1 -1
-	res = sock_recvmsg(sock, &msg, iov_iter_count(to), msg.msg_flags);
+	res = sock_recvmsg(sock, &msg, msg.msg_flags);

  *to = msg.msg_iter;
  return res;
 }

net/socket.c +1 -1
@@ -1696,7 +1695,7 @@ SYSCALL_DEFINE6(recvfrom, int, fd, void __user *, ubuf, size_t, size,
  	msg.msg_iocb = NULL;
  	if (sock->file->f_flags & O_NONBLOCK)
  		flags |= MSG_DONTWAIT;
-	err = sock_recvmsg(sock, &msg, iov_iter_count(&msg.msg_iter), flags);
+	err = sock_recvmsg(sock, &msg, flags);


  if (err >= 0 && addr != NULL) { err2 = move_addr_to_user(&address,

net/socket.c +1 -1
@@ -2073,7 +2072,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
  	struct iovec iovstack[UIO_FASTIOV];
  	struct iovec *iov = iovstack;
  	unsigned long cmsg_ptr;
-	int total_len, len;
+	int len;
  	ssize_t err;


  /* kernel mode address */

net/socket.c +0 -1
@@ -2091,7 +2090,6 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
  		err = copy_msghdr_from_user(msg_sys, msg, &uaddr, &iov);
  	if (err < 0)
  		return err;
-	total_len = iov_iter_count(&msg_sys->msg_iter);


  cmsg_ptr = (unsigned long)msg_sys->msg_control; msg_sys->msg_flags = flags & (MSG_CMSG_CLOEXEC|MSG_CMSG_COMPAT);

net/socket.c +0 -0
@@ -2101,8 +2099,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,


  if (sock->file->f_flags & O_NONBLOCK)
  flags |= MSG_DONTWAIT;

net/socket.c +1 -2
-	err = (nosec ? sock_recvmsg_nosec : sock_recvmsg)(sock, msg_sys,
-							  total_len, flags);
+	err = (nosec ? sock_recvmsg_nosec : sock_recvmsg)(sock, msg_sys, flags);
Quoted 3 lines

  if (err < 0)
  goto out_freeiov;
  len = err;

Looks good. Assuming that the corresponding change to sock_recvmsg is merged.

author
#3 lore raw

PATCH 2/6 cifs: merge the hash calculation helpers

In reply to Al Viro

three practically identical copies...

---
 fs/cifs/cifsencrypt.c   |  97 ++++++++++++++++++++++++-------------------
 fs/cifs/cifsproto.h     |   3 ++
 fs/cifs/smb2transport.c | 107 +++++-------------------------------------------
 3 files changed, 67 insertions(+), 140 deletions(-)
fs/cifs/cifsencrypt.c +54 -43
index 4897dac..6aeb8d4 100644
@@ -66,45 +66,15 @@ cifs_crypto_shash_md5_allocate(struct TCP_Server_Info *server)
 	return 0;
 }
 
-/*
- * Calculate and return the CIFS signature based on the mac key and SMB PDU.
- * The 16 byte signature must be allocated by the caller. Note we only use the
- * 1st eight bytes and that the smb header signature field on input contains
- * the sequence number before this function is called. Also, this function
- * should be called with the server->srv_mutex held.
- */
-static int cifs_calc_signature(struct smb_rqst *rqst,
-			struct TCP_Server_Info *server, char *signature)
+int __cifs_calc_signature(struct smb_rqst *rqst,
+			struct TCP_Server_Info *server, char *signature,
+			struct shash_desc *shash)
 {
 	int i;
 	int rc;
 	struct kvec *iov = rqst->rq_iov;
 	int n_vec = rqst->rq_nvec;
 
-	if (iov == NULL || signature == NULL || server == NULL)
-		return -EINVAL;
-
-	if (!server->secmech.sdescmd5) {
-		rc = cifs_crypto_shash_md5_allocate(server);
-		if (rc) {
-			cifs_dbg(VFS, "%s: Can't alloc md5 crypto\n", __func__);
-			return -1;
-		}
-	}
-
-	rc = crypto_shash_init(&server->secmech.sdescmd5->shash);
-	if (rc) {
-		cifs_dbg(VFS, "%s: Could not init md5\n", __func__);
-		return rc;
-	}
-
-	rc = crypto_shash_update(&server->secmech.sdescmd5->shash,
-		server->session_key.response, server->session_key.len);
-	if (rc) {
-		cifs_dbg(VFS, "%s: Could not update with response\n", __func__);
-		return rc;
-	}
-
 	for (i = 0; i < n_vec; i++) {
 		if (iov[i].iov_len == 0)
 			continue;
@@ -117,12 +87,10 @@ static int cifs_calc_signature(struct smb_rqst *rqst,
 		if (i == 0) {
 			if (iov[0].iov_len <= 8) /* cmd field at offset 9 */
 				break; /* nothing to sign or corrupt header */
-			rc =
-			crypto_shash_update(&server->secmech.sdescmd5->shash,
+			rc = crypto_shash_update(shash,
 				iov[i].iov_base + 4, iov[i].iov_len - 4);
 		} else {
-			rc =
-			crypto_shash_update(&server->secmech.sdescmd5->shash,
+			rc = crypto_shash_update(shash,
 				iov[i].iov_base, iov[i].iov_len);
 		}
 		if (rc) {
@@ -134,21 +102,64 @@ static int cifs_calc_signature(struct smb_rqst *rqst,
 
 	/* now hash over the rq_pages array */
 	for (i = 0; i < rqst->rq_npages; i++) {
-		struct kvec p_iov;
+		void *kaddr = kmap(rqst->rq_pages[i]);
+		size_t len = rqst->rq_pagesz;
+
+		if (i == rqst->rq_npages - 1)
+			len = rqst->rq_tailsz;
+
+		crypto_shash_update(shash, kaddr, len);
 
-		cifs_rqst_page_to_kvec(rqst, i, &p_iov);
-		crypto_shash_update(&server->secmech.sdescmd5->shash,
-					p_iov.iov_base, p_iov.iov_len);
 		kunmap(rqst->rq_pages[i]);
 	}
 
-	rc = crypto_shash_final(&server->secmech.sdescmd5->shash, signature);
+	rc = crypto_shash_final(shash, signature);
 	if (rc)
-		cifs_dbg(VFS, "%s: Could not generate md5 hash\n", __func__);
+		cifs_dbg(VFS, "%s: Could not generate hash\n", __func__);
 
 	return rc;
 }
 
+/*
+ * Calculate and return the CIFS signature based on the mac key and SMB PDU.
+ * The 16 byte signature must be allocated by the caller. Note we only use the
+ * 1st eight bytes and that the smb header signature field on input contains
+ * the sequence number before this function is called. Also, this function
+ * should be called with the server->srv_mutex held.
+ */
+static int cifs_calc_signature(struct smb_rqst *rqst,
+			struct TCP_Server_Info *server, char *signature)
+{
+	int rc;
+
+	if (!rqst->rq_iov || !signature || !server)
+		return -EINVAL;
+
+	if (!server->secmech.sdescmd5) {
+		rc = cifs_crypto_shash_md5_allocate(server);
+		if (rc) {
+			cifs_dbg(VFS, "%s: Can't alloc md5 crypto\n", __func__);
+			return -1;
+		}
+	}
+
+	rc = crypto_shash_init(&server->secmech.sdescmd5->shash);
+	if (rc) {
+		cifs_dbg(VFS, "%s: Could not init md5\n", __func__);
+		return rc;
+	}
+
+	rc = crypto_shash_update(&server->secmech.sdescmd5->shash,
+		server->session_key.response, server->session_key.len);
+	if (rc) {
+		cifs_dbg(VFS, "%s: Could not update with response\n", __func__);
+		return rc;
+	}
+
+	return __cifs_calc_signature(rqst, server, signature,
+				     &server->secmech.sdescmd5->shash);
+}
+
 /* must be called with server->srv_mutex held */
 int cifs_sign_rqst(struct smb_rqst *rqst, struct TCP_Server_Info *server,
 		   __u32 *pexpected_response_sequence_number)
fs/cifs/cifsproto.h +3 -0
index eed7ff5..d9b4f44 100644
@@ -512,4 +512,7 @@ int cifs_create_mf_symlink(unsigned int xid, struct cifs_tcon *tcon,
 			   struct cifs_sb_info *cifs_sb,
 			   const unsigned char *path, char *pbuf,
 			   unsigned int *pbytes_written);
+int __cifs_calc_signature(struct smb_rqst *rqst,
+			struct TCP_Server_Info *server, char *signature,
+			struct shash_desc *shash);
 #endif			/* _CIFSPROTO_H */
fs/cifs/smb2transport.c +10 -97

140 lines not on this page · raw message

Signature · 1 line
2.8.0.rc3
Reviewed-by Tested-by on v1 2/6
#4 lore raw

Re: [PATCH 2/6] cifs: merge the hash calculation helpers

Looks correct.
Tested with -o vers=1.0, -o vers=2.0, and -o vers=2.1. Will test using mount option vers=3.0 as soon as I find a server.

Quoted 315 lines · Al Viro

three practically identical copies...

---
 fs/cifs/cifsencrypt.c   |  97 ++++++++++++++++++++++++-------------------
 fs/cifs/cifsproto.h     |   3 ++
 fs/cifs/smb2transport.c | 107 +++++-------------------------------------------
 3 files changed, 67 insertions(+), 140 deletions(-)
fs/cifs/cifsencrypt.c +54 -43
index 4897dac..6aeb8d4 100644
@@ -66,45 +66,15 @@ cifs_crypto_shash_md5_allocate(struct TCP_Server_Info *server)
        return 0;
 }
 
-/*
- * Calculate and return the CIFS signature based on the mac key and SMB PDU.
- * The 16 byte signature must be allocated by the caller. Note we only use the
- * 1st eight bytes and that the smb header signature field on input contains
- * the sequence number before this function is called. Also, this function
- * should be called with the server->srv_mutex held.
- */
-static int cifs_calc_signature(struct smb_rqst *rqst,
-                       struct TCP_Server_Info *server, char *signature)
+int __cifs_calc_signature(struct smb_rqst *rqst,
+                       struct TCP_Server_Info *server, char *signature,
+                       struct shash_desc *shash)
 {
        int i;
        int rc;
        struct kvec *iov = rqst->rq_iov;
        int n_vec = rqst->rq_nvec;
 
-       if (iov == NULL || signature == NULL || server == NULL)
-               return -EINVAL;
-
-       if (!server->secmech.sdescmd5) {
-               rc = cifs_crypto_shash_md5_allocate(server);
-               if (rc) {
-                       cifs_dbg(VFS, "%s: Can't alloc md5 crypto\n", __func__);
-                       return -1;
-               }
-       }
-
-       rc = crypto_shash_init(&server->secmech.sdescmd5->shash);
-       if (rc) {
-               cifs_dbg(VFS, "%s: Could not init md5\n", __func__);
-               return rc;
-       }
-
-       rc = crypto_shash_update(&server->secmech.sdescmd5->shash,
-               server->session_key.response, server->session_key.len);
-       if (rc) {
-               cifs_dbg(VFS, "%s: Could not update with response\n", __func__);
-               return rc;
-       }
-
        for (i = 0; i < n_vec; i++) {
                if (iov[i].iov_len == 0)
                        continue;
@@ -117,12 +87,10 @@ static int cifs_calc_signature(struct smb_rqst *rqst,
                if (i == 0) {
                        if (iov[0].iov_len <= 8) /* cmd field at offset 9 */
                                break; /* nothing to sign or corrupt header */
-                       rc =
-                       crypto_shash_update(&server->secmech.sdescmd5->shash,
+                       rc = crypto_shash_update(shash,
                                iov[i].iov_base + 4, iov[i].iov_len - 4);
                } else {
-                       rc =
-                       crypto_shash_update(&server->secmech.sdescmd5->shash,
+                       rc = crypto_shash_update(shash,
                                iov[i].iov_base, iov[i].iov_len);
                }
                if (rc) {
@@ -134,21 +102,64 @@ static int cifs_calc_signature(struct smb_rqst *rqst,
 
        /* now hash over the rq_pages array */
        for (i = 0; i < rqst->rq_npages; i++) {
-               struct kvec p_iov;
+               void *kaddr = kmap(rqst->rq_pages[i]);
+               size_t len = rqst->rq_pagesz;
+
+               if (i == rqst->rq_npages - 1)
+                       len = rqst->rq_tailsz;
+
+               crypto_shash_update(shash, kaddr, len);
 
-               cifs_rqst_page_to_kvec(rqst, i, &p_iov);
-               crypto_shash_update(&server->secmech.sdescmd5->shash,
-                                       p_iov.iov_base, p_iov.iov_len);
                kunmap(rqst->rq_pages[i]);
        }
 
-       rc = crypto_shash_final(&server->secmech.sdescmd5->shash, signature);
+       rc = crypto_shash_final(shash, signature);
        if (rc)
-               cifs_dbg(VFS, "%s: Could not generate md5 hash\n", __func__);
+               cifs_dbg(VFS, "%s: Could not generate hash\n", __func__);
 
        return rc;
 }
 
+/*
+ * Calculate and return the CIFS signature based on the mac key and SMB PDU.
+ * The 16 byte signature must be allocated by the caller. Note we only use the
+ * 1st eight bytes and that the smb header signature field on input contains
+ * the sequence number before this function is called. Also, this function
+ * should be called with the server->srv_mutex held.
+ */
+static int cifs_calc_signature(struct smb_rqst *rqst,
+                       struct TCP_Server_Info *server, char *signature)
+{
+       int rc;
+
+       if (!rqst->rq_iov || !signature || !server)
+               return -EINVAL;
+
+       if (!server->secmech.sdescmd5) {
+               rc = cifs_crypto_shash_md5_allocate(server);
+               if (rc) {
+                       cifs_dbg(VFS, "%s: Can't alloc md5 crypto\n", __func__);
+                       return -1;
+               }
+       }
+
+       rc = crypto_shash_init(&server->secmech.sdescmd5->shash);
+       if (rc) {
+               cifs_dbg(VFS, "%s: Could not init md5\n", __func__);
+               return rc;
+       }
+
+       rc = crypto_shash_update(&server->secmech.sdescmd5->shash,
+               server->session_key.response, server->session_key.len);
+       if (rc) {
+               cifs_dbg(VFS, "%s: Could not update with response\n", __func__);
+               return rc;
+       }
+
+       return __cifs_calc_signature(rqst, server, signature,
+                                    &server->secmech.sdescmd5->shash);
+}
+
 /* must be called with server->srv_mutex held */
 int cifs_sign_rqst(struct smb_rqst *rqst, struct TCP_Server_Info *server,
                   __u32 *pexpected_response_sequence_number)
fs/cifs/cifsproto.h +3 -0
index eed7ff5..d9b4f44 100644
@@ -512,4 +512,7 @@ int cifs_create_mf_symlink(unsigned int xid, struct cifs_tcon *tcon,
                           struct cifs_sb_info *cifs_sb,
                           const unsigned char *path, char *pbuf,
                           unsigned int *pbytes_written);
+int __cifs_calc_signature(struct smb_rqst *rqst,
+                       struct TCP_Server_Info *server, char *signature,
+                       struct shash_desc *shash);
 #endif                 /* _CIFSPROTO_H */
fs/cifs/smb2transport.c +10 -98
index 8732a43..bc9a7b6 100644
@@ -135,11 +135,10 @@ smb2_find_smb_ses(struct smb2_hdr *smb2hdr, struct TCP_Server_Info *server)
 int
 smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
 {
-       int i, rc;
+       int rc;
        unsigned char smb2_signature[SMB2_HMACSHA256_SIZE];
        unsigned char *sigptr = smb2_signature;
        struct kvec *iov = rqst->rq_iov;
-       int n_vec = rqst->rq_nvec;
        struct smb2_hdr *smb2_pdu = (struct smb2_hdr *)iov[0].iov_base;
        struct cifs_ses *ses;
 
@@ -171,53 +170,11 @@ smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
                return rc;
        }
 
-       for (i = 0; i < n_vec; i++) {
-               if (iov[i].iov_len == 0)
-                       continue;
-               if (iov[i].iov_base == NULL) {
-                       cifs_dbg(VFS, "null iovec entry\n");
-                       return -EIO;
-               }
-               /*
-                * The first entry includes a length field (which does not get
-                * signed that occupies the first 4 bytes before the header).
-                */
-               if (i == 0) {
-                       if (iov[0].iov_len <= 8) /* cmd field at offset 9 */
-                               break; /* nothing to sign or corrupt header */
-                       rc =
-                       crypto_shash_update(
-                               &server->secmech.sdeschmacsha256->shash,
-                               iov[i].iov_base + 4, iov[i].iov_len - 4);
-               } else {
-                       rc =
-                       crypto_shash_update(
-                               &server->secmech.sdeschmacsha256->shash,
-                               iov[i].iov_base, iov[i].iov_len);
-               }
-               if (rc) {
-                       cifs_dbg(VFS, "%s: Could not update with payload\n",
-                                __func__);
-                       return rc;
-               }
-       }
-
-       /* now hash over the rq_pages array */
-       for (i = 0; i < rqst->rq_npages; i++) {
-               struct kvec p_iov;
-
-               cifs_rqst_page_to_kvec(rqst, i, &p_iov);
-               crypto_shash_update(&server->secmech.sdeschmacsha256->shash,
-                                       p_iov.iov_base, p_iov.iov_len);
-               kunmap(rqst->rq_pages[i]);
-       }
-
-       rc = crypto_shash_final(&server->secmech.sdeschmacsha256->shash,
-                               sigptr);
-       if (rc)
-               cifs_dbg(VFS, "%s: Could not generate sha256 hash\n", __func__);
+       rc = __cifs_calc_signature(rqst, server, sigptr,
+               &server->secmech.sdeschmacsha256->shash);
 
-       memcpy(smb2_pdu->Signature, sigptr, SMB2_SIGNATURE_SIZE);
+       if (!rc)
+               memcpy(smb2_pdu->Signature, sigptr, SMB2_SIGNATURE_SIZE);
 
        return rc;
 }
@@ -395,12 +352,10 @@ generate_smb311signingkey(struct cifs_ses *ses)
 int
 smb3_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
 {
-       int i;
        int rc = 0;
        unsigned char smb3_signature[SMB2_CMACAES_SIZE];
        unsigned char *sigptr = smb3_signature;
        struct kvec *iov = rqst->rq_iov;
-       int n_vec = rqst->rq_nvec;
        struct smb2_hdr *smb2_pdu = (struct smb2_hdr *)iov[0].iov_base;
        struct cifs_ses *ses;
 
@@ -431,54 +386,12 @@ smb3_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
                cifs_dbg(VFS, "%s: Could not init cmac aes\n", __func__);
                return rc;
        }
+
+       rc = __cifs_calc_signature(rqst, server, sigptr,
+                                  &server->secmech.sdesccmacaes->shash);
 
-       for (i = 0; i < n_vec; i++) {
-               if (iov[i].iov_len == 0)
-                       continue;
-               if (iov[i].iov_base == NULL) {
-                       cifs_dbg(VFS, "null iovec entry");
-                       return -EIO;
-               }
-               /*
-                * The first entry includes a length field (which does not get
-                * signed that occupies the first 4 bytes before the header).
-                */
-               if (i == 0) {
-                       if (iov[0].iov_len <= 8) /* cmd field at offset 9 */
-                               break; /* nothing to sign or corrupt header */
-                       rc =
-                       crypto_shash_update(
-                               &server->secmech.sdesccmacaes->shash,
-                               iov[i].iov_base + 4, iov[i].iov_len - 4);
-               } else {
-                       rc =
-                       crypto_shash_update(
-                               &server->secmech.sdesccmacaes->shash,
-                               iov[i].iov_base, iov[i].iov_len);
-               }
-               if (rc) {
-                       cifs_dbg(VFS, "%s: Couldn't update cmac aes with payload\n",
-                                                       __func__);
-                       return rc;
-               }
-       }
-
-       /* now hash over the rq_pages array */
-       for (i = 0; i < rqst->rq_npages; i++) {
-               struct kvec p_iov;
-
-               cifs_rqst_page_to_kvec(rqst, i, &p_iov);
-               crypto_shash_update(&server->secmech.sdesccmacaes->shash,
-                                       p_iov.iov_base, p_iov.iov_len);
-               kunmap(rqst->rq_pages[i]);
-       }
-
-       rc = crypto_shash_final(&server->secmech.sdesccmacaes->shash,
-                                               sigptr);
-       if (rc)
-               cifs_dbg(VFS, "%s: Could not generate cmac aes\n", __func__);
-
-       memcpy(smb2_pdu->Signature, sigptr, SMB2_SIGNATURE_SIZE);
+       if (!rc)
+               memcpy(smb2_pdu->Signature, sigptr, SMB2_SIGNATURE_SIZE);
 
        return rc;
 }
--

2.8.0.rc3

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Reviewed-by on v1 2/6
#5 lore raw

Re: [PATCH 2/6] cifs: merge the hash calculation helpers

In reply to Al Viro
Quoted 305 lines · Al Viro

three practically identical copies...

---
 fs/cifs/cifsencrypt.c   |  97 ++++++++++++++++++++++++------------------- fs/cifs/cifsproto.h     |   3 ++
 fs/cifs/smb2transport.c | 107 +++++-------------------------------------------

 3 files changed, 67 insertions(+), 140 deletions(-)
fs/cifs/cifsencrypt.c +0 -0
index 4897dac..6aeb8d4 100644
@@ -66,45 +66,15 @@ cifs_crypto_shash_md5_allocate(struct TCP_Server_Info *server)
  	return 0;
  }

fs/cifs/cifsencrypt.c +3 -9
-/*
- * Calculate and return the CIFS signature based on the mac key and SMB PDU.
- * The 16 byte signature must be allocated by the caller. Note we only use the
- * 1st eight bytes and that the smb header signature field on input contains
- * the sequence number before this function is called. Also, this function
- * should be called with the server->srv_mutex held.
- */
-static int cifs_calc_signature(struct smb_rqst *rqst,
-			struct TCP_Server_Info *server, char *signature)
+int __cifs_calc_signature(struct smb_rqst *rqst,
+			struct TCP_Server_Info *server, char *signature,
+			struct shash_desc *shash)

 {
  int i;
  int rc;
  struct kvec *iov = rqst->rq_iov;
  int n_vec = rqst->rq_nvec;

fs/cifs/cifsencrypt.c +0 -24
-	if (iov == NULL || signature == NULL || server == NULL)
-		return -EINVAL;
-
-	if (!server->secmech.sdescmd5) {
-		rc = cifs_crypto_shash_md5_allocate(server);
-		if (rc) {
-			cifs_dbg(VFS, "%s: Can't alloc md5 crypto\n", __func__);
-			return -1;
-		}
-	}
-
-	rc = crypto_shash_init(&server->secmech.sdescmd5->shash);
-	if (rc) {
-		cifs_dbg(VFS, "%s: Could not init md5\n", __func__);
-		return rc;
-	}
-
-	rc = crypto_shash_update(&server->secmech.sdescmd5->shash,
-		server->session_key.response, server->session_key.len);
-	if (rc) {
-		cifs_dbg(VFS, "%s: Could not update with response\n", __func__);
-		return rc;
-	}
-

  for (i = 0; i < n_vec; i++) {
  if (iov[i].iov_len == 0)
  continue;

fs/cifs/cifsencrypt.c +0 -0
@@ -117,12 +87,10 @@ static int cifs_calc_signature(struct smb_rqst *rqst,
  		if (i == 0) {

  if (iov[0].iov_len <= 8) /* cmd field at offset 9 */ break; /* nothing to sign or corrupt header */

fs/cifs/cifsencrypt.c +1 -2
-			rc =
-			crypto_shash_update(&server->secmech.sdescmd5->shash,
+			rc = crypto_shash_update(shash,

  iov[i].iov_base + 4, iov[i].iov_len - 4); } else {

fs/cifs/cifsencrypt.c +1 -2
-			rc =
-			crypto_shash_update(&server->secmech.sdescmd5->shash,
+			rc = crypto_shash_update(shash,

  iov[i].iov_base, iov[i].iov_len); }
  if (rc) {

fs/cifs/cifsencrypt.c +0 -0
@@ -134,21 +102,64 @@ static int cifs_calc_signature(struct smb_rqst *rqst,


  /* now hash over the rq_pages array */
  for (i = 0; i < rqst->rq_npages; i++) {

fs/cifs/cifsencrypt.c +7 -1
-		struct kvec p_iov;
+		void *kaddr = kmap(rqst->rq_pages[i]);
+		size_t len = rqst->rq_pagesz;
+
+		if (i == rqst->rq_npages - 1)
+			len = rqst->rq_tailsz;
+
+		crypto_shash_update(shash, kaddr, len);

fs/cifs/cifsencrypt.c +0 -3
-		cifs_rqst_page_to_kvec(rqst, i, &p_iov);
-		crypto_shash_update(&server->secmech.sdescmd5->shash,
-					p_iov.iov_base, p_iov.iov_len);

  kunmap(rqst->rq_pages[i]);
  }

fs/cifs/cifsencrypt.c +1 -1
-	rc = crypto_shash_final(&server->secmech.sdescmd5->shash, signature);
+	rc = crypto_shash_final(shash, signature);

  if (rc)

fs/cifs/cifsencrypt.c +1 -1
-		cifs_dbg(VFS, "%s: Could not generate md5 hash\n", __func__);
+		cifs_dbg(VFS, "%s: Could not generate hash\n", __func__);


  return rc;
 }

fs/cifs/cifsencrypt.c +40 -0
+/*
+ * Calculate and return the CIFS signature based on the mac key and SMB PDU.
+ * The 16 byte signature must be allocated by the caller. Note we only use the
+ * 1st eight bytes and that the smb header signature field on input contains
+ * the sequence number before this function is called. Also, this function
+ * should be called with the server->srv_mutex held.
+ */
+static int cifs_calc_signature(struct smb_rqst *rqst,
+			struct TCP_Server_Info *server, char *signature)
+{
+	int rc;
+
+	if (!rqst->rq_iov || !signature || !server)
+		return -EINVAL;
+
+	if (!server->secmech.sdescmd5) {
+		rc = cifs_crypto_shash_md5_allocate(server);
+		if (rc) {
+			cifs_dbg(VFS, "%s: Can't alloc md5 crypto\n", __func__);
+			return -1;
+		}
+	}
+
+	rc = crypto_shash_init(&server->secmech.sdescmd5->shash);
+	if (rc) {
+		cifs_dbg(VFS, "%s: Could not init md5\n", __func__);
+		return rc;
+	}
+
+	rc = crypto_shash_update(&server->secmech.sdescmd5->shash,
+		server->session_key.response, server->session_key.len);
+	if (rc) {
+		cifs_dbg(VFS, "%s: Could not update with response\n", __func__);
+		return rc;
+	}
+
+	return __cifs_calc_signature(rqst, server, signature,
+				     &server->secmech.sdescmd5->shash);
+}
+

 /* must be called with server->srv_mutex held */
 int cifs_sign_rqst(struct smb_rqst *rqst, struct TCP_Server_Info *server, __u32 *pexpected_response_sequence_number)

fs/cifs/cifsproto.h +3 -0
index eed7ff5..d9b4f44 100644
@@ -512,4 +512,7 @@ int cifs_create_mf_symlink(unsigned int xid, struct cifs_tcon *tcon,
  			   struct cifs_sb_info *cifs_sb,
  			   const unsigned char *path, char *pbuf,
  			   unsigned int *pbytes_written);
+int __cifs_calc_signature(struct smb_rqst *rqst,
+			struct TCP_Server_Info *server, char *signature,
+			struct shash_desc *shash);
  #endif			/* _CIFSPROTO_H */
fs/cifs/smb2transport.c +0 -0
index 8732a43..bc9a7b6 100644
@@ -135,11 +135,10 @@ smb2_find_smb_ses(struct smb2_hdr *smb2hdr, struct TCP_Server_Info *server)

 int
 smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server) {

fs/cifs/smb2transport.c +1 -1
-	int i, rc;
+	int rc;

  unsigned char smb2_signature[SMB2_HMACSHA256_SIZE]; unsigned char *sigptr = smb2_signature; struct kvec *iov = rqst->rq_iov;

fs/cifs/smb2transport.c +0 -1
-	int n_vec = rqst->rq_nvec;

  struct smb2_hdr *smb2_pdu = (struct smb2_hdr *)iov[0].iov_base; struct cifs_ses *ses;

fs/cifs/smb2transport.c +0 -0
@@ -171,53 +170,11 @@ smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
  		return rc;
  	}

fs/cifs/smb2transport.c +2 -45
-	for (i = 0; i < n_vec; i++) {
-		if (iov[i].iov_len == 0)
-			continue;
-		if (iov[i].iov_base == NULL) {
-			cifs_dbg(VFS, "null iovec entry\n");
-			return -EIO;
-		}
-		/*
-		 * The first entry includes a length field (which does not get
-		 * signed that occupies the first 4 bytes before the header).
-		 */
-		if (i == 0) {
-			if (iov[0].iov_len <= 8) /* cmd field at offset 9 */
-				break; /* nothing to sign or corrupt header */
-			rc =
-			crypto_shash_update(
-				&server->secmech.sdeschmacsha256->shash,
-				iov[i].iov_base + 4, iov[i].iov_len - 4);
-		} else {
-			rc =
-			crypto_shash_update(
-				&server->secmech.sdeschmacsha256->shash,
-				iov[i].iov_base, iov[i].iov_len);
-		}
-		if (rc) {
-			cifs_dbg(VFS, "%s: Could not update with payload\n",
-				 __func__);
-			return rc;
-		}
-	}
-
-	/* now hash over the rq_pages array */
-	for (i = 0; i < rqst->rq_npages; i++) {
-		struct kvec p_iov;
-
-		cifs_rqst_page_to_kvec(rqst, i, &p_iov);
-		crypto_shash_update(&server->secmech.sdeschmacsha256->shash,
-					p_iov.iov_base, p_iov.iov_len);
-		kunmap(rqst->rq_pages[i]);
-	}
-
-	rc = crypto_shash_final(&server->secmech.sdeschmacsha256->shash,
-				sigptr);
-	if (rc)
-		cifs_dbg(VFS, "%s: Could not generate sha256 hash\n", __func__);
+	rc = __cifs_calc_signature(rqst, server, sigptr,
+		&server->secmech.sdeschmacsha256->shash);

fs/cifs/smb2transport.c +2 -1
-	memcpy(smb2_pdu->Signature, sigptr, SMB2_SIGNATURE_SIZE);
+	if (!rc)
+		memcpy(smb2_pdu->Signature, sigptr, SMB2_SIGNATURE_SIZE);


  return rc;
 }

fs/cifs/smb2transport.c +0 -0
@@ -395,12 +352,10 @@ generate_smb311signingkey(struct cifs_ses *ses)

 int
 smb3_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server) {

fs/cifs/smb2transport.c +0 -1
-	int i;

  int rc = 0;
  unsigned char smb3_signature[SMB2_CMACAES_SIZE]; unsigned char *sigptr = smb3_signature; struct kvec *iov = rqst->rq_iov;

fs/cifs/smb2transport.c +0 -1
-	int n_vec = rqst->rq_nvec;

  struct smb2_hdr *smb2_pdu = (struct smb2_hdr *)iov[0].iov_base; struct cifs_ses *ses;

fs/cifs/smb2transport.c +3 -0
@@ -431,54 +386,12 @@ smb3_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
  		cifs_dbg(VFS, "%s: Could not init cmac aes\n", __func__);
  		return rc;
  	}
+	
+	rc = __cifs_calc_signature(rqst, server, sigptr,
+				   &server->secmech.sdesccmacaes->shash);

fs/cifs/smb2transport.c +2 -47
-	for (i = 0; i < n_vec; i++) {
-		if (iov[i].iov_len == 0)
-			continue;
-		if (iov[i].iov_base == NULL) {
-			cifs_dbg(VFS, "null iovec entry");
-			return -EIO;
-		}
-		/*
-		 * The first entry includes a length field (which does not get
-		 * signed that occupies the first 4 bytes before the header).
-		 */
-		if (i == 0) {
-			if (iov[0].iov_len <= 8) /* cmd field at offset 9 */
-				break; /* nothing to sign or corrupt header */
-			rc =
-			crypto_shash_update(
-				&server->secmech.sdesccmacaes->shash,
-				iov[i].iov_base + 4, iov[i].iov_len - 4);
-		} else {
-			rc =
-			crypto_shash_update(
-				&server->secmech.sdesccmacaes->shash,
-				iov[i].iov_base, iov[i].iov_len);
-		}
-		if (rc) {
-			cifs_dbg(VFS, "%s: Couldn't update cmac aes with payload\n",
-							__func__);
-			return rc;
-		}
-	}
-
-	/* now hash over the rq_pages array */
-	for (i = 0; i < rqst->rq_npages; i++) {
-		struct kvec p_iov;
-
-		cifs_rqst_page_to_kvec(rqst, i, &p_iov);
-		crypto_shash_update(&server->secmech.sdesccmacaes->shash,
-					p_iov.iov_base, p_iov.iov_len);
-		kunmap(rqst->rq_pages[i]);
-	}
-
-	rc = crypto_shash_final(&server->secmech.sdesccmacaes->shash,
-						sigptr);
-	if (rc)
-		cifs_dbg(VFS, "%s: Could not generate cmac aes\n", __func__);
-
-	memcpy(smb2_pdu->Signature, sigptr, SMB2_SIGNATURE_SIZE);
+	if (!rc)
+		memcpy(smb2_pdu->Signature, sigptr, SMB2_SIGNATURE_SIZE);
Quoted 3 lines


  return rc;
 }

Nice. Long overdue cleanup.

author
#6 lore raw

PATCH 3/6 cifs: quit playing games with draining iovecs

In reply to Al Viro

... and use ITER_BVEC for the page part of request to send

---
 fs/cifs/cifsproto.h |   2 -
 fs/cifs/transport.c | 141 +++++++++++++++-------------------------------------
 2 files changed, 41 insertions(+), 102 deletions(-)
fs/cifs/cifsproto.h +0 -2
index d9b4f44..7d5f53a 100644
@@ -37,8 +37,6 @@ extern void cifs_buf_release(void *);
 extern struct smb_hdr *cifs_small_buf_get(void);
 extern void cifs_small_buf_release(void *);
 extern void free_rsp_buf(int, void *);
-extern void cifs_rqst_page_to_kvec(struct smb_rqst *rqst, unsigned int idx,
-					struct kvec *iov);
 extern int smb_send(struct TCP_Server_Info *, struct smb_hdr *,
 			unsigned int /* length */);
 extern unsigned int _get_xid(void);
fs/cifs/transport.c +41 -100

202 lines not on this page · raw message

Signature · 1 line
2.8.0.rc3
Acked-by on v1 3/6
#7 lore raw

Re: [PATCH 3/6] cifs: quit playing games with draining iovecs

Quoted 258 lines · Al Viro

... and use ITER_BVEC for the page part of request to send

---
 fs/cifs/cifsproto.h |   2 -
 fs/cifs/transport.c | 141 +++++++++++++++---------------------------
----------

 2 files changed, 41 insertions(+), 102 deletions(-)
fs/cifs/cifsproto.h +0 -2
index d9b4f44..7d5f53a 100644
@@ -37,8 +37,6 @@ extern void cifs_buf_release(void *);
  extern struct smb_hdr *cifs_small_buf_get(void);
  extern void cifs_small_buf_release(void *);
  extern void free_rsp_buf(int, void *);
-extern void cifs_rqst_page_to_kvec(struct smb_rqst *rqst, unsigned
 int idx,
-					struct kvec *iov);
  extern int smb_send(struct TCP_Server_Info *, struct smb_hdr *,
  			unsigned int /* length */);

 extern unsigned int _get_xid(void);

fs/cifs/transport.c +1 -2
index 87abe8e..206a597 100644
@@ -124,41 +124,32 @@ cifs_delete_mid(struct mid_q_entry *mid)
  /*
   * smb_send_kvec - send an array of kvecs to the server
   * @server:	Server to send the data to
- * @iov:	Pointer to array of kvecs
- * @n_vec:	length of kvec array
+ * @smb_msg:	Message to send
   * @sent:	amount of data sent on socket is stored here
   *
   * Our basic "send data to server" function. Should be called with

srv_mutex
  * held. The caller is responsible for handling the results. */
 static int
-smb_send_kvec(struct TCP_Server_Info *server, struct kvec *iov, size_t n_vec,

fs/cifs/transport.c +1 -1
-		size_t *sent)
+smb_send_kvec(struct TCP_Server_Info *server, struct msghdr

*smb_msg,

fs/cifs/transport.c +1 -0
+	      size_t *sent)

 {
  int rc = 0;

fs/cifs/transport.c +1 -4
-	int i = 0;
-	struct msghdr smb_msg;
-	unsigned int remaining;
-	size_t first_vec = 0;
+	int retries = 0;

  struct socket *ssocket = server->ssocket;

  *sent = 0;

fs/cifs/transport.c +4 -4
-	smb_msg.msg_name = (struct sockaddr *) &server->dstaddr;
-	smb_msg.msg_namelen = sizeof(struct sockaddr);
-	smb_msg.msg_control = NULL;
-	smb_msg.msg_controllen = 0;
+	smb_msg->msg_name = (struct sockaddr *) &server->dstaddr;
+	smb_msg->msg_namelen = sizeof(struct sockaddr);
+	smb_msg->msg_control = NULL;
+	smb_msg->msg_controllen = 0;

  if (server->noblocksnd)

fs/cifs/transport.c +1 -1
-		smb_msg.msg_flags = MSG_DONTWAIT + MSG_NOSIGNAL;
+		smb_msg->msg_flags = MSG_DONTWAIT + MSG_NOSIGNAL;

  else

fs/cifs/transport.c +1 -5
-		smb_msg.msg_flags = MSG_NOSIGNAL;
-
-	remaining = 0;
-	for (i = 0; i < n_vec; i++)
-		remaining += iov[i].iov_len;
+		smb_msg->msg_flags = MSG_NOSIGNAL;

fs/cifs/transport.c +1 -2
-	i = 0;
-	while (remaining) {
+	while (msg_data_left(smb_msg)) {

  /*
   * If blocking send, we try 3 times, since each can block
   * for 5 seconds. For nonblocking  we have to try more

fs/cifs/transport.c +4 -4
@@ -177,35 +168,21 @@ smb_send_kvec(struct TCP_Server_Info *server,
 struct kvec *iov, size_t n_vec,
  		 * after the retries we will kill the socket and
  		 * reconnect which may clear the network problem.
  		 */
-		rc = kernel_sendmsg(ssocket, &smb_msg,
 &iov[first_vec],
-				    n_vec - first_vec, remaining);
+		rc = sock_sendmsg(ssocket, smb_msg);
  		if (rc == -EAGAIN) {
-			i++;
-			if (i >= 14 || (!server->noblocksnd && (i >
 2))) {
+			retries++;
+			if (retries >= 14 ||
+			    (!server->noblocksnd && (retries > 2)))
 {

  cifs_dbg(VFS, "sends on sock %p stuck for 15 seconds\n",
   ssocket);

fs/cifs/transport.c +1 -2
-				rc = -EAGAIN;
-				break;
+				return -EAGAIN;

  }

fs/cifs/transport.c +1 -1
-			msleep(1 << i);
+			msleep(1 << retries);

  continue;
  }

  if (rc < 0)

fs/cifs/transport.c +0 -12
-			break;
-
-		/* send was at least partially successful */
-		*sent += rc;
-
-		if (rc == remaining) {
-			remaining = 0;
-			break;
-		}
-
-		if (rc > remaining) {
-			cifs_dbg(VFS, "sent %d requested %d\n", rc,

remaining);

fs/cifs/transport.c +1 -2
-			break;
-		}
+			return rc;


  if (rc == 0) {
  /* should never happen, letting socket clear before

fs/cifs/transport.c +0 -0
@@ -215,59 +192,11 @@ smb_send_kvec(struct TCP_Server_Info *server,
 struct kvec *iov, size_t n_vec,
  			continue;
  		}

fs/cifs/transport.c +3 -19
-		remaining -= rc;
-
-		/* the line below resets i */
-		for (i = first_vec; i < n_vec; i++) {
-			if (iov[i].iov_len) {
-				if (rc > iov[i].iov_len) {
-					rc -= iov[i].iov_len;
-					iov[i].iov_len = 0;
-				} else {
-					iov[i].iov_base += rc;
-					iov[i].iov_len -= rc;
-					first_vec = i;
-					break;
-				}
-			}
-		}
-
-		i = 0; /* in case we get ENOSPC on the next send */
-		rc = 0;
+		/* send was at least partially successful */
+		*sent += rc;
+		retries = 0; /* in case we get ENOSPC on the next

send */
  }

fs/cifs/transport.c +0 -5
-	return rc;
-}
-
-/**
- * rqst_page_to_kvec - Turn a slot in the smb_rqst page array into a

kvec
- * @rqst: pointer to smb_rqst
- * @idx: index into the array of the page
- * @iov: pointer to struct kvec that will hold the result
- *
- * Helper function to convert a slot in the rqst->rq_pages array into a kvec.
- * The page will be kmapped and the address placed into iov_base. The length
- * will then be adjusted according to the ptailoff.
- */

fs/cifs/transport.c +0 -8
-void
-cifs_rqst_page_to_kvec(struct smb_rqst *rqst, unsigned int idx,
-			struct kvec *iov)
-{
-	/*
-	 * FIXME: We could avoid this kmap altogether if we used
-	 * kernel_sendpage instead of kernel_sendmsg. That will only
-	 * work if signing is disabled though as sendpage inlines

the

fs/cifs/transport.c +0 -1
-	 * page directly into the fraglist. If userspace modifies

the

fs/cifs/transport.c +0 -1
-	 * page after we calculate the signature, then the server

will

fs/cifs/transport.c +0 -1
-	 * reject it and may break the connection. kernel_sendmsg

does

fs/cifs/transport.c +1 -9
-	 * an extra copy of the data and avoids that issue.
-	 */
-	iov->iov_base = kmap(rqst->rq_pages[idx]);
-
-	/* if last page, don't send beyond this offset into page */
-	if (idx == (rqst->rq_npages - 1))
-		iov->iov_len = rqst->rq_tailsz;
-	else
-		iov->iov_len = rqst->rq_pagesz;
+	return 0;

 }

 static unsigned long

fs/cifs/transport.c +0 -0
@@ -299,8 +228,9 @@ smb_send_rqst(struct TCP_Server_Info *server,
 struct smb_rqst *rqst)

  unsigned int smb_buf_length =
get_rfc1002_length(iov[0].iov_base);
  unsigned long send_length;
  unsigned int i;

fs/cifs/transport.c +1 -1
-	size_t total_len = 0, sent;
+	size_t total_len = 0, sent, size;

  struct socket *ssocket = server->ssocket;

fs/cifs/transport.c +1 -0
+	struct msghdr smb_msg;

  int val = 1;

  if (ssocket == NULL)

fs/cifs/transport.c +0 -0
@@ -321,7 +251,13 @@ smb_send_rqst(struct TCP_Server_Info *server,
 struct smb_rqst *rqst)
  	kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
  				(char *)&val, sizeof(val));

fs/cifs/transport.c +5 -1
-	rc = smb_send_kvec(server, iov, n_vec, &sent);
+	size = 0;
+	for (i = 0; i < n_vec; i++)
+		size += iov[i].iov_len;
+
+	iov_iter_kvec(&smb_msg.msg_iter, WRITE | ITER_KVEC, iov,

n_vec, size);

fs/cifs/transport.c +2 -0
+
+	rc = smb_send_kvec(server, &smb_msg, &sent);

  if (rc < 0)
  goto uncork;

fs/cifs/transport.c +0 -0
@@ -329,11 +265,16 @@ smb_send_rqst(struct TCP_Server_Info *server,
 struct smb_rqst *rqst)


  /* now walk the page array and send each page in it */ for (i = 0; i < rqst->rq_npages; i++) {

fs/cifs/transport.c +10 -5
-		struct kvec p_iov;
-
-		cifs_rqst_page_to_kvec(rqst, i, &p_iov);
-		rc = smb_send_kvec(server, &p_iov, 1, &sent);
-		kunmap(rqst->rq_pages[i]);
+		size_t len = i == rqst->rq_npages - 1
+				? rqst->rq_tailsz
+				: rqst->rq_pagesz;
+		struct bio_vec bvec = {
+			.bv_page = rqst->rq_pages[i],
+			.bv_len = len
+		};
+		iov_iter_bvec(&smb_msg.msg_iter, WRITE | ITER_BVEC,
+			      &bvec, 1, len);
+		rc = smb_send_kvec(server, &smb_msg, &sent);
Quoted 3 lines

  if (rc < 0)
  break;

What's the advantage of using iov_iter_bvec over iov_iter_kvec ?

That said...

author
#8 lore raw

Re: [PATCH 3/6] cifs: quit playing games with draining iovecs

Quoted 1 line · Jeff Layton

What's the advantage of using iov_iter_bvec over iov_iter_kvec ?

No need to screw with kmap() in the caller; moreover, when it comes to actual copying it gets away with kmap_atomic() just around the memcpy(), which is considerably cheaper.

author
#9 lore raw

PATCH 4/6 cifs: no need to wank with copying and advancing iovec on recvmsg side either

In reply to Al Viro
---
 fs/cifs/cifsglob.h |  2 --
 fs/cifs/connect.c  | 72 ++++--------------------------------------------------
 2 files changed, 5 insertions(+), 69 deletions(-)
fs/cifs/cifsglob.h +0 -2
index d21da9f..df03c5e 100644
@@ -615,8 +615,6 @@ struct TCP_Server_Info {
 	bool	sec_mskerberos;		/* supports legacy MS Kerberos */
 	bool	large_buf;		/* is current buffer large? */
 	struct delayed_work	echo; /* echo ping workqueue job */
-	struct kvec *iov;	/* reusable kvec array for receives */
-	unsigned int nr_iov;	/* number of kvecs in array */
 	char	*smallbuf;	/* pointer to current "small" buffer */
 	char	*bigbuf;	/* pointer to current "big" buffer */
 	unsigned int total_read; /* total amount of data read in this pass */
fs/cifs/connect.c +5 -67

107 lines not on this page · raw message

Signature · 1 line
2.8.0.rc3
Reviewed-by on v1 4/6
#10 lore raw

Re: [PATCH 4/6] cifs: no need to wank with copying and advancing iovec on recvmsg side either

Quoted 131 lines · Al Viro

---
 fs/cifs/cifsglob.h |  2 --
 fs/cifs/connect.c  | 72 ++++--------------------------------------------------

 2 files changed, 5 insertions(+), 69 deletions(-)
fs/cifs/cifsglob.h +0 -0
index d21da9f..df03c5e 100644
@@ -615,8 +615,6 @@ struct TCP_Server_Info {

  bool sec_mskerberos; /* supports legacy MS Kerberos */
  bool large_buf; /* is current buffer large? */
  struct delayed_work echo; /* echo ping workqueue job */

fs/cifs/cifsglob.h +0 -2
-	struct kvec *iov;	/* reusable kvec array for receives */
-	unsigned int nr_iov;	/* number of kvecs in array */

  char *smallbuf; /* pointer to current "small" buffer */
  char *bigbuf; /* pointer to current "big" buffer */
  unsigned int total_read; /* total amount of data read in this pass */

fs/cifs/connect.c +0 -0
index a763cd3..eb42665 100644
@@ -501,77 +501,20 @@ server_unresponsive(struct TCP_Server_Info *server)
  	return false;
  }

fs/cifs/connect.c +0 -53
-/*
- * kvec_array_init - clone a kvec array, and advance into it
- * @new:	pointer to memory for cloned array
- * @iov:	pointer to original array
- * @nr_segs:	number of members in original array
- * @bytes:	number of bytes to advance into the cloned array
- *
- * This function will copy the array provided in iov to a section of memory
- * and advance the specified number of bytes into the new array. It returns
- * the number of segments in the new array. "new" must be at least as big as
- * the original iov array.
- */
-static unsigned int
-kvec_array_init(struct kvec *new, struct kvec *iov, unsigned int nr_segs,
-		size_t bytes)
-{
-	size_t base = 0;
-
-	while (bytes || !iov->iov_len) {
-		int copy = min(bytes, iov->iov_len);
-
-		bytes -= copy;
-		base += copy;
-		if (iov->iov_len == base) {
-			iov++;
-			nr_segs--;
-			base = 0;
-		}
-	}
-	memcpy(new, iov, sizeof(*iov) * nr_segs);
-	new->iov_base += base;
-	new->iov_len -= base;
-	return nr_segs;
-}
-
-static struct kvec *
-get_server_iovec(struct TCP_Server_Info *server, unsigned int nr_segs)
-{
-	struct kvec *new_iov;
-
-	if (server->iov && nr_segs <= server->nr_iov)
-		return server->iov;
-
-	/* not big enough -- allocate a new one and release the old */
-	new_iov = kmalloc(sizeof(*new_iov) * nr_segs, GFP_NOFS);
-	if (new_iov) {
-		kfree(server->iov);
-		server->iov = new_iov;
-		server->nr_iov = nr_segs;
-	}
-	return new_iov;
-}
-

 int
 cifs_readv_from_socket(struct TCP_Server_Info *server, struct kvec *iov_orig, unsigned int nr_segs, unsigned int to_read) {
  int length = 0;
  int total_read;

fs/cifs/connect.c +0 -1
-	unsigned int segs;

  struct msghdr smb_msg;

fs/cifs/connect.c +0 -5
-	struct kvec *iov;
-
-	iov = get_server_iovec(server, nr_segs);
-	if (!iov)
-		return -ENOMEM;


  smb_msg.msg_control = NULL;
  smb_msg.msg_controllen = 0;

fs/cifs/connect.c +2 -0
+	iov_iter_kvec(&smb_msg.msg_iter, READ | ITER_KVEC,
+		      iov_orig, nr_segs, to_read);

fs/cifs/connect.c +1 -1
-	for (total_read = 0; to_read; total_read += length, to_read -= length) {
+	for (total_read = 0; msg_data_left(&smb_msg); total_read += length) {

  try_to_freeze();

  if (server_unresponsive(server)) {

fs/cifs/connect.c +0 -0
@@ -579,10 +522,7 @@ cifs_readv_from_socket(struct TCP_Server_Info *server, struct kvec *iov_orig,
  			break;
  		}

fs/cifs/connect.c +1 -4
-		segs = kvec_array_init(iov, iov_orig, nr_segs, total_read);
-
-		length = kernel_recvmsg(server->ssocket, &smb_msg,
-					iov, segs, to_read, 0);
+		length = sock_recvmsg(server->ssocket, &smb_msg, 0);


  if (server->tcpStatus == CifsExiting) { total_read = -ESHUTDOWN;

fs/cifs/connect.c +1 -2
@@ -603,8 +543,7 @@ cifs_readv_from_socket(struct TCP_Server_Info *server, struct kvec *iov_orig,
  			length = 0;
  			continue;
  		} else if (length <= 0) {
-			cifs_dbg(FYI, "Received no data or error: expecting %d\n"
-				 "got %d", to_read, length);
+			cifs_dbg(FYI, "Received no data or error: %d\n", length);
  			cifs_reconnect(server);
  			total_read = -ECONNABORTED;
  			break;
@@ -783,7 +722,6 @@ static void clean_demultiplex_info(struct TCP_Server_Info *server)
  	}


  kfree(server->hostname);

fs/cifs/connect.c +0 -1
-	kfree(server->iov);
Quoted 3 lines

  kfree(server);

  length = atomic_dec_return(&tcpSesAllocCount);

Good riddance.

author
#11 lore raw

PATCH 5/6 cifs_readv_receive: use cifs_read_from_socket()

In reply to Al Viro
---
 fs/cifs/cifssmb.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
fs/cifs/cifssmb.c +4 -7
index 76fcb50..3da077a 100644
@@ -1447,10 +1447,8 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
 	len = min_t(unsigned int, buflen, server->vals->read_rsp_size) -
 							HEADER_SIZE(server) + 1;
 
-	rdata->iov.iov_base = buf + HEADER_SIZE(server) - 1;
-	rdata->iov.iov_len = len;
-
-	length = cifs_readv_from_socket(server, &rdata->iov, 1, len);
+	length = cifs_read_from_socket(server,
+				       buf + HEADER_SIZE(server) - 1, len);
 	if (length < 0)
 		return length;
 	server->total_read += length;
@@ -1502,9 +1500,8 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
 	len = data_offset - server->total_read;
 	if (len > 0) {
 		/* read any junk before data into the rest of smallbuf */
-		rdata->iov.iov_base = buf + server->total_read;
-		rdata->iov.iov_len = len;
-		length = cifs_readv_from_socket(server, &rdata->iov, 1, len);
+		length = cifs_read_from_socket(server,
+					       buf + server->total_read, len);
 		if (length < 0)
 			return length;
 		server->total_read += length;
Signature · 1 line
2.8.0.rc3
Reviewed-by on v1 5/6
#12 lore raw

Re: [PATCH 5/6] cifs_readv_receive: use cifs_read_from_socket()

Quoted 24 lines · Al Viro

---
 fs/cifs/cifssmb.c | 11 ++++-------

 1 file changed, 4 insertions(+), 7 deletions(-)
fs/cifs/cifssmb.c +0 -0
index 76fcb50..3da077a 100644
@@ -1447,10 +1447,8 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)

  len = min_t(unsigned int, buflen, server->vals->read_rsp_size) - HEADER_SIZE(server) + 1;

fs/cifs/cifssmb.c +2 -4
-	rdata->iov.iov_base = buf + HEADER_SIZE(server) - 1;
-	rdata->iov.iov_len = len;
-
-	length = cifs_readv_from_socket(server, &rdata->iov, 1, len);
+	length = cifs_read_from_socket(server,
+				       buf + HEADER_SIZE(server) - 1, len);

  if (length < 0)
  return length;
  server->total_read += length;

fs/cifs/cifssmb.c +0 -0
@@ -1502,9 +1500,8 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
  	len = data_offset - server->total_read;

fs/cifs/cifssmb.c cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid) · line 1507

Quoted 10 lines
fs/cifs/cifssmb.c +2 -3
  	if (len > 0) {
  		/* read any junk before data into the rest of smallbuf */
-		rdata->iov.iov_base = buf + server->total_read;
-		rdata->iov.iov_len = len;
-		length = cifs_readv_from_socket(server, &rdata->iov, 1, len);
+		length = cifs_read_from_socket(server,
+					       buf + server->total_read, len);
  		if (length < 0)
  			return length;
  		server->total_read += length;

This one is orthogonal to the rest. Looks good though.

author
#13 lore raw

PATCH 6/6 cifs: don't bother with kmap on read_pages side

In reply to Al Viro

just do ITER_BVEC recvmsg

---
 fs/cifs/cifsproto.h |  7 +++---
 fs/cifs/connect.c   | 65 ++++++++++++++++++++++++++++-------------------------
 fs/cifs/file.c      | 53 ++++++++++++++-----------------------------
 3 files changed, 55 insertions(+), 70 deletions(-)
fs/cifs/cifsproto.h +3 -4
index 7d5f53a..0f9a6bc 100644
@@ -179,10 +179,9 @@ extern int set_cifs_acl(struct cifs_ntsd *, __u32, struct inode *,
 
 extern void dequeue_mid(struct mid_q_entry *mid, bool malformed);
 extern int cifs_read_from_socket(struct TCP_Server_Info *server, char *buf,
-		     unsigned int to_read);
-extern int cifs_readv_from_socket(struct TCP_Server_Info *server,
-		struct kvec *iov_orig, unsigned int nr_segs,
-		unsigned int to_read);
+			         unsigned int to_read);
+extern int cifs_read_page_from_socket(struct TCP_Server_Info *server,
+				      struct page *page, unsigned int to_read);
 extern void cifs_setup_cifs_sb(struct smb_vol *pvolume_info,
 			       struct cifs_sb_info *cifs_sb);
 extern int cifs_match_super(struct super_block *, void *);
fs/cifs/connect.c +35 -30

97 lines not on this page · raw message

fs/cifs/file.c +17 -36

94 lines not on this page · raw message

Signature · 1 line
2.8.0.rc3
Reviewed-by on v1 6/6
#14 lore raw

Re: [PATCH 6/6] cifs: don't bother with kmap on read_pages side

Quoted 230 lines · Al Viro

just do ITER_BVEC recvmsg

---
 fs/cifs/cifsproto.h |  7 +++---
 fs/cifs/connect.c   | 65 ++++++++++++++++++++++++++++------------------------- fs/cifs/file.c      | 53 ++++++++++++++-----------------------------

 3 files changed, 55 insertions(+), 70 deletions(-)
fs/cifs/cifsproto.h +0 -0
index 7d5f53a..0f9a6bc 100644
@@ -179,10 +179,9 @@ extern int set_cifs_acl(struct cifs_ntsd *, __u32, struct inode *,


 extern void dequeue_mid(struct mid_q_entry *mid, bool malformed); extern int cifs_read_from_socket(struct TCP_Server_Info *server, char *buf,

fs/cifs/cifsproto.h +3 -4
-		     unsigned int to_read);
-extern int cifs_readv_from_socket(struct TCP_Server_Info *server,
-		struct kvec *iov_orig, unsigned int nr_segs,
-		unsigned int to_read);
+			         unsigned int to_read);
+extern int cifs_read_page_from_socket(struct TCP_Server_Info *server,
+				      struct page *page, unsigned int to_read);

 extern void cifs_setup_cifs_sb(struct smb_vol *pvolume_info, struct cifs_sb_info *cifs_sb); extern int cifs_match_super(struct super_block *, void *);

fs/cifs/connect.c +0 -0
index eb42665..e33c5e0 100644
@@ -501,39 +501,34 @@ server_unresponsive(struct TCP_Server_Info *server)
  	return false;
  }

fs/cifs/connect.c +2 -3
-int
-cifs_readv_from_socket(struct TCP_Server_Info *server, struct kvec *iov_orig,
-		       unsigned int nr_segs, unsigned int to_read)
+static int
+cifs_readv_from_socket(struct TCP_Server_Info *server, struct msghdr *smb_msg)

 {
  int length = 0;
  int total_read;

fs/cifs/connect.c +0 -1
-	struct msghdr smb_msg;

fs/cifs/connect.c +2 -4
-	smb_msg.msg_control = NULL;
-	smb_msg.msg_controllen = 0;
-	iov_iter_kvec(&smb_msg.msg_iter, READ | ITER_KVEC,
-		      iov_orig, nr_segs, to_read);
+	smb_msg->msg_control = NULL;
+	smb_msg->msg_controllen = 0;

fs/cifs/connect.c +1 -1
-	for (total_read = 0; msg_data_left(&smb_msg); total_read += length) {
+	for (total_read = 0; msg_data_left(smb_msg); total_read += length) {

  try_to_freeze();

fs/cifs/connect.c +2 -4
-		if (server_unresponsive(server)) {
-			total_read = -ECONNABORTED;
-			break;
-		}
+		if (server_unresponsive(server))
+			return -ECONNABORTED;

fs/cifs/connect.c +1 -1
-		length = sock_recvmsg(server->ssocket, &smb_msg, 0);
+		length = sock_recvmsg(server->ssocket, smb_msg, 0);

fs/cifs/connect.c +4 -4
-		if (server->tcpStatus == CifsExiting) {
-			total_read = -ESHUTDOWN;
-			break;
-		} else if (server->tcpStatus == CifsNeedReconnect) {
+		if (server->tcpStatus == CifsExiting)
+			return -ESHUTDOWN;
+
+		if (server->tcpStatus == CifsNeedReconnect) {

  cifs_reconnect(server);

fs/cifs/connect.c +6 -5
-			total_read = -ECONNABORTED;
-			break;
-		} else if (length == -ERESTARTSYS ||
-			   length == -EAGAIN ||
-			   length == -EINTR) {
+			return -ECONNABORTED;
+		}
+
+		if (length == -ERESTARTSYS ||
+		    length == -EAGAIN ||
+		    length == -EINTR) {

  /*
   * Minimum sleep to prevent looping, allowing socket
   * to clear and app threads to set tcpStatus

fs/cifs/connect.c +7 -4
@@ -542,11 +537,12 @@ cifs_readv_from_socket(struct TCP_Server_Info *server, struct kvec *iov_orig,
  			usleep_range(1000, 2000);
  			length = 0;
  			continue;
-		} else if (length <= 0) {
+		}
+
+		if (length <= 0) {
  			cifs_dbg(FYI, "Received no data or error: %d\n", length);
  			cifs_reconnect(server);
-			total_read = -ECONNABORTED;
-			break;
+			return -ECONNABORTED;
  		}
  	}
  	return total_read;
@@ -556,12 +552,21 @@ int
  cifs_read_from_socket(struct TCP_Server_Info *server, char *buf,
  		      unsigned int to_read)
  {
-	struct kvec iov;
+	struct msghdr smb_msg;
+	struct kvec iov = {.iov_base = buf, .iov_len = to_read};
+	iov_iter_kvec(&smb_msg.msg_iter, READ | ITER_KVEC, &iov, 1, to_read);

fs/cifs/connect.c +2 -2
-	iov.iov_base = buf;
-	iov.iov_len = to_read;
+	return cifs_readv_from_socket(server, &smb_msg);
+}

fs/cifs/connect.c +8 -1
-	return cifs_readv_from_socket(server, &iov, 1, to_read);
+int
+cifs_read_page_from_socket(struct TCP_Server_Info *server, struct page *page,
+		      unsigned int to_read)
+{
+	struct msghdr smb_msg;
+	struct bio_vec bv = {.bv_page = page, .bv_len = to_read};
+	iov_iter_bvec(&smb_msg.msg_iter, READ | ITER_BVEC, &bv, 1, to_read);
+	return cifs_readv_from_socket(server, &smb_msg);

 }

 static bool

fs/cifs/file.c +0 -1
index ff882ae..0f71867 100644
@@ -2855,39 +2855,31 @@ cifs_uncached_read_into_pages(struct TCP_Server_Info *server,
  	int result = 0;
  	unsigned int i;
  	unsigned int nr_pages = rdata->nr_pages;
-	struct kvec iov;


  rdata->got_bytes = 0;
  rdata->tailsz = PAGE_SIZE;
  for (i = 0; i < nr_pages; i++) { struct page *page = rdata->pages[i];

fs/cifs/file.c +1 -0
+		size_t n;

fs/cifs/file.c +1 -17
-		if (len >= PAGE_SIZE) {
-			/* enough data to fill the page */
-			iov.iov_base = kmap(page);
-			iov.iov_len = PAGE_SIZE;
-			cifs_dbg(FYI, "%u: iov_base=%p iov_len=%zu\n",
-				 i, iov.iov_base, iov.iov_len);
-			len -= PAGE_SIZE;
-		} else if (len > 0) {
-			/* enough for partial page, fill and zero the rest */
-			iov.iov_base = kmap(page);
-			iov.iov_len = len;
-			cifs_dbg(FYI, "%u: iov_base=%p iov_len=%zu\n",
-				 i, iov.iov_base, iov.iov_len);
-			memset(iov.iov_base + len, '\0', PAGE_SIZE - len);
-			rdata->tailsz = len;
-			len = 0;
-		} else {
+		if (len <= 0) {

  /* no need to hold page hostage */ rdata->pages[i] = NULL; rdata->nr_pages--; put_page(page);
  continue;
  }

fs/cifs/file.c +11 -3
-
-		result = cifs_readv_from_socket(server, &iov, 1, iov.iov_len);
-		kunmap(page);
+		n = len;
+		if (len >= PAGE_SIZE) {
+			/* enough data to fill the page */
+			n = PAGE_SIZE;
+			len -= n;
+		} else {
+			zero_user(page, len, PAGE_SIZE - len);
+			rdata->tailsz = len;
+			len = 0;
+		}
+		result = cifs_read_page_from_socket(server, page, n);

  if (result < 0)
  break;

fs/cifs/file.c +0 -1
@@ -3303,7 +3295,6 @@ cifs_readpages_read_into_pages(struct TCP_Server_Info *server,
  	u64 eof;
  	pgoff_t eof_index;
  	unsigned int nr_pages = rdata->nr_pages;
-	struct kvec iov;


  /* determine the eof that the server (probably) has */ eof = CIFS_I(rdata->mapping->host)->server_eof;

fs/cifs/file.c +1 -0
@@ -3314,23 +3305,14 @@ cifs_readpages_read_into_pages(struct TCP_Server_Info *server,
  	rdata->tailsz = PAGE_CACHE_SIZE;
  	for (i = 0; i < nr_pages; i++) {
  		struct page *page = rdata->pages[i];
+		size_t n = PAGE_CACHE_SIZE;


  if (len >= PAGE_CACHE_SIZE) {

fs/cifs/file.c +0 -5
-			/* enough data to fill the page */
-			iov.iov_base = kmap(page);
-			iov.iov_len = PAGE_CACHE_SIZE;
-			cifs_dbg(FYI, "%u: idx=%lu iov_base=%p iov_len=%zu\n",
-				 i, page->index, iov.iov_base, iov.iov_len);

  len -= PAGE_CACHE_SIZE;
  } else if (len > 0) {
  /* enough for partial page, fill and zero the rest */

fs/cifs/file.c +2 -7
-			iov.iov_base = kmap(page);
-			iov.iov_len = len;
-			cifs_dbg(FYI, "%u: idx=%lu iov_base=%p iov_len=%zu\n",
-				 i, page->index, iov.iov_base, iov.iov_len);
-			memset(iov.iov_base + len,
-				'\0', PAGE_CACHE_SIZE - len);
-			rdata->tailsz = len;
+			zero_user(page, len, PAGE_CACHE_SIZE - len);
+			n = rdata->tailsz = len;

  len = 0;
  } else if (page->index > eof_index) { /*

fs/cifs/file.c +0 -0
@@ -3360,8 +3342,7 @@ cifs_readpages_read_into_pages(struct TCP_Server_Info *server,
  			continue;
  		}

fs/cifs/file.c +1 -2
-		result = cifs_readv_from_socket(server, &iov, 1, iov.iov_len);
-		kunmap(page);
+		result = cifs_read_page_from_socket(server, page, n);
Quoted 3 lines

  if (result < 0)
  break;

#15 lore raw
In reply to Al Viro

Looking at the series. I briefly looked at 2/6 and looks correct but would add tested-by once I test that patch
against a SMB server.

Quoted 41 lines · Al Viro

Now that sendmsg/recvmsg do not mangle iovecs and are capable of
handling bvec-based ->msg_iter, we can seriously reduce the amount of PITA in cifs. The series below is completely untested, and I would appreciate comments/review/testing/etc.

I'll post the individual patches in followups; for those who prefer to use git it can be found in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git sendmsg.cifs

1/6: [net] drop 'size' argument of sock_recvmsg()
should go via net-next; does what it says.

2/6: cifs: merge the hash calculation helpers
takes the common parts of {cifs,smb2,smb3}_calc_signature() into a
common helper.

3/6: cifs: quit playing games with draining iovecs
Switch smb_send_kvec() to passing msghdr (and thus iov_iter) and
make it use sock_sendmsg() - that allows to avoid draining iovecs, since ->msg_iter will be advanced properly and all we need is to keep it around between the calls of sock_sendmsg(), rather than reinitializing it on each loop iteration. The same thing allows to get rid of messing with kmap() when sending the stuff in ->rq_pages[] - ITER_BVEC will do the right thing.

4/6: cifs: no need to wank with copying and advancing iovec on recvmsg side either
Similar to the previous - use sock_recvmsg() in cifs_readv_from_socket()
and there's no need to modify iovecs, or allocate a copy especially for such modifications, etc.

5/6: cifs_readv_receive: use cifs_read_from_socket()
building a 1-element iovec array for cifs_readv_from_socket() is
an overkill - simple cifs_read_from_socket() will do just fine.

6/6: cifs: don't bother with kmap on read_pages side
Similar to the other half of 3/6: we can use ITER_BVEC for
read-into-page case. Just make cifs_readv_from_socket() take msghdr from caller and use a helper that would feed it a bvec-backed ->msg_iter. --
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Reviewed-by on v1 1/1
#16 lore raw
In reply to Al Viro

Let me know if you want any of them to go in via the cifs tree or prefer going in through your tree (other than patch 1 which could go in the net-next tree are you indicated)

Quoted 41 lines · Al Viro

Now that sendmsg/recvmsg do not mangle iovecs and are capable of
handling bvec-based ->msg_iter, we can seriously reduce the amount of PITA in cifs. The series below is completely untested, and I would appreciate comments/review/testing/etc.

I'll post the individual patches in followups; for those who prefer to use git it can be found in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git sendmsg.cifs

1/6: [net] drop 'size' argument of sock_recvmsg()
should go via net-next; does what it says.

2/6: cifs: merge the hash calculation helpers
takes the common parts of {cifs,smb2,smb3}_calc_signature() into a
common helper.

3/6: cifs: quit playing games with draining iovecs
Switch smb_send_kvec() to passing msghdr (and thus iov_iter) and
make it use sock_sendmsg() - that allows to avoid draining iovecs, since ->msg_iter will be advanced properly and all we need is to keep it around between the calls of sock_sendmsg(), rather than reinitializing it on each loop iteration. The same thing allows to get rid of messing with kmap() when sending the stuff in ->rq_pages[] - ITER_BVEC will do the right thing.

4/6: cifs: no need to wank with copying and advancing iovec on recvmsg side either
Similar to the previous - use sock_recvmsg() in cifs_readv_from_socket()
and there's no need to modify iovecs, or allocate a copy especially for such modifications, etc.

5/6: cifs_readv_receive: use cifs_read_from_socket()
building a 1-element iovec array for cifs_readv_from_socket() is
an overkill - simple cifs_read_from_socket() will do just fine.

6/6: cifs: don't bother with kmap on read_pages side
Similar to the other half of 3/6: we can use ITER_BVEC for
read-into-page case. Just make cifs_readv_from_socket() take msghdr from caller and use a helper that would feed it a bvec-backed ->msg_iter. --
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines
Thanks,

Steve