CIFS: pass page offsets on SMB1 read/write

PATCH In review Long Li 3 participants 8 messages Sep 20, 2018 – Sep 24, 2018

Subsystems (3)

InfiniBand SMB client (CIFS) Kernel SMB3 server (ksmbd)

lore

Share & tools

lore

Long Li

author

2018-09-20 21:18 UTC #0 lore raw

PATCH v3 1/7 CIFS: pass page offsets on SMB1 read/write

From: Long Li <[email protected]>

When issuing SMB1 read/write, pass the page offset to transport.

Signed-off-by Long Li <[email protected]>

---
 fs/cifs/cifssmb.c | 2 ++
 1 file changed, 2 insertions(+)

fs/cifs/cifssmb.c +2 -0

index 41329f4..f82fd34 100644
@@ -1607,6 +1607,7 @@ cifs_readv_callback(struct mid_q_entry *mid)
 	struct smb_rqst rqst = { .rq_iov = rdata->iov,
 				 .rq_nvec = 2,
 				 .rq_pages = rdata->pages,
+				 .rq_offset = rdata->page_offset,
 				 .rq_npages = rdata->nr_pages,
 				 .rq_pagesz = rdata->pagesz,
 				 .rq_tailsz = rdata->tailsz };

@@ -2210,6 +2211,7 @@ cifs_async_writev(struct cifs_writedata *wdata,
 	rqst.rq_iov = iov;
 	rqst.rq_nvec = 2;
 	rqst.rq_pages = wdata->pages;
+	rqst.rq_offset = wdata->page_offset;
 	rqst.rq_npages = wdata->nr_pages;
 	rqst.rq_pagesz = wdata->pagesz;
 	rqst.rq_tailsz = wdata->tailsz;

Signature · 1 line

2.7.4

Long Li

author

2018-09-20 21:18 UTC #1 lore raw

PATCH v3 2/7 CIFS: SMBD: Do not call ib_dereg_mr on invalidated memory registration

From: Long Li <[email protected]>

It is not necessary to deregister a memory registration after it has been successfully invalidated.

Signed-off-by Long Li <[email protected]>

---
 fs/cifs/smbdirect.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

fs/cifs/smbdirect.c +19 ~~-19~~

index 5fdb9a5..5e28236 100644
@@ -2295,8 +2295,12 @@ static void smbd_mr_recovery_work(struct work_struct *work)
 	int rc;
 
 	list_for_each_entry(smbdirect_mr, &info->mr_list, list) {
-		if (smbdirect_mr->state == MR_INVALIDATED ||
-			smbdirect_mr->state == MR_ERROR) {
+		if (smbdirect_mr->state == MR_INVALIDATED)
+			ib_dma_unmap_sg(
+				info->id->device, smbdirect_mr->sgl,
+				smbdirect_mr->sgl_count,
+				smbdirect_mr->dir);
+		else if (smbdirect_mr->state == MR_ERROR) {
 
 			/* recover this MR entry */
 			rc = ib_dereg_mr(smbdirect_mr->mr);

@@ -2320,25 +2324,21 @@ static void smbd_mr_recovery_work(struct work_struct *work)
 				smbd_disconnect_rdma_connection(info);
 				continue;
 			}
+		} else
+			/* This MR is being used, don't recover it */
+			continue;
 
-			if (smbdirect_mr->state == MR_INVALIDATED)
-				ib_dma_unmap_sg(
-					info->id->device, smbdirect_mr->sgl,
-					smbdirect_mr->sgl_count,
-					smbdirect_mr->dir);
-
-			smbdirect_mr->state = MR_READY;
+		smbdirect_mr->state = MR_READY;
 
-			/* smbdirect_mr->state is updated by this function
-			 * and is read and updated by I/O issuing CPUs trying
-			 * to get a MR, the call to atomic_inc_return
-			 * implicates a memory barrier and guarantees this
-			 * value is updated before waking up any calls to
-			 * get_mr() from the I/O issuing CPUs
-			 */
-			if (atomic_inc_return(&info->mr_ready_count) == 1)
-				wake_up_interruptible(&info->wait_mr);
-		}
+		/* smbdirect_mr->state is updated by this function
+		 * and is read and updated by I/O issuing CPUs trying
+		 * to get a MR, the call to atomic_inc_return
+		 * implicates a memory barrier and guarantees this
+		 * value is updated before waking up any calls to
+		 * get_mr() from the I/O issuing CPUs
+		 */
+		if (atomic_inc_return(&info->mr_ready_count) == 1)
+			wake_up_interruptible(&info->wait_mr);
 	}
 }

Signature · 1 line

2.7.4

Long Li

author

2018-09-20 21:18 UTC #2 lore raw

PATCH v3 3/7 CIFS: Add support for direct I/O read

In reply to Long Li

From: Long Li <[email protected]>

With direct I/O read, we transfer the data directly from transport layer to the user data buffer.

Change in v3: add support for kernel AIO

Signed-off-by Long Li <[email protected]>

---
 fs/cifs/cifsfs.h   |   1 +
 fs/cifs/cifsglob.h |   5 ++
 fs/cifs/file.c     | 210 +++++++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 187 insertions(+), 29 deletions(-)

fs/cifs/cifsfs.h +1 -0

index f047e87..ed5479c 100644
@@ -101,6 +101,7 @@ extern int cifs_open(struct inode *inode, struct file *file);
 extern int cifs_close(struct inode *inode, struct file *file);
 extern int cifs_closedir(struct inode *inode, struct file *file);
 extern ssize_t cifs_user_readv(struct kiocb *iocb, struct iov_iter *to);
+extern ssize_t cifs_direct_readv(struct kiocb *iocb, struct iov_iter *to);
 extern ssize_t cifs_strict_readv(struct kiocb *iocb, struct iov_iter *to);
 extern ssize_t cifs_user_writev(struct kiocb *iocb, struct iov_iter *from);
 extern ssize_t cifs_strict_writev(struct kiocb *iocb, struct iov_iter *from);

fs/cifs/cifsglob.h +5 -0

index 9dcaed0..2131fec 100644
@@ -1172,6 +1172,11 @@ struct cifs_aio_ctx {
 	unsigned int		len;
 	unsigned int		total_len;
 	bool			should_dirty;
+	/*
+	 * Indicates if this aio_ctx is for direct_io,
+	 * If yes, iter is a copy of the user passed iov_iter
+	 */
+	bool			direct_io;
 };
 
 struct cifs_readdata;

fs/cifs/file.c +181 ~~-29~~

305 lines not on this page · raw message

Signature · 1 line

2.7.4

Long Li

author

2018-09-20 21:18 UTC #5 lore raw

PATCH v3 4/7 CIFS: Add support for direct I/O write

In reply to Long Li

From: Long Li <[email protected]>

With direct I/O write, user supplied buffers are pinned to the memory and data are transferred directly from user buffers to the transport layer.

Change in v3: add support for kernel AIO

Signed-off-by Long Li <[email protected]>

---
 fs/cifs/cifsfs.h |   1 +
 fs/cifs/file.c   | 196 ++++++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 166 insertions(+), 31 deletions(-)

fs/cifs/cifsfs.h +1 -0

index ed5479c..cc54051 100644
@@ -104,6 +104,7 @@ extern ssize_t cifs_user_readv(struct kiocb *iocb, struct iov_iter *to);
 extern ssize_t cifs_direct_readv(struct kiocb *iocb, struct iov_iter *to);
 extern ssize_t cifs_strict_readv(struct kiocb *iocb, struct iov_iter *to);
 extern ssize_t cifs_user_writev(struct kiocb *iocb, struct iov_iter *from);
+extern ssize_t cifs_direct_writev(struct kiocb *iocb, struct iov_iter *from);
 extern ssize_t cifs_strict_writev(struct kiocb *iocb, struct iov_iter *from);
 extern int cifs_lock(struct file *, int, struct file_lock *);
 extern int cifs_fsync(struct file *, loff_t, loff_t, int);

fs/cifs/file.c +165 ~~-31~~

230 lines not on this page · raw message

Signature · 1 line

2.7.4

Pavel Shilovsky

2018-09-21 22:30 UTC #6 lore raw

Re: [PATCH V3 (resend) 4/7] CIFS: Add support for direct I/O write

чт, 20 сент. 2018 г. в 14:22, Long Li <[email protected]>:

Quoted 163 lines

From: Long Li <[email protected]>

With direct I/O write, user supplied buffers are pinned to the memory and data are transferred directly from user buffers to the transport layer.

Change in v3: add support for kernel AIO

Signed-off-by Long Li <[email protected]>

---
 fs/cifs/cifsfs.h |   1 +
 fs/cifs/file.c   | 196 ++++++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 166 insertions(+), 31 deletions(-)

fs/cifs/cifsfs.h +1 -0

index ed5479c..cc54051 100644
@@ -104,6 +104,7 @@ extern ssize_t cifs_user_readv(struct kiocb *iocb, struct iov_iter *to);
 extern ssize_t cifs_direct_readv(struct kiocb *iocb, struct iov_iter *to);
 extern ssize_t cifs_strict_readv(struct kiocb *iocb, struct iov_iter *to);
 extern ssize_t cifs_user_writev(struct kiocb *iocb, struct iov_iter *from);
+extern ssize_t cifs_direct_writev(struct kiocb *iocb, struct iov_iter *from);
 extern ssize_t cifs_strict_writev(struct kiocb *iocb, struct iov_iter *from);
 extern int cifs_lock(struct file *, int, struct file_lock *);
 extern int cifs_fsync(struct file *, loff_t, loff_t, int);

fs/cifs/file.c +69 ~~-31~~

index 6a939fa..2a5d209 100644
@@ -2537,6 +2537,8 @@ cifs_write_from_iter(loff_t offset, size_t len, struct iov_iter *from,
        loff_t saved_offset = offset;
        pid_t pid;
        struct TCP_Server_Info *server;
+       struct page **pagevec;
+       size_t start;
 
        if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_RWPIDFORWARD)
                pid = open_file->pid;

@@ -2553,38 +2555,74 @@ cifs_write_from_iter(loff_t offset, size_t len, struct iov_iter *from,
                if (rc)
                        break;
 
-               nr_pages = get_numpages(wsize, len, &cur_len);
-               wdata = cifs_writedata_alloc(nr_pages,
+               if (ctx->direct_io) {
+                       cur_len = iov_iter_get_pages_alloc(
+                               from, &pagevec, wsize, &start);
+                       if (cur_len < 0) {
+                               cifs_dbg(VFS,
+                                       "direct_writev couldn't get user pages "
+                                       "(rc=%zd) iter type %d iov_offset %zd count"
+                                       " %zd\n",
+                                       cur_len, from->type,
+                                       from->iov_offset, from->count);
+                               dump_stack();
+                               break;
+                       }
+                       iov_iter_advance(from, cur_len);
+
+                       nr_pages = (cur_len + start + PAGE_SIZE - 1) / PAGE_SIZE;
+
+                       wdata = cifs_writedata_direct_alloc(pagevec,
                                             cifs_uncached_writev_complete);
-               if (!wdata) {
-                       rc = -ENOMEM;
-                       add_credits_and_wake_if(server, credits, 0);
-                       break;
-               }
+                       if (!wdata) {
+                               rc = -ENOMEM;
+                               add_credits_and_wake_if(server, credits, 0);
+                               break;
+                       }
 
-               rc = cifs_write_allocate_pages(wdata->pages, nr_pages);
-               if (rc) {
-                       kfree(wdata);
-                       add_credits_and_wake_if(server, credits, 0);
-                       break;
-               }
 
-               num_pages = nr_pages;
-               rc = wdata_fill_from_iovec(wdata, from, &cur_len, &num_pages);
-               if (rc) {
-                       for (i = 0; i < nr_pages; i++)
-                               put_page(wdata->pages[i]);
-                       kfree(wdata);
-                       add_credits_and_wake_if(server, credits, 0);
-                       break;
-               }
+                       wdata->page_offset = start;
+                       wdata->tailsz =
+                               nr_pages > 1 ?
+                                       cur_len - (PAGE_SIZE - start) -
+                                       (nr_pages - 2) * PAGE_SIZE :
+                                       cur_len;
+               } else {
+                       nr_pages = get_numpages(wsize, len, &cur_len);
+                       wdata = cifs_writedata_alloc(nr_pages,
+                                            cifs_uncached_writev_complete);
+                       if (!wdata) {
+                               rc = -ENOMEM;
+                               add_credits_and_wake_if(server, credits, 0);
+                               break;
+                       }
 
-               /*
-                * Bring nr_pages down to the number of pages we actually used,
-                * and free any pages that we didn't use.
-                */
-               for ( ; nr_pages > num_pages; nr_pages--)
-                       put_page(wdata->pages[nr_pages - 1]);
+                       rc = cifs_write_allocate_pages(wdata->pages, nr_pages);
+                       if (rc) {
+                               kfree(wdata);
+                               add_credits_and_wake_if(server, credits, 0);
+                               break;
+                       }
+
+                       num_pages = nr_pages;
+                       rc = wdata_fill_from_iovec(wdata, from, &cur_len, &num_pages);
+                       if (rc) {
+                               for (i = 0; i < nr_pages; i++)
+                                       put_page(wdata->pages[i]);
+                               kfree(wdata);
+                               add_credits_and_wake_if(server, credits, 0);
+                               break;
+                       }
+
+                       /*
+                        * Bring nr_pages down to the number of pages we actually used,
+                        * and free any pages that we didn't use.
+                        */
+                       for ( ; nr_pages > num_pages; nr_pages--)
+                               put_page(wdata->pages[nr_pages - 1]);
+
+                       wdata->tailsz = cur_len - ((nr_pages - 1) * PAGE_SIZE);
+               }
 
                wdata->sync_mode = WB_SYNC_ALL;
                wdata->nr_pages = nr_pages;

@@ -2593,7 +2631,6 @@ cifs_write_from_iter(loff_t offset, size_t len, struct iov_iter *from,
                wdata->pid = pid;
                wdata->bytes = cur_len;
                wdata->pagesz = PAGE_SIZE;
-               wdata->tailsz = cur_len - ((nr_pages - 1) * PAGE_SIZE);
                wdata->credits = credits;
                wdata->ctx = ctx;
                kref_get(&ctx->refcount);

@@ -2687,8 +2724,9 @@ static void collect_uncached_write_data(struct cifs_aio_ctx *ctx)
                kref_put(&wdata->refcount, cifs_uncached_writedata_release);
        }
 
-       for (i = 0; i < ctx->npages; i++)
-               put_page(ctx->bv[i].bv_page);
+       if (!ctx->direct_io)
+               for (i = 0; i < ctx->npages; i++)
+                       put_page(ctx->bv[i].bv_page);
 
        cifs_stats_bytes_written(tcon, ctx->total_len);
        set_bit(CIFS_INO_INVALID_MAPPING, &CIFS_I(dentry->d_inode)->flags);

fs/cifs/file.c static void collect_uncached_write_data(struct cifs_aio_ctx *ctx) · line 2744

Quoted 4 lines

fs/cifs/file.c +1 -0

@@ -2703,6 +2741,102 @@ static void collect_uncached_write_data(struct cifs_aio_ctx *ctx)
                complete(&ctx->done);
 }
 
+ssize_t cifs_direct_writev(struct kiocb *iocb, struct iov_iter *from)

the same comment as for readv: this function is almost identical to cifs_user_writev, can't the latter be refactored to avoid code duplication?

Quoted 99 lines

fs/cifs/file.c +95 -1

+{
+       struct file *file = iocb->ki_filp;
+       ssize_t total_written = 0;
+       struct cifsFileInfo *cfile;
+       struct cifs_tcon *tcon;
+       struct cifs_sb_info *cifs_sb;
+       struct TCP_Server_Info *server;
+       size_t len = iov_iter_count(from);
+       int rc;
+       struct cifs_aio_ctx *ctx;
+
+       /*
+        * iov_iter_get_pages_alloc doesn't work with ITER_KVEC.
+        * In this case, fall back to non-direct write function.
+        * this could be improved by getting pages directly in ITER_KVEC
+        */
+       if (from->type & ITER_KVEC) {
+               cifs_dbg(FYI, "use non-direct cifs_user_writev for kvec I/O\n");
+               return cifs_user_writev(iocb, from);
+       }
+
+       rc = generic_write_checks(iocb, from);
+       if (rc <= 0)
+               return rc;
+
+       cifs_sb = CIFS_FILE_SB(file);
+       cfile = file->private_data;
+       tcon = tlink_tcon(cfile->tlink);
+       server = tcon->ses->server;
+
+       if (!server->ops->async_writev)
+               return -ENOSYS;
+
+       ctx = cifs_aio_ctx_alloc();
+       if (!ctx)
+               return -ENOMEM;
+
+       ctx->cfile = cifsFileInfo_get(cfile);
+
+       if (!is_sync_kiocb(iocb))
+               ctx->iocb = iocb;
+
+       ctx->pos = iocb->ki_pos;
+
+       ctx->direct_io = true;
+       ctx->iter = *from;
+       ctx->len = len;
+
+       /* grab a lock here due to read response handlers can access ctx */
+       mutex_lock(&ctx->aio_mutex);
+
+       rc = cifs_write_from_iter(iocb->ki_pos, ctx->len, from,
+                                 cfile, cifs_sb, &ctx->list, ctx);
+
+       /*
+        * If at least one write was successfully sent, then discard any rc
+        * value from the later writes. If the other write succeeds, then
+        * we'll end up returning whatever was written. If it fails, then
+        * we'll get a new rc value from that.
+        */
+       if (!list_empty(&ctx->list))
+               rc = 0;
+
+       mutex_unlock(&ctx->aio_mutex);
+
+       if (rc) {
+               kref_put(&ctx->refcount, cifs_aio_ctx_release);
+               return rc;
+       }
+
+       if (!is_sync_kiocb(iocb)) {
+               kref_put(&ctx->refcount, cifs_aio_ctx_release);
+               return -EIOCBQUEUED;
+       }
+
+       rc = wait_for_completion_killable(&ctx->done);
+       if (rc) {
+               mutex_lock(&ctx->aio_mutex);
+               ctx->rc = rc = -EINTR;
+               total_written = ctx->total_len;
+               mutex_unlock(&ctx->aio_mutex);
+       } else {
+               rc = ctx->rc;
+               total_written = ctx->total_len;
+       }
+
+       kref_put(&ctx->refcount, cifs_aio_ctx_release);
+
+       if (unlikely(!total_written))
+               return rc;
+
+       iocb->ki_pos += total_written;
+       return total_written;
+}
+
 ssize_t cifs_user_writev(struct kiocb *iocb, struct iov_iter *from)
 {
        struct file *file = iocb->ki_filp;
--

Quoted 2 lines

2.7.4

--
Best regards,
Pavel Shilovsky

Long Li

author

2018-09-20 21:18 UTC #7 lore raw

PATCH v3 5/7 CIFS: Add direct I/O functions to file_operations

In reply to Long Li

From: Long Li <[email protected]>

With direct read/write functions implemented, add them to file_operations.

Dircet I/O is used under two conditions:
1. When mounting with "cache=none", CIFS uses direct I/O for all user file data transfer.
2. When opening a file with O_DIRECT, CIFS uses direct I/O for all data transfer on this file.

Signed-off-by Long Li <[email protected]>

---
 fs/cifs/cifsfs.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

fs/cifs/cifsfs.c +4 -6

index 7065426..3ba44f1 100644
@@ -1159,9 +1159,8 @@ const struct file_operations cifs_file_strict_ops = {
 };
 
 const struct file_operations cifs_file_direct_ops = {
-	/* BB reevaluate whether they can be done with directio, no cache */
-	.read_iter = cifs_user_readv,
-	.write_iter = cifs_user_writev,
+	.read_iter = cifs_direct_readv,
+	.write_iter = cifs_direct_writev,
 	.open = cifs_open,
 	.release = cifs_close,
 	.lock = cifs_lock,

@@ -1215,9 +1214,8 @@ const struct file_operations cifs_file_strict_nobrl_ops = {
 };
 
 const struct file_operations cifs_file_direct_nobrl_ops = {
-	/* BB reevaluate whether they can be done with directio, no cache */
-	.read_iter = cifs_user_readv,
-	.write_iter = cifs_user_writev,
+	.read_iter = cifs_direct_readv,
+	.write_iter = cifs_direct_writev,
 	.open = cifs_open,
 	.release = cifs_close,
 	.fsync = cifs_fsync,

Signature · 1 line

2.7.4