[PATCH 0/7] Optimize reading in a short read case on reconnects

[Pavel Shilovsky]

This patchset is going to make CIFS module behave according to POSIX -- return a short read if a reconnect happens during the reading.

These patches are rebased on top of the series "SMB 2.1/3 multicredit requests for reading/writing" of 16 patches.

Pavel Shilovsky (7):
CIFS: Indicate reconnect with ECONNABORTED error code
CIFS: Use separate var for the number of bytes got in async read
CIFS: Count got bytes in read_into_pages()
CIFS: Fix possible buffer corruption in direct read
CIFS: Improve indentation in cifs_user_read()
CIFS: Optimize cifs_user_read() in a short read case on reconnects
CIFS: Optimize readpages in a short read case on reconnects

fs/cifs/cifsglob.h | 1 +
fs/cifs/cifssmb.c | 5 ++--
fs/cifs/connect.c | 8 +++---
fs/cifs/file.c | 79 ++++++++++++++++++++++++++++++++++++------------------
fs/cifs/smb2pdu.c | 4 +--
5 files changed, 62 insertions(+), 35 deletions(-)

[Pavel Shilovsky]

and don't mix it with the number of bytes that was requested.

Signed-off-by: Pavel Shilovsky <pshilovsky-***@public.gmane.org>
---
fs/cifs/cifsglob.h | 1 +
fs/cifs/cifssmb.c | 6 +++---
fs/cifs/file.c | 2 +-
fs/cifs/smb2pdu.c | 4 ++--
4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index f33ff4c..0012e1e 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1059,6 +1059,7 @@ struct cifs_readdata {
struct address_space *mapping;
__u64 offset;
unsigned int bytes;
+ unsigned int got_bytes;
pid_t pid;
int result;
struct work_struct work;
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index e411d2e..8af078f 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -1513,7 +1513,7 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
return length;

server->total_read += length;
- rdata->bytes = length;
+ rdata->got_bytes = length;

cifs_dbg(FYI, "total_read=%u buflen=%u remaining=%u\n",
server->total_read, buflen, data_len);
@@ -1556,8 +1556,8 @@ cifs_readv_callback(struct mid_q_entry *mid)
rc);
}
/* FIXME: should this be counted toward the initiating task? */
- task_io_account_read(rdata->bytes);
- cifs_stats_bytes_read(tcon, rdata->bytes);
+ task_io_account_read(rdata->got_bytes);
+ cifs_stats_bytes_read(tcon, rdata->got_bytes);
break;
case MID_REQUEST_SUBMITTED:
case MID_RETRY_NEEDED:
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 0974dd2..7fc9bdc 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2839,7 +2839,7 @@ cifs_uncached_readdata_release(struct kref *refcount)
static int
cifs_readdata_to_iov(struct cifs_readdata *rdata, struct iov_iter *iter)
{
- size_t remaining = rdata->bytes;
+ size_t remaining = rdata->got_bytes;
unsigned int i;

for (i = 0; i < rdata->nr_pages; i++) {
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 5b4ca0c..17c0c31 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1724,8 +1724,8 @@ smb2_readv_callback(struct mid_q_entry *mid)
rc);
}
/* FIXME: should this be counted toward the initiating task? */
- task_io_account_read(rdata->bytes);
- cifs_stats_bytes_read(tcon, rdata->bytes);
+ task_io_account_read(rdata->got_bytes);
+ cifs_stats_bytes_read(tcon, rdata->got_bytes);
break;
case MID_REQUEST_SUBMITTED:
case MID_RETRY_NEEDED:

[Pavel Shilovsky]

that let us not mix it with EAGAIN.

Signed-off-by: Pavel Shilovsky <pshilovsky-***@public.gmane.org>
---
fs/cifs/connect.c | 8 ++++----
fs/cifs/file.c | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 20d75b8..b0427f6 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -557,7 +557,7 @@ cifs_readv_from_socket(struct TCP_Server_Info *server, struct kvec *iov_orig,
try_to_freeze();

if (server_unresponsive(server)) {
- total_read = -EAGAIN;
+ total_read = -ECONNABORTED;
break;
}

@@ -571,7 +571,7 @@ cifs_readv_from_socket(struct TCP_Server_Info *server, struct kvec *iov_orig,
break;
} else if (server->tcpStatus == CifsNeedReconnect) {
cifs_reconnect(server);
- total_read = -EAGAIN;
+ total_read = -ECONNABORTED;
break;
} else if (length == -ERESTARTSYS ||
length == -EAGAIN ||
@@ -588,7 +588,7 @@ cifs_readv_from_socket(struct TCP_Server_Info *server, struct kvec *iov_orig,
cifs_dbg(FYI, "Received no data or error: expecting %d\n"
"got %d", to_read, length);
cifs_reconnect(server);
- total_read = -EAGAIN;
+ total_read = -ECONNABORTED;
break;
}
}
@@ -786,7 +786,7 @@ standard_receive3(struct TCP_Server_Info *server, struct mid_q_entry *mid)
cifs_dbg(VFS, "SMB response too long (%u bytes)\n", pdu_length);
cifs_reconnect(server);
wake_up(&server->response_q);
- return -EAGAIN;
+ return -ECONNABORTED;
}

/* switch to large buffer if too big for a small one */
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index d5f66ba..0974dd2 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2908,7 +2908,7 @@ cifs_uncached_read_into_pages(struct TCP_Server_Info *server,
total_read += result;
}

- return total_read > 0 && result != -EAGAIN ? total_read : result;
+ return total_read > 0 && result != -ECONNABORTED ? total_read : result;
}

static int
@@ -3358,7 +3358,7 @@ cifs_readpages_read_into_pages(struct TCP_Server_Info *server,
total_read += result;
}

- return total_read > 0 && result != -EAGAIN ? total_read : result;
+ return total_read > 0 && result != -ECONNABORTED ? total_read : result;
}

static int

[Pavel Shilovsky]

If there was a short read in the middle of the rdata list,
we can end up with a corrupt output buffer.

Signed-off-by: Pavel Shilovsky <pshilovsky-***@public.gmane.org>
---
fs/cifs/file.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index f6cb765..2927f02 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -3048,7 +3048,9 @@ again:
} else {
rc = cifs_readdata_to_iov(rdata, to);
}
-
+ /* if there was a short read -- discard anything left */
+ if (rdata->got_bytes && rdata->got_bytes < rdata->bytes)
+ rc = -ENODATA;
}
list_del_init(&rdata->list);
kref_put(&rdata->refcount, cifs_uncached_readdata_release);

[Pavel Shilovsky]

that let us know how many bytes we have already got before reconnect.

Signed-off-by: Pavel Shilovsky <pshilovsky-***@public.gmane.org>
---
fs/cifs/cifssmb.c | 1 -
fs/cifs/file.c | 16 ++++++++++------
2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 8af078f..9f13b62 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -1513,7 +1513,6 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
return length;

server->total_read += length;
- rdata->got_bytes = length;

cifs_dbg(FYI, "total_read=%u buflen=%u remaining=%u\n",
server->total_read, buflen, data_len);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 7fc9bdc..f6cb765 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2867,11 +2867,12 @@ static int
cifs_uncached_read_into_pages(struct TCP_Server_Info *server,
struct cifs_readdata *rdata, unsigned int len)
{
- int total_read = 0, result = 0;
+ int result = 0;
unsigned int i;
unsigned int nr_pages = rdata->nr_pages;
struct kvec iov;

+ rdata->got_bytes = 0;
rdata->tailsz = PAGE_SIZE;
for (i = 0; i < nr_pages; i++) {
struct page *page = rdata->pages[i];
@@ -2905,10 +2906,11 @@ cifs_uncached_read_into_pages(struct TCP_Server_Info *server,
if (result < 0)
break;

- total_read += result;
+ rdata->got_bytes += result;
}

- return total_read > 0 && result != -ECONNABORTED ? total_read : result;
+ return rdata->got_bytes > 0 && result != -ECONNABORTED ?
+ rdata->got_bytes : result;
}

static int
@@ -3289,7 +3291,7 @@ static int
cifs_readpages_read_into_pages(struct TCP_Server_Info *server,
struct cifs_readdata *rdata, unsigned int len)
{
- int total_read = 0, result = 0;
+ int result = 0;
unsigned int i;
u64 eof;
pgoff_t eof_index;
@@ -3301,6 +3303,7 @@ cifs_readpages_read_into_pages(struct TCP_Server_Info *server,
eof_index = eof ? (eof - 1) >> PAGE_CACHE_SHIFT : 0;
cifs_dbg(FYI, "eof=%llu eof_index=%lu\n", eof, eof_index);

+ rdata->got_bytes = 0;
rdata->tailsz = PAGE_CACHE_SIZE;
for (i = 0; i < nr_pages; i++) {
struct page *page = rdata->pages[i];
@@ -3355,10 +3358,11 @@ cifs_readpages_read_into_pages(struct TCP_Server_Info *server,
if (result < 0)
break;

- total_read += result;
+ rdata->got_bytes += result;
}

- return total_read > 0 && result != -ECONNABORTED ? total_read : result;
+ return rdata->got_bytes > 0 && result != -ECONNABORTED ?
+ rdata->got_bytes : result;
}

static int

[Pavel Shilovsky]

Signed-off-by: Pavel Shilovsky <pshilovsky-***@public.gmane.org>
---
fs/cifs/file.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 2927f02..6896cb5 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -3026,28 +3026,27 @@ again:
rc = wait_for_completion_killable(&rdata->done);
if (rc)
rc = -EINTR;
- else if (rdata->result) {
- rc = rdata->result;
+ else if (rdata->result == -EAGAIN) {
/* resend call if it's a retryable error */
- if (rc == -EAGAIN) {
- struct list_head tmp_list;
+ struct list_head tmp_list;

- list_del_init(&rdata->list);
- INIT_LIST_HEAD(&tmp_list);
+ list_del_init(&rdata->list);
+ INIT_LIST_HEAD(&tmp_list);

- rc = cifs_send_async_read(rdata->offset,
+ rc = cifs_send_async_read(rdata->offset,
rdata->bytes, rdata->cfile,
cifs_sb, &tmp_list);

- list_splice(&tmp_list, &rdata_list);
+ list_splice(&tmp_list, &rdata_list);

- kref_put(&rdata->refcount,
- cifs_uncached_readdata_release);
- goto again;
- }
- } else {
+ kref_put(&rdata->refcount,
+ cifs_uncached_readdata_release);
+ goto again;
+ } else if (rdata->result)
+ rc = rdata->result;
+ else
rc = cifs_readdata_to_iov(rdata, to);
- }
+
/* if there was a short read -- discard anything left */
if (rdata->got_bytes && rdata->got_bytes < rdata->bytes)
rc = -ENODATA;

[Pavel Shilovsky]

by filling the output buffer with a data got from a partially received
response and requesting the remaining data from the server.

Signed-off-by: Pavel Shilovsky <pshilovsky-***@public.gmane.org>
---
fs/cifs/file.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 6896cb5..d7246cb 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -3029,13 +3029,30 @@ again:
else if (rdata->result == -EAGAIN) {
/* resend call if it's a retryable error */
struct list_head tmp_list;
+ unsigned int got_bytes = rdata->got_bytes;

list_del_init(&rdata->list);
INIT_LIST_HEAD(&tmp_list);

- rc = cifs_send_async_read(rdata->offset,
- rdata->bytes, rdata->cfile,
- cifs_sb, &tmp_list);
+ /*
+ * Got a part of data and then reconnect has
+ * happened -- discard anything left and return
+ * a short read.
+ */
+ if (got_bytes && got_bytes < rdata->bytes) {
+ rc = cifs_readdata_to_iov(rdata, to);
+ if (rc) {
+ kref_put(&rdata->refcount,
+ cifs_uncached_readdata_release);
+ continue;
+ }
+ }
+
+ rc = cifs_send_async_read(
+ rdata->offset + got_bytes,
+ rdata->bytes - got_bytes,
+ rdata->cfile, cifs_sb,
+ &tmp_list);

list_splice(&tmp_list, &rdata_list);

[Pavel Shilovsky]

Post by Pavel Shilovsky
by filling the output buffer with a data got from a partially received
response and requesting the remaining data from the server.
---
fs/cifs/file.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 6896cb5..d7246cb 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
else if (rdata->result == -EAGAIN) {
/* resend call if it's a retryable error */
struct list_head tmp_list;
+ unsigned int got_bytes = rdata->got_bytes;
list_del_init(&rdata->list);
INIT_LIST_HEAD(&tmp_list);
- rc = cifs_send_async_read(rdata->offset,
- rdata->bytes, rdata->cfile,
- cifs_sb, &tmp_list);
+ /*
+ * Got a part of data and then reconnect has
+ * happened -- discard anything left and return
+ * a short read.
+ */

^^^
Ah, wrong version of the comment is posted. Sorry. The right one:
+ /*
+ * Got a part of data and then reconnect has
+ * happened -- fill the buffer and continue
+ * reading.
+ */

will repost this after a feedback.

Post by Pavel Shilovsky
+ if (got_bytes && got_bytes < rdata->bytes) {
+ rc = cifs_readdata_to_iov(rdata, to);
+ if (rc) {
+ kref_put(&rdata->refcount,
+ cifs_uncached_readdata_release);
+ continue;
+ }
+ }
+
+ rc = cifs_send_async_read(
+ rdata->offset + got_bytes,
+ rdata->bytes - got_bytes,
+ rdata->cfile, cifs_sb,
+ &tmp_list);
list_splice(&tmp_list, &rdata_list);
--
1.8.1.2

[Pavel Shilovsky]

Post by Pavel Shilovsky

Post by Pavel Shilovsky
by filling the output buffer with a data got from a partially received
response and requesting the remaining data from the server.
---
fs/cifs/file.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 6896cb5..d7246cb 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
else if (rdata->result == -EAGAIN) {
/* resend call if it's a retryable error */
struct list_head tmp_list;
+ unsigned int got_bytes = rdata->got_bytes;
list_del_init(&rdata->list);
INIT_LIST_HEAD(&tmp_list);
- rc = cifs_send_async_read(rdata->offset,
- rdata->bytes, rdata->cfile,
- cifs_sb, &tmp_list);
+ /*
+ * Got a part of data and then reconnect has
+ * happened -- discard anything left and return
+ * a short read.
+ */

^^^
+ /*
+ * Got a part of data and then reconnect has
+ * happened -- fill the buffer and continue
+ * reading.
+ */
will repost this after a feedback.

Post by Pavel Shilovsky
+ if (got_bytes && got_bytes < rdata->bytes) {
+ rc = cifs_readdata_to_iov(rdata, to);
+ if (rc) {
+ kref_put(&rdata->refcount,
+ cifs_uncached_readdata_release);
+ continue;
+ }
+ }
+
+ rc = cifs_send_async_read(
+ rdata->offset + got_bytes,
+ rdata->bytes - got_bytes,
+ rdata->cfile, cifs_sb,
+ &tmp_list);
list_splice(&tmp_list, &rdata_list);

Also realized that the patch doesn't aware about signed connections
and doesn't take into account that we should update stats in "EAGAIN
&& got_bytes" case. So, this diff should be appended to the patch as
well:

---

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 9f13b62..7d4361f 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -1561,6 +1561,12 @@ cifs_readv_callback(struct mid_q_entry *mid)
case MID_REQUEST_SUBMITTED:
case MID_RETRY_NEEDED:
rdata->result = -EAGAIN;
+ if (server->sign && rdata->got_bytes)
+ /* reset bytes number since we can not check a sign */
+ rdata->got_bytes = 0;
+ /* FIXME: should this be counted toward the initiating task? */
+ task_io_account_read(rdata->got_bytes);
+ cifs_stats_bytes_read(tcon, rdata->got_bytes);
break;
default:
rdata->result = -EIO;
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 17c0c31..768cddb 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1730,6 +1730,12 @@ smb2_readv_callback(struct mid_q_entry *mid)
case MID_REQUEST_SUBMITTED:
case MID_RETRY_NEEDED:
rdata->result = -EAGAIN;
+ if (server->sign && rdata->got_bytes)
+ /* reset bytes number since we can not check a sign */
+ rdata->got_bytes = 0;
+ /* FIXME: should this be counted toward the initiating task? */
+ task_io_account_read(rdata->got_bytes);
+ cifs_stats_bytes_read(tcon, rdata->got_bytes);
break;
default:
if (rdata->result != -ENODATA)

---

Thoughts?

[Pavel Shilovsky]

by marking pages with a data from a partially received response up-to-date.

Signed-off-by: Pavel Shilovsky <pshilovsky-***@public.gmane.org>
---
fs/cifs/file.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index d7246cb..d225895 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -3280,25 +3280,30 @@ int cifs_file_mmap(struct file *file, struct vm_area_struct *vma)
static void
cifs_readv_complete(struct work_struct *work)
{
- unsigned int i;
+ unsigned int i, got_bytes;
struct cifs_readdata *rdata = container_of(work,
struct cifs_readdata, work);

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

lru_cache_add_file(page);

- if (rdata->result == 0) {
+ if (rdata->result == 0 ||
+ (rdata->result == -EAGAIN && got_bytes)) {
flush_dcache_page(page);
SetPageUptodate(page);
}

unlock_page(page);

- if (rdata->result == 0)
+ if (rdata->result == 0 ||
+ (rdata->result == -EAGAIN && got_bytes))
cifs_readpage_to_fscache(rdata->mapping->host, page);

+ got_bytes -= min_t(unsigned int, PAGE_CACHE_SIZE, got_bytes);
+
page_cache_release(page);
rdata->pages[i] = NULL;
}