diff mbox

[v2,06/16] CIFS: Fix cifs_writev_requeue when wsize changes

Message ID 1403863073-19526-7-git-send-email-pshilovsky@samba.org (mailing list archive)
State New, archived
Headers show

Commit Message

Pavel Shilovsky June 27, 2014, 9:57 a.m. UTC 
If wsize changes on reconnect we need to use new writedata structure
that for retrying.

Signed-off-by: Pavel Shilovsky <pshilovsky@samba.org>
---
 fs/cifs/cifsglob.h |  2 ++
 fs/cifs/cifssmb.c  | 84 +++++++++++++++++++++++++++++++++++++++++++-----------
 fs/cifs/smb1ops.c  |  7 +++++
 fs/cifs/smb2ops.c  | 10 +++++++
 4 files changed, 87 insertions(+), 16 deletions(-)

Comments

Shirish Pargaonkar July 20, 2014, 7:49 p.m. UTC | #1 
why bother setting rc (to ENOTSUPP, ENOMEM) if those errors are not returned
once we break out of do while loop?
Instead perhaps log an error message?

On Fri, Jun 27, 2014 at 4:57 AM, Pavel Shilovsky <pshilovsky@samba.org> wrote:
> If wsize changes on reconnect we need to use new writedata structure
> that for retrying.
>
> Signed-off-by: Pavel Shilovsky <pshilovsky@samba.org>
> ---
>  fs/cifs/cifsglob.h |  2 ++
>  fs/cifs/cifssmb.c  | 84 +++++++++++++++++++++++++++++++++++++++++++-----------
>  fs/cifs/smb1ops.c  |  7 +++++
>  fs/cifs/smb2ops.c  | 10 +++++++
>  4 files changed, 87 insertions(+), 16 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index de6aed8..8c53f20 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -404,6 +404,8 @@ struct smb_version_operations {
>                         const struct cifs_fid *, u32 *);
>         int (*set_acl)(struct cifs_ntsd *, __u32, struct inode *, const char *,
>                         int);
> +       /* writepages retry size */
> +       unsigned int (*wp_retry_size)(struct inode *);
>  };
>
>  struct smb_version_values {
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index b7e5b65..41c9067 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -1899,27 +1899,79 @@ cifs_writev_requeue(struct cifs_writedata *wdata)
>         int i, rc;
>         struct inode *inode = wdata->cfile->dentry->d_inode;
>         struct TCP_Server_Info *server;
> +       unsigned int rest_len;
>
> -       for (i = 0; i < wdata->nr_pages; i++) {
> -               lock_page(wdata->pages[i]);
> -               clear_page_dirty_for_io(wdata->pages[i]);
> -       }
> -
> +       server = tlink_tcon(wdata->cfile->tlink)->ses->server;
> +       i = 0;
> +       rest_len = wdata->bytes;
>         do {
> -               server = tlink_tcon(wdata->cfile->tlink)->ses->server;
> -               rc = server->ops->async_writev(wdata, cifs_writedata_release);
> -       } while (rc == -EAGAIN);
> +               struct cifs_writedata *wdata2;
> +               unsigned int j, nr_pages, wsize, tailsz, cur_len;
> +
> +               wsize = server->ops->wp_retry_size(inode);
> +               if (wsize < rest_len) {
> +                       nr_pages = wsize / PAGE_CACHE_SIZE;
> +                       if (!nr_pages) {
> +                               rc = -ENOTSUPP;
> +                               break;
> +                       }
> +                       cur_len = nr_pages * PAGE_CACHE_SIZE;
> +                       tailsz = PAGE_CACHE_SIZE;
> +               } else {
> +                       nr_pages = DIV_ROUND_UP(rest_len, PAGE_CACHE_SIZE);
> +                       cur_len = rest_len;
> +                       tailsz = rest_len - (nr_pages - 1) * PAGE_CACHE_SIZE;
> +               }
>
> -       for (i = 0; i < wdata->nr_pages; i++) {
> -               unlock_page(wdata->pages[i]);
> -               if (rc != 0) {
> -                       SetPageError(wdata->pages[i]);
> -                       end_page_writeback(wdata->pages[i]);
> -                       page_cache_release(wdata->pages[i]);
> +               wdata2 = cifs_writedata_alloc(nr_pages, cifs_writev_complete);
> +               if (!wdata2) {
> +                       rc = -ENOMEM;
> +                       break;
>                 }
> -       }
>
> -       mapping_set_error(inode->i_mapping, rc);
> +               for (j = 0; j < nr_pages; j++) {
> +                       wdata2->pages[j] = wdata->pages[i + j];
> +                       lock_page(wdata2->pages[j]);
> +                       clear_page_dirty_for_io(wdata2->pages[j]);
> +               }
> +
> +               wdata2->sync_mode = wdata->sync_mode;
> +               wdata2->nr_pages = nr_pages;
> +               wdata2->offset = page_offset(wdata2->pages[0]);
> +               wdata2->pagesz = PAGE_CACHE_SIZE;
> +               wdata2->tailsz = tailsz;
> +               wdata2->bytes = cur_len;
> +
> +               wdata2->cfile = find_writable_file(CIFS_I(inode), false);
> +               if (!wdata2->cfile) {
> +                       cifs_dbg(VFS, "No writable handles for inode\n");
> +                       rc = -EBADF;
> +                       break;
> +               }
> +               wdata2->pid = wdata2->cfile->pid;
> +               rc = server->ops->async_writev(wdata2, cifs_writedata_release);
> +
> +               for (j = 0; j < nr_pages; j++) {
> +                       unlock_page(wdata2->pages[j]);
> +                       if (rc != 0 && rc != -EAGAIN) {
> +                               SetPageError(wdata2->pages[j]);
> +                               end_page_writeback(wdata2->pages[j]);
> +                               page_cache_release(wdata2->pages[j]);
> +                       }
> +               }
> +
> +               if (rc) {
> +                       kref_put(&wdata2->refcount, cifs_writedata_release);
> +                       if (rc == -EAGAIN)
> +                               continue;
> +                       mapping_set_error(inode->i_mapping, rc);
> +                       break;
> +               }
> +
> +               rest_len -= cur_len;
> +               i += nr_pages;
> +       } while (i < wdata->nr_pages);
> +
>         kref_put(&wdata->refcount, cifs_writedata_release);
>  }
>
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index d1fdfa8..8a96342 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -1009,6 +1009,12 @@ cifs_is_read_op(__u32 oplock)
>         return oplock == OPLOCK_READ;
>  }
>
> +static unsigned int
> +cifs_wp_retry_size(struct inode *inode)
> +{
> +       return CIFS_SB(inode->i_sb)->wsize;
> +}
> +
>  struct smb_version_operations smb1_operations = {
>         .send_cancel = send_nt_cancel,
>         .compare_fids = cifs_compare_fids,
> @@ -1078,6 +1084,7 @@ struct smb_version_operations smb1_operations = {
>         .query_mf_symlink = cifs_query_mf_symlink,
>         .create_mf_symlink = cifs_create_mf_symlink,
>         .is_read_op = cifs_is_read_op,
> +       .wp_retry_size = cifs_wp_retry_size,
>  #ifdef CONFIG_CIFS_XATTR
>         .query_all_EAs = CIFSSMBQAllEAs,
>         .set_EA = CIFSSMBSetEA,
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 787844b..e35ce5b 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -1104,6 +1104,13 @@ smb3_parse_lease_buf(void *buf, unsigned int *epoch)
>         return le32_to_cpu(lc->lcontext.LeaseState);
>  }
>
> +static unsigned int
> +smb2_wp_retry_size(struct inode *inode)
> +{
> +       return min_t(unsigned int, CIFS_SB(inode->i_sb)->wsize,
> +                    SMB2_MAX_BUFFER_SIZE);
> +}
> +
>  struct smb_version_operations smb20_operations = {
>         .compare_fids = smb2_compare_fids,
>         .setup_request = smb2_setup_request,
> @@ -1177,6 +1184,7 @@ struct smb_version_operations smb20_operations = {
>         .create_lease_buf = smb2_create_lease_buf,
>         .parse_lease_buf = smb2_parse_lease_buf,
>         .clone_range = smb2_clone_range,
> +       .wp_retry_size = smb2_wp_retry_size,
>  };
>
>  struct smb_version_operations smb21_operations = {
> @@ -1252,6 +1260,7 @@ struct smb_version_operations smb21_operations = {
>         .create_lease_buf = smb2_create_lease_buf,
>         .parse_lease_buf = smb2_parse_lease_buf,
>         .clone_range = smb2_clone_range,
> +       .wp_retry_size = smb2_wp_retry_size,
>  };
>
>  struct smb_version_operations smb30_operations = {
> @@ -1330,6 +1339,7 @@ struct smb_version_operations smb30_operations = {
>         .parse_lease_buf = smb3_parse_lease_buf,
>         .clone_range = smb2_clone_range,
>         .validate_negotiate = smb3_validate_negotiate,
> +       .wp_retry_size = smb2_wp_retry_size,
>  };
>
>  struct smb_version_values smb20_values = {
> --
> 1.8.1.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Shilovsky July 21, 2014, 6:43 a.m. UTC | #2 
2014-07-20 23:49 GMT+04:00 Shirish Pargaonkar <shirishpargaonkar@gmail.com>:
> why bother setting rc (to ENOTSUPP, ENOMEM) if those errors are not returned
> once we break out of do while loop?
> Instead perhaps log an error message?

Good point, thanks. It seems like we should call
mapping_set_error(mapping, rc) in the end of the function. In this
case setting rc looks ok. Also, I think we don't need to log messages
in this case.
diff mbox

Patch

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index de6aed8..8c53f20 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -404,6 +404,8 @@  struct smb_version_operations {
 			const struct cifs_fid *, u32 *);
 	int (*set_acl)(struct cifs_ntsd *, __u32, struct inode *, const char *,
 			int);
+	/* writepages retry size */
+	unsigned int (*wp_retry_size)(struct inode *);
 };
 
 struct smb_version_values {
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index b7e5b65..41c9067 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -1899,27 +1899,79 @@  cifs_writev_requeue(struct cifs_writedata *wdata)
 	int i, rc;
 	struct inode *inode = wdata->cfile->dentry->d_inode;
 	struct TCP_Server_Info *server;
+	unsigned int rest_len;
 
-	for (i = 0; i < wdata->nr_pages; i++) {
-		lock_page(wdata->pages[i]);
-		clear_page_dirty_for_io(wdata->pages[i]);
-	}
-
+	server = tlink_tcon(wdata->cfile->tlink)->ses->server;
+	i = 0;
+	rest_len = wdata->bytes;
 	do {
-		server = tlink_tcon(wdata->cfile->tlink)->ses->server;
-		rc = server->ops->async_writev(wdata, cifs_writedata_release);
-	} while (rc == -EAGAIN);
+		struct cifs_writedata *wdata2;
+		unsigned int j, nr_pages, wsize, tailsz, cur_len;
+
+		wsize = server->ops->wp_retry_size(inode);
+		if (wsize < rest_len) {
+			nr_pages = wsize / PAGE_CACHE_SIZE;
+			if (!nr_pages) {
+				rc = -ENOTSUPP;
+				break;
+			}
+			cur_len = nr_pages * PAGE_CACHE_SIZE;
+			tailsz = PAGE_CACHE_SIZE;
+		} else {
+			nr_pages = DIV_ROUND_UP(rest_len, PAGE_CACHE_SIZE);
+			cur_len = rest_len;
+			tailsz = rest_len - (nr_pages - 1) * PAGE_CACHE_SIZE;
+		}
 
-	for (i = 0; i < wdata->nr_pages; i++) {
-		unlock_page(wdata->pages[i]);
-		if (rc != 0) {
-			SetPageError(wdata->pages[i]);
-			end_page_writeback(wdata->pages[i]);
-			page_cache_release(wdata->pages[i]);
+		wdata2 = cifs_writedata_alloc(nr_pages, cifs_writev_complete);
+		if (!wdata2) {
+			rc = -ENOMEM;
+			break;
 		}
-	}
 
-	mapping_set_error(inode->i_mapping, rc);
+		for (j = 0; j < nr_pages; j++) {
+			wdata2->pages[j] = wdata->pages[i + j];
+			lock_page(wdata2->pages[j]);
+			clear_page_dirty_for_io(wdata2->pages[j]);
+		}
+
+		wdata2->sync_mode = wdata->sync_mode;
+		wdata2->nr_pages = nr_pages;
+		wdata2->offset = page_offset(wdata2->pages[0]);
+		wdata2->pagesz = PAGE_CACHE_SIZE;
+		wdata2->tailsz = tailsz;
+		wdata2->bytes = cur_len;
+
+		wdata2->cfile = find_writable_file(CIFS_I(inode), false);
+		if (!wdata2->cfile) {
+			cifs_dbg(VFS, "No writable handles for inode\n");
+			rc = -EBADF;
+			break;
+		}
+		wdata2->pid = wdata2->cfile->pid;
+		rc = server->ops->async_writev(wdata2, cifs_writedata_release);
+
+		for (j = 0; j < nr_pages; j++) {
+			unlock_page(wdata2->pages[j]);
+			if (rc != 0 && rc != -EAGAIN) {
+				SetPageError(wdata2->pages[j]);
+				end_page_writeback(wdata2->pages[j]);
+				page_cache_release(wdata2->pages[j]);
+			}
+		}
+
+		if (rc) {
+			kref_put(&wdata2->refcount, cifs_writedata_release);
+			if (rc == -EAGAIN)
+				continue;
+			mapping_set_error(inode->i_mapping, rc);
+			break;
+		}
+
+		rest_len -= cur_len;
+		i += nr_pages;
+	} while (i < wdata->nr_pages);
+
 	kref_put(&wdata->refcount, cifs_writedata_release);
 }
 
diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
index d1fdfa8..8a96342 100644
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -1009,6 +1009,12 @@  cifs_is_read_op(__u32 oplock)
 	return oplock == OPLOCK_READ;
 }
 
+static unsigned int
+cifs_wp_retry_size(struct inode *inode)
+{
+	return CIFS_SB(inode->i_sb)->wsize;
+}
+
 struct smb_version_operations smb1_operations = {
 	.send_cancel = send_nt_cancel,
 	.compare_fids = cifs_compare_fids,
@@ -1078,6 +1084,7 @@  struct smb_version_operations smb1_operations = {
 	.query_mf_symlink = cifs_query_mf_symlink,
 	.create_mf_symlink = cifs_create_mf_symlink,
 	.is_read_op = cifs_is_read_op,
+	.wp_retry_size = cifs_wp_retry_size,
 #ifdef CONFIG_CIFS_XATTR
 	.query_all_EAs = CIFSSMBQAllEAs,
 	.set_EA = CIFSSMBSetEA,
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 787844b..e35ce5b 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -1104,6 +1104,13 @@  smb3_parse_lease_buf(void *buf, unsigned int *epoch)
 	return le32_to_cpu(lc->lcontext.LeaseState);
 }
 
+static unsigned int
+smb2_wp_retry_size(struct inode *inode)
+{
+	return min_t(unsigned int, CIFS_SB(inode->i_sb)->wsize,
+		     SMB2_MAX_BUFFER_SIZE);
+}
+
 struct smb_version_operations smb20_operations = {
 	.compare_fids = smb2_compare_fids,
 	.setup_request = smb2_setup_request,
@@ -1177,6 +1184,7 @@  struct smb_version_operations smb20_operations = {
 	.create_lease_buf = smb2_create_lease_buf,
 	.parse_lease_buf = smb2_parse_lease_buf,
 	.clone_range = smb2_clone_range,
+	.wp_retry_size = smb2_wp_retry_size,
 };
 
 struct smb_version_operations smb21_operations = {
@@ -1252,6 +1260,7 @@  struct smb_version_operations smb21_operations = {
 	.create_lease_buf = smb2_create_lease_buf,
 	.parse_lease_buf = smb2_parse_lease_buf,
 	.clone_range = smb2_clone_range,
+	.wp_retry_size = smb2_wp_retry_size,
 };
 
 struct smb_version_operations smb30_operations = {
@@ -1330,6 +1339,7 @@  struct smb_version_operations smb30_operations = {
 	.parse_lease_buf = smb3_parse_lease_buf,
 	.clone_range = smb2_clone_range,
 	.validate_negotiate = smb3_validate_negotiate,
+	.wp_retry_size = smb2_wp_retry_size,
 };
 
 struct smb_version_values smb20_values = {