[PATCH v2 0/9] cifs: convert async write code to use less kmapping

[PATCH v2 0/9] cifs: convert async write code to use less kmapping

[Jeff Layton]

This is the second version of this patch series. The main differences
from the first are:

1) move the struct smb_rqst definition from cifsproto.h to cifsglob.h

2) fix up merge conflicts with Steve's for-next tree and drop patches
   that are already there

3) convert the setup_async_request prototype to return a mid_q_entry
   pointer or an ERR_PTR instead of requiring a double pointer argument

Original cover letter follows...

We currently have a problem with the async write code on CONFIG_HIGHMEM
architectures. It uses *way* too much kmap space. Not only does this
limit throughput by adding an artificial bottleneck, but it also can
lead to deadlocks.

Late in the 3.5 cycle, I sent some patches to limit the rsize and wsize
on these arches as a temporary workaround. This patchset is a first pass
at fixing this the right way, by teaching the low-level transport code
how to deal with arrays of pages.

Note that this is just a fix for the write codepaths. The read side will
need a separate set of patches.

The code seems to work correctly, and is probably suitable for the
upcoming 3.6 merge window. This probably also has some non-trivial
conflicts with Pavel's patches since I did not base this on top of his
SMB2 work.

Let me know what you plan to merge first for 3.6 and I can try to
fix those up, or we can ask Pavel to rebase his code on top of this.

Jeff Layton (9):
  cifs: change signing routines to deal with smb_rqst structs
  cifs: convert send code to use smb_rqst structs
  cifs: cork the socket before a send and uncork it afterward
  cifs: teach smb_send_rqst how to handle arrays of pages
  cifs: teach signing routines how to deal with arrays of pages in a
    smb_rqst
  cifs: change cifs_call_async to use smb_rqst structs
  cifs: convert async write code to pass in data via rq_pages array
  cifs: remove the kmap size limit from wsize
  cifs: add deprecation warning to sockopt=TCP_NODELAY option

 fs/cifs/cifsencrypt.c   |  37 ++++++--
 fs/cifs/cifsglob.h      |  18 +++-
 fs/cifs/cifsproto.h     |  25 +++---
 fs/cifs/cifssmb.c       |  59 ++++++------
 fs/cifs/connect.c       |  10 ++-
 fs/cifs/file.c          |  47 ++--------
 fs/cifs/smb2proto.h     |   4 +-
 fs/cifs/smb2transport.c |  14 ++-
 fs/cifs/transport.c     | 233 +++++++++++++++++++++++++++++++++++-------------
 9 files changed, 278 insertions(+), 169 deletions(-)

-- 
1.7.11.2

[PATCH v2 1/9] cifs: change signing routines to deal with smb_rqst structs

[Jeff Layton]

We need a way to represent a call to be sent on the wire that does not
require having all of the page data kmapped. Behold the smb_rqst struct.
This new struct represents an array of kvecs immediately followed by an
array of pages.

Convert the signing routines to use these structs under the hood and
turn the existing functions for this into wrappers around that. For now,
we're just changing these functions to take different args. Later, we'll
teach them how to deal with arrays of pages.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsencrypt.c | 26 ++++++++++++++++++--------
 fs/cifs/cifsglob.h    | 14 ++++++++++++++
 fs/cifs/cifsproto.h   |  7 +++++--
 fs/cifs/cifssmb.c     |  6 ++++--
 fs/cifs/transport.c   |  4 +++-
 5 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 6a0d741..e8953a0 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -37,11 +37,13 @@
  * 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(const struct kvec *iov, int n_vec,
+static int cifs_calc_signature(struct smb_rqst *rqst,
 			struct TCP_Server_Info *server, char *signature)
 {
 	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;
@@ -99,12 +101,12 @@ static int cifs_calc_signature(const struct kvec *iov, int n_vec,
 }
 
 /* must be called with server->srv_mutex held */
-int cifs_sign_smbv(struct kvec *iov, int n_vec, struct TCP_Server_Info *server,
+int cifs_sign_rqst(struct smb_rqst *rqst, struct TCP_Server_Info *server,
 		   __u32 *pexpected_response_sequence_number)
 {
 	int rc = 0;
 	char smb_signature[20];
-	struct smb_hdr *cifs_pdu = (struct smb_hdr *)iov[0].iov_base;
+	struct smb_hdr *cifs_pdu = (struct smb_hdr *)rqst->rq_iov[0].iov_base;
 
 	if ((cifs_pdu == NULL) || (server == NULL))
 		return -EINVAL;
@@ -125,7 +127,7 @@ int cifs_sign_smbv(struct kvec *iov, int n_vec, struct TCP_Server_Info *server,
 	*pexpected_response_sequence_number = server->sequence_number++;
 	server->sequence_number++;
 
-	rc = cifs_calc_signature(iov, n_vec, server, smb_signature);
+	rc = cifs_calc_signature(rqst, server, smb_signature);
 	if (rc)
 		memset(cifs_pdu->Signature.SecuritySignature, 0, 8);
 	else
@@ -134,6 +136,15 @@ int cifs_sign_smbv(struct kvec *iov, int n_vec, struct TCP_Server_Info *server,
 	return rc;
 }
 
+int cifs_sign_smbv(struct kvec *iov, int n_vec, struct TCP_Server_Info *server,
+		   __u32 *pexpected_response_sequence)
+{
+	struct smb_rqst rqst = { .rq_iov = iov,
+				 .rq_nvec = n_vec };
+
+	return cifs_sign_rqst(&rqst, server, pexpected_response_sequence);
+}
+
 /* must be called with server->srv_mutex held */
 int cifs_sign_smb(struct smb_hdr *cifs_pdu, struct TCP_Server_Info *server,
 		  __u32 *pexpected_response_sequence_number)
@@ -147,14 +158,14 @@ int cifs_sign_smb(struct smb_hdr *cifs_pdu, struct TCP_Server_Info *server,
 			      pexpected_response_sequence_number);
 }
 
-int cifs_verify_signature(struct kvec *iov, unsigned int nr_iov,
+int cifs_verify_signature(struct smb_rqst *rqst,
 			  struct TCP_Server_Info *server,
 			  __u32 expected_sequence_number)
 {
 	unsigned int rc;
 	char server_response_sig[8];
 	char what_we_think_sig_should_be[20];
-	struct smb_hdr *cifs_pdu = (struct smb_hdr *)iov[0].iov_base;
+	struct smb_hdr *cifs_pdu = (struct smb_hdr *)rqst->rq_iov[0].iov_base;
 
 	if (cifs_pdu == NULL || server == NULL)
 		return -EINVAL;
@@ -186,8 +197,7 @@ int cifs_verify_signature(struct kvec *iov, unsigned int nr_iov,
 	cifs_pdu->Signature.Sequence.Reserved = 0;
 
 	mutex_lock(&server->srv_mutex);
-	rc = cifs_calc_signature(iov, nr_iov, server,
-				 what_we_think_sig_should_be);
+	rc = cifs_calc_signature(rqst, server, what_we_think_sig_should_be);
 	mutex_unlock(&server->srv_mutex);
 
 	if (rc)
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index bcdf4d4..2a7e4b8 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -158,6 +158,20 @@ struct cifs_cred {
  *****************************************************************
  */
 
+/*
+ * A smb_rqst represents a complete request to be issued to a server. It's
+ * formed by a kvec array, followed by an array of pages. Page data is assumed
+ * to start at the beginning of the first page.
+ */
+struct smb_rqst {
+	struct kvec	*rq_iov;	/* array of kvecs */
+	unsigned int	rq_nvec;	/* number of kvecs in array */
+	struct page	**rq_pages;	/* pointer to array of page ptrs */
+	unsigned int	rq_npages;	/* number pages in array */
+	unsigned int	rq_pagesz;	/* page size to use */
+	unsigned int	rq_tailsz;	/* length of last page */
+};
+
 enum smb_version {
 	Smb_1 = 1,
 	Smb_21,
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index cf7fb18..d02a6a4 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -24,6 +24,7 @@
 
 struct statfs;
 struct smb_vol;
+struct smb_rqst;
 
 /*
  *****************************************************************
@@ -400,10 +401,12 @@ extern void sesInfoFree(struct cifs_ses *);
 extern struct cifs_tcon *tconInfoAlloc(void);
 extern void tconInfoFree(struct cifs_tcon *);
 
-extern int cifs_sign_smb(struct smb_hdr *, struct TCP_Server_Info *, __u32 *);
+extern int cifs_sign_rqst(struct smb_rqst *rqst, struct TCP_Server_Info *server,
+		   __u32 *pexpected_response_sequence_number);
 extern int cifs_sign_smbv(struct kvec *iov, int n_vec, struct TCP_Server_Info *,
 			  __u32 *);
-extern int cifs_verify_signature(struct kvec *iov, unsigned int nr_iov,
+extern int cifs_sign_smb(struct smb_hdr *, struct TCP_Server_Info *, __u32 *);
+extern int cifs_verify_signature(struct smb_rqst *rqst,
 				 struct TCP_Server_Info *server,
 				__u32 expected_sequence_number);
 extern int SMBNTencrypt(unsigned char *, unsigned char *, unsigned char *,
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index cabc7a0..ce5f75c 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -1564,6 +1564,8 @@ cifs_readv_callback(struct mid_q_entry *mid)
 	struct cifs_readdata *rdata = mid->callback_data;
 	struct cifs_tcon *tcon = tlink_tcon(rdata->cfile->tlink);
 	struct TCP_Server_Info *server = tcon->ses->server;
+	struct smb_rqst rqst = { .rq_iov = rdata->iov,
+				 .rq_nvec = rdata->nr_iov };
 
 	cFYI(1, "%s: mid=%llu state=%d result=%d bytes=%u", __func__,
 		mid->mid, mid->mid_state, rdata->result, rdata->bytes);
@@ -1573,8 +1575,8 @@ cifs_readv_callback(struct mid_q_entry *mid)
 		/* result already set, check signature */
 		if (server->sec_mode &
 		    (SECMODE_SIGN_REQUIRED | SECMODE_SIGN_ENABLED)) {
-			if (cifs_verify_signature(rdata->iov, rdata->nr_iov,
-					  server, mid->sequence_number + 1))
+			if (cifs_verify_signature(&rqst, server,
+						  mid->sequence_number + 1))
 				cERROR(1, "Unexpected SMB signature");
 		}
 		/* FIXME: should this be counted toward the initiating task? */
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 83867ef..cc2eda9 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -503,11 +503,13 @@ cifs_check_receive(struct mid_q_entry *mid, struct TCP_Server_Info *server,
 	/* convert the length into a more usable form */
 	if (server->sec_mode & (SECMODE_SIGN_REQUIRED | SECMODE_SIGN_ENABLED)) {
 		struct kvec iov;
+		struct smb_rqst rqst = { .rq_iov = &iov,
+					 .rq_nvec = 1 };
 
 		iov.iov_base = mid->resp_buf;
 		iov.iov_len = len;
 		/* FIXME: add code to kill session */
-		if (cifs_verify_signature(&iov, 1, server,
+		if (cifs_verify_signature(&rqst, server,
 					  mid->sequence_number + 1) != 0)
 			cERROR(1, "Unexpected SMB signature");
 	}
-- 
1.7.11.2

Re: [PATCH v2 1/9] cifs: change signing routines to deal with smb_rqst structs

[Pavel Shilovsky]

2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> We need a way to represent a call to be sent on the wire that does not
> require having all of the page data kmapped. Behold the smb_rqst struct.
> This new struct represents an array of kvecs immediately followed by an
> array of pages.
>
> Convert the signing routines to use these structs under the hood and
> turn the existing functions for this into wrappers around that. For now,
> we're just changing these functions to take different args. Later, we'll
> teach them how to deal with arrays of pages.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifsencrypt.c | 26 ++++++++++++++++++--------
>  fs/cifs/cifsglob.h    | 14 ++++++++++++++
>  fs/cifs/cifsproto.h   |  7 +++++--
>  fs/cifs/cifssmb.c     |  6 ++++--
>  fs/cifs/transport.c   |  4 +++-
>  5 files changed, 44 insertions(+), 13 deletions(-)
>
> diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
> index 6a0d741..e8953a0 100644
> --- a/fs/cifs/cifsencrypt.c
> +++ b/fs/cifs/cifsencrypt.c
> @@ -37,11 +37,13 @@
>   * 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(const struct kvec *iov, int n_vec,
> +static int cifs_calc_signature(struct smb_rqst *rqst,
>                         struct TCP_Server_Info *server, char *signature)
>  {
>         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;
> @@ -99,12 +101,12 @@ static int cifs_calc_signature(const struct kvec *iov, int n_vec,
>  }
>
>  /* must be called with server->srv_mutex held */
> -int cifs_sign_smbv(struct kvec *iov, int n_vec, struct TCP_Server_Info *server,
> +int cifs_sign_rqst(struct smb_rqst *rqst, struct TCP_Server_Info *server,
>                    __u32 *pexpected_response_sequence_number)
>  {
>         int rc = 0;
>         char smb_signature[20];
> -       struct smb_hdr *cifs_pdu = (struct smb_hdr *)iov[0].iov_base;
> +       struct smb_hdr *cifs_pdu = (struct smb_hdr *)rqst->rq_iov[0].iov_base;
>
>         if ((cifs_pdu == NULL) || (server == NULL))
>                 return -EINVAL;
> @@ -125,7 +127,7 @@ int cifs_sign_smbv(struct kvec *iov, int n_vec, struct TCP_Server_Info *server,
>         *pexpected_response_sequence_number = server->sequence_number++;
>         server->sequence_number++;
>
> -       rc = cifs_calc_signature(iov, n_vec, server, smb_signature);
> +       rc = cifs_calc_signature(rqst, server, smb_signature);
>         if (rc)
>                 memset(cifs_pdu->Signature.SecuritySignature, 0, 8);
>         else
> @@ -134,6 +136,15 @@ int cifs_sign_smbv(struct kvec *iov, int n_vec, struct TCP_Server_Info *server,
>         return rc;
>  }
>
> +int cifs_sign_smbv(struct kvec *iov, int n_vec, struct TCP_Server_Info *server,
> +                  __u32 *pexpected_response_sequence)
> +{
> +       struct smb_rqst rqst = { .rq_iov = iov,
> +                                .rq_nvec = n_vec };
> +
> +       return cifs_sign_rqst(&rqst, server, pexpected_response_sequence);
> +}
> +
>  /* must be called with server->srv_mutex held */
>  int cifs_sign_smb(struct smb_hdr *cifs_pdu, struct TCP_Server_Info *server,
>                   __u32 *pexpected_response_sequence_number)
> @@ -147,14 +158,14 @@ int cifs_sign_smb(struct smb_hdr *cifs_pdu, struct TCP_Server_Info *server,
>                               pexpected_response_sequence_number);
>  }
>
> -int cifs_verify_signature(struct kvec *iov, unsigned int nr_iov,
> +int cifs_verify_signature(struct smb_rqst *rqst,
>                           struct TCP_Server_Info *server,
>                           __u32 expected_sequence_number)
>  {
>         unsigned int rc;
>         char server_response_sig[8];
>         char what_we_think_sig_should_be[20];
> -       struct smb_hdr *cifs_pdu = (struct smb_hdr *)iov[0].iov_base;
> +       struct smb_hdr *cifs_pdu = (struct smb_hdr *)rqst->rq_iov[0].iov_base;
>
>         if (cifs_pdu == NULL || server == NULL)
>                 return -EINVAL;
> @@ -186,8 +197,7 @@ int cifs_verify_signature(struct kvec *iov, unsigned int nr_iov,
>         cifs_pdu->Signature.Sequence.Reserved = 0;
>
>         mutex_lock(&server->srv_mutex);
> -       rc = cifs_calc_signature(iov, nr_iov, server,
> -                                what_we_think_sig_should_be);
> +       rc = cifs_calc_signature(rqst, server, what_we_think_sig_should_be);
>         mutex_unlock(&server->srv_mutex);
>
>         if (rc)
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index bcdf4d4..2a7e4b8 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -158,6 +158,20 @@ struct cifs_cred {
>   *****************************************************************
>   */
>
> +/*
> + * A smb_rqst represents a complete request to be issued to a server. It's
> + * formed by a kvec array, followed by an array of pages. Page data is assumed
> + * to start at the beginning of the first page.
> + */
> +struct smb_rqst {
> +       struct kvec     *rq_iov;        /* array of kvecs */
> +       unsigned int    rq_nvec;        /* number of kvecs in array */
> +       struct page     **rq_pages;     /* pointer to array of page ptrs */
> +       unsigned int    rq_npages;      /* number pages in array */
> +       unsigned int    rq_pagesz;      /* page size to use */
> +       unsigned int    rq_tailsz;      /* length of last page */
> +};
> +
>  enum smb_version {
>         Smb_1 = 1,
>         Smb_21,
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index cf7fb18..d02a6a4 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -24,6 +24,7 @@
>
>  struct statfs;
>  struct smb_vol;
> +struct smb_rqst;
>
>  /*
>   *****************************************************************
> @@ -400,10 +401,12 @@ extern void sesInfoFree(struct cifs_ses *);
>  extern struct cifs_tcon *tconInfoAlloc(void);
>  extern void tconInfoFree(struct cifs_tcon *);
>
> -extern int cifs_sign_smb(struct smb_hdr *, struct TCP_Server_Info *, __u32 *);
> +extern int cifs_sign_rqst(struct smb_rqst *rqst, struct TCP_Server_Info *server,
> +                  __u32 *pexpected_response_sequence_number);
>  extern int cifs_sign_smbv(struct kvec *iov, int n_vec, struct TCP_Server_Info *,
>                           __u32 *);
> -extern int cifs_verify_signature(struct kvec *iov, unsigned int nr_iov,
> +extern int cifs_sign_smb(struct smb_hdr *, struct TCP_Server_Info *, __u32 *);
> +extern int cifs_verify_signature(struct smb_rqst *rqst,
>                                  struct TCP_Server_Info *server,
>                                 __u32 expected_sequence_number);
>  extern int SMBNTencrypt(unsigned char *, unsigned char *, unsigned char *,
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index cabc7a0..ce5f75c 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -1564,6 +1564,8 @@ cifs_readv_callback(struct mid_q_entry *mid)
>         struct cifs_readdata *rdata = mid->callback_data;
>         struct cifs_tcon *tcon = tlink_tcon(rdata->cfile->tlink);
>         struct TCP_Server_Info *server = tcon->ses->server;
> +       struct smb_rqst rqst = { .rq_iov = rdata->iov,
> +                                .rq_nvec = rdata->nr_iov };
>
>         cFYI(1, "%s: mid=%llu state=%d result=%d bytes=%u", __func__,
>                 mid->mid, mid->mid_state, rdata->result, rdata->bytes);
> @@ -1573,8 +1575,8 @@ cifs_readv_callback(struct mid_q_entry *mid)
>                 /* result already set, check signature */
>                 if (server->sec_mode &
>                     (SECMODE_SIGN_REQUIRED | SECMODE_SIGN_ENABLED)) {
> -                       if (cifs_verify_signature(rdata->iov, rdata->nr_iov,
> -                                         server, mid->sequence_number + 1))
> +                       if (cifs_verify_signature(&rqst, server,
> +                                                 mid->sequence_number + 1))
>                                 cERROR(1, "Unexpected SMB signature");
>                 }
>                 /* FIXME: should this be counted toward the initiating task? */
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 83867ef..cc2eda9 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -503,11 +503,13 @@ cifs_check_receive(struct mid_q_entry *mid, struct TCP_Server_Info *server,
>         /* convert the length into a more usable form */
>         if (server->sec_mode & (SECMODE_SIGN_REQUIRED | SECMODE_SIGN_ENABLED)) {
>                 struct kvec iov;
> +               struct smb_rqst rqst = { .rq_iov = &iov,
> +                                        .rq_nvec = 1 };
>
>                 iov.iov_base = mid->resp_buf;
>                 iov.iov_len = len;
>                 /* FIXME: add code to kill session */
> -               if (cifs_verify_signature(&iov, 1, server,
> +               if (cifs_verify_signature(&rqst, server,
>                                           mid->sequence_number + 1) != 0)
>                         cERROR(1, "Unexpected SMB signature");
>         }
> --
> 1.7.11.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reviewed-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

-- 
Best regards,
Pavel Shilovsky.

[PATCH v2 2/9] cifs: convert send code to use smb_rqst structs

[Jeff Layton]

Again, just a change in the arguments and some function renaming here.
In later patches, we'll change this code to deal with page arrays.

In this patch, we add a new smb_send_rqst wrapper and have smb_sendv
call that. Then we move most of the existing smb_sendv code into a new
function -- smb_send_kvec. This seems a little redundant, but later
we'll flesh this out to deal with arrays of pages.

Reviewed-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/transport.c | 135 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 90 insertions(+), 45 deletions(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index cc2eda9..d93f15d 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -119,18 +119,29 @@ delete_mid(struct mid_q_entry *mid)
 	DeleteMidQEntry(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
+ * @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_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
+smb_send_kvec(struct TCP_Server_Info *server, struct kvec *iov, size_t n_vec,
+		size_t *sent)
 {
 	int rc = 0;
 	int i = 0;
 	struct msghdr smb_msg;
-	unsigned int len = iov[0].iov_len;
-	unsigned int total_len;
-	int first_vec = 0;
-	unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
+	unsigned int remaining;
+	size_t first_vec = 0;
 	struct socket *ssocket = server->ssocket;
 
+	*sent = 0;
+
 	if (ssocket == NULL)
 		return -ENOTSOCK; /* BB eventually add reconnect code here */
 
@@ -143,56 +154,60 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
 	else
 		smb_msg.msg_flags = MSG_NOSIGNAL;
 
-	total_len = 0;
+	remaining = 0;
 	for (i = 0; i < n_vec; i++)
-		total_len += iov[i].iov_len;
-
-	cFYI(1, "Sending smb:  total_len %d", total_len);
-	dump_smb(iov[0].iov_base, len);
+		remaining += iov[i].iov_len;
 
 	i = 0;
-	while (total_len) {
+	while (remaining) {
+		/*
+		 * If blocking send, we try 3 times, since each can block
+		 * for 5 seconds. For nonblocking  we have to try more
+		 * but wait increasing amounts of time allowing time for
+		 * socket to clear.  The overall time we wait in either
+		 * case to send on the socket is about 15 seconds.
+		 * Similarly we wait for 15 seconds for a response from
+		 * the server in SendReceive[2] for the server to send
+		 * a response back for most types of requests (except
+		 * SMB Write past end of file which can be slow, and
+		 * blocking lock operations). NFS waits slightly longer
+		 * than CIFS, but this can make it take longer for
+		 * nonresponsive servers to be detected and 15 seconds
+		 * is more than enough time for modern networks to
+		 * send a packet.  In most cases if we fail to send
+		 * 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, total_len);
-		if ((rc == -ENOSPC) || (rc == -EAGAIN)) {
+				    n_vec - first_vec, remaining);
+		if (rc == -ENOSPC || rc == -EAGAIN) {
 			i++;
-			/*
-			 * If blocking send we try 3 times, since each can block
-			 * for 5 seconds. For nonblocking  we have to try more
-			 * but wait increasing amounts of time allowing time for
-			 * socket to clear.  The overall time we wait in either
-			 * case to send on the socket is about 15 seconds.
-			 * Similarly we wait for 15 seconds for a response from
-			 * the server in SendReceive[2] for the server to send
-			 * a response back for most types of requests (except
-			 * SMB Write past end of file which can be slow, and
-			 * blocking lock operations). NFS waits slightly longer
-			 * than CIFS, but this can make it take longer for
-			 * nonresponsive servers to be detected and 15 seconds
-			 * is more than enough time for modern networks to
-			 * send a packet.  In most cases if we fail to send
-			 * after the retries we will kill the socket and
-			 * reconnect which may clear the network problem.
-			 */
-			if ((i >= 14) || (!server->noblocksnd && (i > 2))) {
-				cERROR(1, "sends on sock %p stuck for 15 seconds",
-				    ssocket);
+			if (i >= 14 || (!server->noblocksnd && (i > 2))) {
+				cERROR(1, "sends on sock %p stuck for 15 "
+					  "seconds", ssocket);
 				rc = -EAGAIN;
 				break;
 			}
 			msleep(1 << i);
 			continue;
 		}
+
 		if (rc < 0)
 			break;
 
-		if (rc == total_len) {
-			total_len = 0;
+		/* send was at least partially successful */
+		*sent += rc;
+
+		if (rc == remaining) {
+			remaining = 0;
 			break;
-		} else if (rc > total_len) {
-			cERROR(1, "sent %d requested %d", rc, total_len);
+		}
+
+		if (rc > remaining) {
+			cERROR(1, "sent %d requested %d", rc, remaining);
 			break;
 		}
+
 		if (rc == 0) {
 			/* should never happen, letting socket clear before
 			   retrying is our only obvious option here */
@@ -200,7 +215,9 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
 			msleep(500);
 			continue;
 		}
-		total_len -= rc;
+
+		remaining -= rc;
+
 		/* the line below resets i */
 		for (i = first_vec; i < n_vec; i++) {
 			if (iov[i].iov_len) {
@@ -215,16 +232,35 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
 				}
 			}
 		}
+
 		i = 0; /* in case we get ENOSPC on the next send */
+		rc = 0;
 	}
+	return rc;
+}
+
+static int
+smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
+{
+	int rc;
+	struct kvec *iov = rqst->rq_iov;
+	int n_vec = rqst->rq_nvec;
+	unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
+	size_t total_len;
+
+	cFYI(1, "Sending smb: smb_len=%u", smb_buf_length);
+	dump_smb(iov[0].iov_base, iov[0].iov_len);
+
+	rc = smb_send_kvec(server, iov, n_vec, &total_len);
 
 	if ((total_len > 0) && (total_len != smb_buf_length + 4)) {
-		cFYI(1, "partial send (%d remaining), terminating session",
-			total_len);
-		/* If we have only sent part of an SMB then the next SMB
-		   could be taken as the remainder of this one.  We need
-		   to kill the socket so the server throws away the partial
-		   SMB */
+		cFYI(1, "partial send (wanted=%u sent=%zu): terminating "
+			"session", smb_buf_length + 4, total_len);
+		/*
+		 * If we have only sent part of an SMB then the next SMB could
+		 * be taken as the remainder of this one. We need to kill the
+		 * socket so the server throws away the partial SMB
+		 */
 		server->tcpStatus = CifsNeedReconnect;
 	}
 
@@ -236,6 +272,15 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
 	return rc;
 }
 
+static int
+smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
+{
+	struct smb_rqst rqst = { .rq_iov = iov,
+				 .rq_nvec = n_vec };
+
+	return smb_send_rqst(server, &rqst);
+}
+
 int
 smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer,
 	 unsigned int smb_buf_length)
-- 
1.7.11.2

[PATCH v2 3/9] cifs: cork the socket before a send and uncork it afterward

[Jeff Layton]

We want to send SMBs as "atomically" as possible. Prior to sending any
data on the socket, cork it to make sure that no non-full frames go
out. Afterward, uncork it to make sure all of the data gets pushed out
to the wire.

Note that this more or less renders the socket=TCP_NODELAY mount option
obsolete. When TCP_CORK and TCP_NODELAY are used on the same socket,
TCP_NODELAY is essentially ignored.

Acked-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c   |  4 ++++
 fs/cifs/transport.c | 12 ++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 6df6fa1..a828a8c 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1676,6 +1676,10 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
 			if (string == NULL)
 				goto out_nomem;
 
+			/*
+			 * FIXME: since we now cork/uncork the socket while
+			 * 	  sending, should we deprecate this option?
+			 */
 			if (strnicmp(string, "TCP_NODELAY", 11) == 0)
 				vol->sockopt_tcp_nodelay = 1;
 			break;
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index d93f15d..a3e58b2 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -27,6 +27,7 @@
 #include <linux/net.h>
 #include <linux/delay.h>
 #include <linux/freezer.h>
+#include <linux/tcp.h>
 #include <asm/uaccess.h>
 #include <asm/processor.h>
 #include <linux/mempool.h>
@@ -247,12 +248,23 @@ smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
 	int n_vec = rqst->rq_nvec;
 	unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
 	size_t total_len;
+	struct socket *ssocket = server->ssocket;
+	int val = 1;
 
 	cFYI(1, "Sending smb: smb_len=%u", smb_buf_length);
 	dump_smb(iov[0].iov_base, iov[0].iov_len);
 
+	/* cork the socket */
+	kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
+				(char *)&val, sizeof(val));
+
 	rc = smb_send_kvec(server, iov, n_vec, &total_len);
 
+	/* uncork it */
+	val = 0;
+	kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
+				(char *)&val, sizeof(val));
+
 	if ((total_len > 0) && (total_len != smb_buf_length + 4)) {
 		cFYI(1, "partial send (wanted=%u sent=%zu): terminating "
 			"session", smb_buf_length + 4, total_len);
-- 
1.7.11.2

Re: [PATCH v2 3/9] cifs: cork the socket before a send and uncork it afterward

[Pavel Shilovsky]

2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> We want to send SMBs as "atomically" as possible. Prior to sending any
> data on the socket, cork it to make sure that no non-full frames go
> out. Afterward, uncork it to make sure all of the data gets pushed out
> to the wire.
>
> Note that this more or less renders the socket=TCP_NODELAY mount option
> obsolete. When TCP_CORK and TCP_NODELAY are used on the same socket,
> TCP_NODELAY is essentially ignored.
>
> Acked-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/connect.c   |  4 ++++
>  fs/cifs/transport.c | 12 ++++++++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 6df6fa1..a828a8c 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -1676,6 +1676,10 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
>                         if (string == NULL)
>                                 goto out_nomem;
>
> +                       /*
> +                        * FIXME: since we now cork/uncork the socket while
> +                        *        sending, should we deprecate this option?
> +                        */
>                         if (strnicmp(string, "TCP_NODELAY", 11) == 0)
>                                 vol->sockopt_tcp_nodelay = 1;
>                         break;
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index d93f15d..a3e58b2 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -27,6 +27,7 @@
>  #include <linux/net.h>
>  #include <linux/delay.h>
>  #include <linux/freezer.h>
> +#include <linux/tcp.h>
>  #include <asm/uaccess.h>
>  #include <asm/processor.h>
>  #include <linux/mempool.h>
> @@ -247,12 +248,23 @@ smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
>         int n_vec = rqst->rq_nvec;
>         unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
>         size_t total_len;
> +       struct socket *ssocket = server->ssocket;
> +       int val = 1;
>
>         cFYI(1, "Sending smb: smb_len=%u", smb_buf_length);
>         dump_smb(iov[0].iov_base, iov[0].iov_len);
>
> +       /* cork the socket */
> +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
> +                               (char *)&val, sizeof(val));
> +
>         rc = smb_send_kvec(server, iov, n_vec, &total_len);
>
> +       /* uncork it */
> +       val = 0;
> +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
> +                               (char *)&val, sizeof(val));
> +
>         if ((total_len > 0) && (total_len != smb_buf_length + 4)) {
>                 cFYI(1, "partial send (wanted=%u sent=%zu): terminating "
>                         "session", smb_buf_length + 4, total_len);
> --
> 1.7.11.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

I tested it with SMB2 against Windows 7 server. When iosize is 64K
everything is ok but when we increase iosize to 1M (by using
multicredit requests) and the server loses the network connection and
only reboot helps.

Also if I commented corking/uncorking the socket - everything is ok. I
think this change needs some more investigation (how does it deals
with 1M iosize on Samba, etc?)

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH v2 3/9] cifs: cork the socket before a send and uncork it afterward

[Jeff Layton]

On Fri, 27 Jul 2012 03:57:44 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > We want to send SMBs as "atomically" as possible. Prior to sending any
> > data on the socket, cork it to make sure that no non-full frames go
> > out. Afterward, uncork it to make sure all of the data gets pushed out
> > to the wire.
> >
> > Note that this more or less renders the socket=TCP_NODELAY mount option
> > obsolete. When TCP_CORK and TCP_NODELAY are used on the same socket,
> > TCP_NODELAY is essentially ignored.
> >
> > Acked-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/cifs/connect.c   |  4 ++++
> >  fs/cifs/transport.c | 12 ++++++++++++
> >  2 files changed, 16 insertions(+)
> >
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index 6df6fa1..a828a8c 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -1676,6 +1676,10 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
> >                         if (string == NULL)
> >                                 goto out_nomem;
> >
> > +                       /*
> > +                        * FIXME: since we now cork/uncork the socket while
> > +                        *        sending, should we deprecate this option?
> > +                        */
> >                         if (strnicmp(string, "TCP_NODELAY", 11) == 0)
> >                                 vol->sockopt_tcp_nodelay = 1;
> >                         break;
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index d93f15d..a3e58b2 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -27,6 +27,7 @@
> >  #include <linux/net.h>
> >  #include <linux/delay.h>
> >  #include <linux/freezer.h>
> > +#include <linux/tcp.h>
> >  #include <asm/uaccess.h>
> >  #include <asm/processor.h>
> >  #include <linux/mempool.h>
> > @@ -247,12 +248,23 @@ smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
> >         int n_vec = rqst->rq_nvec;
> >         unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
> >         size_t total_len;
> > +       struct socket *ssocket = server->ssocket;
> > +       int val = 1;
> >
> >         cFYI(1, "Sending smb: smb_len=%u", smb_buf_length);
> >         dump_smb(iov[0].iov_base, iov[0].iov_len);
> >
> > +       /* cork the socket */
> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
> > +                               (char *)&val, sizeof(val));
> > +
> >         rc = smb_send_kvec(server, iov, n_vec, &total_len);
> >
> > +       /* uncork it */
> > +       val = 0;
> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
> > +                               (char *)&val, sizeof(val));
> > +
> >         if ((total_len > 0) && (total_len != smb_buf_length + 4)) {
> >                 cFYI(1, "partial send (wanted=%u sent=%zu): terminating "
> >                         "session", smb_buf_length + 4, total_len);
> > --
> > 1.7.11.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> I tested it with SMB2 against Windows 7 server. When iosize is 64K
> everything is ok but when we increase iosize to 1M (by using
> multicredit requests) and the server loses the network connection and
> only reboot helps.
> 
> Also if I commented corking/uncorking the socket - everything is ok. I
> think this change needs some more investigation (how does it deals
> with 1M iosize on Samba, etc?)
> 

Hmm, haven't seen that with a 1M iosize with smb1 against samba.

I'll see if I can reproduce it.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH v2 3/9] cifs: cork the socket before a send and uncork it afterward

[Pavel Shilovsky]

2012/7/27 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Fri, 27 Jul 2012 03:57:44 +0400
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> 2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> > We want to send SMBs as "atomically" as possible. Prior to sending any
>> > data on the socket, cork it to make sure that no non-full frames go
>> > out. Afterward, uncork it to make sure all of the data gets pushed out
>> > to the wire.
>> >
>> > Note that this more or less renders the socket=TCP_NODELAY mount option
>> > obsolete. When TCP_CORK and TCP_NODELAY are used on the same socket,
>> > TCP_NODELAY is essentially ignored.
>> >
>> > Acked-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > ---
>> >  fs/cifs/connect.c   |  4 ++++
>> >  fs/cifs/transport.c | 12 ++++++++++++
>> >  2 files changed, 16 insertions(+)
>> >
>> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> > index 6df6fa1..a828a8c 100644
>> > --- a/fs/cifs/connect.c
>> > +++ b/fs/cifs/connect.c
>> > @@ -1676,6 +1676,10 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
>> >                         if (string == NULL)
>> >                                 goto out_nomem;
>> >
>> > +                       /*
>> > +                        * FIXME: since we now cork/uncork the socket while
>> > +                        *        sending, should we deprecate this option?
>> > +                        */
>> >                         if (strnicmp(string, "TCP_NODELAY", 11) == 0)
>> >                                 vol->sockopt_tcp_nodelay = 1;
>> >                         break;
>> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>> > index d93f15d..a3e58b2 100644
>> > --- a/fs/cifs/transport.c
>> > +++ b/fs/cifs/transport.c
>> > @@ -27,6 +27,7 @@
>> >  #include <linux/net.h>
>> >  #include <linux/delay.h>
>> >  #include <linux/freezer.h>
>> > +#include <linux/tcp.h>
>> >  #include <asm/uaccess.h>
>> >  #include <asm/processor.h>
>> >  #include <linux/mempool.h>
>> > @@ -247,12 +248,23 @@ smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
>> >         int n_vec = rqst->rq_nvec;
>> >         unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
>> >         size_t total_len;
>> > +       struct socket *ssocket = server->ssocket;
>> > +       int val = 1;
>> >
>> >         cFYI(1, "Sending smb: smb_len=%u", smb_buf_length);
>> >         dump_smb(iov[0].iov_base, iov[0].iov_len);
>> >
>> > +       /* cork the socket */
>> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
>> > +                               (char *)&val, sizeof(val));
>> > +
>> >         rc = smb_send_kvec(server, iov, n_vec, &total_len);
>> >
>> > +       /* uncork it */
>> > +       val = 0;
>> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
>> > +                               (char *)&val, sizeof(val));
>> > +
>> >         if ((total_len > 0) && (total_len != smb_buf_length + 4)) {
>> >                 cFYI(1, "partial send (wanted=%u sent=%zu): terminating "
>> >                         "session", smb_buf_length + 4, total_len);
>> > --
>> > 1.7.11.2
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> I tested it with SMB2 against Windows 7 server. When iosize is 64K
>> everything is ok but when we increase iosize to 1M (by using
>> multicredit requests) and the server loses the network connection and
>> only reboot helps.
>>
>> Also if I commented corking/uncorking the socket - everything is ok. I
>> think this change needs some more investigation (how does it deals
>> with 1M iosize on Samba, etc?)
>>
>
> Hmm, haven't seen that with a 1M iosize with smb1 against samba.
>
> I'll see if I can reproduce it.
>
> --
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Forgot to mentioned how I reproduce it - dbench with 5 clients.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH v2 3/9] cifs: cork the socket before a send and uncork it afterward

[Jeff Layton]

On Fri, 27 Jul 2012 10:05:32 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2012/7/27 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > On Fri, 27 Jul 2012 03:57:44 +0400
> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> 2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >> > We want to send SMBs as "atomically" as possible. Prior to sending any
> >> > data on the socket, cork it to make sure that no non-full frames go
> >> > out. Afterward, uncork it to make sure all of the data gets pushed out
> >> > to the wire.
> >> >
> >> > Note that this more or less renders the socket=TCP_NODELAY mount option
> >> > obsolete. When TCP_CORK and TCP_NODELAY are used on the same socket,
> >> > TCP_NODELAY is essentially ignored.
> >> >
> >> > Acked-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> >> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> > ---
> >> >  fs/cifs/connect.c   |  4 ++++
> >> >  fs/cifs/transport.c | 12 ++++++++++++
> >> >  2 files changed, 16 insertions(+)
> >> >
> >> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >> > index 6df6fa1..a828a8c 100644
> >> > --- a/fs/cifs/connect.c
> >> > +++ b/fs/cifs/connect.c
> >> > @@ -1676,6 +1676,10 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
> >> >                         if (string == NULL)
> >> >                                 goto out_nomem;
> >> >
> >> > +                       /*
> >> > +                        * FIXME: since we now cork/uncork the socket while
> >> > +                        *        sending, should we deprecate this option?
> >> > +                        */
> >> >                         if (strnicmp(string, "TCP_NODELAY", 11) == 0)
> >> >                                 vol->sockopt_tcp_nodelay = 1;
> >> >                         break;
> >> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> >> > index d93f15d..a3e58b2 100644
> >> > --- a/fs/cifs/transport.c
> >> > +++ b/fs/cifs/transport.c
> >> > @@ -27,6 +27,7 @@
> >> >  #include <linux/net.h>
> >> >  #include <linux/delay.h>
> >> >  #include <linux/freezer.h>
> >> > +#include <linux/tcp.h>
> >> >  #include <asm/uaccess.h>
> >> >  #include <asm/processor.h>
> >> >  #include <linux/mempool.h>
> >> > @@ -247,12 +248,23 @@ smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
> >> >         int n_vec = rqst->rq_nvec;
> >> >         unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
> >> >         size_t total_len;
> >> > +       struct socket *ssocket = server->ssocket;
> >> > +       int val = 1;
> >> >
> >> >         cFYI(1, "Sending smb: smb_len=%u", smb_buf_length);
> >> >         dump_smb(iov[0].iov_base, iov[0].iov_len);
> >> >
> >> > +       /* cork the socket */
> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
> >> > +                               (char *)&val, sizeof(val));
> >> > +
> >> >         rc = smb_send_kvec(server, iov, n_vec, &total_len);
> >> >
> >> > +       /* uncork it */
> >> > +       val = 0;
> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
> >> > +                               (char *)&val, sizeof(val));
> >> > +
> >> >         if ((total_len > 0) && (total_len != smb_buf_length + 4)) {
> >> >                 cFYI(1, "partial send (wanted=%u sent=%zu): terminating "
> >> >                         "session", smb_buf_length + 4, total_len);
> >> > --
> >> > 1.7.11.2
> >> >
> >> > --
> >> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> >> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> >> I tested it with SMB2 against Windows 7 server. When iosize is 64K
> >> everything is ok but when we increase iosize to 1M (by using
> >> multicredit requests) and the server loses the network connection and
> >> only reboot helps.
> >>
> >> Also if I commented corking/uncorking the socket - everything is ok. I
> >> think this change needs some more investigation (how does it deals
> >> with 1M iosize on Samba, etc?)
> >>
> >
> > Hmm, haven't seen that with a 1M iosize with smb1 against samba.
> >
> > I'll see if I can reproduce it.
> >
> > --
> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> Forgot to mentioned how I reproduce it - dbench with 5 clients.
> 

Ok, I've built a cifs.ko from your smb2-dev-cifs-3.6 branch. Here are my mount options:

//win7.poochiereds.net/export /mnt/win7 cifs rw,relatime,vers=2.1,sec=ntlmsspi,cache=strict,unc=\\win7.poochiereds.net\export,username=testuser,domain=WIN7,uid=0,noforceuid,gid=0,noforcegid,addr=192.168.1.32,file_mode=0755,dir_mode=0755,nounix,serverino,rsize=1048576,wsize=1048576,actimeo=1 0 0

I'm running dbench against it with 5 clients, but am seeing no hangs. Do I need to do something else to reproduce it?

Note that I am seeing a number of these sorts of warning messages:

[84306.348564] CIFS VFS: No task to wake, unknown frame received! NumMids 4
[84306.353262] Received Data is: : dump of 68 bytes of data at 0xffff88000172aab0
[84306.353266]  6c000000 424d53fe 00000040 00000000 . . . l \xfffffffe S M B @ . . . . . . .
[84306.353269]  00000012 00000001 00000000 ffffffff . . . . . . . . . . . . \xffffffff \xffffffff \xffffffff \xffffffff
[84306.353271]  ffffffff 00000000 00000000 00000000 \xffffffff \xffffffff \xffffffff \xffffffff . . . . . . . . . . . .
[84306.353273]  00000000 00000000 00000000 00000000 . . . . . . . . . . . . . . . .
[84306.353275]  00000000 . . . .
[84307.761162] CIFS VFS: No task to wake, unknown frame received! NumMids 5
[84307.764904] Received Data is: : dump of 68 bytes of data at 0xffff88000172aab0
[84307.764908]  6c000000 424d53fe 00000040 00000000 . . . l \xfffffffe S M B @ . . . . . . .
[84307.764911]  00000012 00000001 00000000 ffffffff . . . . . . . . . . . . \xffffffff \xffffffff \xffffffff \xffffffff
[84307.764914]  ffffffff 00000000 00000000 00000000 \xffffffff \xffffffff \xffffffff \xffffffff . . . . . . . . . . . .
[84307.764916]  00000000 00000000 00000000 00000000 . . . . . . . . . . . . . . . .
[84307.764917]  00000000 . . . .
[84311.566786] CIFS VFS: No task to wake, unknown frame received! NumMids 5
[84311.570630] Received Data is: : dump of 68 bytes of data at 0xffff8800017a38f0
[84311.570634]  6c000000 424d53fe 00000040 00000000 . . . l \xfffffffe S M B @ . . . . . . .
[84311.570637]  00000012 00000001 00000000 ffffffff . . . . . . . . . . . . \xffffffff \xffffffff \xffffffff \xffffffff
[84311.570639]  ffffffff 00000000 00000000 00000000 \xffffffff \xffffffff \xffffffff \xffffffff . . . . . . . . . . . .
[84311.570642]  00000000 00000000 00000000 00000000 . . . . . . . . . . . . . . . .
[84311.570643]  00000000 . . . .

I get the same result whether I use TCP_CORK or not, so I don't think
that's related.

Also, after running dbench on a vers=2.1 mount and unplugging the kmod,
I saw a bunch of these sorts of warnings, indicating memory leaks in
the SMB2 code. Those may be related to the warnings above:

[84980.135644] =============================================================================
[84980.146094] BUG cifs_small_rq (Tainted: G           O): Objects remaining on kmem_cache_close()
[84980.156757] -----------------------------------------------------------------------------
[84980.156757] 
[84980.177277] INFO: Slab 0xffffea00003e3d00 objects=19 used=17 fp=0xffff88000f8f6700 flags=0x10000000004080
[84980.188416] Pid: 20224, comm: rmmod Tainted: G           O 3.6.0-0.rc0.git2.1.fc18.x86_64 #1
[84980.199692] Call Trace:
[84980.210231]  [<ffffffff811ac61f>] slab_err+0xaf/0xd0
[84980.221112]  [<ffffffff811b1329>] ? kmem_cache_destroy+0x249/0x3d0
[84980.232334]  [<ffffffff811b124b>] ? kmem_cache_destroy+0x16b/0x3d0
[84980.243424]  [<ffffffff811b126f>] kmem_cache_destroy+0x18f/0x3d0
[84980.254653]  [<ffffffff8115dff5>] ? mempool_destroy+0x55/0x60
[84980.265816]  [<ffffffffa0357d29>] cifs_destroy_request_bufs+0x39/0x3b [cifs]
[84980.277350]  [<ffffffffa0357f4f>] exit_cifs+0x30/0xe1 [cifs]
[84980.287385]  [<ffffffff810e1914>] sys_delete_module+0x1a4/0x300
[84980.298287]  [<ffffffff816d0795>] ? retint_swapgs+0x13/0x1b
[84980.309234]  [<ffffffff811008bc>] ? __audit_syscall_entry+0xcc/0x300
[84980.320486]  [<ffffffff8134879e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[84980.331739]  [<ffffffff816d9269>] system_call_fastpath+0x16/0x1b
[84980.342929] INFO: Object 0xffff88000f8f4000 @offset=0
[84980.354043] INFO: Allocated in mempool_alloc_slab+0x15/0x20 age=173263 cpu=0 pid=19880
[84980.365669] 	__slab_alloc+0x422/0x4d2
[84980.376882] 	kmem_cache_alloc+0x227/0x260
[84980.387943] 	mempool_alloc_slab+0x15/0x20
[84980.398932] 	mempool_alloc+0x68/0x180
[84980.409688] 	cifs_small_buf_get+0x1a/0x30 [cifs]
[84980.420650] 	cifs_demultiplex_thread+0x405/0x950 [cifs]
[84980.431773] 	kthread+0xb7/0xc0
[84980.442675] 	kernel_thread_helper+0x4/0x10

That probably needs to be investigated (and fixed) as well...

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH v2 3/9] cifs: cork the socket before a send and uncork it afterward

[Pavel Shilovsky]

2012/7/29 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Fri, 27 Jul 2012 10:05:32 +0400
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> 2012/7/27 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> > On Fri, 27 Jul 2012 03:57:44 +0400
>> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >
>> >> 2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> >> > We want to send SMBs as "atomically" as possible. Prior to sending any
>> >> > data on the socket, cork it to make sure that no non-full frames go
>> >> > out. Afterward, uncork it to make sure all of the data gets pushed out
>> >> > to the wire.
>> >> >
>> >> > Note that this more or less renders the socket=TCP_NODELAY mount option
>> >> > obsolete. When TCP_CORK and TCP_NODELAY are used on the same socket,
>> >> > TCP_NODELAY is essentially ignored.
>> >> >
>> >> > Acked-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>> >> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> >> > ---
>> >> >  fs/cifs/connect.c   |  4 ++++
>> >> >  fs/cifs/transport.c | 12 ++++++++++++
>> >> >  2 files changed, 16 insertions(+)
>> >> >
>> >> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> >> > index 6df6fa1..a828a8c 100644
>> >> > --- a/fs/cifs/connect.c
>> >> > +++ b/fs/cifs/connect.c
>> >> > @@ -1676,6 +1676,10 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
>> >> >                         if (string == NULL)
>> >> >                                 goto out_nomem;
>> >> >
>> >> > +                       /*
>> >> > +                        * FIXME: since we now cork/uncork the socket while
>> >> > +                        *        sending, should we deprecate this option?
>> >> > +                        */
>> >> >                         if (strnicmp(string, "TCP_NODELAY", 11) == 0)
>> >> >                                 vol->sockopt_tcp_nodelay = 1;
>> >> >                         break;
>> >> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>> >> > index d93f15d..a3e58b2 100644
>> >> > --- a/fs/cifs/transport.c
>> >> > +++ b/fs/cifs/transport.c
>> >> > @@ -27,6 +27,7 @@
>> >> >  #include <linux/net.h>
>> >> >  #include <linux/delay.h>
>> >> >  #include <linux/freezer.h>
>> >> > +#include <linux/tcp.h>
>> >> >  #include <asm/uaccess.h>
>> >> >  #include <asm/processor.h>
>> >> >  #include <linux/mempool.h>
>> >> > @@ -247,12 +248,23 @@ smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
>> >> >         int n_vec = rqst->rq_nvec;
>> >> >         unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
>> >> >         size_t total_len;
>> >> > +       struct socket *ssocket = server->ssocket;
>> >> > +       int val = 1;
>> >> >
>> >> >         cFYI(1, "Sending smb: smb_len=%u", smb_buf_length);
>> >> >         dump_smb(iov[0].iov_base, iov[0].iov_len);
>> >> >
>> >> > +       /* cork the socket */
>> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
>> >> > +                               (char *)&val, sizeof(val));
>> >> > +
>> >> >         rc = smb_send_kvec(server, iov, n_vec, &total_len);
>> >> >
>> >> > +       /* uncork it */
>> >> > +       val = 0;
>> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
>> >> > +                               (char *)&val, sizeof(val));
>> >> > +
>> >> >         if ((total_len > 0) && (total_len != smb_buf_length + 4)) {
>> >> >                 cFYI(1, "partial send (wanted=%u sent=%zu): terminating "
>> >> >                         "session", smb_buf_length + 4, total_len);
>> >> > --
>> >> > 1.7.11.2
>> >> >
>> >> > --
>> >> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> >> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >>
>> >> I tested it with SMB2 against Windows 7 server. When iosize is 64K
>> >> everything is ok but when we increase iosize to 1M (by using
>> >> multicredit requests) and the server loses the network connection and
>> >> only reboot helps.
>> >>
>> >> Also if I commented corking/uncorking the socket - everything is ok. I
>> >> think this change needs some more investigation (how does it deals
>> >> with 1M iosize on Samba, etc?)
>> >>
>> >
>> > Hmm, haven't seen that with a 1M iosize with smb1 against samba.
>> >
>> > I'll see if I can reproduce it.
>> >
>> > --
>> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>
>> Forgot to mentioned how I reproduce it - dbench with 5 clients.
>>
>
> Ok, I've built a cifs.ko from your smb2-dev-cifs-3.6 branch. Here are my mount options:
>
> //win7.poochiereds.net/export /mnt/win7 cifs rw,relatime,vers=2.1,sec=ntlmsspi,cache=strict,unc=\\win7.poochiereds.net\export,username=testuser,domain=WIN7,uid=0,noforceuid,gid=0,noforcegid,addr=192.168.1.32,file_mode=0755,dir_mode=0755,nounix,serverino,rsize=1048576,wsize=1048576,actimeo=1 0 0

I don't set rsize and wsize explicitly but I don't think it's related.
On what connection did you test it? I use 100Mbit LAN.

>
> I'm running dbench against it with 5 clients, but am seeing no hangs. Do I need to do something else to reproduce it?
>
> Note that I am seeing a number of these sorts of warning messages:
>
> [84306.348564] CIFS VFS: No task to wake, unknown frame received! NumMids 4
> [84306.353262] Received Data is: : dump of 68 bytes of data at 0xffff88000172aab0
> [84306.353266]  6c000000 424d53fe 00000040 00000000 . . . l \xfffffffe S M B @ . . . . . . .
> [84306.353269]  00000012 00000001 00000000 ffffffff . . . . . . . . . . . . \xffffffff \xffffffff \xffffffff \xffffffff
> [84306.353271]  ffffffff 00000000 00000000 00000000 \xffffffff \xffffffff \xffffffff \xffffffff . . . . . . . . . . . .
> [84306.353273]  00000000 00000000 00000000 00000000 . . . . . . . . . . . . . . . .
> [84306.353275]  00000000 . . . .
> [84307.761162] CIFS VFS: No task to wake, unknown frame received! NumMids 5
> [84307.764904] Received Data is: : dump of 68 bytes of data at 0xffff88000172aab0
> [84307.764908]  6c000000 424d53fe 00000040 00000000 . . . l \xfffffffe S M B @ . . . . . . .
> [84307.764911]  00000012 00000001 00000000 ffffffff . . . . . . . . . . . . \xffffffff \xffffffff \xffffffff \xffffffff
> [84307.764914]  ffffffff 00000000 00000000 00000000 \xffffffff \xffffffff \xffffffff \xffffffff . . . . . . . . . . . .
> [84307.764916]  00000000 00000000 00000000 00000000 . . . . . . . . . . . . . . . .
> [84307.764917]  00000000 . . . .
> [84311.566786] CIFS VFS: No task to wake, unknown frame received! NumMids 5
> [84311.570630] Received Data is: : dump of 68 bytes of data at 0xffff8800017a38f0
> [84311.570634]  6c000000 424d53fe 00000040 00000000 . . . l \xfffffffe S M B @ . . . . . . .
> [84311.570637]  00000012 00000001 00000000 ffffffff . . . . . . . . . . . . \xffffffff \xffffffff \xffffffff \xffffffff
> [84311.570639]  ffffffff 00000000 00000000 00000000 \xffffffff \xffffffff \xffffffff \xffffffff . . . . . . . . . . . .
> [84311.570642]  00000000 00000000 00000000 00000000 . . . . . . . . . . . . . . . .
> [84311.570643]  00000000 . . . .

That's why the lease break issue that I mentioned later has been fixed yet.


>
> I get the same result whether I use TCP_CORK or not, so I don't think
> that's related.
>
> Also, after running dbench on a vers=2.1 mount and unplugging the kmod,
> I saw a bunch of these sorts of warnings, indicating memory leaks in
> the SMB2 code. Those may be related to the warnings above:
>
> [84980.135644] =============================================================================
> [84980.146094] BUG cifs_small_rq (Tainted: G           O): Objects remaining on kmem_cache_close()
> [84980.156757] -----------------------------------------------------------------------------
> [84980.156757]
> [84980.177277] INFO: Slab 0xffffea00003e3d00 objects=19 used=17 fp=0xffff88000f8f6700 flags=0x10000000004080
> [84980.188416] Pid: 20224, comm: rmmod Tainted: G           O 3.6.0-0.rc0.git2.1.fc18.x86_64 #1
> [84980.199692] Call Trace:
> [84980.210231]  [<ffffffff811ac61f>] slab_err+0xaf/0xd0
> [84980.221112]  [<ffffffff811b1329>] ? kmem_cache_destroy+0x249/0x3d0
> [84980.232334]  [<ffffffff811b124b>] ? kmem_cache_destroy+0x16b/0x3d0
> [84980.243424]  [<ffffffff811b126f>] kmem_cache_destroy+0x18f/0x3d0
> [84980.254653]  [<ffffffff8115dff5>] ? mempool_destroy+0x55/0x60
> [84980.265816]  [<ffffffffa0357d29>] cifs_destroy_request_bufs+0x39/0x3b [cifs]
> [84980.277350]  [<ffffffffa0357f4f>] exit_cifs+0x30/0xe1 [cifs]
> [84980.287385]  [<ffffffff810e1914>] sys_delete_module+0x1a4/0x300
> [84980.298287]  [<ffffffff816d0795>] ? retint_swapgs+0x13/0x1b
> [84980.309234]  [<ffffffff811008bc>] ? __audit_syscall_entry+0xcc/0x300
> [84980.320486]  [<ffffffff8134879e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> [84980.331739]  [<ffffffff816d9269>] system_call_fastpath+0x16/0x1b
> [84980.342929] INFO: Object 0xffff88000f8f4000 @offset=0
> [84980.354043] INFO: Allocated in mempool_alloc_slab+0x15/0x20 age=173263 cpu=0 pid=19880
> [84980.365669]  __slab_alloc+0x422/0x4d2
> [84980.376882]  kmem_cache_alloc+0x227/0x260
> [84980.387943]  mempool_alloc_slab+0x15/0x20
> [84980.398932]  mempool_alloc+0x68/0x180
> [84980.409688]  cifs_small_buf_get+0x1a/0x30 [cifs]
> [84980.420650]  cifs_demultiplex_thread+0x405/0x950 [cifs]
> [84980.431773]  kthread+0xb7/0xc0
> [84980.442675]  kernel_thread_helper+0x4/0x10
>
> That probably needs to be investigated (and fixed) as well...

I have not seen something like this. Do you mean you load module,
mount, run dbench with 5 clients, umount share and unload module and
this errors appeared?

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH v2 3/9] cifs: cork the socket before a send and uncork it afterward

[Jeff Layton]

On Mon, 30 Jul 2012 23:11:10 +0200
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2012/7/29 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > On Fri, 27 Jul 2012 10:05:32 +0400
> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> 2012/7/27 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >> > On Fri, 27 Jul 2012 03:57:44 +0400
> >> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> >
> >> >> 2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >> >> > We want to send SMBs as "atomically" as possible. Prior to sending any
> >> >> > data on the socket, cork it to make sure that no non-full frames go
> >> >> > out. Afterward, uncork it to make sure all of the data gets pushed out
> >> >> > to the wire.
> >> >> >
> >> >> > Note that this more or less renders the socket=TCP_NODELAY mount option
> >> >> > obsolete. When TCP_CORK and TCP_NODELAY are used on the same socket,
> >> >> > TCP_NODELAY is essentially ignored.
> >> >> >
> >> >> > Acked-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> >> >> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> >> > ---
> >> >> >  fs/cifs/connect.c   |  4 ++++
> >> >> >  fs/cifs/transport.c | 12 ++++++++++++
> >> >> >  2 files changed, 16 insertions(+)
> >> >> >
> >> >> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >> >> > index 6df6fa1..a828a8c 100644
> >> >> > --- a/fs/cifs/connect.c
> >> >> > +++ b/fs/cifs/connect.c
> >> >> > @@ -1676,6 +1676,10 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
> >> >> >                         if (string == NULL)
> >> >> >                                 goto out_nomem;
> >> >> >
> >> >> > +                       /*
> >> >> > +                        * FIXME: since we now cork/uncork the socket while
> >> >> > +                        *        sending, should we deprecate this option?
> >> >> > +                        */
> >> >> >                         if (strnicmp(string, "TCP_NODELAY", 11) == 0)
> >> >> >                                 vol->sockopt_tcp_nodelay = 1;
> >> >> >                         break;
> >> >> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> >> >> > index d93f15d..a3e58b2 100644
> >> >> > --- a/fs/cifs/transport.c
> >> >> > +++ b/fs/cifs/transport.c
> >> >> > @@ -27,6 +27,7 @@
> >> >> >  #include <linux/net.h>
> >> >> >  #include <linux/delay.h>
> >> >> >  #include <linux/freezer.h>
> >> >> > +#include <linux/tcp.h>
> >> >> >  #include <asm/uaccess.h>
> >> >> >  #include <asm/processor.h>
> >> >> >  #include <linux/mempool.h>
> >> >> > @@ -247,12 +248,23 @@ smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
> >> >> >         int n_vec = rqst->rq_nvec;
> >> >> >         unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
> >> >> >         size_t total_len;
> >> >> > +       struct socket *ssocket = server->ssocket;
> >> >> > +       int val = 1;
> >> >> >
> >> >> >         cFYI(1, "Sending smb: smb_len=%u", smb_buf_length);
> >> >> >         dump_smb(iov[0].iov_base, iov[0].iov_len);
> >> >> >
> >> >> > +       /* cork the socket */
> >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
> >> >> > +                               (char *)&val, sizeof(val));
> >> >> > +
> >> >> >         rc = smb_send_kvec(server, iov, n_vec, &total_len);
> >> >> >
> >> >> > +       /* uncork it */
> >> >> > +       val = 0;
> >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
> >> >> > +                               (char *)&val, sizeof(val));
> >> >> > +
> >> >> >         if ((total_len > 0) && (total_len != smb_buf_length + 4)) {
> >> >> >                 cFYI(1, "partial send (wanted=%u sent=%zu): terminating "
> >> >> >                         "session", smb_buf_length + 4, total_len);
> >> >> > --
> >> >> > 1.7.11.2
> >> >> >
> >> >> > --
> >> >> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> >> >> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> >>
> >> >> I tested it with SMB2 against Windows 7 server. When iosize is 64K
> >> >> everything is ok but when we increase iosize to 1M (by using
> >> >> multicredit requests) and the server loses the network connection and
> >> >> only reboot helps.
> >> >>
> >> >> Also if I commented corking/uncorking the socket - everything is ok. I
> >> >> think this change needs some more investigation (how does it deals
> >> >> with 1M iosize on Samba, etc?)
> >> >>
> >> >
> >> > Hmm, haven't seen that with a 1M iosize with smb1 against samba.
> >> >
> >> > I'll see if I can reproduce it.
> >> >
> >> > --
> >> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>
> >> Forgot to mentioned how I reproduce it - dbench with 5 clients.
> >>
> >
> > Ok, I've built a cifs.ko from your smb2-dev-cifs-3.6 branch. Here are my mount options:
> >
> > //win7.poochiereds.net/export /mnt/win7 cifs rw,relatime,vers=2.1,sec=ntlmsspi,cache=strict,unc=\\win7.poochiereds.net\export,username=testuser,domain=WIN7,uid=0,noforceuid,gid=0,noforcegid,addr=192.168.1.32,file_mode=0755,dir_mode=0755,nounix,serverino,rsize=1048576,wsize=1048576,actimeo=1 0 0
> 
> I don't set rsize and wsize explicitly but I don't think it's related.
> On what connection did you test it? I use 100Mbit LAN.
> 

The clients and servers are both KVM guests. I'll give it a go over a
physical network tomorrow.

> >
> > I'm running dbench against it with 5 clients, but am seeing no hangs. Do I need to do something else to reproduce it?
> >
> > Note that I am seeing a number of these sorts of warning messages:
> >
> > [84306.348564] CIFS VFS: No task to wake, unknown frame received! NumMids 4
> > [84306.353262] Received Data is: : dump of 68 bytes of data at 0xffff88000172aab0
> > [84306.353266]  6c000000 424d53fe 00000040 00000000 . . . l \xfffffffe S M B @ . . . . . . .
> > [84306.353269]  00000012 00000001 00000000 ffffffff . . . . . . . . . . . . \xffffffff \xffffffff \xffffffff \xffffffff
> > [84306.353271]  ffffffff 00000000 00000000 00000000 \xffffffff \xffffffff \xffffffff \xffffffff . . . . . . . . . . . .
> > [84306.353273]  00000000 00000000 00000000 00000000 . . . . . . . . . . . . . . . .
> > [84306.353275]  00000000 . . . .
> > [84307.761162] CIFS VFS: No task to wake, unknown frame received! NumMids 5
> > [84307.764904] Received Data is: : dump of 68 bytes of data at 0xffff88000172aab0
> > [84307.764908]  6c000000 424d53fe 00000040 00000000 . . . l \xfffffffe S M B @ . . . . . . .
> > [84307.764911]  00000012 00000001 00000000 ffffffff . . . . . . . . . . . . \xffffffff \xffffffff \xffffffff \xffffffff
> > [84307.764914]  ffffffff 00000000 00000000 00000000 \xffffffff \xffffffff \xffffffff \xffffffff . . . . . . . . . . . .
> > [84307.764916]  00000000 00000000 00000000 00000000 . . . . . . . . . . . . . . . .
> > [84307.764917]  00000000 . . . .
> > [84311.566786] CIFS VFS: No task to wake, unknown frame received! NumMids 5
> > [84311.570630] Received Data is: : dump of 68 bytes of data at 0xffff8800017a38f0
> > [84311.570634]  6c000000 424d53fe 00000040 00000000 . . . l \xfffffffe S M B @ . . . . . . .
> > [84311.570637]  00000012 00000001 00000000 ffffffff . . . . . . . . . . . . \xffffffff \xffffffff \xffffffff \xffffffff
> > [84311.570639]  ffffffff 00000000 00000000 00000000 \xffffffff \xffffffff \xffffffff \xffffffff . . . . . . . . . . . .
> > [84311.570642]  00000000 00000000 00000000 00000000 . . . . . . . . . . . . . . . .
> > [84311.570643]  00000000 . . . .
> 
> That's why the lease break issue that I mentioned later has been fixed yet.
> 
> 
> >
> > I get the same result whether I use TCP_CORK or not, so I don't think
> > that's related.
> >
> > Also, after running dbench on a vers=2.1 mount and unplugging the kmod,
> > I saw a bunch of these sorts of warnings, indicating memory leaks in
> > the SMB2 code. Those may be related to the warnings above:
> >
> > [84980.135644] =============================================================================
> > [84980.146094] BUG cifs_small_rq (Tainted: G           O): Objects remaining on kmem_cache_close()
> > [84980.156757] -----------------------------------------------------------------------------
> > [84980.156757]
> > [84980.177277] INFO: Slab 0xffffea00003e3d00 objects=19 used=17 fp=0xffff88000f8f6700 flags=0x10000000004080
> > [84980.188416] Pid: 20224, comm: rmmod Tainted: G           O 3.6.0-0.rc0.git2.1.fc18.x86_64 #1
> > [84980.199692] Call Trace:
> > [84980.210231]  [<ffffffff811ac61f>] slab_err+0xaf/0xd0
> > [84980.221112]  [<ffffffff811b1329>] ? kmem_cache_destroy+0x249/0x3d0
> > [84980.232334]  [<ffffffff811b124b>] ? kmem_cache_destroy+0x16b/0x3d0
> > [84980.243424]  [<ffffffff811b126f>] kmem_cache_destroy+0x18f/0x3d0
> > [84980.254653]  [<ffffffff8115dff5>] ? mempool_destroy+0x55/0x60
> > [84980.265816]  [<ffffffffa0357d29>] cifs_destroy_request_bufs+0x39/0x3b [cifs]
> > [84980.277350]  [<ffffffffa0357f4f>] exit_cifs+0x30/0xe1 [cifs]
> > [84980.287385]  [<ffffffff810e1914>] sys_delete_module+0x1a4/0x300
> > [84980.298287]  [<ffffffff816d0795>] ? retint_swapgs+0x13/0x1b
> > [84980.309234]  [<ffffffff811008bc>] ? __audit_syscall_entry+0xcc/0x300
> > [84980.320486]  [<ffffffff8134879e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> > [84980.331739]  [<ffffffff816d9269>] system_call_fastpath+0x16/0x1b
> > [84980.342929] INFO: Object 0xffff88000f8f4000 @offset=0
> > [84980.354043] INFO: Allocated in mempool_alloc_slab+0x15/0x20 age=173263 cpu=0 pid=19880
> > [84980.365669]  __slab_alloc+0x422/0x4d2
> > [84980.376882]  kmem_cache_alloc+0x227/0x260
> > [84980.387943]  mempool_alloc_slab+0x15/0x20
> > [84980.398932]  mempool_alloc+0x68/0x180
> > [84980.409688]  cifs_small_buf_get+0x1a/0x30 [cifs]
> > [84980.420650]  cifs_demultiplex_thread+0x405/0x950 [cifs]
> > [84980.431773]  kthread+0xb7/0xc0
> > [84980.442675]  kernel_thread_helper+0x4/0x10
> >
> > That probably needs to be investigated (and fixed) as well...
> 
> I have not seen something like this. Do you mean you load module,
> mount, run dbench with 5 clients, umount share and unload module and
> this errors appeared?
> 

Yes. You may need a kernel with slab debugging enabled.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH v2 3/9] cifs: cork the socket before a send and uncork it afterward

[Jeff Layton]

On Mon, 30 Jul 2012 21:17:53 -0400
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> On Mon, 30 Jul 2012 23:11:10 +0200
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > 2012/7/29 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > > On Fri, 27 Jul 2012 10:05:32 +0400
> > > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > >
> > >> 2012/7/27 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > >> > On Fri, 27 Jul 2012 03:57:44 +0400
> > >> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > >> >
> > >> >> 2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > >> >> > We want to send SMBs as "atomically" as possible. Prior to sending any
> > >> >> > data on the socket, cork it to make sure that no non-full frames go
> > >> >> > out. Afterward, uncork it to make sure all of the data gets pushed out
> > >> >> > to the wire.
> > >> >> >
> > >> >> > Note that this more or less renders the socket=TCP_NODELAY mount option
> > >> >> > obsolete. When TCP_CORK and TCP_NODELAY are used on the same socket,
> > >> >> > TCP_NODELAY is essentially ignored.
> > >> >> >
> > >> >> > Acked-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> > >> >> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > >> >> > ---
> > >> >> >  fs/cifs/connect.c   |  4 ++++
> > >> >> >  fs/cifs/transport.c | 12 ++++++++++++
> > >> >> >  2 files changed, 16 insertions(+)
> > >> >> >
> > >> >> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > >> >> > index 6df6fa1..a828a8c 100644
> > >> >> > --- a/fs/cifs/connect.c
> > >> >> > +++ b/fs/cifs/connect.c
> > >> >> > @@ -1676,6 +1676,10 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
> > >> >> >                         if (string == NULL)
> > >> >> >                                 goto out_nomem;
> > >> >> >
> > >> >> > +                       /*
> > >> >> > +                        * FIXME: since we now cork/uncork the socket while
> > >> >> > +                        *        sending, should we deprecate this option?
> > >> >> > +                        */
> > >> >> >                         if (strnicmp(string, "TCP_NODELAY", 11) == 0)
> > >> >> >                                 vol->sockopt_tcp_nodelay = 1;
> > >> >> >                         break;
> > >> >> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > >> >> > index d93f15d..a3e58b2 100644
> > >> >> > --- a/fs/cifs/transport.c
> > >> >> > +++ b/fs/cifs/transport.c
> > >> >> > @@ -27,6 +27,7 @@
> > >> >> >  #include <linux/net.h>
> > >> >> >  #include <linux/delay.h>
> > >> >> >  #include <linux/freezer.h>
> > >> >> > +#include <linux/tcp.h>
> > >> >> >  #include <asm/uaccess.h>
> > >> >> >  #include <asm/processor.h>
> > >> >> >  #include <linux/mempool.h>
> > >> >> > @@ -247,12 +248,23 @@ smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
> > >> >> >         int n_vec = rqst->rq_nvec;
> > >> >> >         unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
> > >> >> >         size_t total_len;
> > >> >> > +       struct socket *ssocket = server->ssocket;
> > >> >> > +       int val = 1;
> > >> >> >
> > >> >> >         cFYI(1, "Sending smb: smb_len=%u", smb_buf_length);
> > >> >> >         dump_smb(iov[0].iov_base, iov[0].iov_len);
> > >> >> >
> > >> >> > +       /* cork the socket */
> > >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
> > >> >> > +                               (char *)&val, sizeof(val));
> > >> >> > +
> > >> >> >         rc = smb_send_kvec(server, iov, n_vec, &total_len);
> > >> >> >
> > >> >> > +       /* uncork it */
> > >> >> > +       val = 0;
> > >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
> > >> >> > +                               (char *)&val, sizeof(val));
> > >> >> > +
> > >> >> >         if ((total_len > 0) && (total_len != smb_buf_length + 4)) {
> > >> >> >                 cFYI(1, "partial send (wanted=%u sent=%zu): terminating "
> > >> >> >                         "session", smb_buf_length + 4, total_len);
> > >> >> > --
> > >> >> > 1.7.11.2
> > >> >> >
> > >> >> > --
> > >> >> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > >> >> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > >> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > >> >>
> > >> >> I tested it with SMB2 against Windows 7 server. When iosize is 64K
> > >> >> everything is ok but when we increase iosize to 1M (by using
> > >> >> multicredit requests) and the server loses the network connection and
> > >> >> only reboot helps.
> > >> >>
> > >> >> Also if I commented corking/uncorking the socket - everything is ok. I
> > >> >> think this change needs some more investigation (how does it deals
> > >> >> with 1M iosize on Samba, etc?)
> > >> >>
> > >> >
> > >> > Hmm, haven't seen that with a 1M iosize with smb1 against samba.
> > >> >
> > >> > I'll see if I can reproduce it.
> > >> >
> > >> > --
> > >> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > >>
> > >> Forgot to mentioned how I reproduce it - dbench with 5 clients.
> > >>
> > >
> > > Ok, I've built a cifs.ko from your smb2-dev-cifs-3.6 branch. Here are my mount options:
> > >
> > > //win7.poochiereds.net/export /mnt/win7 cifs rw,relatime,vers=2.1,sec=ntlmsspi,cache=strict,unc=\\win7.poochiereds.net\export,username=testuser,domain=WIN7,uid=0,noforceuid,gid=0,noforcegid,addr=192.168.1.32,file_mode=0755,dir_mode=0755,nounix,serverino,rsize=1048576,wsize=1048576,actimeo=1 0 0
> > 
> > I don't set rsize and wsize explicitly but I don't think it's related.
> > On what connection did you test it? I use 100Mbit LAN.
> > 
> 
> The clients and servers are both KVM guests. I'll give it a go over a
> physical network tomorrow.
> 

Ok, ran it with client as a KVM guest and the server as win7 running on
bare metal over a gigE network. It still ran just fine.

So what are the symptoms that you see here? Does dbench just hang? If
so, could you collect /proc/<pid>/stack from the hung process(es)?
Maybe that would tell us what's going on...

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH v2 3/9] cifs: cork the socket before a send and uncork it afterward

[Pavel Shilovsky]

2012/7/31 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> On Mon, 30 Jul 2012 21:17:53 -0400
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
>> On Mon, 30 Jul 2012 23:11:10 +0200
>> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>> > 2012/7/29 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> > > On Fri, 27 Jul 2012 10:05:32 +0400
>> > > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > >
>> > >> 2012/7/27 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> > >> > On Fri, 27 Jul 2012 03:57:44 +0400
>> > >> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > >> >
>> > >> >> 2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> > >> >> > We want to send SMBs as "atomically" as possible. Prior to sending any
>> > >> >> > data on the socket, cork it to make sure that no non-full frames go
>> > >> >> > out. Afterward, uncork it to make sure all of the data gets pushed out
>> > >> >> > to the wire.
>> > >> >> >
>> > >> >> > Note that this more or less renders the socket=TCP_NODELAY mount option
>> > >> >> > obsolete. When TCP_CORK and TCP_NODELAY are used on the same socket,
>> > >> >> > TCP_NODELAY is essentially ignored.
>> > >> >> >
>> > >> >> > Acked-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>> > >> >> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > >> >> > ---
>> > >> >> >  fs/cifs/connect.c   |  4 ++++
>> > >> >> >  fs/cifs/transport.c | 12 ++++++++++++
>> > >> >> >  2 files changed, 16 insertions(+)
>> > >> >> >
>> > >> >> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> > >> >> > index 6df6fa1..a828a8c 100644
>> > >> >> > --- a/fs/cifs/connect.c
>> > >> >> > +++ b/fs/cifs/connect.c
>> > >> >> > @@ -1676,6 +1676,10 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
>> > >> >> >                         if (string == NULL)
>> > >> >> >                                 goto out_nomem;
>> > >> >> >
>> > >> >> > +                       /*
>> > >> >> > +                        * FIXME: since we now cork/uncork the socket while
>> > >> >> > +                        *        sending, should we deprecate this option?
>> > >> >> > +                        */
>> > >> >> >                         if (strnicmp(string, "TCP_NODELAY", 11) == 0)
>> > >> >> >                                 vol->sockopt_tcp_nodelay = 1;
>> > >> >> >                         break;
>> > >> >> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>> > >> >> > index d93f15d..a3e58b2 100644
>> > >> >> > --- a/fs/cifs/transport.c
>> > >> >> > +++ b/fs/cifs/transport.c
>> > >> >> > @@ -27,6 +27,7 @@
>> > >> >> >  #include <linux/net.h>
>> > >> >> >  #include <linux/delay.h>
>> > >> >> >  #include <linux/freezer.h>
>> > >> >> > +#include <linux/tcp.h>
>> > >> >> >  #include <asm/uaccess.h>
>> > >> >> >  #include <asm/processor.h>
>> > >> >> >  #include <linux/mempool.h>
>> > >> >> > @@ -247,12 +248,23 @@ smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
>> > >> >> >         int n_vec = rqst->rq_nvec;
>> > >> >> >         unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
>> > >> >> >         size_t total_len;
>> > >> >> > +       struct socket *ssocket = server->ssocket;
>> > >> >> > +       int val = 1;
>> > >> >> >
>> > >> >> >         cFYI(1, "Sending smb: smb_len=%u", smb_buf_length);
>> > >> >> >         dump_smb(iov[0].iov_base, iov[0].iov_len);
>> > >> >> >
>> > >> >> > +       /* cork the socket */
>> > >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
>> > >> >> > +                               (char *)&val, sizeof(val));
>> > >> >> > +
>> > >> >> >         rc = smb_send_kvec(server, iov, n_vec, &total_len);
>> > >> >> >
>> > >> >> > +       /* uncork it */
>> > >> >> > +       val = 0;
>> > >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
>> > >> >> > +                               (char *)&val, sizeof(val));
>> > >> >> > +
>> > >> >> >         if ((total_len > 0) && (total_len != smb_buf_length + 4)) {
>> > >> >> >                 cFYI(1, "partial send (wanted=%u sent=%zu): terminating "
>> > >> >> >                         "session", smb_buf_length + 4, total_len);
>> > >> >> > --
>> > >> >> > 1.7.11.2
>> > >> >> >
>> > >> >> > --
>> > >> >> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> > >> >> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> > >> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> > >> >>
>> > >> >> I tested it with SMB2 against Windows 7 server. When iosize is 64K
>> > >> >> everything is ok but when we increase iosize to 1M (by using
>> > >> >> multicredit requests) and the server loses the network connection and
>> > >> >> only reboot helps.
>> > >> >>
>> > >> >> Also if I commented corking/uncorking the socket - everything is ok. I
>> > >> >> think this change needs some more investigation (how does it deals
>> > >> >> with 1M iosize on Samba, etc?)
>> > >> >>
>> > >> >
>> > >> > Hmm, haven't seen that with a 1M iosize with smb1 against samba.
>> > >> >
>> > >> > I'll see if I can reproduce it.
>> > >> >
>> > >> > --
>> > >> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > >>
>> > >> Forgot to mentioned how I reproduce it - dbench with 5 clients.
>> > >>
>> > >
>> > > Ok, I've built a cifs.ko from your smb2-dev-cifs-3.6 branch. Here are my mount options:
>> > >
>> > > //win7.poochiereds.net/export /mnt/win7 cifs rw,relatime,vers=2.1,sec=ntlmsspi,cache=strict,unc=\\win7.poochiereds.net\export,username=testuser,domain=WIN7,uid=0,noforceuid,gid=0,noforcegid,addr=192.168.1.32,file_mode=0755,dir_mode=0755,nounix,serverino,rsize=1048576,wsize=1048576,actimeo=1 0 0
>> >
>> > I don't set rsize and wsize explicitly but I don't think it's related.
>> > On what connection did you test it? I use 100Mbit LAN.
>> >
>>
>> The clients and servers are both KVM guests. I'll give it a go over a
>> physical network tomorrow.
>>
>
> Ok, ran it with client as a KVM guest and the server as win7 running on
> bare metal over a gigE network. It still ran just fine.
>
> So what are the symptoms that you see here? Does dbench just hang? If
> so, could you collect /proc/<pid>/stack from the hung process(es)?
> Maybe that would tell us what's going on...

Windows 7 server doesn't response to packets after some time running
dbench. Also, I even can't ping google.com from this Windows machine.
It seems that everything ok with dbench and Linux machine.

So, it looks like Windows server problem on my configuration but of
course seems very strage. I will try this patch with Samba server
further.

This patch doesn't break things with Windows untill we use multicredit
requests (more than 64K, that are not targeted to 3.6 kernel). But I
am going to target multicredit requests feature for 3.7. May be we
should make cork/nodelay switchable? Or just merge the patchset
without this patch for 3.6 and delay this patch for 3.7 - we will have
much time to investigate this strange behavior?

Thoughts?

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH v2 3/9] cifs: cork the socket before a send and uncork it afterward

[Jeff Layton]

On Wed, 1 Aug 2012 15:37:57 +0200
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2012/7/31 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> > On Mon, 30 Jul 2012 21:17:53 -0400
> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >
> >> On Mon, 30 Jul 2012 23:11:10 +0200
> >> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>
> >> > 2012/7/29 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >> > > On Fri, 27 Jul 2012 10:05:32 +0400
> >> > > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> > >
> >> > >> 2012/7/27 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >> > >> > On Fri, 27 Jul 2012 03:57:44 +0400
> >> > >> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> > >> >
> >> > >> >> 2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >> > >> >> > We want to send SMBs as "atomically" as possible. Prior to sending any
> >> > >> >> > data on the socket, cork it to make sure that no non-full frames go
> >> > >> >> > out. Afterward, uncork it to make sure all of the data gets pushed out
> >> > >> >> > to the wire.
> >> > >> >> >
> >> > >> >> > Note that this more or less renders the socket=TCP_NODELAY mount option
> >> > >> >> > obsolete. When TCP_CORK and TCP_NODELAY are used on the same socket,
> >> > >> >> > TCP_NODELAY is essentially ignored.
> >> > >> >> >
> >> > >> >> > Acked-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> >> > >> >> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> > >> >> > ---
> >> > >> >> >  fs/cifs/connect.c   |  4 ++++
> >> > >> >> >  fs/cifs/transport.c | 12 ++++++++++++
> >> > >> >> >  2 files changed, 16 insertions(+)
> >> > >> >> >
> >> > >> >> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >> > >> >> > index 6df6fa1..a828a8c 100644
> >> > >> >> > --- a/fs/cifs/connect.c
> >> > >> >> > +++ b/fs/cifs/connect.c
> >> > >> >> > @@ -1676,6 +1676,10 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
> >> > >> >> >                         if (string == NULL)
> >> > >> >> >                                 goto out_nomem;
> >> > >> >> >
> >> > >> >> > +                       /*
> >> > >> >> > +                        * FIXME: since we now cork/uncork the socket while
> >> > >> >> > +                        *        sending, should we deprecate this option?
> >> > >> >> > +                        */
> >> > >> >> >                         if (strnicmp(string, "TCP_NODELAY", 11) == 0)
> >> > >> >> >                                 vol->sockopt_tcp_nodelay = 1;
> >> > >> >> >                         break;
> >> > >> >> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> >> > >> >> > index d93f15d..a3e58b2 100644
> >> > >> >> > --- a/fs/cifs/transport.c
> >> > >> >> > +++ b/fs/cifs/transport.c
> >> > >> >> > @@ -27,6 +27,7 @@
> >> > >> >> >  #include <linux/net.h>
> >> > >> >> >  #include <linux/delay.h>
> >> > >> >> >  #include <linux/freezer.h>
> >> > >> >> > +#include <linux/tcp.h>
> >> > >> >> >  #include <asm/uaccess.h>
> >> > >> >> >  #include <asm/processor.h>
> >> > >> >> >  #include <linux/mempool.h>
> >> > >> >> > @@ -247,12 +248,23 @@ smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
> >> > >> >> >         int n_vec = rqst->rq_nvec;
> >> > >> >> >         unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
> >> > >> >> >         size_t total_len;
> >> > >> >> > +       struct socket *ssocket = server->ssocket;
> >> > >> >> > +       int val = 1;
> >> > >> >> >
> >> > >> >> >         cFYI(1, "Sending smb: smb_len=%u", smb_buf_length);
> >> > >> >> >         dump_smb(iov[0].iov_base, iov[0].iov_len);
> >> > >> >> >
> >> > >> >> > +       /* cork the socket */
> >> > >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
> >> > >> >> > +                               (char *)&val, sizeof(val));
> >> > >> >> > +
> >> > >> >> >         rc = smb_send_kvec(server, iov, n_vec, &total_len);
> >> > >> >> >
> >> > >> >> > +       /* uncork it */
> >> > >> >> > +       val = 0;
> >> > >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
> >> > >> >> > +                               (char *)&val, sizeof(val));
> >> > >> >> > +
> >> > >> >> >         if ((total_len > 0) && (total_len != smb_buf_length + 4)) {
> >> > >> >> >                 cFYI(1, "partial send (wanted=%u sent=%zu): terminating "
> >> > >> >> >                         "session", smb_buf_length + 4, total_len);
> >> > >> >> > --
> >> > >> >> > 1.7.11.2
> >> > >> >> >
> >> > >> >> > --
> >> > >> >> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> >> > >> >> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> > >> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> > >> >>
> >> > >> >> I tested it with SMB2 against Windows 7 server. When iosize is 64K
> >> > >> >> everything is ok but when we increase iosize to 1M (by using
> >> > >> >> multicredit requests) and the server loses the network connection and
> >> > >> >> only reboot helps.
> >> > >> >>
> >> > >> >> Also if I commented corking/uncorking the socket - everything is ok. I
> >> > >> >> think this change needs some more investigation (how does it deals
> >> > >> >> with 1M iosize on Samba, etc?)
> >> > >> >>
> >> > >> >
> >> > >> > Hmm, haven't seen that with a 1M iosize with smb1 against samba.
> >> > >> >
> >> > >> > I'll see if I can reproduce it.
> >> > >> >
> >> > >> > --
> >> > >> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> > >>
> >> > >> Forgot to mentioned how I reproduce it - dbench with 5 clients.
> >> > >>
> >> > >
> >> > > Ok, I've built a cifs.ko from your smb2-dev-cifs-3.6 branch. Here are my mount options:
> >> > >
> >> > > //win7.poochiereds.net/export /mnt/win7 cifs rw,relatime,vers=2.1,sec=ntlmsspi,cache=strict,unc=\\win7.poochiereds.net\export,username=testuser,domain=WIN7,uid=0,noforceuid,gid=0,noforcegid,addr=192.168.1.32,file_mode=0755,dir_mode=0755,nounix,serverino,rsize=1048576,wsize=1048576,actimeo=1 0 0
> >> >
> >> > I don't set rsize and wsize explicitly but I don't think it's related.
> >> > On what connection did you test it? I use 100Mbit LAN.
> >> >
> >>
> >> The clients and servers are both KVM guests. I'll give it a go over a
> >> physical network tomorrow.
> >>
> >
> > Ok, ran it with client as a KVM guest and the server as win7 running on
> > bare metal over a gigE network. It still ran just fine.
> >
> > So what are the symptoms that you see here? Does dbench just hang? If
> > so, could you collect /proc/<pid>/stack from the hung process(es)?
> > Maybe that would tell us what's going on...
> 
> Windows 7 server doesn't response to packets after some time running
> dbench. Also, I even can't ping google.com from this Windows machine.
> It seems that everything ok with dbench and Linux machine.
> 
> So, it looks like Windows server problem on my configuration but of
> course seems very strage. I will try this patch with Samba server
> further.
> 
> This patch doesn't break things with Windows untill we use multicredit
> requests (more than 64K, that are not targeted to 3.6 kernel). But I
> am going to target multicredit requests feature for 3.7. May be we
> should make cork/nodelay switchable? Or just merge the patchset
> without this patch for 3.6 and delay this patch for 3.7 - we will have
> much time to investigate this strange behavior?
> 
> Thoughts?
> 

If the server isn't responding then that seems like something is broken
on the server. Maybe you have a broken network driver? Do you have any
captures?

I don't think we should ship w/o the TCP_CORK code unless there is
clear evidence that it's a problem on the Linux end. We really don't
want the server sending half-baked SMBs, and that's much more likely to
occur if you don't cork the socket prior to sending.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH v2 3/9] cifs: cork the socket before a send and uncork it afterward

[Pavel Shilovsky]

2012/8/1 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> On Wed, 1 Aug 2012 15:37:57 +0200
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> 2012/7/31 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>> > On Mon, 30 Jul 2012 21:17:53 -0400
>> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> >
>> >> On Mon, 30 Jul 2012 23:11:10 +0200
>> >> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >>
>> >> > 2012/7/29 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> >> > > On Fri, 27 Jul 2012 10:05:32 +0400
>> >> > > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> > >
>> >> > >> 2012/7/27 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> >> > >> > On Fri, 27 Jul 2012 03:57:44 +0400
>> >> > >> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> > >> >
>> >> > >> >> 2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> >> > >> >> > We want to send SMBs as "atomically" as possible. Prior to sending any
>> >> > >> >> > data on the socket, cork it to make sure that no non-full frames go
>> >> > >> >> > out. Afterward, uncork it to make sure all of the data gets pushed out
>> >> > >> >> > to the wire.
>> >> > >> >> >
>> >> > >> >> > Note that this more or less renders the socket=TCP_NODELAY mount option
>> >> > >> >> > obsolete. When TCP_CORK and TCP_NODELAY are used on the same socket,
>> >> > >> >> > TCP_NODELAY is essentially ignored.
>> >> > >> >> >
>> >> > >> >> > Acked-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>> >> > >> >> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> >> > >> >> > ---
>> >> > >> >> >  fs/cifs/connect.c   |  4 ++++
>> >> > >> >> >  fs/cifs/transport.c | 12 ++++++++++++
>> >> > >> >> >  2 files changed, 16 insertions(+)
>> >> > >> >> >
>> >> > >> >> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> >> > >> >> > index 6df6fa1..a828a8c 100644
>> >> > >> >> > --- a/fs/cifs/connect.c
>> >> > >> >> > +++ b/fs/cifs/connect.c
>> >> > >> >> > @@ -1676,6 +1676,10 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
>> >> > >> >> >                         if (string == NULL)
>> >> > >> >> >                                 goto out_nomem;
>> >> > >> >> >
>> >> > >> >> > +                       /*
>> >> > >> >> > +                        * FIXME: since we now cork/uncork the socket while
>> >> > >> >> > +                        *        sending, should we deprecate this option?
>> >> > >> >> > +                        */
>> >> > >> >> >                         if (strnicmp(string, "TCP_NODELAY", 11) == 0)
>> >> > >> >> >                                 vol->sockopt_tcp_nodelay = 1;
>> >> > >> >> >                         break;
>> >> > >> >> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>> >> > >> >> > index d93f15d..a3e58b2 100644
>> >> > >> >> > --- a/fs/cifs/transport.c
>> >> > >> >> > +++ b/fs/cifs/transport.c
>> >> > >> >> > @@ -27,6 +27,7 @@
>> >> > >> >> >  #include <linux/net.h>
>> >> > >> >> >  #include <linux/delay.h>
>> >> > >> >> >  #include <linux/freezer.h>
>> >> > >> >> > +#include <linux/tcp.h>
>> >> > >> >> >  #include <asm/uaccess.h>
>> >> > >> >> >  #include <asm/processor.h>
>> >> > >> >> >  #include <linux/mempool.h>
>> >> > >> >> > @@ -247,12 +248,23 @@ smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
>> >> > >> >> >         int n_vec = rqst->rq_nvec;
>> >> > >> >> >         unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
>> >> > >> >> >         size_t total_len;
>> >> > >> >> > +       struct socket *ssocket = server->ssocket;
>> >> > >> >> > +       int val = 1;
>> >> > >> >> >
>> >> > >> >> >         cFYI(1, "Sending smb: smb_len=%u", smb_buf_length);
>> >> > >> >> >         dump_smb(iov[0].iov_base, iov[0].iov_len);
>> >> > >> >> >
>> >> > >> >> > +       /* cork the socket */
>> >> > >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
>> >> > >> >> > +                               (char *)&val, sizeof(val));
>> >> > >> >> > +
>> >> > >> >> >         rc = smb_send_kvec(server, iov, n_vec, &total_len);
>> >> > >> >> >
>> >> > >> >> > +       /* uncork it */
>> >> > >> >> > +       val = 0;
>> >> > >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
>> >> > >> >> > +                               (char *)&val, sizeof(val));
>> >> > >> >> > +
>> >> > >> >> >         if ((total_len > 0) && (total_len != smb_buf_length + 4)) {
>> >> > >> >> >                 cFYI(1, "partial send (wanted=%u sent=%zu): terminating "
>> >> > >> >> >                         "session", smb_buf_length + 4, total_len);
>> >> > >> >> > --
>> >> > >> >> > 1.7.11.2
>> >> > >> >> >
>> >> > >> >> > --
>> >> > >> >> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> >> > >> >> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> >> > >> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >> > >> >>
>> >> > >> >> I tested it with SMB2 against Windows 7 server. When iosize is 64K
>> >> > >> >> everything is ok but when we increase iosize to 1M (by using
>> >> > >> >> multicredit requests) and the server loses the network connection and
>> >> > >> >> only reboot helps.
>> >> > >> >>
>> >> > >> >> Also if I commented corking/uncorking the socket - everything is ok. I
>> >> > >> >> think this change needs some more investigation (how does it deals
>> >> > >> >> with 1M iosize on Samba, etc?)
>> >> > >> >>
>> >> > >> >
>> >> > >> > Hmm, haven't seen that with a 1M iosize with smb1 against samba.
>> >> > >> >
>> >> > >> > I'll see if I can reproduce it.
>> >> > >> >
>> >> > >> > --
>> >> > >> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> >> > >>
>> >> > >> Forgot to mentioned how I reproduce it - dbench with 5 clients.
>> >> > >>
>> >> > >
>> >> > > Ok, I've built a cifs.ko from your smb2-dev-cifs-3.6 branch. Here are my mount options:
>> >> > >
>> >> > > //win7.poochiereds.net/export /mnt/win7 cifs rw,relatime,vers=2.1,sec=ntlmsspi,cache=strict,unc=\\win7.poochiereds.net\export,username=testuser,domain=WIN7,uid=0,noforceuid,gid=0,noforcegid,addr=192.168.1.32,file_mode=0755,dir_mode=0755,nounix,serverino,rsize=1048576,wsize=1048576,actimeo=1 0 0
>> >> >
>> >> > I don't set rsize and wsize explicitly but I don't think it's related.
>> >> > On what connection did you test it? I use 100Mbit LAN.
>> >> >
>> >>
>> >> The clients and servers are both KVM guests. I'll give it a go over a
>> >> physical network tomorrow.
>> >>
>> >
>> > Ok, ran it with client as a KVM guest and the server as win7 running on
>> > bare metal over a gigE network. It still ran just fine.
>> >
>> > So what are the symptoms that you see here? Does dbench just hang? If
>> > so, could you collect /proc/<pid>/stack from the hung process(es)?
>> > Maybe that would tell us what's going on...
>>
>> Windows 7 server doesn't response to packets after some time running
>> dbench. Also, I even can't ping google.com from this Windows machine.
>> It seems that everything ok with dbench and Linux machine.
>>
>> So, it looks like Windows server problem on my configuration but of
>> course seems very strage. I will try this patch with Samba server
>> further.
>>
>> This patch doesn't break things with Windows untill we use multicredit
>> requests (more than 64K, that are not targeted to 3.6 kernel). But I
>> am going to target multicredit requests feature for 3.7. May be we
>> should make cork/nodelay switchable? Or just merge the patchset
>> without this patch for 3.6 and delay this patch for 3.7 - we will have
>> much time to investigate this strange behavior?
>>
>> Thoughts?
>>
>
> If the server isn't responding then that seems like something is broken
> on the server. Maybe you have a broken network driver? Do you have any
> captures?

Now I don't have them but I can collect captures when return from vacations.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH v2 3/9] cifs: cork the socket before a send and uncork it afterward

[Steve French]

I suspect that there is a relationship between this and the queueing
problems we saw with Windows Vista and Windows 7 depending on max
multiplex request size (for cifs).

On Sat, Aug 4, 2012 at 3:51 PM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2012/8/1 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>> On Wed, 1 Aug 2012 15:37:57 +0200
>> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>>> 2012/7/31 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>>> > On Mon, 30 Jul 2012 21:17:53 -0400
>>> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>> >
>>> >> On Mon, 30 Jul 2012 23:11:10 +0200
>>> >> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> >>
>>> >> > 2012/7/29 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>> >> > > On Fri, 27 Jul 2012 10:05:32 +0400
>>> >> > > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> >> > >
>>> >> > >> 2012/7/27 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>> >> > >> > On Fri, 27 Jul 2012 03:57:44 +0400
>>> >> > >> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> >> > >> >
>>> >> > >> >> 2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>> >> > >> >> > We want to send SMBs as "atomically" as possible. Prior to sending any
>>> >> > >> >> > data on the socket, cork it to make sure that no non-full frames go
>>> >> > >> >> > out. Afterward, uncork it to make sure all of the data gets pushed out
>>> >> > >> >> > to the wire.
>>> >> > >> >> >
>>> >> > >> >> > Note that this more or less renders the socket=TCP_NODELAY mount option
>>> >> > >> >> > obsolete. When TCP_CORK and TCP_NODELAY are used on the same socket,
>>> >> > >> >> > TCP_NODELAY is essentially ignored.
>>> >> > >> >> >
>>> >> > >> >> > Acked-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>>> >> > >> >> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> >> > >> >> > ---
>>> >> > >> >> >  fs/cifs/connect.c   |  4 ++++
>>> >> > >> >> >  fs/cifs/transport.c | 12 ++++++++++++
>>> >> > >> >> >  2 files changed, 16 insertions(+)
>>> >> > >> >> >
>>> >> > >> >> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>>> >> > >> >> > index 6df6fa1..a828a8c 100644
>>> >> > >> >> > --- a/fs/cifs/connect.c
>>> >> > >> >> > +++ b/fs/cifs/connect.c
>>> >> > >> >> > @@ -1676,6 +1676,10 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
>>> >> > >> >> >                         if (string == NULL)
>>> >> > >> >> >                                 goto out_nomem;
>>> >> > >> >> >
>>> >> > >> >> > +                       /*
>>> >> > >> >> > +                        * FIXME: since we now cork/uncork the socket while
>>> >> > >> >> > +                        *        sending, should we deprecate this option?
>>> >> > >> >> > +                        */
>>> >> > >> >> >                         if (strnicmp(string, "TCP_NODELAY", 11) == 0)
>>> >> > >> >> >                                 vol->sockopt_tcp_nodelay = 1;
>>> >> > >> >> >                         break;
>>> >> > >> >> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>>> >> > >> >> > index d93f15d..a3e58b2 100644
>>> >> > >> >> > --- a/fs/cifs/transport.c
>>> >> > >> >> > +++ b/fs/cifs/transport.c
>>> >> > >> >> > @@ -27,6 +27,7 @@
>>> >> > >> >> >  #include <linux/net.h>
>>> >> > >> >> >  #include <linux/delay.h>
>>> >> > >> >> >  #include <linux/freezer.h>
>>> >> > >> >> > +#include <linux/tcp.h>
>>> >> > >> >> >  #include <asm/uaccess.h>
>>> >> > >> >> >  #include <asm/processor.h>
>>> >> > >> >> >  #include <linux/mempool.h>
>>> >> > >> >> > @@ -247,12 +248,23 @@ smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
>>> >> > >> >> >         int n_vec = rqst->rq_nvec;
>>> >> > >> >> >         unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
>>> >> > >> >> >         size_t total_len;
>>> >> > >> >> > +       struct socket *ssocket = server->ssocket;
>>> >> > >> >> > +       int val = 1;
>>> >> > >> >> >
>>> >> > >> >> >         cFYI(1, "Sending smb: smb_len=%u", smb_buf_length);
>>> >> > >> >> >         dump_smb(iov[0].iov_base, iov[0].iov_len);
>>> >> > >> >> >
>>> >> > >> >> > +       /* cork the socket */
>>> >> > >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
>>> >> > >> >> > +                               (char *)&val, sizeof(val));
>>> >> > >> >> > +
>>> >> > >> >> >         rc = smb_send_kvec(server, iov, n_vec, &total_len);
>>> >> > >> >> >
>>> >> > >> >> > +       /* uncork it */
>>> >> > >> >> > +       val = 0;
>>> >> > >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
>>> >> > >> >> > +                               (char *)&val, sizeof(val));
>>> >> > >> >> > +
>>> >> > >> >> >         if ((total_len > 0) && (total_len != smb_buf_length + 4)) {
>>> >> > >> >> >                 cFYI(1, "partial send (wanted=%u sent=%zu): terminating "
>>> >> > >> >> >                         "session", smb_buf_length + 4, total_len);
>>> >> > >> >> > --
>>> >> > >> >> > 1.7.11.2
>>> >> > >> >> >
>>> >> > >> >> > --
>>> >> > >> >> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>>> >> > >> >> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> >> > >> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> >> > >> >>
>>> >> > >> >> I tested it with SMB2 against Windows 7 server. When iosize is 64K
>>> >> > >> >> everything is ok but when we increase iosize to 1M (by using
>>> >> > >> >> multicredit requests) and the server loses the network connection and
>>> >> > >> >> only reboot helps.
>>> >> > >> >>
>>> >> > >> >> Also if I commented corking/uncorking the socket - everything is ok. I
>>> >> > >> >> think this change needs some more investigation (how does it deals
>>> >> > >> >> with 1M iosize on Samba, etc?)
>>> >> > >> >>
>>> >> > >> >
>>> >> > >> > Hmm, haven't seen that with a 1M iosize with smb1 against samba.
>>> >> > >> >
>>> >> > >> > I'll see if I can reproduce it.
>>> >> > >> >
>>> >> > >> > --
>>> >> > >> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> >> > >>
>>> >> > >> Forgot to mentioned how I reproduce it - dbench with 5 clients.
>>> >> > >>
>>> >> > >
>>> >> > > Ok, I've built a cifs.ko from your smb2-dev-cifs-3.6 branch. Here are my mount options:
>>> >> > >
>>> >> > > //win7.poochiereds.net/export /mnt/win7 cifs rw,relatime,vers=2.1,sec=ntlmsspi,cache=strict,unc=\\win7.poochiereds.net\export,username=testuser,domain=WIN7,uid=0,noforceuid,gid=0,noforcegid,addr=192.168.1.32,file_mode=0755,dir_mode=0755,nounix,serverino,rsize=1048576,wsize=1048576,actimeo=1 0 0
>>> >> >
>>> >> > I don't set rsize and wsize explicitly but I don't think it's related.
>>> >> > On what connection did you test it? I use 100Mbit LAN.
>>> >> >
>>> >>
>>> >> The clients and servers are both KVM guests. I'll give it a go over a
>>> >> physical network tomorrow.
>>> >>
>>> >
>>> > Ok, ran it with client as a KVM guest and the server as win7 running on
>>> > bare metal over a gigE network. It still ran just fine.
>>> >
>>> > So what are the symptoms that you see here? Does dbench just hang? If
>>> > so, could you collect /proc/<pid>/stack from the hung process(es)?
>>> > Maybe that would tell us what's going on...
>>>
>>> Windows 7 server doesn't response to packets after some time running
>>> dbench. Also, I even can't ping google.com from this Windows machine.
>>> It seems that everything ok with dbench and Linux machine.
>>>
>>> So, it looks like Windows server problem on my configuration but of
>>> course seems very strage. I will try this patch with Samba server
>>> further.
>>>
>>> This patch doesn't break things with Windows untill we use multicredit
>>> requests (more than 64K, that are not targeted to 3.6 kernel). But I
>>> am going to target multicredit requests feature for 3.7. May be we
>>> should make cork/nodelay switchable? Or just merge the patchset
>>> without this patch for 3.6 and delay this patch for 3.7 - we will have
>>> much time to investigate this strange behavior?
>>>
>>> Thoughts?
>>>
>>
>> If the server isn't responding then that seems like something is broken
>> on the server. Maybe you have a broken network driver? Do you have any
>> captures?
>
> Now I don't have them but I can collect captures when return from vacations.
>
> --
> Best regards,
> Pavel Shilovsky.



-- 
Thanks,

Steve

Re: [PATCH v2 3/9] cifs: cork the socket before a send and uncork it afterward

[Pavel Shilovsky]

2012/8/5 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> I suspect that there is a relationship between this and the queueing
> problems we saw with Windows Vista and Windows 7 depending on max
> multiplex request size (for cifs).
>
> On Sat, Aug 4, 2012 at 3:51 PM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> 2012/8/1 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>>> On Wed, 1 Aug 2012 15:37:57 +0200
>>> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>
>>>> 2012/7/31 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>>>> > On Mon, 30 Jul 2012 21:17:53 -0400
>>>> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>> >
>>>> >> On Mon, 30 Jul 2012 23:11:10 +0200
>>>> >> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> >>
>>>> >> > 2012/7/29 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>>> >> > > On Fri, 27 Jul 2012 10:05:32 +0400
>>>> >> > > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> >> > >
>>>> >> > >> 2012/7/27 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>>> >> > >> > On Fri, 27 Jul 2012 03:57:44 +0400
>>>> >> > >> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> >> > >> >
>>>> >> > >> >> 2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>>> >> > >> >> > We want to send SMBs as "atomically" as possible. Prior to sending any
>>>> >> > >> >> > data on the socket, cork it to make sure that no non-full frames go
>>>> >> > >> >> > out. Afterward, uncork it to make sure all of the data gets pushed out
>>>> >> > >> >> > to the wire.
>>>> >> > >> >> >
>>>> >> > >> >> > Note that this more or less renders the socket=TCP_NODELAY mount option
>>>> >> > >> >> > obsolete. When TCP_CORK and TCP_NODELAY are used on the same socket,
>>>> >> > >> >> > TCP_NODELAY is essentially ignored.
>>>> >> > >> >> >
>>>> >> > >> >> > Acked-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>>>> >> > >> >> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>> >> > >> >> > ---
>>>> >> > >> >> >  fs/cifs/connect.c   |  4 ++++
>>>> >> > >> >> >  fs/cifs/transport.c | 12 ++++++++++++
>>>> >> > >> >> >  2 files changed, 16 insertions(+)
>>>> >> > >> >> >
>>>> >> > >> >> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>>>> >> > >> >> > index 6df6fa1..a828a8c 100644
>>>> >> > >> >> > --- a/fs/cifs/connect.c
>>>> >> > >> >> > +++ b/fs/cifs/connect.c
>>>> >> > >> >> > @@ -1676,6 +1676,10 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
>>>> >> > >> >> >                         if (string == NULL)
>>>> >> > >> >> >                                 goto out_nomem;
>>>> >> > >> >> >
>>>> >> > >> >> > +                       /*
>>>> >> > >> >> > +                        * FIXME: since we now cork/uncork the socket while
>>>> >> > >> >> > +                        *        sending, should we deprecate this option?
>>>> >> > >> >> > +                        */
>>>> >> > >> >> >                         if (strnicmp(string, "TCP_NODELAY", 11) == 0)
>>>> >> > >> >> >                                 vol->sockopt_tcp_nodelay = 1;
>>>> >> > >> >> >                         break;
>>>> >> > >> >> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>>>> >> > >> >> > index d93f15d..a3e58b2 100644
>>>> >> > >> >> > --- a/fs/cifs/transport.c
>>>> >> > >> >> > +++ b/fs/cifs/transport.c
>>>> >> > >> >> > @@ -27,6 +27,7 @@
>>>> >> > >> >> >  #include <linux/net.h>
>>>> >> > >> >> >  #include <linux/delay.h>
>>>> >> > >> >> >  #include <linux/freezer.h>
>>>> >> > >> >> > +#include <linux/tcp.h>
>>>> >> > >> >> >  #include <asm/uaccess.h>
>>>> >> > >> >> >  #include <asm/processor.h>
>>>> >> > >> >> >  #include <linux/mempool.h>
>>>> >> > >> >> > @@ -247,12 +248,23 @@ smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
>>>> >> > >> >> >         int n_vec = rqst->rq_nvec;
>>>> >> > >> >> >         unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
>>>> >> > >> >> >         size_t total_len;
>>>> >> > >> >> > +       struct socket *ssocket = server->ssocket;
>>>> >> > >> >> > +       int val = 1;
>>>> >> > >> >> >
>>>> >> > >> >> >         cFYI(1, "Sending smb: smb_len=%u", smb_buf_length);
>>>> >> > >> >> >         dump_smb(iov[0].iov_base, iov[0].iov_len);
>>>> >> > >> >> >
>>>> >> > >> >> > +       /* cork the socket */
>>>> >> > >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
>>>> >> > >> >> > +                               (char *)&val, sizeof(val));
>>>> >> > >> >> > +
>>>> >> > >> >> >         rc = smb_send_kvec(server, iov, n_vec, &total_len);
>>>> >> > >> >> >
>>>> >> > >> >> > +       /* uncork it */
>>>> >> > >> >> > +       val = 0;
>>>> >> > >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
>>>> >> > >> >> > +                               (char *)&val, sizeof(val));
>>>> >> > >> >> > +
>>>> >> > >> >> >         if ((total_len > 0) && (total_len != smb_buf_length + 4)) {
>>>> >> > >> >> >                 cFYI(1, "partial send (wanted=%u sent=%zu): terminating "
>>>> >> > >> >> >                         "session", smb_buf_length + 4, total_len);
>>>> >> > >> >> > --
>>>> >> > >> >> > 1.7.11.2
>>>> >> > >> >> >
>>>> >> > >> >> > --
>>>> >> > >> >> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>>>> >> > >> >> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>> >> > >> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>> >> > >> >>
>>>> >> > >> >> I tested it with SMB2 against Windows 7 server. When iosize is 64K
>>>> >> > >> >> everything is ok but when we increase iosize to 1M (by using
>>>> >> > >> >> multicredit requests) and the server loses the network connection and
>>>> >> > >> >> only reboot helps.
>>>> >> > >> >>
>>>> >> > >> >> Also if I commented corking/uncorking the socket - everything is ok. I
>>>> >> > >> >> think this change needs some more investigation (how does it deals
>>>> >> > >> >> with 1M iosize on Samba, etc?)
>>>> >> > >> >>
>>>> >> > >> >
>>>> >> > >> > Hmm, haven't seen that with a 1M iosize with smb1 against samba.
>>>> >> > >> >
>>>> >> > >> > I'll see if I can reproduce it.
>>>> >> > >> >
>>>> >> > >> > --
>>>> >> > >> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>> >> > >>
>>>> >> > >> Forgot to mentioned how I reproduce it - dbench with 5 clients.
>>>> >> > >>
>>>> >> > >
>>>> >> > > Ok, I've built a cifs.ko from your smb2-dev-cifs-3.6 branch. Here are my mount options:
>>>> >> > >
>>>> >> > > //win7.poochiereds.net/export /mnt/win7 cifs rw,relatime,vers=2.1,sec=ntlmsspi,cache=strict,unc=\\win7.poochiereds.net\export,username=testuser,domain=WIN7,uid=0,noforceuid,gid=0,noforcegid,addr=192.168.1.32,file_mode=0755,dir_mode=0755,nounix,serverino,rsize=1048576,wsize=1048576,actimeo=1 0 0
>>>> >> >
>>>> >> > I don't set rsize and wsize explicitly but I don't think it's related.
>>>> >> > On what connection did you test it? I use 100Mbit LAN.
>>>> >> >
>>>> >>
>>>> >> The clients and servers are both KVM guests. I'll give it a go over a
>>>> >> physical network tomorrow.
>>>> >>
>>>> >
>>>> > Ok, ran it with client as a KVM guest and the server as win7 running on
>>>> > bare metal over a gigE network. It still ran just fine.
>>>> >
>>>> > So what are the symptoms that you see here? Does dbench just hang? If
>>>> > so, could you collect /proc/<pid>/stack from the hung process(es)?
>>>> > Maybe that would tell us what's going on...
>>>>
>>>> Windows 7 server doesn't response to packets after some time running
>>>> dbench. Also, I even can't ping google.com from this Windows machine.
>>>> It seems that everything ok with dbench and Linux machine.
>>>>
>>>> So, it looks like Windows server problem on my configuration but of
>>>> course seems very strage. I will try this patch with Samba server
>>>> further.
>>>>
>>>> This patch doesn't break things with Windows untill we use multicredit
>>>> requests (more than 64K, that are not targeted to 3.6 kernel). But I
>>>> am going to target multicredit requests feature for 3.7. May be we
>>>> should make cork/nodelay switchable? Or just merge the patchset
>>>> without this patch for 3.6 and delay this patch for 3.7 - we will have
>>>> much time to investigate this strange behavior?
>>>>
>>>> Thoughts?
>>>>
>>>
>>> If the server isn't responding then that seems like something is broken
>>> on the server. Maybe you have a broken network driver? Do you have any
>>> captures?
>>
>> Now I don't have them but I can collect captures when return from vacations.
>>
>> --
>> Best regards,
>> Pavel Shilovsky.
>
>
>
> --
> Thanks,
>
> Steve

Tested it again with the same work environment and couldn't reproduce
it - seems very strange, because later it was 100% reproducible.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH v2 3/9] cifs: cork the socket before a send and uncork it afterward

[Jeff Layton]

On Wed, 1 Aug 2012 15:37:57 +0200
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2012/7/31 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> > On Mon, 30 Jul 2012 21:17:53 -0400
> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >
> >> On Mon, 30 Jul 2012 23:11:10 +0200
> >> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>
> >> > 2012/7/29 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >> > > On Fri, 27 Jul 2012 10:05:32 +0400
> >> > > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> > >
> >> > >> 2012/7/27 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >> > >> > On Fri, 27 Jul 2012 03:57:44 +0400
> >> > >> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> > >> >
> >> > >> >> 2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >> > >> >> > We want to send SMBs as "atomically" as possible. Prior to sending any
> >> > >> >> > data on the socket, cork it to make sure that no non-full frames go
> >> > >> >> > out. Afterward, uncork it to make sure all of the data gets pushed out
> >> > >> >> > to the wire.
> >> > >> >> >
> >> > >> >> > Note that this more or less renders the socket=TCP_NODELAY mount option
> >> > >> >> > obsolete. When TCP_CORK and TCP_NODELAY are used on the same socket,
> >> > >> >> > TCP_NODELAY is essentially ignored.
> >> > >> >> >
> >> > >> >> > Acked-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> >> > >> >> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> > >> >> > ---
> >> > >> >> >  fs/cifs/connect.c   |  4 ++++
> >> > >> >> >  fs/cifs/transport.c | 12 ++++++++++++
> >> > >> >> >  2 files changed, 16 insertions(+)
> >> > >> >> >
> >> > >> >> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >> > >> >> > index 6df6fa1..a828a8c 100644
> >> > >> >> > --- a/fs/cifs/connect.c
> >> > >> >> > +++ b/fs/cifs/connect.c
> >> > >> >> > @@ -1676,6 +1676,10 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
> >> > >> >> >                         if (string == NULL)
> >> > >> >> >                                 goto out_nomem;
> >> > >> >> >
> >> > >> >> > +                       /*
> >> > >> >> > +                        * FIXME: since we now cork/uncork the socket while
> >> > >> >> > +                        *        sending, should we deprecate this option?
> >> > >> >> > +                        */
> >> > >> >> >                         if (strnicmp(string, "TCP_NODELAY", 11) == 0)
> >> > >> >> >                                 vol->sockopt_tcp_nodelay = 1;
> >> > >> >> >                         break;
> >> > >> >> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> >> > >> >> > index d93f15d..a3e58b2 100644
> >> > >> >> > --- a/fs/cifs/transport.c
> >> > >> >> > +++ b/fs/cifs/transport.c
> >> > >> >> > @@ -27,6 +27,7 @@
> >> > >> >> >  #include <linux/net.h>
> >> > >> >> >  #include <linux/delay.h>
> >> > >> >> >  #include <linux/freezer.h>
> >> > >> >> > +#include <linux/tcp.h>
> >> > >> >> >  #include <asm/uaccess.h>
> >> > >> >> >  #include <asm/processor.h>
> >> > >> >> >  #include <linux/mempool.h>
> >> > >> >> > @@ -247,12 +248,23 @@ smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
> >> > >> >> >         int n_vec = rqst->rq_nvec;
> >> > >> >> >         unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
> >> > >> >> >         size_t total_len;
> >> > >> >> > +       struct socket *ssocket = server->ssocket;
> >> > >> >> > +       int val = 1;
> >> > >> >> >
> >> > >> >> >         cFYI(1, "Sending smb: smb_len=%u", smb_buf_length);
> >> > >> >> >         dump_smb(iov[0].iov_base, iov[0].iov_len);
> >> > >> >> >
> >> > >> >> > +       /* cork the socket */
> >> > >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
> >> > >> >> > +                               (char *)&val, sizeof(val));
> >> > >> >> > +
> >> > >> >> >         rc = smb_send_kvec(server, iov, n_vec, &total_len);
> >> > >> >> >
> >> > >> >> > +       /* uncork it */
> >> > >> >> > +       val = 0;
> >> > >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
> >> > >> >> > +                               (char *)&val, sizeof(val));
> >> > >> >> > +
> >> > >> >> >         if ((total_len > 0) && (total_len != smb_buf_length + 4)) {
> >> > >> >> >                 cFYI(1, "partial send (wanted=%u sent=%zu): terminating "
> >> > >> >> >                         "session", smb_buf_length + 4, total_len);
> >> > >> >> > --
> >> > >> >> > 1.7.11.2
> >> > >> >> >
> >> > >> >> > --
> >> > >> >> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> >> > >> >> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> > >> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> > >> >>
> >> > >> >> I tested it with SMB2 against Windows 7 server. When iosize is 64K
> >> > >> >> everything is ok but when we increase iosize to 1M (by using
> >> > >> >> multicredit requests) and the server loses the network connection and
> >> > >> >> only reboot helps.
> >> > >> >>
> >> > >> >> Also if I commented corking/uncorking the socket - everything is ok. I
> >> > >> >> think this change needs some more investigation (how does it deals
> >> > >> >> with 1M iosize on Samba, etc?)
> >> > >> >>
> >> > >> >
> >> > >> > Hmm, haven't seen that with a 1M iosize with smb1 against samba.
> >> > >> >
> >> > >> > I'll see if I can reproduce it.
> >> > >> >
> >> > >> > --
> >> > >> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> > >>
> >> > >> Forgot to mentioned how I reproduce it - dbench with 5 clients.
> >> > >>
> >> > >
> >> > > Ok, I've built a cifs.ko from your smb2-dev-cifs-3.6 branch. Here are my mount options:
> >> > >
> >> > > //win7.poochiereds.net/export /mnt/win7 cifs rw,relatime,vers=2.1,sec=ntlmsspi,cache=strict,unc=\\win7.poochiereds.net\export,username=testuser,domain=WIN7,uid=0,noforceuid,gid=0,noforcegid,addr=192.168.1.32,file_mode=0755,dir_mode=0755,nounix,serverino,rsize=1048576,wsize=1048576,actimeo=1 0 0
> >> >
> >> > I don't set rsize and wsize explicitly but I don't think it's related.
> >> > On what connection did you test it? I use 100Mbit LAN.
> >> >
> >>
> >> The clients and servers are both KVM guests. I'll give it a go over a
> >> physical network tomorrow.
> >>
> >
> > Ok, ran it with client as a KVM guest and the server as win7 running on
> > bare metal over a gigE network. It still ran just fine.
> >
> > So what are the symptoms that you see here? Does dbench just hang? If
> > so, could you collect /proc/<pid>/stack from the hung process(es)?
> > Maybe that would tell us what's going on...
> 
> Windows 7 server doesn't response to packets after some time running
> dbench. Also, I even can't ping google.com from this Windows machine.
> It seems that everything ok with dbench and Linux machine.
> 
> So, it looks like Windows server problem on my configuration but of
> course seems very strage. I will try this patch with Samba server
> further.
> 
> This patch doesn't break things with Windows untill we use multicredit
> requests (more than 64K, that are not targeted to 3.6 kernel). But I
> am going to target multicredit requests feature for 3.7. May be we
> should make cork/nodelay switchable? Or just merge the patchset
> without this patch for 3.6 and delay this patch for 3.7 - we will have
> much time to investigate this strange behavior?
> 

This really sounds like you just have a defective network driver (or
hardware) on your windows machine. What sort of hardware does this
windows machine have and what driver are you running on it?

Also, was my testing not using multicredit requests? If not, how do I
enable them? I'd like to try and reproduce this if possible.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH v2 3/9] cifs: cork the socket before a send and uncork it afterward

[Pavel Shilovsky]

2012/8/1 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> On Wed, 1 Aug 2012 15:37:57 +0200
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> 2012/7/31 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>> > On Mon, 30 Jul 2012 21:17:53 -0400
>> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> >
>> >> On Mon, 30 Jul 2012 23:11:10 +0200
>> >> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >>
>> >> > 2012/7/29 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> >> > > On Fri, 27 Jul 2012 10:05:32 +0400
>> >> > > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> > >
>> >> > >> 2012/7/27 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> >> > >> > On Fri, 27 Jul 2012 03:57:44 +0400
>> >> > >> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> > >> >
>> >> > >> >> 2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> >> > >> >> > We want to send SMBs as "atomically" as possible. Prior to sending any
>> >> > >> >> > data on the socket, cork it to make sure that no non-full frames go
>> >> > >> >> > out. Afterward, uncork it to make sure all of the data gets pushed out
>> >> > >> >> > to the wire.
>> >> > >> >> >
>> >> > >> >> > Note that this more or less renders the socket=TCP_NODELAY mount option
>> >> > >> >> > obsolete. When TCP_CORK and TCP_NODELAY are used on the same socket,
>> >> > >> >> > TCP_NODELAY is essentially ignored.
>> >> > >> >> >
>> >> > >> >> > Acked-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>> >> > >> >> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> >> > >> >> > ---
>> >> > >> >> >  fs/cifs/connect.c   |  4 ++++
>> >> > >> >> >  fs/cifs/transport.c | 12 ++++++++++++
>> >> > >> >> >  2 files changed, 16 insertions(+)
>> >> > >> >> >
>> >> > >> >> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> >> > >> >> > index 6df6fa1..a828a8c 100644
>> >> > >> >> > --- a/fs/cifs/connect.c
>> >> > >> >> > +++ b/fs/cifs/connect.c
>> >> > >> >> > @@ -1676,6 +1676,10 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
>> >> > >> >> >                         if (string == NULL)
>> >> > >> >> >                                 goto out_nomem;
>> >> > >> >> >
>> >> > >> >> > +                       /*
>> >> > >> >> > +                        * FIXME: since we now cork/uncork the socket while
>> >> > >> >> > +                        *        sending, should we deprecate this option?
>> >> > >> >> > +                        */
>> >> > >> >> >                         if (strnicmp(string, "TCP_NODELAY", 11) == 0)
>> >> > >> >> >                                 vol->sockopt_tcp_nodelay = 1;
>> >> > >> >> >                         break;
>> >> > >> >> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>> >> > >> >> > index d93f15d..a3e58b2 100644
>> >> > >> >> > --- a/fs/cifs/transport.c
>> >> > >> >> > +++ b/fs/cifs/transport.c
>> >> > >> >> > @@ -27,6 +27,7 @@
>> >> > >> >> >  #include <linux/net.h>
>> >> > >> >> >  #include <linux/delay.h>
>> >> > >> >> >  #include <linux/freezer.h>
>> >> > >> >> > +#include <linux/tcp.h>
>> >> > >> >> >  #include <asm/uaccess.h>
>> >> > >> >> >  #include <asm/processor.h>
>> >> > >> >> >  #include <linux/mempool.h>
>> >> > >> >> > @@ -247,12 +248,23 @@ smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
>> >> > >> >> >         int n_vec = rqst->rq_nvec;
>> >> > >> >> >         unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
>> >> > >> >> >         size_t total_len;
>> >> > >> >> > +       struct socket *ssocket = server->ssocket;
>> >> > >> >> > +       int val = 1;
>> >> > >> >> >
>> >> > >> >> >         cFYI(1, "Sending smb: smb_len=%u", smb_buf_length);
>> >> > >> >> >         dump_smb(iov[0].iov_base, iov[0].iov_len);
>> >> > >> >> >
>> >> > >> >> > +       /* cork the socket */
>> >> > >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
>> >> > >> >> > +                               (char *)&val, sizeof(val));
>> >> > >> >> > +
>> >> > >> >> >         rc = smb_send_kvec(server, iov, n_vec, &total_len);
>> >> > >> >> >
>> >> > >> >> > +       /* uncork it */
>> >> > >> >> > +       val = 0;
>> >> > >> >> > +       kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
>> >> > >> >> > +                               (char *)&val, sizeof(val));
>> >> > >> >> > +
>> >> > >> >> >         if ((total_len > 0) && (total_len != smb_buf_length + 4)) {
>> >> > >> >> >                 cFYI(1, "partial send (wanted=%u sent=%zu): terminating "
>> >> > >> >> >                         "session", smb_buf_length + 4, total_len);
>> >> > >> >> > --
>> >> > >> >> > 1.7.11.2
>> >> > >> >> >
>> >> > >> >> > --
>> >> > >> >> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> >> > >> >> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> >> > >> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >> > >> >>
>> >> > >> >> I tested it with SMB2 against Windows 7 server. When iosize is 64K
>> >> > >> >> everything is ok but when we increase iosize to 1M (by using
>> >> > >> >> multicredit requests) and the server loses the network connection and
>> >> > >> >> only reboot helps.
>> >> > >> >>
>> >> > >> >> Also if I commented corking/uncorking the socket - everything is ok. I
>> >> > >> >> think this change needs some more investigation (how does it deals
>> >> > >> >> with 1M iosize on Samba, etc?)
>> >> > >> >>
>> >> > >> >
>> >> > >> > Hmm, haven't seen that with a 1M iosize with smb1 against samba.
>> >> > >> >
>> >> > >> > I'll see if I can reproduce it.
>> >> > >> >
>> >> > >> > --
>> >> > >> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> >> > >>
>> >> > >> Forgot to mentioned how I reproduce it - dbench with 5 clients.
>> >> > >>
>> >> > >
>> >> > > Ok, I've built a cifs.ko from your smb2-dev-cifs-3.6 branch. Here are my mount options:
>> >> > >
>> >> > > //win7.poochiereds.net/export /mnt/win7 cifs rw,relatime,vers=2.1,sec=ntlmsspi,cache=strict,unc=\\win7.poochiereds.net\export,username=testuser,domain=WIN7,uid=0,noforceuid,gid=0,noforcegid,addr=192.168.1.32,file_mode=0755,dir_mode=0755,nounix,serverino,rsize=1048576,wsize=1048576,actimeo=1 0 0
>> >> >
>> >> > I don't set rsize and wsize explicitly but I don't think it's related.
>> >> > On what connection did you test it? I use 100Mbit LAN.
>> >> >
>> >>
>> >> The clients and servers are both KVM guests. I'll give it a go over a
>> >> physical network tomorrow.
>> >>
>> >
>> > Ok, ran it with client as a KVM guest and the server as win7 running on
>> > bare metal over a gigE network. It still ran just fine.
>> >
>> > So what are the symptoms that you see here? Does dbench just hang? If
>> > so, could you collect /proc/<pid>/stack from the hung process(es)?
>> > Maybe that would tell us what's going on...
>>
>> Windows 7 server doesn't response to packets after some time running
>> dbench. Also, I even can't ping google.com from this Windows machine.
>> It seems that everything ok with dbench and Linux machine.
>>
>> So, it looks like Windows server problem on my configuration but of
>> course seems very strage. I will try this patch with Samba server
>> further.
>>
>> This patch doesn't break things with Windows untill we use multicredit
>> requests (more than 64K, that are not targeted to 3.6 kernel). But I
>> am going to target multicredit requests feature for 3.7. May be we
>> should make cork/nodelay switchable? Or just merge the patchset
>> without this patch for 3.6 and delay this patch for 3.7 - we will have
>> much time to investigate this strange behavior?
>>
>
> This really sounds like you just have a defective network driver (or
> hardware) on your windows machine. What sort of hardware does this
> windows machine have and what driver are you running on it?

It's LAN adapter that is on board on asus p5k se motherboard.

>
> Also, was my testing not using multicredit requests? If not, how do I
> enable them? I'd like to try and reproduce this if possible.

If you set rsize/wsize to 1M the client uses multicredit requests
because according to SMB2 protocol one credit wasts on 64K payload -
seems you have already done it.

-- 
Best regards,
Pavel Shilovsky.

[PATCH v2 4/9] cifs: teach smb_send_rqst how to handle arrays of pages

[Jeff Layton]

Add code that allows smb_send_rqst to send an array of pages after the
initial kvec array has been sent. For now, we simply kmap the page
array and send it using the standard smb_send_kvec function. Eventually,
we may want to convert this code to use kernel_sendpage under the hood
and avoid the kmap altogether for the page data.

Reviewed-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/transport.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 54 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index a3e58b2..946a462 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -28,6 +28,7 @@
 #include <linux/delay.h>
 #include <linux/freezer.h>
 #include <linux/tcp.h>
+#include <linux/highmem.h>
 #include <asm/uaccess.h>
 #include <asm/processor.h>
 #include <linux/mempool.h>
@@ -240,6 +241,38 @@ smb_send_kvec(struct TCP_Server_Info *server, struct kvec *iov, size_t n_vec,
 	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.
+ */
+static 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
+	 * page directly into the fraglist. If userspace modifies the
+	 * page after we calculate the signature, then the server will
+	 * reject it and may break the connection. kernel_sendmsg does
+	 * 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;
+}
+
 static int
 smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
 {
@@ -247,7 +280,8 @@ smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
 	struct kvec *iov = rqst->rq_iov;
 	int n_vec = rqst->rq_nvec;
 	unsigned int smb_buf_length = get_rfc1002_length(iov[0].iov_base);
-	size_t total_len;
+	unsigned int i;
+	size_t total_len = 0, sent;
 	struct socket *ssocket = server->ssocket;
 	int val = 1;
 
@@ -258,8 +292,26 @@ smb_send_rqst(struct TCP_Server_Info *server, struct smb_rqst *rqst)
 	kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
 				(char *)&val, sizeof(val));
 
-	rc = smb_send_kvec(server, iov, n_vec, &total_len);
+	rc = smb_send_kvec(server, iov, n_vec, &sent);
+	if (rc < 0)
+		goto uncork;
+
+	total_len += sent;
+
+	/* now walk the page array and send each page in it */
+	for (i = 0; i < rqst->rq_npages; i++) {
+		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]);
+		if (rc < 0)
+			break;
+
+		total_len += sent;
+	}
 
+uncork:
 	/* uncork it */
 	val = 0;
 	kernel_setsockopt(ssocket, SOL_TCP, TCP_CORK,
-- 
1.7.11.2

[PATCH v2 5/9] cifs: teach signing routines how to deal with arrays of pages in a smb_rqst

[Jeff Layton]

Use the smb_send_rqst helper function to kmap each page in the array
and update the hash for that chunk.

Reviewed-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsencrypt.c | 11 +++++++++++
 fs/cifs/cifsproto.h   |  2 ++
 fs/cifs/transport.c   |  2 +-
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index e8953a0..511d1ae 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -29,6 +29,7 @@
 #include "ntlmssp.h"
 #include <linux/ctype.h>
 #include <linux/random.h>
+#include <linux/highmem.h>
 
 /*
  * Calculate and return the CIFS signature based on the mac key and SMB PDU.
@@ -93,6 +94,16 @@ 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;
+
+		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);
 	if (rc)
 		cERROR(1, "%s: Could not generate md5 hash", __func__);
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index d02a6a4..3620bec 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -36,6 +36,8 @@ extern struct smb_hdr *cifs_buf_get(void);
 extern void cifs_buf_release(void *);
 extern struct smb_hdr *cifs_small_buf_get(void);
 extern void cifs_small_buf_release(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);
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 946a462..5673ef1 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -251,7 +251,7 @@ smb_send_kvec(struct TCP_Server_Info *server, struct kvec *iov, size_t n_vec,
  * The page will be kmapped and the address placed into iov_base. The length
  * will then be adjusted according to the ptailoff.
  */
-static void
+void
 cifs_rqst_page_to_kvec(struct smb_rqst *rqst, unsigned int idx,
 			struct kvec *iov)
 {
-- 
1.7.11.2

[PATCH v2 6/9] cifs: change cifs_call_async to use smb_rqst structs

[Jeff Layton]

For now, none of the callers populate rq_pages. That will be done for
writes in a later patch. While we're at it, change the prototype of
setup_async_request not to need a return pointer argument. Just
return the pointer to the mid_q_entry or an ERR_PTR.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsglob.h      |  4 ++--
 fs/cifs/cifsproto.h     | 12 ++++++------
 fs/cifs/cifssmb.c       | 20 +++++++++++++-------
 fs/cifs/smb2proto.h     |  4 +---
 fs/cifs/smb2transport.c | 14 ++++++--------
 fs/cifs/transport.c     | 30 ++++++++++++++----------------
 6 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 2a7e4b8..a0bb713 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -194,8 +194,8 @@ struct smb_version_operations {
 	int (*setup_request)(struct cifs_ses *, struct kvec *, unsigned int,
 			     struct mid_q_entry **);
 	/* setup async request: allocate mid, sign message */
-	int (*setup_async_request)(struct TCP_Server_Info *, struct kvec *,
-				   unsigned int, struct mid_q_entry **);
+	struct mid_q_entry *(*setup_async_request)(struct TCP_Server_Info *,
+						struct smb_rqst *);
 	/* check response: verify signature, map error */
 	int (*check_receive)(struct mid_q_entry *, struct TCP_Server_Info *,
 			     bool);
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 3620bec..8075f23 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -69,10 +69,10 @@ extern struct mid_q_entry *AllocMidQEntry(const struct smb_hdr *smb_buffer,
 					struct TCP_Server_Info *server);
 extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
 extern void cifs_wake_up_task(struct mid_q_entry *mid);
-extern int cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
-			   unsigned int nvec, mid_receive_t *receive,
-			   mid_callback_t *callback, void *cbdata,
-			   const int flags);
+extern int cifs_call_async(struct TCP_Server_Info *server,
+			struct smb_rqst *rqst,
+			mid_receive_t *receive, mid_callback_t *callback,
+			void *cbdata, const int flags);
 extern int SendReceive(const unsigned int /* xid */ , struct cifs_ses *,
 			struct smb_hdr * /* input */ ,
 			struct smb_hdr * /* out */ ,
@@ -81,8 +81,8 @@ extern int SendReceiveNoRsp(const unsigned int xid, struct cifs_ses *ses,
 			    char *in_buf, int flags);
 extern int cifs_setup_request(struct cifs_ses *, struct kvec *, unsigned int,
 			      struct mid_q_entry **);
-extern int cifs_setup_async_request(struct TCP_Server_Info *, struct kvec *,
-				    unsigned int, struct mid_q_entry **);
+extern struct mid_q_entry *cifs_setup_async_request(struct TCP_Server_Info *,
+						struct smb_rqst *);
 extern int cifs_check_receive(struct mid_q_entry *mid,
 			struct TCP_Server_Info *server, bool log_error);
 extern int SendReceive2(const unsigned int /* xid */ , struct cifs_ses *,
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index ce5f75c..7365460 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -751,6 +751,8 @@ CIFSSMBEcho(struct TCP_Server_Info *server)
 	ECHO_REQ *smb;
 	int rc = 0;
 	struct kvec iov;
+	struct smb_rqst rqst = { .rq_iov = &iov,
+				 .rq_nvec = 1 };
 
 	cFYI(1, "In echo request");
 
@@ -768,7 +770,7 @@ CIFSSMBEcho(struct TCP_Server_Info *server)
 	iov.iov_base = smb;
 	iov.iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4;
 
-	rc = cifs_call_async(server, &iov, 1, NULL, cifs_echo_callback,
+	rc = cifs_call_async(server, &rqst, NULL, cifs_echo_callback,
 			     server, CIFS_ASYNC_OP | CIFS_ECHO_OP);
 	if (rc)
 		cFYI(1, "Echo request failed: %d", rc);
@@ -1604,6 +1606,8 @@ cifs_async_readv(struct cifs_readdata *rdata)
 	READ_REQ *smb = NULL;
 	int wct;
 	struct cifs_tcon *tcon = tlink_tcon(rdata->cfile->tlink);
+	struct smb_rqst rqst = { .rq_iov = rdata->iov,
+				 .rq_nvec = 1 };
 
 	cFYI(1, "%s: offset=%llu bytes=%u", __func__,
 		rdata->offset, rdata->bytes);
@@ -1647,9 +1651,8 @@ cifs_async_readv(struct cifs_readdata *rdata)
 	rdata->iov[0].iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4;
 
 	kref_get(&rdata->refcount);
-	rc = cifs_call_async(tcon->ses->server, rdata->iov, 1,
-			     cifs_readv_receive, cifs_readv_callback,
-			     rdata, 0);
+	rc = cifs_call_async(tcon->ses->server, &rqst, cifs_readv_receive,
+			     cifs_readv_callback, rdata, 0);
 
 	if (rc == 0)
 		cifs_stats_inc(&tcon->stats.cifs_stats.num_reads);
@@ -2052,6 +2055,7 @@ cifs_async_writev(struct cifs_writedata *wdata)
 	int wct;
 	struct cifs_tcon *tcon = tlink_tcon(wdata->cfile->tlink);
 	struct kvec *iov = NULL;
+	struct smb_rqst rqst = { };
 
 	if (tcon->ses->capabilities & CAP_LARGE_FILES) {
 		wct = 14;
@@ -2068,11 +2072,13 @@ cifs_async_writev(struct cifs_writedata *wdata)
 		goto async_writev_out;
 
 	/* 1 iov per page + 1 for header */
-	iov = kzalloc((wdata->nr_pages + 1) * sizeof(*iov), GFP_NOFS);
+	rqst.rq_nvec = wdata->nr_pages + 1;
+	iov = kzalloc((rqst.rq_nvec) * sizeof(*iov), GFP_NOFS);
 	if (iov == NULL) {
 		rc = -ENOMEM;
 		goto async_writev_out;
 	}
+	rqst.rq_iov = iov;
 
 	smb->hdr.Pid = cpu_to_le16((__u16)wdata->pid);
 	smb->hdr.PidHigh = cpu_to_le16((__u16)(wdata->pid >> 16));
@@ -2121,8 +2127,8 @@ cifs_async_writev(struct cifs_writedata *wdata)
 	}
 
 	kref_get(&wdata->refcount);
-	rc = cifs_call_async(tcon->ses->server, iov, wdata->nr_pages + 1,
-			     NULL, cifs_writev_callback, wdata, 0);
+	rc = cifs_call_async(tcon->ses->server, &rqst, NULL,
+				cifs_writev_callback, wdata, 0);
 
 	if (rc == 0)
 		cifs_stats_inc(&tcon->stats.cifs_stats.num_writes);
diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
index 902bbe2..dbb798c 100644
--- a/fs/cifs/smb2proto.h
+++ b/fs/cifs/smb2proto.h
@@ -43,9 +43,7 @@ extern int smb2_check_receive(struct mid_q_entry *mid,
 			      struct TCP_Server_Info *server, bool log_error);
 extern int smb2_setup_request(struct cifs_ses *ses, struct kvec *iov,
 			      unsigned int nvec, struct mid_q_entry **ret_mid);
-extern int smb2_setup_async_request(struct TCP_Server_Info *server,
-				    struct kvec *iov, unsigned int nvec,
-				    struct mid_q_entry **ret_mid);
+extern struct mid_q_entry *smb2_setup_async_request(struct TCP_Server_Info *server, struct smb_rqst *rqst);
 extern void smb2_echo_request(struct work_struct *work);
 
 extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
index 31f5d42..c0a7a42 100644
--- a/fs/cifs/smb2transport.c
+++ b/fs/cifs/smb2transport.c
@@ -148,25 +148,23 @@ smb2_setup_request(struct cifs_ses *ses, struct kvec *iov,
 	return rc;
 }
 
-int
-smb2_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
-			 unsigned int nvec, struct mid_q_entry **ret_mid)
+struct mid_q_entry *
+smb2_setup_async_request(struct TCP_Server_Info *server,
+			struct smb_rqst *rqst)
 {
-	int rc = 0;
-	struct smb2_hdr *hdr = (struct smb2_hdr *)iov[0].iov_base;
+	struct smb2_hdr *hdr = (struct smb2_hdr *)smb_rqst->iov[0].iov_base;
 	struct mid_q_entry *mid;
 
 	smb2_seq_num_into_buf(server, hdr);
 
 	mid = smb2_mid_entry_alloc(hdr, server);
 	if (mid == NULL)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	/* rc = smb2_sign_smb2(iov, nvec, server);
 	if (rc) {
 		DeleteMidQEntry(mid);
 		return rc;
 	}*/
-	*ret_mid = mid;
-	return rc;
+	return mid;
 }
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 5673ef1..73cfa6f 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -454,12 +454,11 @@ wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *midQ)
 	return 0;
 }
 
-int
-cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
-			 unsigned int nvec, struct mid_q_entry **ret_mid)
+struct mid_q_entry *
+cifs_setup_async_request(struct TCP_Server_Info *server, struct smb_rqst *rqst)
 {
 	int rc;
-	struct smb_hdr *hdr = (struct smb_hdr *)iov[0].iov_base;
+	struct smb_hdr *hdr = (struct smb_hdr *)rqst->rq_iov[0].iov_base;
 	struct mid_q_entry *mid;
 
 	/* enable signing if server requires it */
@@ -468,16 +467,15 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
 
 	mid = AllocMidQEntry(hdr, server);
 	if (mid == NULL)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
-	rc = cifs_sign_smbv(iov, nvec, server, &mid->sequence_number);
+	rc = cifs_sign_rqst(rqst, server, &mid->sequence_number);
 	if (rc) {
 		DeleteMidQEntry(mid);
-		return rc;
+		return ERR_PTR(rc);
 	}
 
-	*ret_mid = mid;
-	return 0;
+	return mid;
 }
 
 /*
@@ -485,9 +483,9 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
  * the result. Caller is responsible for dealing with timeouts.
  */
 int
-cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
-		unsigned int nvec, mid_receive_t *receive,
-		mid_callback_t *callback, void *cbdata, const int flags)
+cifs_call_async(struct TCP_Server_Info *server, struct smb_rqst *rqst,
+		mid_receive_t *receive, mid_callback_t *callback,
+		void *cbdata, const int flags)
 {
 	int rc, timeout, optype;
 	struct mid_q_entry *mid;
@@ -500,12 +498,12 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
 		return rc;
 
 	mutex_lock(&server->srv_mutex);
-	rc = server->ops->setup_async_request(server, iov, nvec, &mid);
-	if (rc) {
+	mid = server->ops->setup_async_request(server, rqst);
+	if (IS_ERR(mid)) {
 		mutex_unlock(&server->srv_mutex);
 		add_credits(server, 1, optype);
 		wake_up(&server->request_q);
-		return rc;
+		return PTR_ERR(mid);
 	}
 
 	mid->receive = receive;
@@ -520,7 +518,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
 
 
 	cifs_in_send_inc(server);
-	rc = smb_sendv(server, iov, nvec);
+	rc = smb_send_rqst(server, rqst);
 	cifs_in_send_dec(server);
 	cifs_save_when_sent(mid);
 	mutex_unlock(&server->srv_mutex);
-- 
1.7.11.2

Re: [PATCH v2 6/9] cifs: change cifs_call_async to use smb_rqst structs

[Pavel Shilovsky]

2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> For now, none of the callers populate rq_pages. That will be done for
> writes in a later patch. While we're at it, change the prototype of
> setup_async_request not to need a return pointer argument. Just
> return the pointer to the mid_q_entry or an ERR_PTR.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifsglob.h      |  4 ++--
>  fs/cifs/cifsproto.h     | 12 ++++++------
>  fs/cifs/cifssmb.c       | 20 +++++++++++++-------
>  fs/cifs/smb2proto.h     |  4 +---
>  fs/cifs/smb2transport.c | 14 ++++++--------
>  fs/cifs/transport.c     | 30 ++++++++++++++----------------
>  6 files changed, 42 insertions(+), 42 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 2a7e4b8..a0bb713 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -194,8 +194,8 @@ struct smb_version_operations {
>         int (*setup_request)(struct cifs_ses *, struct kvec *, unsigned int,
>                              struct mid_q_entry **);
>         /* setup async request: allocate mid, sign message */
> -       int (*setup_async_request)(struct TCP_Server_Info *, struct kvec *,
> -                                  unsigned int, struct mid_q_entry **);
> +       struct mid_q_entry *(*setup_async_request)(struct TCP_Server_Info *,
> +                                               struct smb_rqst *);
>         /* check response: verify signature, map error */
>         int (*check_receive)(struct mid_q_entry *, struct TCP_Server_Info *,
>                              bool);
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index 3620bec..8075f23 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -69,10 +69,10 @@ extern struct mid_q_entry *AllocMidQEntry(const struct smb_hdr *smb_buffer,
>                                         struct TCP_Server_Info *server);
>  extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
>  extern void cifs_wake_up_task(struct mid_q_entry *mid);
> -extern int cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
> -                          unsigned int nvec, mid_receive_t *receive,
> -                          mid_callback_t *callback, void *cbdata,
> -                          const int flags);
> +extern int cifs_call_async(struct TCP_Server_Info *server,
> +                       struct smb_rqst *rqst,
> +                       mid_receive_t *receive, mid_callback_t *callback,
> +                       void *cbdata, const int flags);
>  extern int SendReceive(const unsigned int /* xid */ , struct cifs_ses *,
>                         struct smb_hdr * /* input */ ,
>                         struct smb_hdr * /* out */ ,
> @@ -81,8 +81,8 @@ extern int SendReceiveNoRsp(const unsigned int xid, struct cifs_ses *ses,
>                             char *in_buf, int flags);
>  extern int cifs_setup_request(struct cifs_ses *, struct kvec *, unsigned int,
>                               struct mid_q_entry **);
> -extern int cifs_setup_async_request(struct TCP_Server_Info *, struct kvec *,
> -                                   unsigned int, struct mid_q_entry **);
> +extern struct mid_q_entry *cifs_setup_async_request(struct TCP_Server_Info *,
> +                                               struct smb_rqst *);
>  extern int cifs_check_receive(struct mid_q_entry *mid,
>                         struct TCP_Server_Info *server, bool log_error);
>  extern int SendReceive2(const unsigned int /* xid */ , struct cifs_ses *,
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index ce5f75c..7365460 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -751,6 +751,8 @@ CIFSSMBEcho(struct TCP_Server_Info *server)
>         ECHO_REQ *smb;
>         int rc = 0;
>         struct kvec iov;
> +       struct smb_rqst rqst = { .rq_iov = &iov,
> +                                .rq_nvec = 1 };
>
>         cFYI(1, "In echo request");
>
> @@ -768,7 +770,7 @@ CIFSSMBEcho(struct TCP_Server_Info *server)
>         iov.iov_base = smb;
>         iov.iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4;
>
> -       rc = cifs_call_async(server, &iov, 1, NULL, cifs_echo_callback,
> +       rc = cifs_call_async(server, &rqst, NULL, cifs_echo_callback,
>                              server, CIFS_ASYNC_OP | CIFS_ECHO_OP);
>         if (rc)
>                 cFYI(1, "Echo request failed: %d", rc);
> @@ -1604,6 +1606,8 @@ cifs_async_readv(struct cifs_readdata *rdata)
>         READ_REQ *smb = NULL;
>         int wct;
>         struct cifs_tcon *tcon = tlink_tcon(rdata->cfile->tlink);
> +       struct smb_rqst rqst = { .rq_iov = rdata->iov,
> +                                .rq_nvec = 1 };
>
>         cFYI(1, "%s: offset=%llu bytes=%u", __func__,
>                 rdata->offset, rdata->bytes);
> @@ -1647,9 +1651,8 @@ cifs_async_readv(struct cifs_readdata *rdata)
>         rdata->iov[0].iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4;
>
>         kref_get(&rdata->refcount);
> -       rc = cifs_call_async(tcon->ses->server, rdata->iov, 1,
> -                            cifs_readv_receive, cifs_readv_callback,
> -                            rdata, 0);
> +       rc = cifs_call_async(tcon->ses->server, &rqst, cifs_readv_receive,
> +                            cifs_readv_callback, rdata, 0);
>
>         if (rc == 0)
>                 cifs_stats_inc(&tcon->stats.cifs_stats.num_reads);
> @@ -2052,6 +2055,7 @@ cifs_async_writev(struct cifs_writedata *wdata)
>         int wct;
>         struct cifs_tcon *tcon = tlink_tcon(wdata->cfile->tlink);
>         struct kvec *iov = NULL;
> +       struct smb_rqst rqst = { };
>
>         if (tcon->ses->capabilities & CAP_LARGE_FILES) {
>                 wct = 14;
> @@ -2068,11 +2072,13 @@ cifs_async_writev(struct cifs_writedata *wdata)
>                 goto async_writev_out;
>
>         /* 1 iov per page + 1 for header */
> -       iov = kzalloc((wdata->nr_pages + 1) * sizeof(*iov), GFP_NOFS);
> +       rqst.rq_nvec = wdata->nr_pages + 1;
> +       iov = kzalloc((rqst.rq_nvec) * sizeof(*iov), GFP_NOFS);
>         if (iov == NULL) {
>                 rc = -ENOMEM;
>                 goto async_writev_out;
>         }
> +       rqst.rq_iov = iov;
>
>         smb->hdr.Pid = cpu_to_le16((__u16)wdata->pid);
>         smb->hdr.PidHigh = cpu_to_le16((__u16)(wdata->pid >> 16));
> @@ -2121,8 +2127,8 @@ cifs_async_writev(struct cifs_writedata *wdata)
>         }
>
>         kref_get(&wdata->refcount);
> -       rc = cifs_call_async(tcon->ses->server, iov, wdata->nr_pages + 1,
> -                            NULL, cifs_writev_callback, wdata, 0);
> +       rc = cifs_call_async(tcon->ses->server, &rqst, NULL,
> +                               cifs_writev_callback, wdata, 0);
>
>         if (rc == 0)
>                 cifs_stats_inc(&tcon->stats.cifs_stats.num_writes);
> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> index 902bbe2..dbb798c 100644
> --- a/fs/cifs/smb2proto.h
> +++ b/fs/cifs/smb2proto.h
> @@ -43,9 +43,7 @@ extern int smb2_check_receive(struct mid_q_entry *mid,
>                               struct TCP_Server_Info *server, bool log_error);
>  extern int smb2_setup_request(struct cifs_ses *ses, struct kvec *iov,
>                               unsigned int nvec, struct mid_q_entry **ret_mid);
> -extern int smb2_setup_async_request(struct TCP_Server_Info *server,
> -                                   struct kvec *iov, unsigned int nvec,
> -                                   struct mid_q_entry **ret_mid);
> +extern struct mid_q_entry *smb2_setup_async_request(struct TCP_Server_Info *server, struct smb_rqst *rqst);
>  extern void smb2_echo_request(struct work_struct *work);
>
>  extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
> index 31f5d42..c0a7a42 100644
> --- a/fs/cifs/smb2transport.c
> +++ b/fs/cifs/smb2transport.c
> @@ -148,25 +148,23 @@ smb2_setup_request(struct cifs_ses *ses, struct kvec *iov,
>         return rc;
>  }
>
> -int
> -smb2_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
> -                        unsigned int nvec, struct mid_q_entry **ret_mid)
> +struct mid_q_entry *
> +smb2_setup_async_request(struct TCP_Server_Info *server,
> +                       struct smb_rqst *rqst)
>  {
> -       int rc = 0;
> -       struct smb2_hdr *hdr = (struct smb2_hdr *)iov[0].iov_base;
> +       struct smb2_hdr *hdr = (struct smb2_hdr *)smb_rqst->iov[0].iov_base;
>         struct mid_q_entry *mid;
>
>         smb2_seq_num_into_buf(server, hdr);
>
>         mid = smb2_mid_entry_alloc(hdr, server);
>         if (mid == NULL)
> -               return -ENOMEM;
> +               return ERR_PTR(-ENOMEM);
>
>         /* rc = smb2_sign_smb2(iov, nvec, server);
>         if (rc) {
>                 DeleteMidQEntry(mid);
>                 return rc;
>         }*/
> -       *ret_mid = mid;
> -       return rc;
> +       return mid;
>  }
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 5673ef1..73cfa6f 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -454,12 +454,11 @@ wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *midQ)
>         return 0;
>  }
>
> -int
> -cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
> -                        unsigned int nvec, struct mid_q_entry **ret_mid)
> +struct mid_q_entry *
> +cifs_setup_async_request(struct TCP_Server_Info *server, struct smb_rqst *rqst)
>  {
>         int rc;
> -       struct smb_hdr *hdr = (struct smb_hdr *)iov[0].iov_base;
> +       struct smb_hdr *hdr = (struct smb_hdr *)rqst->rq_iov[0].iov_base;
>         struct mid_q_entry *mid;
>
>         /* enable signing if server requires it */
> @@ -468,16 +467,15 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
>
>         mid = AllocMidQEntry(hdr, server);
>         if (mid == NULL)
> -               return -ENOMEM;
> +               return ERR_PTR(-ENOMEM);
>
> -       rc = cifs_sign_smbv(iov, nvec, server, &mid->sequence_number);
> +       rc = cifs_sign_rqst(rqst, server, &mid->sequence_number);
>         if (rc) {
>                 DeleteMidQEntry(mid);
> -               return rc;
> +               return ERR_PTR(rc);
>         }
>
> -       *ret_mid = mid;
> -       return 0;
> +       return mid;
>  }
>
>  /*
> @@ -485,9 +483,9 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
>   * the result. Caller is responsible for dealing with timeouts.
>   */
>  int
> -cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
> -               unsigned int nvec, mid_receive_t *receive,
> -               mid_callback_t *callback, void *cbdata, const int flags)
> +cifs_call_async(struct TCP_Server_Info *server, struct smb_rqst *rqst,
> +               mid_receive_t *receive, mid_callback_t *callback,
> +               void *cbdata, const int flags)
>  {
>         int rc, timeout, optype;
>         struct mid_q_entry *mid;
> @@ -500,12 +498,12 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
>                 return rc;
>
>         mutex_lock(&server->srv_mutex);
> -       rc = server->ops->setup_async_request(server, iov, nvec, &mid);
> -       if (rc) {
> +       mid = server->ops->setup_async_request(server, rqst);
> +       if (IS_ERR(mid)) {
>                 mutex_unlock(&server->srv_mutex);
>                 add_credits(server, 1, optype);
>                 wake_up(&server->request_q);
> -               return rc;
> +               return PTR_ERR(mid);
>         }
>
>         mid->receive = receive;
> @@ -520,7 +518,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
>
>
>         cifs_in_send_inc(server);
> -       rc = smb_sendv(server, iov, nvec);
> +       rc = smb_send_rqst(server, rqst);
>         cifs_in_send_dec(server);
>         cifs_save_when_sent(mid);
>         mutex_unlock(&server->srv_mutex);
> --
> 1.7.11.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Like the idea of changing setup_async_request prototype.

Reviewed-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH v2 6/9] cifs: change cifs_call_async to use smb_rqst structs

[Pavel Shilovsky]

2012/7/25 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> 2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> For now, none of the callers populate rq_pages. That will be done for
>> writes in a later patch. While we're at it, change the prototype of
>> setup_async_request not to need a return pointer argument. Just
>> return the pointer to the mid_q_entry or an ERR_PTR.
>>
>> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  fs/cifs/cifsglob.h      |  4 ++--
>>  fs/cifs/cifsproto.h     | 12 ++++++------
>>  fs/cifs/cifssmb.c       | 20 +++++++++++++-------
>>  fs/cifs/smb2proto.h     |  4 +---
>>  fs/cifs/smb2transport.c | 14 ++++++--------
>>  fs/cifs/transport.c     | 30 ++++++++++++++----------------
>>  6 files changed, 42 insertions(+), 42 deletions(-)
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index 2a7e4b8..a0bb713 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -194,8 +194,8 @@ struct smb_version_operations {
>>         int (*setup_request)(struct cifs_ses *, struct kvec *, unsigned int,
>>                              struct mid_q_entry **);
>>         /* setup async request: allocate mid, sign message */
>> -       int (*setup_async_request)(struct TCP_Server_Info *, struct kvec *,
>> -                                  unsigned int, struct mid_q_entry **);
>> +       struct mid_q_entry *(*setup_async_request)(struct TCP_Server_Info *,
>> +                                               struct smb_rqst *);
>>         /* check response: verify signature, map error */
>>         int (*check_receive)(struct mid_q_entry *, struct TCP_Server_Info *,
>>                              bool);
>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>> index 3620bec..8075f23 100644
>> --- a/fs/cifs/cifsproto.h
>> +++ b/fs/cifs/cifsproto.h
>> @@ -69,10 +69,10 @@ extern struct mid_q_entry *AllocMidQEntry(const struct smb_hdr *smb_buffer,
>>                                         struct TCP_Server_Info *server);
>>  extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
>>  extern void cifs_wake_up_task(struct mid_q_entry *mid);
>> -extern int cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
>> -                          unsigned int nvec, mid_receive_t *receive,
>> -                          mid_callback_t *callback, void *cbdata,
>> -                          const int flags);
>> +extern int cifs_call_async(struct TCP_Server_Info *server,
>> +                       struct smb_rqst *rqst,
>> +                       mid_receive_t *receive, mid_callback_t *callback,
>> +                       void *cbdata, const int flags);
>>  extern int SendReceive(const unsigned int /* xid */ , struct cifs_ses *,
>>                         struct smb_hdr * /* input */ ,
>>                         struct smb_hdr * /* out */ ,
>> @@ -81,8 +81,8 @@ extern int SendReceiveNoRsp(const unsigned int xid, struct cifs_ses *ses,
>>                             char *in_buf, int flags);
>>  extern int cifs_setup_request(struct cifs_ses *, struct kvec *, unsigned int,
>>                               struct mid_q_entry **);
>> -extern int cifs_setup_async_request(struct TCP_Server_Info *, struct kvec *,
>> -                                   unsigned int, struct mid_q_entry **);
>> +extern struct mid_q_entry *cifs_setup_async_request(struct TCP_Server_Info *,
>> +                                               struct smb_rqst *);
>>  extern int cifs_check_receive(struct mid_q_entry *mid,
>>                         struct TCP_Server_Info *server, bool log_error);
>>  extern int SendReceive2(const unsigned int /* xid */ , struct cifs_ses *,
>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
>> index ce5f75c..7365460 100644
>> --- a/fs/cifs/cifssmb.c
>> +++ b/fs/cifs/cifssmb.c
>> @@ -751,6 +751,8 @@ CIFSSMBEcho(struct TCP_Server_Info *server)
>>         ECHO_REQ *smb;
>>         int rc = 0;
>>         struct kvec iov;
>> +       struct smb_rqst rqst = { .rq_iov = &iov,
>> +                                .rq_nvec = 1 };
>>
>>         cFYI(1, "In echo request");
>>
>> @@ -768,7 +770,7 @@ CIFSSMBEcho(struct TCP_Server_Info *server)
>>         iov.iov_base = smb;
>>         iov.iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4;
>>
>> -       rc = cifs_call_async(server, &iov, 1, NULL, cifs_echo_callback,
>> +       rc = cifs_call_async(server, &rqst, NULL, cifs_echo_callback,
>>                              server, CIFS_ASYNC_OP | CIFS_ECHO_OP);
>>         if (rc)
>>                 cFYI(1, "Echo request failed: %d", rc);
>> @@ -1604,6 +1606,8 @@ cifs_async_readv(struct cifs_readdata *rdata)
>>         READ_REQ *smb = NULL;
>>         int wct;
>>         struct cifs_tcon *tcon = tlink_tcon(rdata->cfile->tlink);
>> +       struct smb_rqst rqst = { .rq_iov = rdata->iov,
>> +                                .rq_nvec = 1 };
>>
>>         cFYI(1, "%s: offset=%llu bytes=%u", __func__,
>>                 rdata->offset, rdata->bytes);
>> @@ -1647,9 +1651,8 @@ cifs_async_readv(struct cifs_readdata *rdata)
>>         rdata->iov[0].iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4;
>>
>>         kref_get(&rdata->refcount);
>> -       rc = cifs_call_async(tcon->ses->server, rdata->iov, 1,
>> -                            cifs_readv_receive, cifs_readv_callback,
>> -                            rdata, 0);
>> +       rc = cifs_call_async(tcon->ses->server, &rqst, cifs_readv_receive,
>> +                            cifs_readv_callback, rdata, 0);
>>
>>         if (rc == 0)
>>                 cifs_stats_inc(&tcon->stats.cifs_stats.num_reads);
>> @@ -2052,6 +2055,7 @@ cifs_async_writev(struct cifs_writedata *wdata)
>>         int wct;
>>         struct cifs_tcon *tcon = tlink_tcon(wdata->cfile->tlink);
>>         struct kvec *iov = NULL;
>> +       struct smb_rqst rqst = { };
>>
>>         if (tcon->ses->capabilities & CAP_LARGE_FILES) {
>>                 wct = 14;
>> @@ -2068,11 +2072,13 @@ cifs_async_writev(struct cifs_writedata *wdata)
>>                 goto async_writev_out;
>>
>>         /* 1 iov per page + 1 for header */
>> -       iov = kzalloc((wdata->nr_pages + 1) * sizeof(*iov), GFP_NOFS);
>> +       rqst.rq_nvec = wdata->nr_pages + 1;
>> +       iov = kzalloc((rqst.rq_nvec) * sizeof(*iov), GFP_NOFS);
>>         if (iov == NULL) {
>>                 rc = -ENOMEM;
>>                 goto async_writev_out;
>>         }
>> +       rqst.rq_iov = iov;
>>
>>         smb->hdr.Pid = cpu_to_le16((__u16)wdata->pid);
>>         smb->hdr.PidHigh = cpu_to_le16((__u16)(wdata->pid >> 16));
>> @@ -2121,8 +2127,8 @@ cifs_async_writev(struct cifs_writedata *wdata)
>>         }
>>
>>         kref_get(&wdata->refcount);
>> -       rc = cifs_call_async(tcon->ses->server, iov, wdata->nr_pages + 1,
>> -                            NULL, cifs_writev_callback, wdata, 0);
>> +       rc = cifs_call_async(tcon->ses->server, &rqst, NULL,
>> +                               cifs_writev_callback, wdata, 0);
>>
>>         if (rc == 0)
>>                 cifs_stats_inc(&tcon->stats.cifs_stats.num_writes);
>> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
>> index 902bbe2..dbb798c 100644
>> --- a/fs/cifs/smb2proto.h
>> +++ b/fs/cifs/smb2proto.h
>> @@ -43,9 +43,7 @@ extern int smb2_check_receive(struct mid_q_entry *mid,
>>                               struct TCP_Server_Info *server, bool log_error);
>>  extern int smb2_setup_request(struct cifs_ses *ses, struct kvec *iov,
>>                               unsigned int nvec, struct mid_q_entry **ret_mid);
>> -extern int smb2_setup_async_request(struct TCP_Server_Info *server,
>> -                                   struct kvec *iov, unsigned int nvec,
>> -                                   struct mid_q_entry **ret_mid);
>> +extern struct mid_q_entry *smb2_setup_async_request(struct TCP_Server_Info *server, struct smb_rqst *rqst);

This exceeds 80 length and causes checkpatch.pl warnings.

>>  extern void smb2_echo_request(struct work_struct *work);
>>
>>  extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
>> diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
>> index 31f5d42..c0a7a42 100644
>> --- a/fs/cifs/smb2transport.c
>> +++ b/fs/cifs/smb2transport.c
>> @@ -148,25 +148,23 @@ smb2_setup_request(struct cifs_ses *ses, struct kvec *iov,
>>         return rc;
>>  }
>>
>> -int
>> -smb2_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
>> -                        unsigned int nvec, struct mid_q_entry **ret_mid)
>> +struct mid_q_entry *
>> +smb2_setup_async_request(struct TCP_Server_Info *server,
>> +                       struct smb_rqst *rqst)
>>  {
>> -       int rc = 0;
>> -       struct smb2_hdr *hdr = (struct smb2_hdr *)iov[0].iov_base;
>> +       struct smb2_hdr *hdr = (struct smb2_hdr *)smb_rqst->iov[0].iov_base;
>>         struct mid_q_entry *mid;
>>
>>         smb2_seq_num_into_buf(server, hdr);
>>
>>         mid = smb2_mid_entry_alloc(hdr, server);
>>         if (mid == NULL)
>> -               return -ENOMEM;
>> +               return ERR_PTR(-ENOMEM);
>>
>>         /* rc = smb2_sign_smb2(iov, nvec, server);
>>         if (rc) {
>>                 DeleteMidQEntry(mid);
>>                 return rc;
>>         }*/
>> -       *ret_mid = mid;
>> -       return rc;
>> +       return mid;
>>  }
>> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>> index 5673ef1..73cfa6f 100644
>> --- a/fs/cifs/transport.c
>> +++ b/fs/cifs/transport.c
>> @@ -454,12 +454,11 @@ wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *midQ)
>>         return 0;
>>  }
>>
>> -int
>> -cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
>> -                        unsigned int nvec, struct mid_q_entry **ret_mid)
>> +struct mid_q_entry *
>> +cifs_setup_async_request(struct TCP_Server_Info *server, struct smb_rqst *rqst)
>>  {
>>         int rc;
>> -       struct smb_hdr *hdr = (struct smb_hdr *)iov[0].iov_base;
>> +       struct smb_hdr *hdr = (struct smb_hdr *)rqst->rq_iov[0].iov_base;
>>         struct mid_q_entry *mid;
>>
>>         /* enable signing if server requires it */
>> @@ -468,16 +467,15 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
>>
>>         mid = AllocMidQEntry(hdr, server);
>>         if (mid == NULL)
>> -               return -ENOMEM;
>> +               return ERR_PTR(-ENOMEM);
>>
>> -       rc = cifs_sign_smbv(iov, nvec, server, &mid->sequence_number);
>> +       rc = cifs_sign_rqst(rqst, server, &mid->sequence_number);
>>         if (rc) {
>>                 DeleteMidQEntry(mid);
>> -               return rc;
>> +               return ERR_PTR(rc);
>>         }
>>
>> -       *ret_mid = mid;
>> -       return 0;
>> +       return mid;
>>  }
>>
>>  /*
>> @@ -485,9 +483,9 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
>>   * the result. Caller is responsible for dealing with timeouts.
>>   */
>>  int
>> -cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
>> -               unsigned int nvec, mid_receive_t *receive,
>> -               mid_callback_t *callback, void *cbdata, const int flags)
>> +cifs_call_async(struct TCP_Server_Info *server, struct smb_rqst *rqst,
>> +               mid_receive_t *receive, mid_callback_t *callback,
>> +               void *cbdata, const int flags)
>>  {
>>         int rc, timeout, optype;
>>         struct mid_q_entry *mid;
>> @@ -500,12 +498,12 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
>>                 return rc;
>>
>>         mutex_lock(&server->srv_mutex);
>> -       rc = server->ops->setup_async_request(server, iov, nvec, &mid);
>> -       if (rc) {
>> +       mid = server->ops->setup_async_request(server, rqst);
>> +       if (IS_ERR(mid)) {
>>                 mutex_unlock(&server->srv_mutex);
>>                 add_credits(server, 1, optype);
>>                 wake_up(&server->request_q);
>> -               return rc;
>> +               return PTR_ERR(mid);
>>         }
>>
>>         mid->receive = receive;
>> @@ -520,7 +518,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
>>
>>
>>         cifs_in_send_inc(server);
>> -       rc = smb_sendv(server, iov, nvec);
>> +       rc = smb_send_rqst(server, rqst);
>>         cifs_in_send_dec(server);
>>         cifs_save_when_sent(mid);
>>         mutex_unlock(&server->srv_mutex);
>> --
>> 1.7.11.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> Like the idea of changing setup_async_request prototype.
>
> Reviewed-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>
> --
> Best regards,
> Pavel Shilovsky.



-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH v2 6/9] cifs: change cifs_call_async to use smb_rqst structs

[Pavel Shilovsky]

2012/7/26 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> 2012/7/25 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>> 2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>>> For now, none of the callers populate rq_pages. That will be done for
>>> writes in a later patch. While we're at it, change the prototype of
>>> setup_async_request not to need a return pointer argument. Just
>>> return the pointer to the mid_q_entry or an ERR_PTR.
>>>
>>> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  fs/cifs/cifsglob.h      |  4 ++--
>>>  fs/cifs/cifsproto.h     | 12 ++++++------
>>>  fs/cifs/cifssmb.c       | 20 +++++++++++++-------
>>>  fs/cifs/smb2proto.h     |  4 +---
>>>  fs/cifs/smb2transport.c | 14 ++++++--------
>>>  fs/cifs/transport.c     | 30 ++++++++++++++----------------
>>>  6 files changed, 42 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>>> index 2a7e4b8..a0bb713 100644
>>> --- a/fs/cifs/cifsglob.h
>>> +++ b/fs/cifs/cifsglob.h
>>> @@ -194,8 +194,8 @@ struct smb_version_operations {
>>>         int (*setup_request)(struct cifs_ses *, struct kvec *, unsigned int,
>>>                              struct mid_q_entry **);
>>>         /* setup async request: allocate mid, sign message */
>>> -       int (*setup_async_request)(struct TCP_Server_Info *, struct kvec *,
>>> -                                  unsigned int, struct mid_q_entry **);
>>> +       struct mid_q_entry *(*setup_async_request)(struct TCP_Server_Info *,
>>> +                                               struct smb_rqst *);
>>>         /* check response: verify signature, map error */
>>>         int (*check_receive)(struct mid_q_entry *, struct TCP_Server_Info *,
>>>                              bool);
>>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>>> index 3620bec..8075f23 100644
>>> --- a/fs/cifs/cifsproto.h
>>> +++ b/fs/cifs/cifsproto.h
>>> @@ -69,10 +69,10 @@ extern struct mid_q_entry *AllocMidQEntry(const struct smb_hdr *smb_buffer,
>>>                                         struct TCP_Server_Info *server);
>>>  extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
>>>  extern void cifs_wake_up_task(struct mid_q_entry *mid);
>>> -extern int cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
>>> -                          unsigned int nvec, mid_receive_t *receive,
>>> -                          mid_callback_t *callback, void *cbdata,
>>> -                          const int flags);
>>> +extern int cifs_call_async(struct TCP_Server_Info *server,
>>> +                       struct smb_rqst *rqst,
>>> +                       mid_receive_t *receive, mid_callback_t *callback,
>>> +                       void *cbdata, const int flags);
>>>  extern int SendReceive(const unsigned int /* xid */ , struct cifs_ses *,
>>>                         struct smb_hdr * /* input */ ,
>>>                         struct smb_hdr * /* out */ ,
>>> @@ -81,8 +81,8 @@ extern int SendReceiveNoRsp(const unsigned int xid, struct cifs_ses *ses,
>>>                             char *in_buf, int flags);
>>>  extern int cifs_setup_request(struct cifs_ses *, struct kvec *, unsigned int,
>>>                               struct mid_q_entry **);
>>> -extern int cifs_setup_async_request(struct TCP_Server_Info *, struct kvec *,
>>> -                                   unsigned int, struct mid_q_entry **);
>>> +extern struct mid_q_entry *cifs_setup_async_request(struct TCP_Server_Info *,
>>> +                                               struct smb_rqst *);
>>>  extern int cifs_check_receive(struct mid_q_entry *mid,
>>>                         struct TCP_Server_Info *server, bool log_error);
>>>  extern int SendReceive2(const unsigned int /* xid */ , struct cifs_ses *,
>>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
>>> index ce5f75c..7365460 100644
>>> --- a/fs/cifs/cifssmb.c
>>> +++ b/fs/cifs/cifssmb.c
>>> @@ -751,6 +751,8 @@ CIFSSMBEcho(struct TCP_Server_Info *server)
>>>         ECHO_REQ *smb;
>>>         int rc = 0;
>>>         struct kvec iov;
>>> +       struct smb_rqst rqst = { .rq_iov = &iov,
>>> +                                .rq_nvec = 1 };
>>>
>>>         cFYI(1, "In echo request");
>>>
>>> @@ -768,7 +770,7 @@ CIFSSMBEcho(struct TCP_Server_Info *server)
>>>         iov.iov_base = smb;
>>>         iov.iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4;
>>>
>>> -       rc = cifs_call_async(server, &iov, 1, NULL, cifs_echo_callback,
>>> +       rc = cifs_call_async(server, &rqst, NULL, cifs_echo_callback,
>>>                              server, CIFS_ASYNC_OP | CIFS_ECHO_OP);
>>>         if (rc)
>>>                 cFYI(1, "Echo request failed: %d", rc);
>>> @@ -1604,6 +1606,8 @@ cifs_async_readv(struct cifs_readdata *rdata)
>>>         READ_REQ *smb = NULL;
>>>         int wct;
>>>         struct cifs_tcon *tcon = tlink_tcon(rdata->cfile->tlink);
>>> +       struct smb_rqst rqst = { .rq_iov = rdata->iov,
>>> +                                .rq_nvec = 1 };
>>>
>>>         cFYI(1, "%s: offset=%llu bytes=%u", __func__,
>>>                 rdata->offset, rdata->bytes);
>>> @@ -1647,9 +1651,8 @@ cifs_async_readv(struct cifs_readdata *rdata)
>>>         rdata->iov[0].iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4;
>>>
>>>         kref_get(&rdata->refcount);
>>> -       rc = cifs_call_async(tcon->ses->server, rdata->iov, 1,
>>> -                            cifs_readv_receive, cifs_readv_callback,
>>> -                            rdata, 0);
>>> +       rc = cifs_call_async(tcon->ses->server, &rqst, cifs_readv_receive,
>>> +                            cifs_readv_callback, rdata, 0);
>>>
>>>         if (rc == 0)
>>>                 cifs_stats_inc(&tcon->stats.cifs_stats.num_reads);
>>> @@ -2052,6 +2055,7 @@ cifs_async_writev(struct cifs_writedata *wdata)
>>>         int wct;
>>>         struct cifs_tcon *tcon = tlink_tcon(wdata->cfile->tlink);
>>>         struct kvec *iov = NULL;
>>> +       struct smb_rqst rqst = { };
>>>
>>>         if (tcon->ses->capabilities & CAP_LARGE_FILES) {
>>>                 wct = 14;
>>> @@ -2068,11 +2072,13 @@ cifs_async_writev(struct cifs_writedata *wdata)
>>>                 goto async_writev_out;
>>>
>>>         /* 1 iov per page + 1 for header */
>>> -       iov = kzalloc((wdata->nr_pages + 1) * sizeof(*iov), GFP_NOFS);
>>> +       rqst.rq_nvec = wdata->nr_pages + 1;
>>> +       iov = kzalloc((rqst.rq_nvec) * sizeof(*iov), GFP_NOFS);
>>>         if (iov == NULL) {
>>>                 rc = -ENOMEM;
>>>                 goto async_writev_out;
>>>         }
>>> +       rqst.rq_iov = iov;
>>>
>>>         smb->hdr.Pid = cpu_to_le16((__u16)wdata->pid);
>>>         smb->hdr.PidHigh = cpu_to_le16((__u16)(wdata->pid >> 16));
>>> @@ -2121,8 +2127,8 @@ cifs_async_writev(struct cifs_writedata *wdata)
>>>         }
>>>
>>>         kref_get(&wdata->refcount);
>>> -       rc = cifs_call_async(tcon->ses->server, iov, wdata->nr_pages + 1,
>>> -                            NULL, cifs_writev_callback, wdata, 0);
>>> +       rc = cifs_call_async(tcon->ses->server, &rqst, NULL,
>>> +                               cifs_writev_callback, wdata, 0);
>>>
>>>         if (rc == 0)
>>>                 cifs_stats_inc(&tcon->stats.cifs_stats.num_writes);
>>> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
>>> index 902bbe2..dbb798c 100644
>>> --- a/fs/cifs/smb2proto.h
>>> +++ b/fs/cifs/smb2proto.h
>>> @@ -43,9 +43,7 @@ extern int smb2_check_receive(struct mid_q_entry *mid,
>>>                               struct TCP_Server_Info *server, bool log_error);
>>>  extern int smb2_setup_request(struct cifs_ses *ses, struct kvec *iov,
>>>                               unsigned int nvec, struct mid_q_entry **ret_mid);
>>> -extern int smb2_setup_async_request(struct TCP_Server_Info *server,
>>> -                                   struct kvec *iov, unsigned int nvec,
>>> -                                   struct mid_q_entry **ret_mid);
>>> +extern struct mid_q_entry *smb2_setup_async_request(struct TCP_Server_Info *server, struct smb_rqst *rqst);
>
> This exceeds 80 length and causes checkpatch.pl warnings.
>
>>>  extern void smb2_echo_request(struct work_struct *work);
>>>
>>>  extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
>>> diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
>>> index 31f5d42..c0a7a42 100644
>>> --- a/fs/cifs/smb2transport.c
>>> +++ b/fs/cifs/smb2transport.c
>>> @@ -148,25 +148,23 @@ smb2_setup_request(struct cifs_ses *ses, struct kvec *iov,
>>>         return rc;
>>>  }
>>>
>>> -int
>>> -smb2_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
>>> -                        unsigned int nvec, struct mid_q_entry **ret_mid)
>>> +struct mid_q_entry *
>>> +smb2_setup_async_request(struct TCP_Server_Info *server,
>>> +                       struct smb_rqst *rqst)
>>>  {
>>> -       int rc = 0;
>>> -       struct smb2_hdr *hdr = (struct smb2_hdr *)iov[0].iov_base;
>>> +       struct smb2_hdr *hdr = (struct smb2_hdr *)smb_rqst->iov[0].iov_base;
>>>         struct mid_q_entry *mid;
>>>
>>>         smb2_seq_num_into_buf(server, hdr);
>>>
>>>         mid = smb2_mid_entry_alloc(hdr, server);
>>>         if (mid == NULL)
>>> -               return -ENOMEM;
>>> +               return ERR_PTR(-ENOMEM);
>>>
>>>         /* rc = smb2_sign_smb2(iov, nvec, server);
>>>         if (rc) {
>>>                 DeleteMidQEntry(mid);
>>>                 return rc;
>>>         }*/
>>> -       *ret_mid = mid;
>>> -       return rc;
>>> +       return mid;
>>>  }
>>> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>>> index 5673ef1..73cfa6f 100644
>>> --- a/fs/cifs/transport.c
>>> +++ b/fs/cifs/transport.c
>>> @@ -454,12 +454,11 @@ wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *midQ)
>>>         return 0;
>>>  }
>>>
>>> -int
>>> -cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
>>> -                        unsigned int nvec, struct mid_q_entry **ret_mid)
>>> +struct mid_q_entry *
>>> +cifs_setup_async_request(struct TCP_Server_Info *server, struct smb_rqst *rqst)
>>>  {
>>>         int rc;
>>> -       struct smb_hdr *hdr = (struct smb_hdr *)iov[0].iov_base;
>>> +       struct smb_hdr *hdr = (struct smb_hdr *)rqst->rq_iov[0].iov_base;
>>>         struct mid_q_entry *mid;
>>>
>>>         /* enable signing if server requires it */
>>> @@ -468,16 +467,15 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
>>>
>>>         mid = AllocMidQEntry(hdr, server);
>>>         if (mid == NULL)
>>> -               return -ENOMEM;
>>> +               return ERR_PTR(-ENOMEM);
>>>
>>> -       rc = cifs_sign_smbv(iov, nvec, server, &mid->sequence_number);
>>> +       rc = cifs_sign_rqst(rqst, server, &mid->sequence_number);
>>>         if (rc) {
>>>                 DeleteMidQEntry(mid);
>>> -               return rc;
>>> +               return ERR_PTR(rc);
>>>         }
>>>
>>> -       *ret_mid = mid;
>>> -       return 0;
>>> +       return mid;
>>>  }
>>>
>>>  /*
>>> @@ -485,9 +483,9 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
>>>   * the result. Caller is responsible for dealing with timeouts.
>>>   */
>>>  int
>>> -cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
>>> -               unsigned int nvec, mid_receive_t *receive,
>>> -               mid_callback_t *callback, void *cbdata, const int flags)
>>> +cifs_call_async(struct TCP_Server_Info *server, struct smb_rqst *rqst,
>>> +               mid_receive_t *receive, mid_callback_t *callback,
>>> +               void *cbdata, const int flags)
>>>  {
>>>         int rc, timeout, optype;
>>>         struct mid_q_entry *mid;
>>> @@ -500,12 +498,12 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
>>>                 return rc;
>>>
>>>         mutex_lock(&server->srv_mutex);
>>> -       rc = server->ops->setup_async_request(server, iov, nvec, &mid);
>>> -       if (rc) {
>>> +       mid = server->ops->setup_async_request(server, rqst);
>>> +       if (IS_ERR(mid)) {
>>>                 mutex_unlock(&server->srv_mutex);
>>>                 add_credits(server, 1, optype);
>>>                 wake_up(&server->request_q);
>>> -               return rc;
>>> +               return PTR_ERR(mid);
>>>         }
>>>
>>>         mid->receive = receive;
>>> @@ -520,7 +518,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
>>>
>>>
>>>         cifs_in_send_inc(server);
>>> -       rc = smb_sendv(server, iov, nvec);
>>> +       rc = smb_send_rqst(server, rqst);
>>>         cifs_in_send_dec(server);
>>>         cifs_save_when_sent(mid);
>>>         mutex_unlock(&server->srv_mutex);
>>> --
>>> 1.7.11.2
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> Like the idea of changing setup_async_request prototype.
>>
>> Reviewed-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>>
>> --
>> Best regards,
>> Pavel Shilovsky.
>
>
>
> --
> Best regards,
> Pavel Shilovsky.

Also it doesn't build - needs smth like this:

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 62b3f17..a525db4 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1102,6 +1102,8 @@ SMB2_echo(struct TCP_Server_Info *server)
        struct smb2_echo_req *req;
        int rc = 0;
        struct kvec iov;
+       struct smb_rqst rqst = { .rq_iov = &iov,
+                                .rq_nvec = 1 };

        cFYI(1, "In echo request");

@@ -1115,7 +1117,7 @@ SMB2_echo(struct TCP_Server_Info *server)
        /* 4 for rfc1002 length field */
        iov.iov_len = get_rfc1002_length(req) + 4;

-       rc = cifs_call_async(server, &iov, 1, NULL, smb2_echo_callback, server,
+       rc = cifs_call_async(server, &rqst, NULL, smb2_echo_callback, server,
                             CIFS_ECHO_OP);
        if (rc)
                cFYI(1, "Echo request failed: %d", rc);
diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
index c0a7a42..ee20740 100644
--- a/fs/cifs/smb2transport.c
+++ b/fs/cifs/smb2transport.c
@@ -152,7 +152,7 @@ struct mid_q_entry *
 smb2_setup_async_request(struct TCP_Server_Info *server,
                        struct smb_rqst *rqst)
 {
-       struct smb2_hdr *hdr = (struct smb2_hdr *)smb_rqst->iov[0].iov_base;
+       struct smb2_hdr *hdr = (struct smb2_hdr *)rqst->rq_iov[0].iov_base;
        struct mid_q_entry *mid;

        smb2_seq_num_into_buf(server, hdr);

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH v2 6/9] cifs: change cifs_call_async to use smb_rqst structs

[Jeff Layton]

On Thu, 26 Jul 2012 12:54:10 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2012/7/26 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> > 2012/7/25 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> >> 2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >>> For now, none of the callers populate rq_pages. That will be done for
> >>> writes in a later patch. While we're at it, change the prototype of
> >>> setup_async_request not to need a return pointer argument. Just
> >>> return the pointer to the mid_q_entry or an ERR_PTR.
> >>>
> >>> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>> ---
> >>>  fs/cifs/cifsglob.h      |  4 ++--
> >>>  fs/cifs/cifsproto.h     | 12 ++++++------
> >>>  fs/cifs/cifssmb.c       | 20 +++++++++++++-------
> >>>  fs/cifs/smb2proto.h     |  4 +---
> >>>  fs/cifs/smb2transport.c | 14 ++++++--------
> >>>  fs/cifs/transport.c     | 30 ++++++++++++++----------------
> >>>  6 files changed, 42 insertions(+), 42 deletions(-)
> >>>
> >>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >>> index 2a7e4b8..a0bb713 100644
> >>> --- a/fs/cifs/cifsglob.h
> >>> +++ b/fs/cifs/cifsglob.h
> >>> @@ -194,8 +194,8 @@ struct smb_version_operations {
> >>>         int (*setup_request)(struct cifs_ses *, struct kvec *, unsigned int,
> >>>                              struct mid_q_entry **);
> >>>         /* setup async request: allocate mid, sign message */
> >>> -       int (*setup_async_request)(struct TCP_Server_Info *, struct kvec *,
> >>> -                                  unsigned int, struct mid_q_entry **);
> >>> +       struct mid_q_entry *(*setup_async_request)(struct TCP_Server_Info *,
> >>> +                                               struct smb_rqst *);
> >>>         /* check response: verify signature, map error */
> >>>         int (*check_receive)(struct mid_q_entry *, struct TCP_Server_Info *,
> >>>                              bool);
> >>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> >>> index 3620bec..8075f23 100644
> >>> --- a/fs/cifs/cifsproto.h
> >>> +++ b/fs/cifs/cifsproto.h
> >>> @@ -69,10 +69,10 @@ extern struct mid_q_entry *AllocMidQEntry(const struct smb_hdr *smb_buffer,
> >>>                                         struct TCP_Server_Info *server);
> >>>  extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
> >>>  extern void cifs_wake_up_task(struct mid_q_entry *mid);
> >>> -extern int cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
> >>> -                          unsigned int nvec, mid_receive_t *receive,
> >>> -                          mid_callback_t *callback, void *cbdata,
> >>> -                          const int flags);
> >>> +extern int cifs_call_async(struct TCP_Server_Info *server,
> >>> +                       struct smb_rqst *rqst,
> >>> +                       mid_receive_t *receive, mid_callback_t *callback,
> >>> +                       void *cbdata, const int flags);
> >>>  extern int SendReceive(const unsigned int /* xid */ , struct cifs_ses *,
> >>>                         struct smb_hdr * /* input */ ,
> >>>                         struct smb_hdr * /* out */ ,
> >>> @@ -81,8 +81,8 @@ extern int SendReceiveNoRsp(const unsigned int xid, struct cifs_ses *ses,
> >>>                             char *in_buf, int flags);
> >>>  extern int cifs_setup_request(struct cifs_ses *, struct kvec *, unsigned int,
> >>>                               struct mid_q_entry **);
> >>> -extern int cifs_setup_async_request(struct TCP_Server_Info *, struct kvec *,
> >>> -                                   unsigned int, struct mid_q_entry **);
> >>> +extern struct mid_q_entry *cifs_setup_async_request(struct TCP_Server_Info *,
> >>> +                                               struct smb_rqst *);
> >>>  extern int cifs_check_receive(struct mid_q_entry *mid,
> >>>                         struct TCP_Server_Info *server, bool log_error);
> >>>  extern int SendReceive2(const unsigned int /* xid */ , struct cifs_ses *,
> >>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> >>> index ce5f75c..7365460 100644
> >>> --- a/fs/cifs/cifssmb.c
> >>> +++ b/fs/cifs/cifssmb.c
> >>> @@ -751,6 +751,8 @@ CIFSSMBEcho(struct TCP_Server_Info *server)
> >>>         ECHO_REQ *smb;
> >>>         int rc = 0;
> >>>         struct kvec iov;
> >>> +       struct smb_rqst rqst = { .rq_iov = &iov,
> >>> +                                .rq_nvec = 1 };
> >>>
> >>>         cFYI(1, "In echo request");
> >>>
> >>> @@ -768,7 +770,7 @@ CIFSSMBEcho(struct TCP_Server_Info *server)
> >>>         iov.iov_base = smb;
> >>>         iov.iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4;
> >>>
> >>> -       rc = cifs_call_async(server, &iov, 1, NULL, cifs_echo_callback,
> >>> +       rc = cifs_call_async(server, &rqst, NULL, cifs_echo_callback,
> >>>                              server, CIFS_ASYNC_OP | CIFS_ECHO_OP);
> >>>         if (rc)
> >>>                 cFYI(1, "Echo request failed: %d", rc);
> >>> @@ -1604,6 +1606,8 @@ cifs_async_readv(struct cifs_readdata *rdata)
> >>>         READ_REQ *smb = NULL;
> >>>         int wct;
> >>>         struct cifs_tcon *tcon = tlink_tcon(rdata->cfile->tlink);
> >>> +       struct smb_rqst rqst = { .rq_iov = rdata->iov,
> >>> +                                .rq_nvec = 1 };
> >>>
> >>>         cFYI(1, "%s: offset=%llu bytes=%u", __func__,
> >>>                 rdata->offset, rdata->bytes);
> >>> @@ -1647,9 +1651,8 @@ cifs_async_readv(struct cifs_readdata *rdata)
> >>>         rdata->iov[0].iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4;
> >>>
> >>>         kref_get(&rdata->refcount);
> >>> -       rc = cifs_call_async(tcon->ses->server, rdata->iov, 1,
> >>> -                            cifs_readv_receive, cifs_readv_callback,
> >>> -                            rdata, 0);
> >>> +       rc = cifs_call_async(tcon->ses->server, &rqst, cifs_readv_receive,
> >>> +                            cifs_readv_callback, rdata, 0);
> >>>
> >>>         if (rc == 0)
> >>>                 cifs_stats_inc(&tcon->stats.cifs_stats.num_reads);
> >>> @@ -2052,6 +2055,7 @@ cifs_async_writev(struct cifs_writedata *wdata)
> >>>         int wct;
> >>>         struct cifs_tcon *tcon = tlink_tcon(wdata->cfile->tlink);
> >>>         struct kvec *iov = NULL;
> >>> +       struct smb_rqst rqst = { };
> >>>
> >>>         if (tcon->ses->capabilities & CAP_LARGE_FILES) {
> >>>                 wct = 14;
> >>> @@ -2068,11 +2072,13 @@ cifs_async_writev(struct cifs_writedata *wdata)
> >>>                 goto async_writev_out;
> >>>
> >>>         /* 1 iov per page + 1 for header */
> >>> -       iov = kzalloc((wdata->nr_pages + 1) * sizeof(*iov), GFP_NOFS);
> >>> +       rqst.rq_nvec = wdata->nr_pages + 1;
> >>> +       iov = kzalloc((rqst.rq_nvec) * sizeof(*iov), GFP_NOFS);
> >>>         if (iov == NULL) {
> >>>                 rc = -ENOMEM;
> >>>                 goto async_writev_out;
> >>>         }
> >>> +       rqst.rq_iov = iov;
> >>>
> >>>         smb->hdr.Pid = cpu_to_le16((__u16)wdata->pid);
> >>>         smb->hdr.PidHigh = cpu_to_le16((__u16)(wdata->pid >> 16));
> >>> @@ -2121,8 +2127,8 @@ cifs_async_writev(struct cifs_writedata *wdata)
> >>>         }
> >>>
> >>>         kref_get(&wdata->refcount);
> >>> -       rc = cifs_call_async(tcon->ses->server, iov, wdata->nr_pages + 1,
> >>> -                            NULL, cifs_writev_callback, wdata, 0);
> >>> +       rc = cifs_call_async(tcon->ses->server, &rqst, NULL,
> >>> +                               cifs_writev_callback, wdata, 0);
> >>>
> >>>         if (rc == 0)
> >>>                 cifs_stats_inc(&tcon->stats.cifs_stats.num_writes);
> >>> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> >>> index 902bbe2..dbb798c 100644
> >>> --- a/fs/cifs/smb2proto.h
> >>> +++ b/fs/cifs/smb2proto.h
> >>> @@ -43,9 +43,7 @@ extern int smb2_check_receive(struct mid_q_entry *mid,
> >>>                               struct TCP_Server_Info *server, bool log_error);
> >>>  extern int smb2_setup_request(struct cifs_ses *ses, struct kvec *iov,
> >>>                               unsigned int nvec, struct mid_q_entry **ret_mid);
> >>> -extern int smb2_setup_async_request(struct TCP_Server_Info *server,
> >>> -                                   struct kvec *iov, unsigned int nvec,
> >>> -                                   struct mid_q_entry **ret_mid);
> >>> +extern struct mid_q_entry *smb2_setup_async_request(struct TCP_Server_Info *server, struct smb_rqst *rqst);
> >
> > This exceeds 80 length and causes checkpatch.pl warnings.
> >
> >>>  extern void smb2_echo_request(struct work_struct *work);
> >>>
> >>>  extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> >>> diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
> >>> index 31f5d42..c0a7a42 100644
> >>> --- a/fs/cifs/smb2transport.c
> >>> +++ b/fs/cifs/smb2transport.c
> >>> @@ -148,25 +148,23 @@ smb2_setup_request(struct cifs_ses *ses, struct kvec *iov,
> >>>         return rc;
> >>>  }
> >>>
> >>> -int
> >>> -smb2_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
> >>> -                        unsigned int nvec, struct mid_q_entry **ret_mid)
> >>> +struct mid_q_entry *
> >>> +smb2_setup_async_request(struct TCP_Server_Info *server,
> >>> +                       struct smb_rqst *rqst)
> >>>  {
> >>> -       int rc = 0;
> >>> -       struct smb2_hdr *hdr = (struct smb2_hdr *)iov[0].iov_base;
> >>> +       struct smb2_hdr *hdr = (struct smb2_hdr *)smb_rqst->iov[0].iov_base;
> >>>         struct mid_q_entry *mid;
> >>>
> >>>         smb2_seq_num_into_buf(server, hdr);
> >>>
> >>>         mid = smb2_mid_entry_alloc(hdr, server);
> >>>         if (mid == NULL)
> >>> -               return -ENOMEM;
> >>> +               return ERR_PTR(-ENOMEM);
> >>>
> >>>         /* rc = smb2_sign_smb2(iov, nvec, server);
> >>>         if (rc) {
> >>>                 DeleteMidQEntry(mid);
> >>>                 return rc;
> >>>         }*/
> >>> -       *ret_mid = mid;
> >>> -       return rc;
> >>> +       return mid;
> >>>  }
> >>> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> >>> index 5673ef1..73cfa6f 100644
> >>> --- a/fs/cifs/transport.c
> >>> +++ b/fs/cifs/transport.c
> >>> @@ -454,12 +454,11 @@ wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *midQ)
> >>>         return 0;
> >>>  }
> >>>
> >>> -int
> >>> -cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
> >>> -                        unsigned int nvec, struct mid_q_entry **ret_mid)
> >>> +struct mid_q_entry *
> >>> +cifs_setup_async_request(struct TCP_Server_Info *server, struct smb_rqst *rqst)
> >>>  {
> >>>         int rc;
> >>> -       struct smb_hdr *hdr = (struct smb_hdr *)iov[0].iov_base;
> >>> +       struct smb_hdr *hdr = (struct smb_hdr *)rqst->rq_iov[0].iov_base;
> >>>         struct mid_q_entry *mid;
> >>>
> >>>         /* enable signing if server requires it */
> >>> @@ -468,16 +467,15 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
> >>>
> >>>         mid = AllocMidQEntry(hdr, server);
> >>>         if (mid == NULL)
> >>> -               return -ENOMEM;
> >>> +               return ERR_PTR(-ENOMEM);
> >>>
> >>> -       rc = cifs_sign_smbv(iov, nvec, server, &mid->sequence_number);
> >>> +       rc = cifs_sign_rqst(rqst, server, &mid->sequence_number);
> >>>         if (rc) {
> >>>                 DeleteMidQEntry(mid);
> >>> -               return rc;
> >>> +               return ERR_PTR(rc);
> >>>         }
> >>>
> >>> -       *ret_mid = mid;
> >>> -       return 0;
> >>> +       return mid;
> >>>  }
> >>>
> >>>  /*
> >>> @@ -485,9 +483,9 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
> >>>   * the result. Caller is responsible for dealing with timeouts.
> >>>   */
> >>>  int
> >>> -cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
> >>> -               unsigned int nvec, mid_receive_t *receive,
> >>> -               mid_callback_t *callback, void *cbdata, const int flags)
> >>> +cifs_call_async(struct TCP_Server_Info *server, struct smb_rqst *rqst,
> >>> +               mid_receive_t *receive, mid_callback_t *callback,
> >>> +               void *cbdata, const int flags)
> >>>  {
> >>>         int rc, timeout, optype;
> >>>         struct mid_q_entry *mid;
> >>> @@ -500,12 +498,12 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
> >>>                 return rc;
> >>>
> >>>         mutex_lock(&server->srv_mutex);
> >>> -       rc = server->ops->setup_async_request(server, iov, nvec, &mid);
> >>> -       if (rc) {
> >>> +       mid = server->ops->setup_async_request(server, rqst);
> >>> +       if (IS_ERR(mid)) {
> >>>                 mutex_unlock(&server->srv_mutex);
> >>>                 add_credits(server, 1, optype);
> >>>                 wake_up(&server->request_q);
> >>> -               return rc;
> >>> +               return PTR_ERR(mid);
> >>>         }
> >>>
> >>>         mid->receive = receive;
> >>> @@ -520,7 +518,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
> >>>
> >>>
> >>>         cifs_in_send_inc(server);
> >>> -       rc = smb_sendv(server, iov, nvec);
> >>> +       rc = smb_send_rqst(server, rqst);
> >>>         cifs_in_send_dec(server);
> >>>         cifs_save_when_sent(mid);
> >>>         mutex_unlock(&server->srv_mutex);
> >>> --
> >>> 1.7.11.2
> >>>
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> >>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> >> Like the idea of changing setup_async_request prototype.
> >>
> >> Reviewed-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> >>
> >> --
> >> Best regards,
> >> Pavel Shilovsky.
> >
> >
> >
> > --
> > Best regards,
> > Pavel Shilovsky.
> 
> Also it doesn't build - needs smth like this:
> 
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 62b3f17..a525db4 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -1102,6 +1102,8 @@ SMB2_echo(struct TCP_Server_Info *server)
>         struct smb2_echo_req *req;
>         int rc = 0;
>         struct kvec iov;
> +       struct smb_rqst rqst = { .rq_iov = &iov,
> +                                .rq_nvec = 1 };
> 
>         cFYI(1, "In echo request");
> 
> @@ -1115,7 +1117,7 @@ SMB2_echo(struct TCP_Server_Info *server)
>         /* 4 for rfc1002 length field */
>         iov.iov_len = get_rfc1002_length(req) + 4;
> 
> -       rc = cifs_call_async(server, &iov, 1, NULL, smb2_echo_callback, server,
> +       rc = cifs_call_async(server, &rqst, NULL, smb2_echo_callback, server,
>                              CIFS_ECHO_OP);
>         if (rc)
>                 cFYI(1, "Echo request failed: %d", rc);
> diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
> index c0a7a42..ee20740 100644
> --- a/fs/cifs/smb2transport.c
> +++ b/fs/cifs/smb2transport.c
> @@ -152,7 +152,7 @@ struct mid_q_entry *
>  smb2_setup_async_request(struct TCP_Server_Info *server,
>                         struct smb_rqst *rqst)
>  {
> -       struct smb2_hdr *hdr = (struct smb2_hdr *)smb_rqst->iov[0].iov_base;
> +       struct smb2_hdr *hdr = (struct smb2_hdr *)rqst->rq_iov[0].iov_base;
>         struct mid_q_entry *mid;
> 
>         smb2_seq_num_into_buf(server, hdr);
> 

My bad -- I didn't test the build with CONFIG_CIFS_SMB2 enabled. I'll
respin the patch.

Thanks,
-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH v2 6/9] cifs: change cifs_call_async to use smb_rqst structs

[Jeff Layton]

On Thu, 26 Jul 2012 12:54:10 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2012/7/26 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> > 2012/7/25 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> >> 2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >>> For now, none of the callers populate rq_pages. That will be done for
> >>> writes in a later patch. While we're at it, change the prototype of
> >>> setup_async_request not to need a return pointer argument. Just
> >>> return the pointer to the mid_q_entry or an ERR_PTR.
> >>>
> >>> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>> ---
> >>>  fs/cifs/cifsglob.h      |  4 ++--
> >>>  fs/cifs/cifsproto.h     | 12 ++++++------
> >>>  fs/cifs/cifssmb.c       | 20 +++++++++++++-------
> >>>  fs/cifs/smb2proto.h     |  4 +---
> >>>  fs/cifs/smb2transport.c | 14 ++++++--------
> >>>  fs/cifs/transport.c     | 30 ++++++++++++++----------------
> >>>  6 files changed, 42 insertions(+), 42 deletions(-)
> >>>
> >>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >>> index 2a7e4b8..a0bb713 100644
> >>> --- a/fs/cifs/cifsglob.h
> >>> +++ b/fs/cifs/cifsglob.h
> >>> @@ -194,8 +194,8 @@ struct smb_version_operations {
> >>>         int (*setup_request)(struct cifs_ses *, struct kvec *, unsigned int,
> >>>                              struct mid_q_entry **);
> >>>         /* setup async request: allocate mid, sign message */
> >>> -       int (*setup_async_request)(struct TCP_Server_Info *, struct kvec *,
> >>> -                                  unsigned int, struct mid_q_entry **);
> >>> +       struct mid_q_entry *(*setup_async_request)(struct TCP_Server_Info *,
> >>> +                                               struct smb_rqst *);
> >>>         /* check response: verify signature, map error */
> >>>         int (*check_receive)(struct mid_q_entry *, struct TCP_Server_Info *,
> >>>                              bool);
> >>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> >>> index 3620bec..8075f23 100644
> >>> --- a/fs/cifs/cifsproto.h
> >>> +++ b/fs/cifs/cifsproto.h
> >>> @@ -69,10 +69,10 @@ extern struct mid_q_entry *AllocMidQEntry(const struct smb_hdr *smb_buffer,
> >>>                                         struct TCP_Server_Info *server);
> >>>  extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
> >>>  extern void cifs_wake_up_task(struct mid_q_entry *mid);
> >>> -extern int cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
> >>> -                          unsigned int nvec, mid_receive_t *receive,
> >>> -                          mid_callback_t *callback, void *cbdata,
> >>> -                          const int flags);
> >>> +extern int cifs_call_async(struct TCP_Server_Info *server,
> >>> +                       struct smb_rqst *rqst,
> >>> +                       mid_receive_t *receive, mid_callback_t *callback,
> >>> +                       void *cbdata, const int flags);
> >>>  extern int SendReceive(const unsigned int /* xid */ , struct cifs_ses *,
> >>>                         struct smb_hdr * /* input */ ,
> >>>                         struct smb_hdr * /* out */ ,
> >>> @@ -81,8 +81,8 @@ extern int SendReceiveNoRsp(const unsigned int xid, struct cifs_ses *ses,
> >>>                             char *in_buf, int flags);
> >>>  extern int cifs_setup_request(struct cifs_ses *, struct kvec *, unsigned int,
> >>>                               struct mid_q_entry **);
> >>> -extern int cifs_setup_async_request(struct TCP_Server_Info *, struct kvec *,
> >>> -                                   unsigned int, struct mid_q_entry **);
> >>> +extern struct mid_q_entry *cifs_setup_async_request(struct TCP_Server_Info *,
> >>> +                                               struct smb_rqst *);
> >>>  extern int cifs_check_receive(struct mid_q_entry *mid,
> >>>                         struct TCP_Server_Info *server, bool log_error);
> >>>  extern int SendReceive2(const unsigned int /* xid */ , struct cifs_ses *,
> >>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> >>> index ce5f75c..7365460 100644
> >>> --- a/fs/cifs/cifssmb.c
> >>> +++ b/fs/cifs/cifssmb.c
> >>> @@ -751,6 +751,8 @@ CIFSSMBEcho(struct TCP_Server_Info *server)
> >>>         ECHO_REQ *smb;
> >>>         int rc = 0;
> >>>         struct kvec iov;
> >>> +       struct smb_rqst rqst = { .rq_iov = &iov,
> >>> +                                .rq_nvec = 1 };
> >>>
> >>>         cFYI(1, "In echo request");
> >>>
> >>> @@ -768,7 +770,7 @@ CIFSSMBEcho(struct TCP_Server_Info *server)
> >>>         iov.iov_base = smb;
> >>>         iov.iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4;
> >>>
> >>> -       rc = cifs_call_async(server, &iov, 1, NULL, cifs_echo_callback,
> >>> +       rc = cifs_call_async(server, &rqst, NULL, cifs_echo_callback,
> >>>                              server, CIFS_ASYNC_OP | CIFS_ECHO_OP);
> >>>         if (rc)
> >>>                 cFYI(1, "Echo request failed: %d", rc);
> >>> @@ -1604,6 +1606,8 @@ cifs_async_readv(struct cifs_readdata *rdata)
> >>>         READ_REQ *smb = NULL;
> >>>         int wct;
> >>>         struct cifs_tcon *tcon = tlink_tcon(rdata->cfile->tlink);
> >>> +       struct smb_rqst rqst = { .rq_iov = rdata->iov,
> >>> +                                .rq_nvec = 1 };
> >>>
> >>>         cFYI(1, "%s: offset=%llu bytes=%u", __func__,
> >>>                 rdata->offset, rdata->bytes);
> >>> @@ -1647,9 +1651,8 @@ cifs_async_readv(struct cifs_readdata *rdata)
> >>>         rdata->iov[0].iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4;
> >>>
> >>>         kref_get(&rdata->refcount);
> >>> -       rc = cifs_call_async(tcon->ses->server, rdata->iov, 1,
> >>> -                            cifs_readv_receive, cifs_readv_callback,
> >>> -                            rdata, 0);
> >>> +       rc = cifs_call_async(tcon->ses->server, &rqst, cifs_readv_receive,
> >>> +                            cifs_readv_callback, rdata, 0);
> >>>
> >>>         if (rc == 0)
> >>>                 cifs_stats_inc(&tcon->stats.cifs_stats.num_reads);
> >>> @@ -2052,6 +2055,7 @@ cifs_async_writev(struct cifs_writedata *wdata)
> >>>         int wct;
> >>>         struct cifs_tcon *tcon = tlink_tcon(wdata->cfile->tlink);
> >>>         struct kvec *iov = NULL;
> >>> +       struct smb_rqst rqst = { };
> >>>
> >>>         if (tcon->ses->capabilities & CAP_LARGE_FILES) {
> >>>                 wct = 14;
> >>> @@ -2068,11 +2072,13 @@ cifs_async_writev(struct cifs_writedata *wdata)
> >>>                 goto async_writev_out;
> >>>
> >>>         /* 1 iov per page + 1 for header */
> >>> -       iov = kzalloc((wdata->nr_pages + 1) * sizeof(*iov), GFP_NOFS);
> >>> +       rqst.rq_nvec = wdata->nr_pages + 1;
> >>> +       iov = kzalloc((rqst.rq_nvec) * sizeof(*iov), GFP_NOFS);
> >>>         if (iov == NULL) {
> >>>                 rc = -ENOMEM;
> >>>                 goto async_writev_out;
> >>>         }
> >>> +       rqst.rq_iov = iov;
> >>>
> >>>         smb->hdr.Pid = cpu_to_le16((__u16)wdata->pid);
> >>>         smb->hdr.PidHigh = cpu_to_le16((__u16)(wdata->pid >> 16));
> >>> @@ -2121,8 +2127,8 @@ cifs_async_writev(struct cifs_writedata *wdata)
> >>>         }
> >>>
> >>>         kref_get(&wdata->refcount);
> >>> -       rc = cifs_call_async(tcon->ses->server, iov, wdata->nr_pages + 1,
> >>> -                            NULL, cifs_writev_callback, wdata, 0);
> >>> +       rc = cifs_call_async(tcon->ses->server, &rqst, NULL,
> >>> +                               cifs_writev_callback, wdata, 0);
> >>>
> >>>         if (rc == 0)
> >>>                 cifs_stats_inc(&tcon->stats.cifs_stats.num_writes);
> >>> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> >>> index 902bbe2..dbb798c 100644
> >>> --- a/fs/cifs/smb2proto.h
> >>> +++ b/fs/cifs/smb2proto.h
> >>> @@ -43,9 +43,7 @@ extern int smb2_check_receive(struct mid_q_entry *mid,
> >>>                               struct TCP_Server_Info *server, bool log_error);
> >>>  extern int smb2_setup_request(struct cifs_ses *ses, struct kvec *iov,
> >>>                               unsigned int nvec, struct mid_q_entry **ret_mid);
> >>> -extern int smb2_setup_async_request(struct TCP_Server_Info *server,
> >>> -                                   struct kvec *iov, unsigned int nvec,
> >>> -                                   struct mid_q_entry **ret_mid);
> >>> +extern struct mid_q_entry *smb2_setup_async_request(struct TCP_Server_Info *server, struct smb_rqst *rqst);
> >
> > This exceeds 80 length and causes checkpatch.pl warnings.
> >
> >>>  extern void smb2_echo_request(struct work_struct *work);
> >>>
> >>>  extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
> >>> diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
> >>> index 31f5d42..c0a7a42 100644
> >>> --- a/fs/cifs/smb2transport.c
> >>> +++ b/fs/cifs/smb2transport.c
> >>> @@ -148,25 +148,23 @@ smb2_setup_request(struct cifs_ses *ses, struct kvec *iov,
> >>>         return rc;
> >>>  }
> >>>
> >>> -int
> >>> -smb2_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
> >>> -                        unsigned int nvec, struct mid_q_entry **ret_mid)
> >>> +struct mid_q_entry *
> >>> +smb2_setup_async_request(struct TCP_Server_Info *server,
> >>> +                       struct smb_rqst *rqst)
> >>>  {
> >>> -       int rc = 0;
> >>> -       struct smb2_hdr *hdr = (struct smb2_hdr *)iov[0].iov_base;
> >>> +       struct smb2_hdr *hdr = (struct smb2_hdr *)smb_rqst->iov[0].iov_base;
> >>>         struct mid_q_entry *mid;
> >>>
> >>>         smb2_seq_num_into_buf(server, hdr);
> >>>
> >>>         mid = smb2_mid_entry_alloc(hdr, server);
> >>>         if (mid == NULL)
> >>> -               return -ENOMEM;
> >>> +               return ERR_PTR(-ENOMEM);
> >>>
> >>>         /* rc = smb2_sign_smb2(iov, nvec, server);
> >>>         if (rc) {
> >>>                 DeleteMidQEntry(mid);
> >>>                 return rc;
> >>>         }*/
> >>> -       *ret_mid = mid;
> >>> -       return rc;
> >>> +       return mid;
> >>>  }
> >>> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> >>> index 5673ef1..73cfa6f 100644
> >>> --- a/fs/cifs/transport.c
> >>> +++ b/fs/cifs/transport.c
> >>> @@ -454,12 +454,11 @@ wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *midQ)
> >>>         return 0;
> >>>  }
> >>>
> >>> -int
> >>> -cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
> >>> -                        unsigned int nvec, struct mid_q_entry **ret_mid)
> >>> +struct mid_q_entry *
> >>> +cifs_setup_async_request(struct TCP_Server_Info *server, struct smb_rqst *rqst)
> >>>  {
> >>>         int rc;
> >>> -       struct smb_hdr *hdr = (struct smb_hdr *)iov[0].iov_base;
> >>> +       struct smb_hdr *hdr = (struct smb_hdr *)rqst->rq_iov[0].iov_base;
> >>>         struct mid_q_entry *mid;
> >>>
> >>>         /* enable signing if server requires it */
> >>> @@ -468,16 +467,15 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
> >>>
> >>>         mid = AllocMidQEntry(hdr, server);
> >>>         if (mid == NULL)
> >>> -               return -ENOMEM;
> >>> +               return ERR_PTR(-ENOMEM);
> >>>
> >>> -       rc = cifs_sign_smbv(iov, nvec, server, &mid->sequence_number);
> >>> +       rc = cifs_sign_rqst(rqst, server, &mid->sequence_number);
> >>>         if (rc) {
> >>>                 DeleteMidQEntry(mid);
> >>> -               return rc;
> >>> +               return ERR_PTR(rc);
> >>>         }
> >>>
> >>> -       *ret_mid = mid;
> >>> -       return 0;
> >>> +       return mid;
> >>>  }
> >>>
> >>>  /*
> >>> @@ -485,9 +483,9 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
> >>>   * the result. Caller is responsible for dealing with timeouts.
> >>>   */
> >>>  int
> >>> -cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
> >>> -               unsigned int nvec, mid_receive_t *receive,
> >>> -               mid_callback_t *callback, void *cbdata, const int flags)
> >>> +cifs_call_async(struct TCP_Server_Info *server, struct smb_rqst *rqst,
> >>> +               mid_receive_t *receive, mid_callback_t *callback,
> >>> +               void *cbdata, const int flags)
> >>>  {
> >>>         int rc, timeout, optype;
> >>>         struct mid_q_entry *mid;
> >>> @@ -500,12 +498,12 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
> >>>                 return rc;
> >>>
> >>>         mutex_lock(&server->srv_mutex);
> >>> -       rc = server->ops->setup_async_request(server, iov, nvec, &mid);
> >>> -       if (rc) {
> >>> +       mid = server->ops->setup_async_request(server, rqst);
> >>> +       if (IS_ERR(mid)) {
> >>>                 mutex_unlock(&server->srv_mutex);
> >>>                 add_credits(server, 1, optype);
> >>>                 wake_up(&server->request_q);
> >>> -               return rc;
> >>> +               return PTR_ERR(mid);
> >>>         }
> >>>
> >>>         mid->receive = receive;
> >>> @@ -520,7 +518,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
> >>>
> >>>
> >>>         cifs_in_send_inc(server);
> >>> -       rc = smb_sendv(server, iov, nvec);
> >>> +       rc = smb_send_rqst(server, rqst);
> >>>         cifs_in_send_dec(server);
> >>>         cifs_save_when_sent(mid);
> >>>         mutex_unlock(&server->srv_mutex);
> >>> --
> >>> 1.7.11.2
> >>>
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> >>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> >> Like the idea of changing setup_async_request prototype.
> >>
> >> Reviewed-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> >>
> >> --
> >> Best regards,
> >> Pavel Shilovsky.
> >
> >
> >
> > --
> > Best regards,
> > Pavel Shilovsky.
> 
> Also it doesn't build - needs smth like this:
> 
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 62b3f17..a525db4 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -1102,6 +1102,8 @@ SMB2_echo(struct TCP_Server_Info *server)
>         struct smb2_echo_req *req;
>         int rc = 0;
>         struct kvec iov;
> +       struct smb_rqst rqst = { .rq_iov = &iov,
> +                                .rq_nvec = 1 };
> 
>         cFYI(1, "In echo request");
> 
> @@ -1115,7 +1117,7 @@ SMB2_echo(struct TCP_Server_Info *server)
>         /* 4 for rfc1002 length field */
>         iov.iov_len = get_rfc1002_length(req) + 4;
> 
> -       rc = cifs_call_async(server, &iov, 1, NULL, smb2_echo_callback, server,
> +       rc = cifs_call_async(server, &rqst, NULL, smb2_echo_callback, server,
>                              CIFS_ECHO_OP);
>         if (rc)
>                 cFYI(1, "Echo request failed: %d", rc);
> diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
> index c0a7a42..ee20740 100644
> --- a/fs/cifs/smb2transport.c
> +++ b/fs/cifs/smb2transport.c
> @@ -152,7 +152,7 @@ struct mid_q_entry *
>  smb2_setup_async_request(struct TCP_Server_Info *server,
>                         struct smb_rqst *rqst)
>  {
> -       struct smb2_hdr *hdr = (struct smb2_hdr *)smb_rqst->iov[0].iov_base;
> +       struct smb2_hdr *hdr = (struct smb2_hdr *)rqst->rq_iov[0].iov_base;
>         struct mid_q_entry *mid;
> 
>         smb2_seq_num_into_buf(server, hdr);
> 

The fixed patch is in my tree, along with a fix for the 80 column
warning from checkpatch.pl. I confirmed it this time by building with
CONFIG_CIFS_SMB2 enabled. Let me know if you'd like me to re-post the
series.

Thanks,
-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[PATCH v2 7/9] cifs: convert async write code to pass in data via rq_pages array

[Jeff Layton]

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsproto.h |  4 ++--
 fs/cifs/cifssmb.c   | 39 +++++++++++----------------------------
 fs/cifs/file.c      | 47 +++++++++--------------------------------------
 3 files changed, 22 insertions(+), 68 deletions(-)

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 8075f23..179e29a 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -504,8 +504,8 @@ struct cifs_writedata {
 	pid_t				pid;
 	unsigned int			bytes;
 	int				result;
-	void (*marshal_iov) (struct kvec *iov,
-			     struct cifs_writedata *wdata);
+	unsigned int			pagesz;
+	unsigned int			tailsz;
 	unsigned int			nr_pages;
 	struct page			*pages[1];
 };
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 7365460..0b0a33c 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -2050,11 +2050,11 @@ cifs_writev_callback(struct mid_q_entry *mid)
 int
 cifs_async_writev(struct cifs_writedata *wdata)
 {
-	int i, rc = -EACCES;
+	int rc = -EACCES;
 	WRITE_REQ *smb = NULL;
 	int wct;
 	struct cifs_tcon *tcon = tlink_tcon(wdata->cfile->tlink);
-	struct kvec *iov = NULL;
+	struct kvec iov;
 	struct smb_rqst rqst = { };
 
 	if (tcon->ses->capabilities & CAP_LARGE_FILES) {
@@ -2071,15 +2071,6 @@ cifs_async_writev(struct cifs_writedata *wdata)
 	if (rc)
 		goto async_writev_out;
 
-	/* 1 iov per page + 1 for header */
-	rqst.rq_nvec = wdata->nr_pages + 1;
-	iov = kzalloc((rqst.rq_nvec) * sizeof(*iov), GFP_NOFS);
-	if (iov == NULL) {
-		rc = -ENOMEM;
-		goto async_writev_out;
-	}
-	rqst.rq_iov = iov;
-
 	smb->hdr.Pid = cpu_to_le16((__u16)wdata->pid);
 	smb->hdr.PidHigh = cpu_to_le16((__u16)(wdata->pid >> 16));
 
@@ -2096,18 +2087,15 @@ cifs_async_writev(struct cifs_writedata *wdata)
 	    cpu_to_le16(offsetof(struct smb_com_write_req, Data) - 4);
 
 	/* 4 for RFC1001 length + 1 for BCC */
-	iov[0].iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4 + 1;
-	iov[0].iov_base = smb;
+	iov.iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4 + 1;
+	iov.iov_base = smb;
 
-	/*
-	 * This function should marshal up the page array into the kvec
-	 * array, reserving [0] for the header. It should kmap the pages
-	 * and set the iov_len properly for each one. It may also set
-	 * wdata->bytes too.
-	 */
-	cifs_kmap_lock();
-	wdata->marshal_iov(iov, wdata);
-	cifs_kmap_unlock();
+	rqst.rq_iov = &iov;
+	rqst.rq_nvec = 1;
+	rqst.rq_pages = wdata->pages;
+	rqst.rq_npages = wdata->nr_pages;
+	rqst.rq_pagesz = wdata->pagesz;
+	rqst.rq_tailsz = wdata->tailsz;
 
 	cFYI(1, "async write at %llu %u bytes", wdata->offset, wdata->bytes);
 
@@ -2123,7 +2111,7 @@ cifs_async_writev(struct cifs_writedata *wdata)
 				(struct smb_com_writex_req *)smb;
 		inc_rfc1001_len(&smbw->hdr, wdata->bytes + 5);
 		put_bcc(wdata->bytes + 5, &smbw->hdr);
-		iov[0].iov_len += 4; /* pad bigger by four bytes */
+		iov.iov_len += 4; /* pad bigger by four bytes */
 	}
 
 	kref_get(&wdata->refcount);
@@ -2135,13 +2123,8 @@ cifs_async_writev(struct cifs_writedata *wdata)
 	else
 		kref_put(&wdata->refcount, cifs_writedata_release);
 
-	/* send is done, unmap pages */
-	for (i = 0; i < wdata->nr_pages; i++)
-		kunmap(wdata->pages[i]);
-
 async_writev_out:
 	cifs_small_buf_release(smb);
-	kfree(iov);
 	return rc;
 }
 
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index cd5b144..b18d963 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1709,27 +1709,6 @@ static int cifs_partialpagewrite(struct page *page, unsigned from, unsigned to)
 	return rc;
 }
 
-/*
- * Marshal up the iov array, reserving the first one for the header. Also,
- * set wdata->bytes.
- */
-static void
-cifs_writepages_marshal_iov(struct kvec *iov, struct cifs_writedata *wdata)
-{
-	int i;
-	struct inode *inode = wdata->cfile->dentry->d_inode;
-	loff_t size = i_size_read(inode);
-
-	/* marshal up the pages into iov array */
-	wdata->bytes = 0;
-	for (i = 0; i < wdata->nr_pages; i++) {
-		iov[i + 1].iov_len = min(size - page_offset(wdata->pages[i]),
-					(loff_t)PAGE_CACHE_SIZE);
-		iov[i + 1].iov_base = kmap(wdata->pages[i]);
-		wdata->bytes += iov[i + 1].iov_len;
-	}
-}
-
 static int cifs_writepages(struct address_space *mapping,
 			   struct writeback_control *wbc)
 {
@@ -1739,6 +1718,7 @@ static int cifs_writepages(struct address_space *mapping,
 	struct cifs_writedata *wdata;
 	struct page *page;
 	int rc = 0;
+	loff_t isize = i_size_read(mapping->host);
 
 	/*
 	 * If wsize is smaller than the page cache size, default to writing
@@ -1843,7 +1823,7 @@ retry:
 			 */
 			set_page_writeback(page);
 
-			if (page_offset(page) >= mapping->host->i_size) {
+			if (page_offset(page) >= isize) {
 				done = true;
 				unlock_page(page);
 				end_page_writeback(page);
@@ -1874,7 +1854,11 @@ retry:
 		wdata->sync_mode = wbc->sync_mode;
 		wdata->nr_pages = nr_pages;
 		wdata->offset = page_offset(wdata->pages[0]);
-		wdata->marshal_iov = cifs_writepages_marshal_iov;
+		wdata->pagesz = PAGE_CACHE_SIZE;
+		wdata->tailsz = min(isize - page_offset(wdata->pages[nr_pages - 1]),
+					(loff_t)PAGE_CACHE_SIZE);
+		wdata->bytes = ((nr_pages - 1) * PAGE_CACHE_SIZE) +
+					wdata->tailsz;
 
 		do {
 			if (wdata->cfile != NULL)
@@ -2163,20 +2147,6 @@ size_t get_numpages(const size_t wsize, const size_t len, size_t *cur_len)
 }
 
 static void
-cifs_uncached_marshal_iov(struct kvec *iov, struct cifs_writedata *wdata)
-{
-	int i;
-	size_t bytes = wdata->bytes;
-
-	/* marshal up the pages into iov array */
-	for (i = 0; i < wdata->nr_pages; i++) {
-		iov[i + 1].iov_len = min_t(size_t, bytes, PAGE_SIZE);
-		iov[i + 1].iov_base = kmap(wdata->pages[i]);
-		bytes -= iov[i + 1].iov_len;
-	}
-}
-
-static void
 cifs_uncached_writev_complete(struct work_struct *work)
 {
 	int i;
@@ -2289,7 +2259,8 @@ cifs_iovec_write(struct file *file, const struct iovec *iov,
 		wdata->cfile = cifsFileInfo_get(open_file);
 		wdata->pid = pid;
 		wdata->bytes = cur_len;
-		wdata->marshal_iov = cifs_uncached_marshal_iov;
+		wdata->pagesz = PAGE_SIZE;
+		wdata->tailsz = cur_len - ((nr_pages - 1) * PAGE_SIZE);
 		rc = cifs_uncached_retry_writev(wdata);
 		if (rc) {
 			kref_put(&wdata->refcount, cifs_writedata_release);
-- 
1.7.11.2

Re: [PATCH v2 7/9] cifs: convert async write code to pass in data via rq_pages array

[Pavel Shilovsky]

2012/7/25 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifsproto.h |  4 ++--
>  fs/cifs/cifssmb.c   | 39 +++++++++++----------------------------
>  fs/cifs/file.c      | 47 +++++++++--------------------------------------
>  3 files changed, 22 insertions(+), 68 deletions(-)
>
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index 8075f23..179e29a 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -504,8 +504,8 @@ struct cifs_writedata {
>         pid_t                           pid;
>         unsigned int                    bytes;
>         int                             result;
> -       void (*marshal_iov) (struct kvec *iov,
> -                            struct cifs_writedata *wdata);
> +       unsigned int                    pagesz;
> +       unsigned int                    tailsz;
>         unsigned int                    nr_pages;
>         struct page                     *pages[1];
>  };
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 7365460..0b0a33c 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -2050,11 +2050,11 @@ cifs_writev_callback(struct mid_q_entry *mid)
>  int
>  cifs_async_writev(struct cifs_writedata *wdata)
>  {
> -       int i, rc = -EACCES;
> +       int rc = -EACCES;
>         WRITE_REQ *smb = NULL;
>         int wct;
>         struct cifs_tcon *tcon = tlink_tcon(wdata->cfile->tlink);
> -       struct kvec *iov = NULL;
> +       struct kvec iov;
>         struct smb_rqst rqst = { };
>
>         if (tcon->ses->capabilities & CAP_LARGE_FILES) {
> @@ -2071,15 +2071,6 @@ cifs_async_writev(struct cifs_writedata *wdata)
>         if (rc)
>                 goto async_writev_out;
>
> -       /* 1 iov per page + 1 for header */
> -       rqst.rq_nvec = wdata->nr_pages + 1;
> -       iov = kzalloc((rqst.rq_nvec) * sizeof(*iov), GFP_NOFS);
> -       if (iov == NULL) {
> -               rc = -ENOMEM;
> -               goto async_writev_out;
> -       }
> -       rqst.rq_iov = iov;
> -
>         smb->hdr.Pid = cpu_to_le16((__u16)wdata->pid);
>         smb->hdr.PidHigh = cpu_to_le16((__u16)(wdata->pid >> 16));
>
> @@ -2096,18 +2087,15 @@ cifs_async_writev(struct cifs_writedata *wdata)
>             cpu_to_le16(offsetof(struct smb_com_write_req, Data) - 4);
>
>         /* 4 for RFC1001 length + 1 for BCC */
> -       iov[0].iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4 + 1;
> -       iov[0].iov_base = smb;
> +       iov.iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4 + 1;
> +       iov.iov_base = smb;
>
> -       /*
> -        * This function should marshal up the page array into the kvec
> -        * array, reserving [0] for the header. It should kmap the pages
> -        * and set the iov_len properly for each one. It may also set
> -        * wdata->bytes too.
> -        */
> -       cifs_kmap_lock();
> -       wdata->marshal_iov(iov, wdata);
> -       cifs_kmap_unlock();
> +       rqst.rq_iov = &iov;
> +       rqst.rq_nvec = 1;
> +       rqst.rq_pages = wdata->pages;
> +       rqst.rq_npages = wdata->nr_pages;
> +       rqst.rq_pagesz = wdata->pagesz;
> +       rqst.rq_tailsz = wdata->tailsz;
>
>         cFYI(1, "async write at %llu %u bytes", wdata->offset, wdata->bytes);
>
> @@ -2123,7 +2111,7 @@ cifs_async_writev(struct cifs_writedata *wdata)
>                                 (struct smb_com_writex_req *)smb;
>                 inc_rfc1001_len(&smbw->hdr, wdata->bytes + 5);
>                 put_bcc(wdata->bytes + 5, &smbw->hdr);
> -               iov[0].iov_len += 4; /* pad bigger by four bytes */
> +               iov.iov_len += 4; /* pad bigger by four bytes */
>         }
>
>         kref_get(&wdata->refcount);
> @@ -2135,13 +2123,8 @@ cifs_async_writev(struct cifs_writedata *wdata)
>         else
>                 kref_put(&wdata->refcount, cifs_writedata_release);
>
> -       /* send is done, unmap pages */
> -       for (i = 0; i < wdata->nr_pages; i++)
> -               kunmap(wdata->pages[i]);
> -
>  async_writev_out:
>         cifs_small_buf_release(smb);
> -       kfree(iov);
>         return rc;
>  }
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index cd5b144..b18d963 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1709,27 +1709,6 @@ static int cifs_partialpagewrite(struct page *page, unsigned from, unsigned to)
>         return rc;
>  }
>
> -/*
> - * Marshal up the iov array, reserving the first one for the header. Also,
> - * set wdata->bytes.
> - */
> -static void
> -cifs_writepages_marshal_iov(struct kvec *iov, struct cifs_writedata *wdata)
> -{
> -       int i;
> -       struct inode *inode = wdata->cfile->dentry->d_inode;
> -       loff_t size = i_size_read(inode);
> -
> -       /* marshal up the pages into iov array */
> -       wdata->bytes = 0;
> -       for (i = 0; i < wdata->nr_pages; i++) {
> -               iov[i + 1].iov_len = min(size - page_offset(wdata->pages[i]),
> -                                       (loff_t)PAGE_CACHE_SIZE);
> -               iov[i + 1].iov_base = kmap(wdata->pages[i]);
> -               wdata->bytes += iov[i + 1].iov_len;
> -       }
> -}
> -
>  static int cifs_writepages(struct address_space *mapping,
>                            struct writeback_control *wbc)
>  {
> @@ -1739,6 +1718,7 @@ static int cifs_writepages(struct address_space *mapping,
>         struct cifs_writedata *wdata;
>         struct page *page;
>         int rc = 0;
> +       loff_t isize = i_size_read(mapping->host);
>
>         /*
>          * If wsize is smaller than the page cache size, default to writing
> @@ -1843,7 +1823,7 @@ retry:
>                          */
>                         set_page_writeback(page);
>
> -                       if (page_offset(page) >= mapping->host->i_size) {
> +                       if (page_offset(page) >= isize) {
>                                 done = true;
>                                 unlock_page(page);
>                                 end_page_writeback(page);
> @@ -1874,7 +1854,11 @@ retry:
>                 wdata->sync_mode = wbc->sync_mode;
>                 wdata->nr_pages = nr_pages;
>                 wdata->offset = page_offset(wdata->pages[0]);
> -               wdata->marshal_iov = cifs_writepages_marshal_iov;
> +               wdata->pagesz = PAGE_CACHE_SIZE;
> +               wdata->tailsz = min(isize - page_offset(wdata->pages[nr_pages - 1]),
> +                                       (loff_t)PAGE_CACHE_SIZE);
> +               wdata->bytes = ((nr_pages - 1) * PAGE_CACHE_SIZE) +
> +                                       wdata->tailsz;
>
>                 do {
>                         if (wdata->cfile != NULL)
> @@ -2163,20 +2147,6 @@ size_t get_numpages(const size_t wsize, const size_t len, size_t *cur_len)
>  }
>
>  static void
> -cifs_uncached_marshal_iov(struct kvec *iov, struct cifs_writedata *wdata)
> -{
> -       int i;
> -       size_t bytes = wdata->bytes;
> -
> -       /* marshal up the pages into iov array */
> -       for (i = 0; i < wdata->nr_pages; i++) {
> -               iov[i + 1].iov_len = min_t(size_t, bytes, PAGE_SIZE);
> -               iov[i + 1].iov_base = kmap(wdata->pages[i]);
> -               bytes -= iov[i + 1].iov_len;
> -       }
> -}
> -
> -static void
>  cifs_uncached_writev_complete(struct work_struct *work)
>  {
>         int i;
> @@ -2289,7 +2259,8 @@ cifs_iovec_write(struct file *file, const struct iovec *iov,
>                 wdata->cfile = cifsFileInfo_get(open_file);
>                 wdata->pid = pid;
>                 wdata->bytes = cur_len;
> -               wdata->marshal_iov = cifs_uncached_marshal_iov;
> +               wdata->pagesz = PAGE_SIZE;
> +               wdata->tailsz = cur_len - ((nr_pages - 1) * PAGE_SIZE);
>                 rc = cifs_uncached_retry_writev(wdata);
>                 if (rc) {
>                         kref_put(&wdata->refcount, cifs_writedata_release);
> --
> 1.7.11.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reviewed-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

-- 
Best regards,
Pavel Shilovsky.

[PATCH v2 8/9] cifs: remove the kmap size limit from wsize

[Jeff Layton]

Now that we're not kmapping so much at once, there's no need to cap
the wsize at the amount that can be simultaneously kmapped.

Reviewed-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index a828a8c..c4d5285 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3352,9 +3352,6 @@ cifs_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *pvolume_info)
 		wsize = min_t(unsigned int, wsize,
 				server->maxBuf - sizeof(WRITE_REQ) + 4);
 
-	/* limit to the amount that we can kmap at once */
-	wsize = min_t(unsigned int, wsize, CIFS_KMAP_SIZE_LIMIT);
-
 	/* hard limit of CIFS_MAX_WSIZE */
 	wsize = min_t(unsigned int, wsize, CIFS_MAX_WSIZE);
 
-- 
1.7.11.2

[PATCH v2 9/9] cifs: add deprecation warning to sockopt=TCP_NODELAY option

[Jeff Layton]

Now that we're using TCP_CORK on the socket, there's no value in
continuting to support this option. Schedule it for removal in 3.8.

Reviewed-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index c4d5285..df9bb26 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1676,12 +1676,13 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
 			if (string == NULL)
 				goto out_nomem;
 
-			/*
-			 * FIXME: since we now cork/uncork the socket while
-			 * 	  sending, should we deprecate this option?
-			 */
-			if (strnicmp(string, "TCP_NODELAY", 11) == 0)
+			if (strnicmp(string, "TCP_NODELAY", 11) == 0) {
+				printk(KERN_WARNING "CIFS: the "
+					"sockopt=TCP_NODELAY option has been "
+					"deprecated and will be removed "
+					"in 3.8\n");
 				vol->sockopt_tcp_nodelay = 1;
+			}
 			break;
 		case Opt_netbiosname:
 			string = match_strdup(args);
-- 
1.7.11.2