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

From: Jeff Layton
Date: Tue Apr 19 2016 - 14:24:50 EST


On Sat, 2016-04-09 at 21:53 +0100, Al Viro wrote:
> just do ITER_BVEC recvmsg
>
> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> ---
> Âfs/cifs/cifsproto.h |ÂÂ7 +++---
> Âfs/cifs/connect.cÂÂÂ| 65 ++++++++++++++++++++++++++++-------------------------
> Âfs/cifs/file.cÂÂÂÂÂÂ| 53 ++++++++++++++-----------------------------
> Â3 files changed, 55 insertions(+), 70 deletions(-)
>
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index 7d5f53a..0f9a6bc 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -179,10 +179,9 @@ extern int set_cifs_acl(struct cifs_ntsd *, __u32, struct inode *,
> Â
> Âextern void dequeue_mid(struct mid_q_entry *mid, bool malformed);
> Âextern int cifs_read_from_socket(struct TCP_Server_Info *server, char *buf,
> - ÂÂÂÂÂunsigned int to_read);
> -extern int cifs_readv_from_socket(struct TCP_Server_Info *server,
> - struct kvec *iov_orig, unsigned int nr_segs,
> - unsigned int to_read);
> + ÂÂÂÂÂÂÂÂÂunsigned int to_read);
> +extern int cifs_read_page_from_socket(struct TCP_Server_Info *server,
> + ÂÂÂÂÂÂstruct page *page, unsigned int to_read);
> Âextern void cifs_setup_cifs_sb(struct smb_vol *pvolume_info,
> Â ÂÂÂÂÂÂÂstruct cifs_sb_info *cifs_sb);
> Âextern int cifs_match_super(struct super_block *, void *);
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index eb42665..e33c5e0 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -501,39 +501,34 @@ server_unresponsive(struct TCP_Server_Info *server)
> Â return false;
> Â}
> Â
> -int
> -cifs_readv_from_socket(struct TCP_Server_Info *server, struct kvec *iov_orig,
> - ÂÂÂÂÂÂÂunsigned int nr_segs, unsigned int to_read)
> +static int
> +cifs_readv_from_socket(struct TCP_Server_Info *server, struct msghdr *smb_msg)
> Â{
> Â int length = 0;
> Â int total_read;
> - struct msghdr smb_msg;
> Â
> - smb_msg.msg_control = NULL;
> - smb_msg.msg_controllen = 0;
> - iov_iter_kvec(&smb_msg.msg_iter, READ | ITER_KVEC,
> - ÂÂÂÂÂÂiov_orig, nr_segs, to_read);
> + smb_msg->msg_control = NULL;
> + smb_msg->msg_controllen = 0;
> Â
> - for (total_read = 0; msg_data_left(&smb_msg); total_read += length) {
> + for (total_read = 0; msg_data_left(smb_msg); total_read += length) {
> Â try_to_freeze();
> Â
> - if (server_unresponsive(server)) {
> - total_read = -ECONNABORTED;
> - break;
> - }
> + if (server_unresponsive(server))
> + return -ECONNABORTED;
> Â
> - length = sock_recvmsg(server->ssocket, &smb_msg, 0);
> + length = sock_recvmsg(server->ssocket, smb_msg, 0);
> Â
> - if (server->tcpStatus == CifsExiting) {
> - total_read = -ESHUTDOWN;
> - break;
> - } else if (server->tcpStatus == CifsNeedReconnect) {
> + if (server->tcpStatus == CifsExiting)
> + return -ESHUTDOWN;
> +
> + if (server->tcpStatus == CifsNeedReconnect) {
> Â cifs_reconnect(server);
> - total_read = -ECONNABORTED;
> - break;
> - } else if (length == -ERESTARTSYS ||
> - ÂÂÂlength == -EAGAIN ||
> - ÂÂÂlength == -EINTR) {
> + return -ECONNABORTED;
> + }
> +
> + if (length == -ERESTARTSYS ||
> + ÂÂÂÂlength == -EAGAIN ||
> + ÂÂÂÂlength == -EINTR) {
> Â /*
> Â Â* Minimum sleep to prevent looping, allowing socket
> Â Â* to clear and app threads to set tcpStatus
> @@ -542,11 +537,12 @@ cifs_readv_from_socket(struct TCP_Server_Info *server, struct kvec *iov_orig,
> Â usleep_range(1000, 2000);
> Â length = 0;
> Â continue;
> - } else if (length <= 0) {
> + }
> +
> + if (length <= 0) {
> Â cifs_dbg(FYI, "Received no data or error: %d\n", length);
> Â cifs_reconnect(server);
> - total_read = -ECONNABORTED;
> - break;
> + return -ECONNABORTED;
> Â }
> Â }
> Â return total_read;
> @@ -556,12 +552,21 @@ int
> Âcifs_read_from_socket(struct TCP_Server_Info *server, char *buf,
> Â ÂÂÂÂÂÂunsigned int to_read)
> Â{
> - struct kvec iov;
> + struct msghdr smb_msg;
> + struct kvec iov = {.iov_base = buf, .iov_len = to_read};
> + iov_iter_kvec(&smb_msg.msg_iter, READ | ITER_KVEC, &iov, 1, to_read);
> Â
> - iov.iov_base = buf;
> - iov.iov_len = to_read;
> + return cifs_readv_from_socket(server, &smb_msg);
> +}
> Â
> - return cifs_readv_from_socket(server, &iov, 1, to_read);
> +int
> +cifs_read_page_from_socket(struct TCP_Server_Info *server, struct page *page,
> + ÂÂÂÂÂÂunsigned int to_read)
> +{
> + struct msghdr smb_msg;
> + struct bio_vec bv = {.bv_page = page, .bv_len = to_read};
> + iov_iter_bvec(&smb_msg.msg_iter, READ | ITER_BVEC, &bv, 1, to_read);
> + return cifs_readv_from_socket(server, &smb_msg);
> Â}
> Â
> Âstatic bool
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index ff882ae..0f71867 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -2855,39 +2855,31 @@ cifs_uncached_read_into_pages(struct TCP_Server_Info *server,
> Â int result = 0;
> Â unsigned int i;
> Â unsigned int nr_pages = rdata->nr_pages;
> - struct kvec iov;
> Â
> Â rdata->got_bytes = 0;
> Â rdata->tailsz = PAGE_SIZE;
> Â for (i = 0; i < nr_pages; i++) {
> Â struct page *page = rdata->pages[i];
> + size_t n;
> Â
> - if (len >= PAGE_SIZE) {
> - /* enough data to fill the page */
> - iov.iov_base = kmap(page);
> - iov.iov_len = PAGE_SIZE;
> - cifs_dbg(FYI, "%u: iov_base=%p iov_len=%zu\n",
> - Âi, iov.iov_base, iov.iov_len);
> - len -= PAGE_SIZE;
> - } else if (len > 0) {
> - /* enough for partial page, fill and zero the rest */
> - iov.iov_base = kmap(page);
> - iov.iov_len = len;
> - cifs_dbg(FYI, "%u: iov_base=%p iov_len=%zu\n",
> - Âi, iov.iov_base, iov.iov_len);
> - memset(iov.iov_base + len, '\0', PAGE_SIZE - len);
> - rdata->tailsz = len;
> - len = 0;
> - } else {
> + if (len <= 0) {
> Â /* no need to hold page hostage */
> Â rdata->pages[i] = NULL;
> Â rdata->nr_pages--;
> Â put_page(page);
> Â continue;
> Â }
> -
> - result = cifs_readv_from_socket(server, &iov, 1, iov.iov_len);
> - kunmap(page);
> + n = len;
> + if (len >= PAGE_SIZE) {
> + /* enough data to fill the page */
> + n = PAGE_SIZE;
> + len -= n;
> + } else {
> + zero_user(page, len, PAGE_SIZE - len);
> + rdata->tailsz = len;
> + len = 0;
> + }
> + result = cifs_read_page_from_socket(server, page, n);
> Â if (result < 0)
> Â break;
> Â
> @@ -3303,7 +3295,6 @@ cifs_readpages_read_into_pages(struct TCP_Server_Info *server,
> Â u64 eof;
> Â pgoff_t eof_index;
> Â unsigned int nr_pages = rdata->nr_pages;
> - struct kvec iov;
> Â
> Â /* determine the eof that the server (probably) has */
> Â eof = CIFS_I(rdata->mapping->host)->server_eof;
> @@ -3314,23 +3305,14 @@ cifs_readpages_read_into_pages(struct TCP_Server_Info *server,
> Â rdata->tailsz = PAGE_CACHE_SIZE;
> Â for (i = 0; i < nr_pages; i++) {
> Â struct page *page = rdata->pages[i];
> + size_t n = PAGE_CACHE_SIZE;
> Â
> Â if (len >= PAGE_CACHE_SIZE) {
> - /* enough data to fill the page */
> - iov.iov_base = kmap(page);
> - iov.iov_len = PAGE_CACHE_SIZE;
> - cifs_dbg(FYI, "%u: idx=%lu iov_base=%p iov_len=%zu\n",
> - Âi, page->index, iov.iov_base, iov.iov_len);
> Â len -= PAGE_CACHE_SIZE;
> Â } else if (len > 0) {
> Â /* enough for partial page, fill and zero the rest */
> - iov.iov_base = kmap(page);
> - iov.iov_len = len;
> - cifs_dbg(FYI, "%u: idx=%lu iov_base=%p iov_len=%zu\n",
> - Âi, page->index, iov.iov_base, iov.iov_len);
> - memset(iov.iov_base + len,
> - '\0', PAGE_CACHE_SIZE - len);
> - rdata->tailsz = len;
> + zero_user(page, len, PAGE_CACHE_SIZE - len);
> + n = rdata->tailsz = len;
> Â len = 0;
> Â } else if (page->index > eof_index) {
> Â /*
> @@ -3360,8 +3342,7 @@ cifs_readpages_read_into_pages(struct TCP_Server_Info *server,
> Â continue;
> Â }
> Â
> - result = cifs_readv_from_socket(server, &iov, 1, iov.iov_len);
> - kunmap(page);
> + result = cifs_read_page_from_socket(server, page, n);
> Â if (result < 0)
> Â break;
> Â

Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxxxxxxx>