CIFS: Add direct I/O support

-----Original Message-----
From: [email protected] [mailto:linux-rdma- [email protected]] On Behalf Of Long Li
Sent: Wednesday, May 30, 2018 3:48 PM
To: Steve French <[email protected]>; [email protected]; samba- [email protected]; [email protected]; linux- [email protected]
Cc: Long Li <[email protected]>
Subject: [Patch v2 02/15] CIFS: Add support for direct pages in rdata

From: Long Li <[email protected]>

Add a function to allocate rdata without allocating pages for data transfer. This gives the caller an option to pass a number of pages that point to the data buffer.

rdata is still reponsible for free those pages after it's done.

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

---
fs/cifs/cifsglob.h | 2 +-
fs/cifs/file.c | 23 ++++++++++++++++++++---

2 files changed, 21 insertions(+), 4 deletions(-)

fs/cifs/cifsglob.h +1 -1

index 8d16c3e..56864a87 100644
@@ -1179,7 +1179,7 @@ struct cifs_readdata {
 	unsigned int			tailsz;
 	unsigned int			credits;
 	unsigned int			nr_pages;
-	struct page			*pages[];
+	struct page			**pages;
 };
 
 struct cifs_writedata;

fs/cifs/file.c +0 -0

index 23fd430..1c98293 100644
@@ -2880,13 +2880,13 @@ cifs_strict_writev(struct kiocb *iocb, struct
 iov_iter *from)
 }
 

static struct cifs_readdata *

fs/cifs/file.c +1 -1

-cifs_readdata_alloc(unsigned int nr_pages, work_func_t complete)
+cifs_readdata_direct_alloc(struct page **pages, work_func_t complete)

{
	struct cifs_readdata *rdata;

fs/cifs/file.c +1 -2

-	rdata = kzalloc(sizeof(*rdata) + (sizeof(struct page *) * nr_pages),
-			GFP_KERNEL);
+	rdata = kzalloc(sizeof(*rdata), GFP_KERNEL);

	if (rdata != NULL) {

fs/cifs/file.c +1 -0

+		rdata->pages = pages;

		kref_init(&rdata->refcount);
		INIT_LIST_HEAD(&rdata->list);
		init_completion(&rdata->done);

fs/cifs/file.c +16 -0

@@ -2896,6 +2896,22 @@ cifs_readdata_alloc(unsigned int nr_pages,
 work_func_t complete)
 	return rdata;
 }
 
+static struct cifs_readdata *
+cifs_readdata_alloc(unsigned int nr_pages, work_func_t complete)
+{
+	struct page **pages =
+		kzalloc(sizeof(struct page *) * nr_pages, GFP_KERNEL);
+	struct cifs_readdata *ret = NULL;
+
+	if (pages) {
+		ret = cifs_readdata_direct_alloc(pages, complete);
+		if (!ret)
+			kfree(pages);
+	}
+
+	return ret;
+}
+

void
cifs_readdata_release(struct kref *refcount)
{

fs/cifs/file.c +0 -1

 	kfree(rdata);
 }
 
--

2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Subject: RE: [Patch v2 02/15] CIFS: Add support for direct pages in rdata

Quoted 84 lines

-----Original Message-----
From: [email protected] [mailto:linux-rdma- [email protected]] On Behalf Of Long Li
Sent: Wednesday, May 30, 2018 3:48 PM
To: Steve French <[email protected]>; [email protected]; samba- [email protected]; [email protected]; linux- [email protected]
Cc: Long Li <[email protected]>
Subject: [Patch v2 02/15] CIFS: Add support for direct pages in rdata

From: Long Li <[email protected]>

Add a function to allocate rdata without allocating pages for data transfer. This gives the caller an option to pass a number of pages that point to the data buffer.

rdata is still reponsible for free those pages after it's done.

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

---
fs/cifs/cifsglob.h | 2 +-
fs/cifs/file.c | 23 ++++++++++++++++++++---

2 files changed, 21 insertions(+), 4 deletions(-)

fs/cifs/cifsglob.h index renamed from fs/cifs/cifsglob.h +0 -0

8d16c3e..56864a87 100644

fs/cifs/cifsglob.h +1 -1

@@ -1179,7 +1179,7 @@ struct cifs_readdata {
 	unsigned int			tailsz;
 	unsigned int			credits;
 	unsigned int			nr_pages;
-	struct page			*pages[];
+	struct page			**pages;
 };
 
 struct cifs_writedata;

fs/cifs/file.c index 23fd430..1c98293 renamed from fs/cifs/file.c +0 -0

100644

fs/cifs/file.c +0 -0

@@ -2880,13 +2880,13 @@ cifs_strict_writev(struct kiocb *iocb, struct
 iov_iter *from)  }
 

static struct cifs_readdata *

fs/cifs/file.c +1 -1

-cifs_readdata_alloc(unsigned int nr_pages, work_func_t complete)
+cifs_readdata_direct_alloc(struct page **pages, work_func_t complete)

{
	struct cifs_readdata *rdata;

fs/cifs/file.c +1 -2

-	rdata = kzalloc(sizeof(*rdata) + (sizeof(struct page *) * nr_pages),
-			GFP_KERNEL);
+	rdata = kzalloc(sizeof(*rdata), GFP_KERNEL);

	if (rdata != NULL) {

fs/cifs/file.c +1 -0

+		rdata->pages = pages;

		kref_init(&rdata->refcount);
		INIT_LIST_HEAD(&rdata->list);
		init_completion(&rdata->done);

fs/cifs/file.c +15 -0

@@ -2896,6 +2896,22 @@ cifs_readdata_alloc(unsigned int nr_pages,
 work_func_t complete)
 	return rdata;
 }
 
+static struct cifs_readdata *
+cifs_readdata_alloc(unsigned int nr_pages, work_func_t complete) {
+	struct page **pages =
+		kzalloc(sizeof(struct page *) * nr_pages, GFP_KERNEL);
+	struct cifs_readdata *ret = NULL;
+
+	if (pages) {
+		ret = cifs_readdata_direct_alloc(pages, complete);
+		if (!ret)
+			kfree(pages);
+	}
+
+	return ret;
+}
+

void
cifs_readdata_release(struct kref *refcount) { @@ -2910,6 +2926,7 @@ cifs_readdata_release(struct kref *refcount)

	if (rdata->cfile)
		cifsFileInfo_put(rdata->cfile);

fs/cifs/file.c cifs_readdata_release(struct kref *refcount) · line 2929

Quoted 1 line

fs/cifs/file.c +1 -0

+	kvfree(rdata->pages);

Is the kvfree() correct?

You use kzalloc() and kfree in cifs_readdata_alloc().

Mike

Quoted 11 lines

	kfree(rdata);
}

--
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at
https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.k

e

Quoted 1 line

rnel.org%2Fmajordomo-

info.html&data=02%7C01%7Clongli%40microsoft.com%7C

Quoted 1 line

e810388a534643737ab108d5c66bd6df%7C72f988bf86f141af91ab2d7cd011db

47%7C1

Quoted 1 line

%7C0%7C636633088833938755&sdata=iHKiji2rUhLHpbH5x13SJBWCvHExSr4a

rz9Xiv3

Quoted 1 line

1rMQ%3D&reserved=0

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at
https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.ke
rnel.org%2Fmajordomo- info.html&data=02%7C01%7Clongli%40microsoft.com%7Ce810388a53464373 7ab108d5c66bd6df%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63 6633088833938755&sdata=iHKiji2rUhLHpbH5x13SJBWCvHExSr4arz9Xiv31rMQ %3D&reserved=0

From: Long Li <[email protected]>

Add a function to allocate rdata without allocating pages for data transfer. This gives the caller an option to pass a number of pages that point to the data buffer.

rdata is still reponsible for free those pages after it's done.

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

---
  fs/cifs/cifsglob.h |  2 +-
  fs/cifs/file.c     | 23 ++++++++++++++++++++---
  2 files changed, 21 insertions(+), 4 deletions(-)

fs/cifs/cifsglob.h +0 -0

index 8d16c3e..56864a87 100644

fs/cifs/cifsglob.h +0 -0

  };
 
  struct cifs_writedata;

fs/cifs/file.c +20 -3

index 23fd430..1c98293 100644
@@ -2880,13 +2880,13 @@ cifs_strict_writev(struct kiocb *iocb, struct iov_iter *from)
  }
 
  static struct cifs_readdata *
-cifs_readdata_alloc(unsigned int nr_pages, work_func_t complete)
+cifs_readdata_direct_alloc(struct page **pages, work_func_t complete)
  {
  	struct cifs_readdata *rdata;
 
-	rdata = kzalloc(sizeof(*rdata) + (sizeof(struct page *) * nr_pages),
-			GFP_KERNEL);
+	rdata = kzalloc(sizeof(*rdata), GFP_KERNEL);
  	if (rdata != NULL) {
+		rdata->pages = pages;
  		kref_init(&rdata->refcount);
  		INIT_LIST_HEAD(&rdata->list);
  		init_completion(&rdata->done);

@@ -2896,6 +2896,22 @@ cifs_readdata_alloc(unsigned int nr_pages, work_func_t complete)
  	return rdata;
  }
 
+static struct cifs_readdata *
+cifs_readdata_alloc(unsigned int nr_pages, work_func_t complete)
+{
+	struct page **pages =
+		kzalloc(sizeof(struct page *) * nr_pages, GFP_KERNEL);
+	struct cifs_readdata *ret = NULL;
+
+	if (pages) {
+		ret = cifs_readdata_direct_alloc(pages, complete);
+		if (!ret)
+			kfree(pages);
+	}
+
+	return ret;
+}
+
  void
  cifs_readdata_release(struct kref *refcount)
  {

@@ -2910,6 +2926,7 @@ cifs_readdata_release(struct kref *refcount)
  	if (rdata->cfile)
  		cifsFileInfo_put(rdata->cfile);
 
+	kvfree(rdata->pages);
  	kfree(rdata);
  }
 

From: Tom Talpey <[email protected]>
Sent: Saturday, June 23, 2018 6:50 PM
To: Long Li <[email protected]>; Steve French <[email protected]>; [email protected]; [email protected]; linux- [email protected]; [email protected]
Subject: Re: [Patch v2 02/15] CIFS: Add support for direct pages in rdata

Quoted 2 lines · Long Li

From: Long Li <[email protected]>

Quoted 5 lines

Add a function to allocate rdata without allocating pages for data transfer. This gives the caller an option to pass a number of pages that point to the data buffer.

rdata is still reponsible for free those pages after it's done.

"Caller" is still responsible? Or is the rdata somehow freeing itself via another mechanism?

Quoted 13 lines

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

---
  fs/cifs/cifsglob.h |  2 +-
  fs/cifs/file.c     | 23 ++++++++++++++++++++---
  2 files changed, 21 insertions(+), 4 deletions(-)

fs/cifs/cifsglob.h index renamed from fs/cifs/cifsglob.h +0 -0

8d16c3e..56864a87 100644

fs/cifs/cifsglob.h struct cifs_readdata { · line 1182

Quoted 5 lines

fs/cifs/cifsglob.h +1 -1

@@ -1179,7 +1179,7 @@ struct cifs_readdata {
  	unsigned int			tailsz;
  	unsigned int			credits;
  	unsigned int			nr_pages;
-	struct page			*pages[];
+	struct page			**pages;

Technically speaking, these are syntactically equivalent. It may not be worth changing this historic definition.

Tom.

fs/cifs/file.c cifs_strict_writev(struct kiocb *iocb, struct

Quoted 8 lines

fs/cifs/cifsglob.h +0 -0

  };
 
  struct cifs_writedata;

fs/cifs/file.c index 23fd430..1c98293 renamed from fs/cifs/file.c +0 -0

100644

fs/cifs/file.c +0 -0

@@ -2880,13 +2880,13 @@ cifs_strict_writev(struct kiocb *iocb, struct

iov_iter *from)

fs/cifs/file.c cifs_readdata_alloc(unsigned int nr_pages,

Quoted 17 lines

fs/cifs/file.c +3 -3

  }
 
  static struct cifs_readdata *
-cifs_readdata_alloc(unsigned int nr_pages, work_func_t complete)
+cifs_readdata_direct_alloc(struct page **pages, work_func_t complete)
  {
  	struct cifs_readdata *rdata;
 
-	rdata = kzalloc(sizeof(*rdata) + (sizeof(struct page *) * nr_pages),
-			GFP_KERNEL);
+	rdata = kzalloc(sizeof(*rdata), GFP_KERNEL);
  	if (rdata != NULL) {
+		rdata->pages = pages;
  		kref_init(&rdata->refcount);
  		INIT_LIST_HEAD(&rdata->list);
  		init_completion(&rdata->done);

@@ -2896,6 +2896,22 @@ cifs_readdata_alloc(unsigned int nr_pages,

work_func_t complete)

Quoted 30 lines

fs/cifs/file.c +16 -0

  	return rdata;
  }
 
+static struct cifs_readdata *
+cifs_readdata_alloc(unsigned int nr_pages, work_func_t complete) {
+	struct page **pages =
+		kzalloc(sizeof(struct page *) * nr_pages, GFP_KERNEL);
+	struct cifs_readdata *ret = NULL;
+
+	if (pages) {
+		ret = cifs_readdata_direct_alloc(pages, complete);
+		if (!ret)
+			kfree(pages);
+	}
+
+	return ret;
+}
+
  void
  cifs_readdata_release(struct kref *refcount)
  {

@@ -2910,6 +2926,7 @@ cifs_readdata_release(struct kref *refcount)
  	if (rdata->cfile)
  		cifsFileInfo_put(rdata->cfile);
 
+	kvfree(rdata->pages);
  	kfree(rdata);
  }
 

Quoted 7 lines · Long Li

From: Long Li <[email protected]>

Add a function to allocate rdata without allocating pages for data transfer. This gives the caller an option to pass a number of pages that point to the data buffer.

rdata is still reponsible for free those pages after it's done.

"Caller" is still responsible? Or is the rdata somehow freeing itself via another mechanism?

Quoted 6 lines

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

 fs/cifs/cifsglob.h |  2 +-
 fs/cifs/file.c     | 23 ++++++++++++++++++++---
 2 files changed, 21 insertions(+), 4 deletions(-)

fs/cifs/cifsglob.h struct cifs_readdata { · line 1182

Quoted 9 lines

fs/cifs/cifsglob.h +2 -1

index 8d16c3e..56864a87 100644
+++ b/fs/cifs/cifsglob.h

@@ -1179,7 +1179,7 @@ struct cifs_readdata {
 	unsigned int			tailsz;
 	unsigned int			credits;
 	unsigned int			nr_pages;
-	struct page			*pages[];
+	struct page			**pages;

Technically speaking, these are syntactically equivalent. It may not be worth changing this historic definition.

Quoted 18 lines · Tom Talpey

Quoted 7 lines · Long Li

From: Long Li <[email protected]>

Add a function to allocate rdata without allocating pages for data transfer. This gives the caller an option to pass a number of pages that point to the data buffer.

rdata is still reponsible for free those pages after it's done.

"Caller" is still responsible? Or is the rdata somehow freeing itself via another mechanism?

Quoted 6 lines

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

  fs/cifs/cifsglob.h |  2 +-
  fs/cifs/file.c     | 23 ++++++++++++++++++++---
  2 files changed, 21 insertions(+), 4 deletions(-)

Quoted 12 lines

fs/cifs/cifsglob.h struct cifs_readdata { · line 1182

Quoted 9 lines

fs/cifs/cifsglob.h +2 -1

index 8d16c3e..56864a87 100644
+++ b/fs/cifs/cifsglob.h

@@ -1179,7 +1179,7 @@ struct cifs_readdata {
  	unsigned int			tailsz;
  	unsigned int			credits;
  	unsigned int			nr_pages;
-	struct page			*pages[];
+	struct page			**pages;

Technically speaking, these are syntactically equivalent. It may not be worth changing this historic definition.

[] is a C99 'flex array', it has a different allocation behavior than ** and is not interchangeable..

Subject: Re: [Patch v2 02/15] CIFS: Add support for direct pages in rdata

Quoted 32 lines · Jason Gunthorpe

Quoted 30 lines · Tom Talpey

Quoted 7 lines · Long Li

From: Long Li <[email protected]>

Add a function to allocate rdata without allocating pages for data transfer. This gives the caller an option to pass a number of pages that point to the data buffer.

rdata is still reponsible for free those pages after it's done.

"Caller" is still responsible? Or is the rdata somehow freeing itself via another mechanism?

Quoted 10 lines

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

  fs/cifs/cifsglob.h |  2 +-
  fs/cifs/file.c     | 23 ++++++++++++++++++++---
  2 files changed, 21 insertions(+), 4 deletions(-)

fs/cifs/cifsglob.h index renamed from fs/cifs/cifsglob.h +0 -0

8d16c3e..56864a87 100644
+++ b/fs/cifs/cifsglob.h

fs/cifs/cifsglob.h index struct cifs_readdata { · line 1182

Quoted 5 lines

fs/cifs/cifsglob.h index renamed from fs/cifs/cifsglob.h +1 -1

@@ -1179,7 +1179,7 @@ struct cifs_readdata {
  	unsigned int			tailsz;
  	unsigned int			credits;
  	unsigned int			nr_pages;
-	struct page			*pages[];
+	struct page			**pages;

Technically speaking, these are syntactically equivalent. It may not be worth changing this historic definition.

Quoted 2 lines

[] is a C99 'flex array', it has a different allocation behavior than ** and is not interchangeable..

In that case, it's an even better reason to not change the declaration.

From: Long Li <[email protected]>

With offset defined in rdata, transport functions need to look at this offset when reading data into the correct places in pages.

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

---
  fs/cifs/cifsproto.h |  4 +++-
  fs/cifs/connect.c   |  5 +++--
  fs/cifs/file.c      | 52 +++++++++++++++++++++++++++++++++++++---------------
  fs/cifs/smb2ops.c   |  2 +-
  fs/cifs/smb2pdu.c   |  1 +
  5 files changed, 45 insertions(+), 19 deletions(-)

fs/cifs/cifsproto.h +3 -1

index dc80f84..1f27d8e 100644
@@ -203,7 +203,9 @@ 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_read_page_from_socket(struct TCP_Server_Info *server,
-				      struct page *page, unsigned int to_read);
+					struct page *page,
+					unsigned int page_offset,
+					unsigned int to_read);
  extern int cifs_setup_cifs_sb(struct smb_vol *pvolume_info,
  			       struct cifs_sb_info *cifs_sb);
  extern int cifs_match_super(struct super_block *, void *);

fs/cifs/connect.c +3 -2

index 83b0234..8501da0 100644
@@ -594,10 +594,11 @@ cifs_read_from_socket(struct TCP_Server_Info *server, char *buf,
 
  int
  cifs_read_page_from_socket(struct TCP_Server_Info *server, struct page *page,
-		      unsigned int to_read)
+	unsigned int page_offset, unsigned int to_read)
  {
  	struct msghdr smb_msg;
-	struct bio_vec bv = {.bv_page = page, .bv_len = to_read};
+	struct bio_vec bv = {
+		.bv_page = page, .bv_len = to_read, .bv_offset = page_offset};
  	iov_iter_bvec(&smb_msg.msg_iter, READ | ITER_BVEC, &bv, 1, to_read);
  	return cifs_readv_from_socket(server, &smb_msg);
  }

fs/cifs/file.c +12 -7

index 1c98293..87eece6 100644
@@ -3026,12 +3026,20 @@ uncached_fill_pages(struct TCP_Server_Info *server,
  	int result = 0;
  	unsigned int i;
  	unsigned int nr_pages = rdata->nr_pages;
+	unsigned int page_offset = rdata->page_offset;
 
  	rdata->got_bytes = 0;
  	rdata->tailsz = PAGE_SIZE;
  	for (i = 0; i < nr_pages; i++) {
  		struct page *page = rdata->pages[i];
  		size_t n;
+		unsigned int segment_size = rdata->pagesz;
+
+		if (i == 0)
+			segment_size -= page_offset;
+		else
+			page_offset = 0;
+
 
  		if (len <= 0) {
  			/* no need to hold page hostage */

@@ -3040,24 +3048,25 @@ uncached_fill_pages(struct TCP_Server_Info *server,
  			put_page(page);
  			continue;
  		}
+
  		n = len;
-		if (len >= PAGE_SIZE) {
+		if (len >= segment_size)
  			/* enough data to fill the page */
-			n = PAGE_SIZE;
-			len -= n;
-		} else {
-			zero_user(page, len, PAGE_SIZE - len);
+			n = segment_size;
+		else
  			rdata->tailsz = len;
-			len = 0;
-		}

fs/cifs/file.c +16 -5

  		else
-			result = cifs_read_page_from_socket(server, page, n);
+			result = cifs_read_page_from_socket(
+					server, page, page_offset, n);
  		if (result < 0)
  			break;
 

@@ -3130,6 +3139,7 @@ cifs_send_async_read(loff_t offset, size_t len, struct cifsFileInfo *open_file,
  		rdata->bytes = cur_len;
  		rdata->pid = pid;
  		rdata->pagesz = PAGE_SIZE;
+		rdata->tailsz = PAGE_SIZE;
  		rdata->read_into_pages = cifs_uncached_read_into_pages;
  		rdata->copy_into_pages = cifs_uncached_copy_into_pages;
  		rdata->credits = credits;

@@ -3574,6 +3584,7 @@ readpages_fill_pages(struct TCP_Server_Info *server,
  	u64 eof;
  	pgoff_t eof_index;
  	unsigned int nr_pages = rdata->nr_pages;
+	unsigned int page_offset = rdata->page_offset;
 
  	/* determine the eof that the server (probably) has */
  	eof = CIFS_I(rdata->mapping->host)->server_eof;

@@ -3584,13 +3595,21 @@ readpages_fill_pages(struct TCP_Server_Info *server,
  	rdata->tailsz = PAGE_SIZE;
  	for (i = 0; i < nr_pages; i++) {
  		struct page *page = rdata->pages[i];
-		size_t n = PAGE_SIZE;
+		unsigned int to_read = rdata->pagesz;
+		size_t n;
+
+		if (i == 0)
+			to_read -= page_offset;
+		else
+			page_offset = 0;
+
+		n = to_read;
 
-		if (len >= PAGE_SIZE) {
-			len -= PAGE_SIZE;
+		if (len >= to_read) {
+			len -= to_read;
  		} else if (len > 0) {
  			/* enough for partial page, fill and zero the rest */
-			zero_user(page, len, PAGE_SIZE - len);
+			zero_user(page, len + page_offset, to_read - len);
  			n = rdata->tailsz = len;
  			len = 0;
  		} else if (page->index > eof_index) {

fs/cifs/file.c +3 -1

  		else
-			result = cifs_read_page_from_socket(server, page, n);
+			result = cifs_read_page_from_socket(
+					server, page, page_offset, n);
  		if (result < 0)
  			break;
 

@@ -3807,6 +3828,7 @@ static int cifs_readpages(struct file *file, struct address_space *mapping,
  		rdata->bytes = bytes;
  		rdata->pid = pid;
  		rdata->pagesz = PAGE_SIZE;
+		rdata->tailsz = PAGE_SIZE;
  		rdata->read_into_pages = cifs_readpages_read_into_pages;
  		rdata->copy_into_pages = cifs_readpages_copy_into_pages;
  		rdata->credits = credits;

fs/cifs/smb2ops.c +1 -1

index 7c0edd2..1fa1c29 100644
@@ -2467,7 +2467,7 @@ read_data_into_pages(struct TCP_Server_Info *server, struct page **pages,
  			zero_user(page, len, PAGE_SIZE - len);
  			len = 0;
  		}
-		length = cifs_read_page_from_socket(server, page, n);
+		length = cifs_read_page_from_socket(server, page, 0, n);
  		if (length < 0)
  			return length;
  		server->total_read += length;

fs/cifs/smb2pdu.c +1 -0

index a02f6b6..f603fbe 100644
@@ -2683,6 +2683,7 @@ smb2_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 };

From: Tom Talpey <[email protected]>
Sent: Saturday, June 23, 2018 6:59 PM
To: Long Li <[email protected]>; Steve French <[email protected]>; [email protected]; [email protected]; linux- [email protected]; [email protected]
Subject: Re: [Patch v2 03/15] CIFS: Use offset when reading pages

Quoted 10 lines · Long Li

From: Long Li <[email protected]>

With offset defined in rdata, transport functions need to look at this offset when reading data into the correct places in pages.

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

---
  fs/cifs/cifsproto.h |  4 +++-
  fs/cifs/connect.c   |  5 +++--
  fs/cifs/file.c      | 52 +++++++++++++++++++++++++++++++++++++--------

-------

fs/cifs/cifsproto.h extern void dequeue_mid(struct mid_q_entry *mid,

Quoted 9 lines

  fs/cifs/smb2ops.c   |  2 +-
  fs/cifs/smb2pdu.c   |  1 +
  5 files changed, 45 insertions(+), 19 deletions(-)

fs/cifs/cifsproto.h index renamed from fs/cifs/cifsproto.h +0 -0

dc80f84..1f27d8e 100644

fs/cifs/cifsproto.h +0 -0

@@ -203,7 +203,9 @@ extern void dequeue_mid(struct mid_q_entry *mid,

bool malformed);

fs/cifs/cifsproto.h extern void dequeue_mid(struct mid_q_entry *mid, · line 203

Quoted 1 line

fs/cifs/cifsproto.h +0 -0

  extern int cifs_read_from_socket(struct TCP_Server_Info *server, char

*buf,

Quoted 14 lines

fs/cifs/cifsproto.h +3 -2

  			         unsigned int to_read);
  extern int cifs_read_page_from_socket(struct TCP_Server_Info *server,
-				      struct page *page, unsigned int to_read);
+					struct page *page,
+					unsigned int page_offset,
+					unsigned int to_read);
  extern int 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 83b0234..8501da0

100644

fs/cifs/connect.c cifs_read_from_socket(struct TCP_Server_Info · line 597

Quoted 4 lines

fs/cifs/connect.c +0 -0

@@ -594,10 +594,11 @@ cifs_read_from_socket(struct TCP_Server_Info
 *server, char *buf,
 
  int
  cifs_read_page_from_socket(struct TCP_Server_Info *server, struct page

*page,

fs/cifs/connect.c cifs_read_from_socket(struct TCP_Server_Info *server, char *buf,

Quoted 7 lines

fs/cifs/connect.c +3 -2

-		      unsigned int to_read)
+	unsigned int page_offset, unsigned int to_read)
  {
  	struct msghdr smb_msg;
-	struct bio_vec bv = {.bv_page = page, .bv_len = to_read};
+	struct bio_vec bv = {
+		.bv_page = page, .bv_len = to_read, .bv_offset =

page_offset};

fs/cifs/connect.c cifs_read_from_socket(struct TCP_Server_Info · line 603

Quoted 1 line

fs/cifs/connect.c +0 -0

  	iov_iter_bvec(&smb_msg.msg_iter, READ | ITER_BVEC, &bv, 1,

to_read);

fs/cifs/file.c uncached_fill_pages(struct TCP_Server_Info

Quoted 7 lines

fs/cifs/connect.c +0 -0

  	return cifs_readv_from_socket(server, &smb_msg);

  }

fs/cifs/file.c index 1c98293..87eece6 renamed from fs/cifs/file.c +0 -0

100644

fs/cifs/file.c +0 -0

@@ -3026,12 +3026,20 @@ uncached_fill_pages(struct TCP_Server_Info

*server,

Quoted 10 lines

fs/cifs/file.c +1 -0

  	int result = 0;
  	unsigned int i;
  	unsigned int nr_pages = rdata->nr_pages;
+	unsigned int page_offset = rdata->page_offset;
 
  	rdata->got_bytes = 0;
  	rdata->tailsz = PAGE_SIZE;
  	for (i = 0; i < nr_pages; i++) {
  		struct page *page = rdata->pages[i];
  		size_t n;

fs/cifs/file.c uncached_fill_pages(struct TCP_Server_Info *server,

Quoted 10 lines

fs/cifs/file.c +7 -0

+		unsigned int segment_size = rdata->pagesz;
+
+		if (i == 0)
+			segment_size -= page_offset;
+		else
+			page_offset = 0;
+
 
  		if (len <= 0) {
  			/* no need to hold page hostage */ @@ -3040,24

+3048,25 @@

Quoted 24 lines

uncached_fill_pages(struct TCP_Server_Info *server,
put_page(page); continue;
}

fs/cifs/file.c +8 -8

+
  		n = len;
-		if (len >= PAGE_SIZE) {
+		if (len >= segment_size)
  			/* enough data to fill the page */
-			n = PAGE_SIZE;
-			len -= n;
-		} else {
-			zero_user(page, len, PAGE_SIZE - len);
+			n = segment_size;
+		else
  			rdata->tailsz = len;
-			len = 0;
-		}
+		len -= n;
+
  		if (iter)
-			result = copy_page_from_iter(page, 0, n, iter);
+			result = copy_page_from_iter(
+					page, page_offset, n, iter);

Quoted 4 lines

  #ifdef CONFIG_CIFS_SMB_DIRECT
  		else if (rdata->mr)
  			result = n;
  #endif

I hadn't noticed this type of xonditional code before. It's kind of ugly to modify the data handling code like this. Do you plan to break this out into an smbdirect-specific hander, like the cases above and below?

fs/cifs/file.c

Quoted 3 lines

else

fs/cifs/file.c +0 -1

-			result = cifs_read_page_from_socket(server, page,

n);

fs/cifs/file.c cifs_send_async_read(loff_t offset, size_t len,

Quoted 6 lines

fs/cifs/file.c +2 -0

+			result = cifs_read_page_from_socket(
+					server, page, page_offset, n);
  		if (result < 0)
  			break;

@@ -3130,6 +3139,7 @@ cifs_send_async_read(loff_t offset, size_t len,

struct cifsFileInfo *open_file,

fs/cifs/file.c readpages_fill_pages(struct TCP_Server_Info

Quoted 8 lines

fs/cifs/file.c +1 -0

  		rdata->bytes = cur_len;
  		rdata->pid = pid;
  		rdata->pagesz = PAGE_SIZE;
+		rdata->tailsz = PAGE_SIZE;
  		rdata->read_into_pages = cifs_uncached_read_into_pages;
  		rdata->copy_into_pages = cifs_uncached_copy_into_pages;
  		rdata->credits = credits;

@@ -3574,6 +3584,7 @@ readpages_fill_pages(struct TCP_Server_Info

*server,

fs/cifs/file.c readpages_fill_pages(struct TCP_Server_Info

Quoted 8 lines

fs/cifs/file.c +1 -0

  	u64 eof;
  	pgoff_t eof_index;
  	unsigned int nr_pages = rdata->nr_pages;
+	unsigned int page_offset = rdata->page_offset;
 
  	/* determine the eof that the server (probably) has */
  	eof = CIFS_I(rdata->mapping->host)->server_eof;

@@ -3584,13 +3595,21 @@ readpages_fill_pages(struct TCP_Server_Info

*server,

Quoted 15 lines

fs/cifs/file.c +9 -2

  	rdata->tailsz = PAGE_SIZE;
  	for (i = 0; i < nr_pages; i++) {
  		struct page *page = rdata->pages[i];
-		size_t n = PAGE_SIZE;
+		unsigned int to_read = rdata->pagesz;
+		size_t n;
+
+		if (i == 0)
+			to_read -= page_offset;
+		else
+			page_offset = 0;
+
+		n = to_read;
 
-		if (len >= PAGE_SIZE) {

fs/cifs/file.c readpages_fill_pages(struct TCP_Server_Info *server,

Quoted 10 lines

fs/cifs/file.c +3 -2

-			len -= PAGE_SIZE;
+		if (len >= to_read) {
+			len -= to_read;
  		} else if (len > 0) {
  			/* enough for partial page, fill and zero the rest */
-			zero_user(page, len, PAGE_SIZE - len);
+			zero_user(page, len + page_offset, to_read - len);
  			n = rdata->tailsz = len;
  			len = 0;
  		} else if (page->index > eof_index) { @@ -3622,13 +3641,15

@@

fs/cifs/file.c uncached_fill_pages(struct TCP_Server_Info *server, · line 3066

Quoted 11 lines

readpages_fill_pages(struct TCP_Server_Info *server,
}

if (iter)

fs/cifs/file.c +2 -1

-			result = copy_page_from_iter(page, 0, n, iter);
+			result = copy_page_from_iter(
+					page, page_offset, n, iter);
  #ifdef CONFIG_CIFS_SMB_DIRECT
  		else if (rdata->mr)
  			result = n;
  #endif

Ditto previous comment/question.

fs/cifs/file.c

Quoted 2 lines

else

fs/cifs/file.c +0 -1

-			result = cifs_read_page_from_socket(server, page,

n);

fs/cifs/file.c static int cifs_readpages(struct file *file, struct

Quoted 6 lines

fs/cifs/file.c +2 -0

+			result = cifs_read_page_from_socket(
+					server, page, page_offset, n);
  		if (result < 0)
  			break;

@@ -3807,6 +3828,7 @@ static int cifs_readpages(struct file *file, struct

address_space *mapping,

fs/cifs/smb2ops.c read_data_into_pages(struct TCP_Server_Info

Quoted 12 lines

fs/cifs/file.c +1 -0

  		rdata->bytes = bytes;
  		rdata->pid = pid;
  		rdata->pagesz = PAGE_SIZE;
+		rdata->tailsz = PAGE_SIZE;
  		rdata->read_into_pages = cifs_readpages_read_into_pages;
  		rdata->copy_into_pages = cifs_readpages_copy_into_pages;
  		rdata->credits = credits;

fs/cifs/smb2ops.c index renamed from fs/cifs/smb2ops.c +0 -0

7c0edd2..1fa1c29 100644

fs/cifs/smb2ops.c +0 -0

@@ -2467,7 +2467,7 @@ read_data_into_pages(struct TCP_Server_Info

*server, struct page **pages,

Quoted 21 lines

fs/cifs/smb2ops.c +1 -1

  			zero_user(page, len, PAGE_SIZE - len);
  			len = 0;
  		}
-		length = cifs_read_page_from_socket(server, page, n);
+		length = cifs_read_page_from_socket(server, page, 0, n);
  		if (length < 0)
  			return length;
  		server->total_read += length;

fs/cifs/smb2pdu.c index renamed from fs/cifs/smb2pdu.c +0 -0

a02f6b6..f603fbe 100644

fs/cifs/smb2pdu.c +1 -0

@@ -2683,6 +2683,7 @@ smb2_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 };

From: Long Li <[email protected]>

Add a function to allocate wdata without allocating pages for data transfer. This gives the caller an option to pass a number of pages that point to the data buffer to write to.

wdata is reponsible for free those pages after it's done.

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

---
  fs/cifs/cifsglob.h  |  2 +-
  fs/cifs/cifsproto.h |  2 ++
  fs/cifs/cifssmb.c   | 17 ++++++++++++++---
  3 files changed, 17 insertions(+), 4 deletions(-)

fs/cifs/cifsglob.h +0 -0

index 56864a87..7f62c98 100644

fs/cifs/cifsglob.h +0 -0

  };
 
  /*

fs/cifs/cifsproto.h +2 -0

index 1f27d8e..7933c5f 100644
@@ -533,6 +533,8 @@ int cifs_async_writev(struct cifs_writedata *wdata,
  void cifs_writev_complete(struct work_struct *work);
  struct cifs_writedata *cifs_writedata_alloc(unsigned int nr_pages,
  						work_func_t complete);
+struct cifs_writedata *cifs_writedata_direct_alloc(struct page **pages,
+						work_func_t complete);
  void cifs_writedata_release(struct kref *refcount);
  int cifs_query_mf_symlink(unsigned int xid, struct cifs_tcon *tcon,
  			  struct cifs_sb_info *cifs_sb,

fs/cifs/cifssmb.c +1 -0

index c8e4278..5aca336 100644
@@ -1952,6 +1952,7 @@ cifs_writedata_release(struct kref *refcount)
  	if (wdata->cfile)
  		cifsFileInfo_put(wdata->cfile);
 
+	kvfree(wdata->pages);
  	kfree(wdata);
  }
 

fs/cifs/cifssmb.c +11 -3

+	if (pages)
+		return cifs_writedata_direct_alloc(pages, complete);
+
+	return NULL;
+}
+
+struct cifs_writedata *
+cifs_writedata_direct_alloc(struct page **pages, work_func_t complete)
+{
  	struct cifs_writedata *wdata;
 
-	/* writedata + number of page pointers */
-	wdata = kzalloc(sizeof(*wdata) +
-			sizeof(struct page *) * nr_pages, GFP_NOFS);
+	wdata = kzalloc(sizeof(*wdata), GFP_NOFS);
  	if (wdata != NULL) {
+		wdata->pages = pages;
  		kref_init(&wdata->refcount);
  		INIT_LIST_HEAD(&wdata->list);
  		init_completion(&wdata->done);

Subject: Re: [Patch v2 04/15] CIFS: Add support for direct pages in wdata

Quoted 7 lines · Long Li

From: Long Li <[email protected]>

Add a function to allocate wdata without allocating pages for data transfer. This gives the caller an option to pass a number of pages that point to the data buffer to write to.

wdata is reponsible for free those pages after it's done.

Same comment as for the earlier patch. "Caller" is responsible for "freeing" those pages? Confusing, as worded.

Quoted 13 lines

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

---
  fs/cifs/cifsglob.h  |  2 +-
  fs/cifs/cifsproto.h |  2 ++
  fs/cifs/cifssmb.c   | 17 ++++++++++++++---
  3 files changed, 17 insertions(+), 4 deletions(-)

fs/cifs/cifsglob.h index renamed from fs/cifs/cifsglob.h +0 -0

56864a87..7f62c98 100644

fs/cifs/cifsglob.h struct cifs_writedata { · line 1208

Quoted 5 lines

fs/cifs/cifsglob.h +1 -1

@@ -1205,7 +1205,7 @@ struct cifs_writedata {
  	unsigned int			tailsz;
  	unsigned int			credits;
  	unsigned int			nr_pages;
-	struct page			*pages[];
+	struct page			**pages;

Also same comment as for earlier patch, these are syntactically equivalent and maybe not needed to change.

fs/cifs/cifssmb.c cifs_writev_complete(struct work_struct

Quoted 29 lines

fs/cifs/cifsglob.h +0 -0

  };
 
  /*

fs/cifs/cifsproto.h index renamed from fs/cifs/cifsproto.h +0 -0

1f27d8e..7933c5f 100644

fs/cifs/cifsproto.h +2 -0

@@ -533,6 +533,8 @@ int cifs_async_writev(struct cifs_writedata *wdata,
  void cifs_writev_complete(struct work_struct *work);
  struct cifs_writedata *cifs_writedata_alloc(unsigned int nr_pages,
  						work_func_t complete);
+struct cifs_writedata *cifs_writedata_direct_alloc(struct page **pages,
+						work_func_t complete);
  void cifs_writedata_release(struct kref *refcount);
  int cifs_query_mf_symlink(unsigned int xid, struct cifs_tcon *tcon,
  			  struct cifs_sb_info *cifs_sb,

fs/cifs/cifssmb.c index renamed from fs/cifs/cifssmb.c +0 -0

c8e4278..5aca336 100644

fs/cifs/cifssmb.c +1 -0

@@ -1952,6 +1952,7 @@ cifs_writedata_release(struct kref *refcount)
  	if (wdata->cfile)
  		cifsFileInfo_put(wdata->cfile);
 
+	kvfree(wdata->pages);
  	kfree(wdata);
  }
 

@@ -2075,12 +2076,22 @@ cifs_writev_complete(struct work_struct

*work)

fs/cifs/cifssmb.c cifs_writev_complete(struct work_struct *work) · line 2080

Quoted 5 lines

fs/cifs/cifssmb.c +2 -0

  struct cifs_writedata *
  cifs_writedata_alloc(unsigned int nr_pages, work_func_t complete)
  {
+	struct page **pages =
+		kzalloc(sizeof(struct page *) * nr_pages, GFP_NOFS);

Why do you do a GFP_NOFS here but GFP_KERNEL in the earlier patch?

Quoted 12 lines

fs/cifs/cifssmb.c +9 -0

+	if (pages)
+		return cifs_writedata_direct_alloc(pages, complete);
+
+	return NULL;
+}
+
+struct cifs_writedata *
+cifs_writedata_direct_alloc(struct page **pages, work_func_t
+complete) {
  	struct cifs_writedata *wdata;

fs/cifs/cifssmb.c cifs_writev_complete(struct work_struct *work) · line 2097

Quoted 10 lines

fs/cifs/cifssmb.c +2 -3

 
-	/* writedata + number of page pointers */
-	wdata = kzalloc(sizeof(*wdata) +
-			sizeof(struct page *) * nr_pages, GFP_NOFS);
+	wdata = kzalloc(sizeof(*wdata), GFP_NOFS);
  	if (wdata != NULL) {
+		wdata->pages = pages;
  		kref_init(&wdata->refcount);
  		INIT_LIST_HEAD(&wdata->list);
  		init_completion(&wdata->done);

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at
https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.ke
rnel.org%2Fmajordomo- info.html&amp;data=02%7C01%7Clongli%40microsoft.com%7C20e0bcbdcce3 402133ee08d5d9767977%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0% 7C636654025229749549&amp;sdata=p4P9AqS8cnOHErCFc1XALeaZxHuB7%2B VOF10SjaaycAI%3D&amp;reserved=0

From: Long Li <[email protected]>

It's possible that the page offset is non-zero in the pages in a request, change the function to calculate the correct data buffer length.

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

---
  fs/cifs/transport.c | 20 +++++++++++++++++---
  1 file changed, 17 insertions(+), 3 deletions(-)

fs/cifs/transport.c +7 -3

index 927226a..d6b5523 100644
@@ -212,10 +212,24 @@ rqst_len(struct smb_rqst *rqst)
  	for (i = 0; i < rqst->rq_nvec; i++)
  		buflen += iov[i].iov_len;
 
-	/* add in the page array if there is one */
+	/*
+	 * Add in the page array if there is one. The caller needs to make
+	 * sure rq_offset and rq_tailsz are set correctly. If a buffer of
+	 * multiple pages ends at page boundary, rq_tailsz needs to be set to
+	 * PAGE_SIZE.
+	 */
  	if (rqst->rq_npages) {
-		buflen += rqst->rq_pagesz * (rqst->rq_npages - 1);
-		buflen += rqst->rq_tailsz;
+		if (rqst->rq_npages == 1)

fs/cifs/transport.c +0 -0

  	}
 
  	return buflen;

Subject: Re: [Patch v2 05/15] CIFS: Calculate the correct request length based on page offset and tail size

Quoted 29 lines · Long Li

From: Long Li <[email protected]>

It's possible that the page offset is non-zero in the pages in a request, change the function to calculate the correct data buffer length.

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

---
  fs/cifs/transport.c | 20 +++++++++++++++++---
  1 file changed, 17 insertions(+), 3 deletions(-)

fs/cifs/transport.c index renamed from fs/cifs/transport.c +0 -0

927226a..d6b5523 100644

fs/cifs/transport.c +7 -3

@@ -212,10 +212,24 @@ rqst_len(struct smb_rqst *rqst)
  	for (i = 0; i < rqst->rq_nvec; i++)
  		buflen += iov[i].iov_len;
 
-	/* add in the page array if there is one */
+	/*
+	 * Add in the page array if there is one. The caller needs to make
+	 * sure rq_offset and rq_tailsz are set correctly. If a buffer of
+	 * multiple pages ends at page boundary, rq_tailsz needs to be set to
+	 * PAGE_SIZE.
+	 */
  	if (rqst->rq_npages) {
-		buflen += rqst->rq_pagesz * (rqst->rq_npages - 1);
-		buflen += rqst->rq_tailsz;
+		if (rqst->rq_npages == 1)

fs/cifs/transport.c rqst_len(struct smb_rqst *rqst) · line 232

Quoted 10 lines

fs/cifs/transport.c +10 -0

+			buflen += rqst->rq_tailsz;
+		else {
+			/*
+			 * If there is more than one page, calculate the
+			 * buffer length based on rq_offset and rq_tailsz
+			 */
+			buflen += rqst->rq_pagesz * (rqst->rq_npages - 1) -
+					rqst->rq_offset;
+			buflen += rqst->rq_tailsz;
+		}

Wouldn't it be simpler to keep the original code, but then just subtract the rq_offset?

	buflen += rqst->rq_pagesz * (rqst->rq_npages - 1);
	buflen += rqst->rq_tailsz;
	buflen -= rqst->rq_offset;

It's kind of confusing as written. Also, what if it's just one page, but has a non-zero offset? Is that somehow not possible? My suggested code would take that into account, yours doesn't.

Tom.

Quoted 4 lines

fs/cifs/transport.c +0 -0

  	}
 
  	return buflen;

From: Long Li <[email protected]>

Introduce a function rqst_page_get_length to return the page offset and length for a given page in smb_rqst. This function is to be used by following patches.

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

---
  fs/cifs/cifsproto.h |  3 +++
  fs/cifs/misc.c      | 17 +++++++++++++++++
  2 files changed, 20 insertions(+)

fs/cifs/cifsproto.h +3 -0

index 7933c5f..89dda14 100644
@@ -557,4 +557,7 @@ int cifs_alloc_hash(const char *name, struct crypto_shash **shash,
  		    struct sdesc **sdesc);
  void cifs_free_hash(struct crypto_shash **shash, struct sdesc **sdesc);
 
+extern void rqst_page_get_length(struct smb_rqst *rqst, unsigned int page,
+				unsigned int *len, unsigned int *offset);
+
  #endif			/* _CIFSPROTO_H */

fs/cifs/misc.c +1 -0

index 96849b5..e951417 100644
@@ -905,3 +905,20 @@ cifs_free_hash(struct crypto_shash **shash, struct sdesc **sdesc)
  		crypto_free_shash(*shash);
  	*shash = NULL;
  }
+

Subject: Re: [Patch v2 06/15] CIFS: Introduce helper function to get page offset and length in smb_rqst

fs/cifs/cifsproto.h int cifs_alloc_hash(const char *name, struct

Quoted 17 lines · Long Li

From: Long Li <[email protected]>

Introduce a function rqst_page_get_length to return the page offset and length for a given page in smb_rqst. This function is to be used by following patches.

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

---
  fs/cifs/cifsproto.h |  3 +++
  fs/cifs/misc.c      | 17 +++++++++++++++++
  2 files changed, 20 insertions(+)

fs/cifs/cifsproto.h index renamed from fs/cifs/cifsproto.h +0 -0

7933c5f..89dda14 100644

fs/cifs/cifsproto.h +0 -0

@@ -557,4 +557,7 @@ int cifs_alloc_hash(const char *name, struct

crypto_shash **shash,

fs/cifs/cifsproto.h int cifs_alloc_hash(const char *name, struct · line 561

Quoted 5 lines

fs/cifs/cifsproto.h +1 -0

  		    struct sdesc **sdesc);
  void cifs_free_hash(struct crypto_shash **shash, struct sdesc
 **sdesc);
 
+extern void rqst_page_get_length(struct smb_rqst *rqst, unsigned int

page,

fs/cifs/misc.c cifs_free_hash(struct crypto_shash **shash,

Quoted 8 lines

fs/cifs/cifsproto.h +2 -0

+				unsigned int *len, unsigned int *offset);
+

  #endif			/* _CIFSPROTO_H */

fs/cifs/misc.c index 96849b5..e951417 renamed from fs/cifs/misc.c +0 -0

100644

fs/cifs/misc.c +0 -0

@@ -905,3 +905,20 @@ cifs_free_hash(struct crypto_shash **shash,

struct sdesc **sdesc)

Quoted 4 lines

fs/cifs/misc.c +1 -0

  		crypto_free_shash(*shash);
  	*shash = NULL;
  }
+

fs/cifs/misc.c cifs_free_hash(struct crypto_shash **shash, struct sdesc **sdesc) · line 918

Quoted 10 lines

fs/cifs/misc.c +10 -0

+/**
+ * rqst_page_get_length - obtain the length and offset for a page in
+smb_rqst
+ * Input: rqst - a smb_rqst, page - a page index for rqst
+ * Output: *len - the length for this page, *offset - the offset for
+this page  */ void rqst_page_get_length(struct smb_rqst *rqst,
+unsigned int page,
+				unsigned int *len, unsigned int *offset) {
+	*len = rqst->rq_pagesz;
+	*offset = (page == 0) ? rqst->rq_offset : 0;

Really? Page 0 always has a zero offset??

fs/cifs/misc.c cifs_free_hash(struct crypto_shash **shash, struct sdesc **sdesc)

Quoted 7 lines

fs/cifs/misc.c +5 -0

+
+	if (rqst->rq_npages == 1 || page == rqst->rq_npages-1)
+		*len = rqst->rq_tailsz;
+	else if (page == 0)
+		*len = rqst->rq_pagesz - rqst->rq_offset; }

This subroutine does what patch 5 does inline. Why not push this patch up in the sequence and use the helper?

Tom.

Quoted 37 lines

Subject: Re: [Patch v2 06/15] CIFS: Introduce helper function to get page offset and length in smb_rqst

fs/cifs/cifsproto.h int cifs_alloc_hash(const char *name, struct

Quoted 17 lines · Long Li

From: Long Li <[email protected]>

Introduce a function rqst_page_get_length to return the page offset and length for a given page in smb_rqst. This function is to be used by following patches.

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

---
   fs/cifs/cifsproto.h |  3 +++
   fs/cifs/misc.c      | 17 +++++++++++++++++
   2 files changed, 20 insertions(+)

fs/cifs/cifsproto.h index renamed from fs/cifs/cifsproto.h +0 -0

7933c5f..89dda14 100644

fs/cifs/cifsproto.h +0 -0

@@ -557,4 +557,7 @@ int cifs_alloc_hash(const char *name, struct

crypto_shash **shash,

fs/cifs/cifsproto.h int cifs_alloc_hash(const char *name, struct · line 561

Quoted 5 lines

fs/cifs/cifsproto.h +1 -0

   		    struct sdesc **sdesc);
   void cifs_free_hash(struct crypto_shash **shash, struct sdesc
 **sdesc);
 
+extern void rqst_page_get_length(struct smb_rqst *rqst, unsigned int

page,

fs/cifs/misc.c cifs_free_hash(struct crypto_shash **shash,

Quoted 8 lines

fs/cifs/cifsproto.h +2 -0

+				unsigned int *len, unsigned int *offset);
+

   #endif			/* _CIFSPROTO_H */

fs/cifs/misc.c index 96849b5..e951417 renamed from fs/cifs/misc.c +0 -0

100644

fs/cifs/misc.c +0 -0

@@ -905,3 +905,20 @@ cifs_free_hash(struct crypto_shash **shash,

struct sdesc **sdesc)

Quoted 16 lines

Quoted 4 lines

fs/cifs/misc.c +1 -0

   		crypto_free_shash(*shash);
   	*shash = NULL;
   }
+

fs/cifs/misc.c cifs_free_hash(struct crypto_shash **shash, struct sdesc **sdesc) · line 918

Quoted 10 lines

fs/cifs/misc.c +10 -0

+/**
+ * rqst_page_get_length - obtain the length and offset for a page in
+smb_rqst
+ * Input: rqst - a smb_rqst, page - a page index for rqst
+ * Output: *len - the length for this page, *offset - the offset for
+this page  */ void rqst_page_get_length(struct smb_rqst *rqst,
+unsigned int page,
+				unsigned int *len, unsigned int *offset) {
+	*len = rqst->rq_pagesz;
+	*offset = (page == 0) ? rqst->rq_offset : 0;

Really? Page 0 always has a zero offset??

I think you are misreading this line. The offset for page 0 is rq_offset, the offset for all subsequent pages are 0.

Quoted 9 lines

fs/cifs/misc.c cifs_free_hash(struct crypto_shash **shash, struct sdesc **sdesc)

Quoted 6 lines

fs/cifs/misc.c +5 -0

+
+	if (rqst->rq_npages == 1 || page == rqst->rq_npages-1)
+		*len = rqst->rq_tailsz;
+	else if (page == 0)
+		*len = rqst->rq_pagesz - rqst->rq_offset; }

This subroutine does what patch 5 does inline. Why not push this patch up in the sequence and use the helper?

This is actually a little different. This function returns the length and offset for a given page in the request. There might be multiple pages in a request.

The other function calculates the total length of all the pages in a request.

Subject: Re: [Patch v2 06/15] CIFS: Introduce helper function to get page offset and length in smb_rqst

Quoted 55 lines · Long Li

Quoted 38 lines

Subject: Re: [Patch v2 06/15] CIFS: Introduce helper function to get page offset and length in smb_rqst

fs/cifs/cifsproto.h int cifs_alloc_hash(const char *name, struct

Quoted 17 lines · Long Li

From: Long Li <[email protected]>

Introduce a function rqst_page_get_length to return the page offset and length for a given page in smb_rqst. This function is to be used by following patches.

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

---
   fs/cifs/cifsproto.h |  3 +++
   fs/cifs/misc.c      | 17 +++++++++++++++++
   2 files changed, 20 insertions(+)

fs/cifs/cifsproto.h index renamed from fs/cifs/cifsproto.h +0 -0

7933c5f..89dda14 100644

fs/cifs/cifsproto.h +0 -0

@@ -557,4 +557,7 @@ int cifs_alloc_hash(const char *name, struct

crypto_shash **shash,

fs/cifs/cifsproto.h int cifs_alloc_hash(const char *name, struct · line 562

Quoted 6 lines

fs/cifs/cifsproto.h +2 -0

   		    struct sdesc **sdesc);
   void cifs_free_hash(struct crypto_shash **shash, struct sdesc
 **sdesc);
 
+extern void rqst_page_get_length(struct smb_rqst *rqst, unsigned
+int

page,

fs/cifs/misc.c cifs_free_hash(struct crypto_shash **shash,

Quoted 8 lines

fs/cifs/cifsproto.h +2 -0

+				unsigned int *len, unsigned int *offset);
+
   #endif			/* _CIFSPROTO_H */

fs/cifs/misc.c index 96849b5..e951417 renamed from fs/cifs/misc.c +0 -0

100644

fs/cifs/misc.c +0 -0

@@ -905,3 +905,20 @@ cifs_free_hash(struct crypto_shash **shash,

struct sdesc **sdesc)

Quoted 16 lines

Quoted 4 lines

fs/cifs/misc.c +1 -0

   		crypto_free_shash(*shash);
   	*shash = NULL;
   }
+

fs/cifs/misc.c cifs_free_hash(struct crypto_shash **shash, struct sdesc **sdesc) · line 918

Quoted 10 lines

fs/cifs/misc.c +10 -0

+/**
+ * rqst_page_get_length - obtain the length and offset for a page
+in smb_rqst
+ * Input: rqst - a smb_rqst, page - a page index for rqst
+ * Output: *len - the length for this page, *offset - the offset
+for this page  */ void rqst_page_get_length(struct smb_rqst *rqst,
+unsigned int page,
+				unsigned int *len, unsigned int *offset) {
+	*len = rqst->rq_pagesz;
+	*offset = (page == 0) ? rqst->rq_offset : 0;

Really? Page 0 always has a zero offset??

Quoted 1 line

I think you are misreading this line. The offset for page 0 is rq_offset, the

offset for all subsequent pages are 0.

Ah, yes, sorry I did read it incorrectly.

Quoted 11 lines

Quoted 9 lines

fs/cifs/misc.c cifs_free_hash(struct crypto_shash **shash, struct sdesc **sdesc)

Quoted 6 lines

fs/cifs/misc.c +5 -0

+
+	if (rqst->rq_npages == 1 || page == rqst->rq_npages-1)
+		*len = rqst->rq_tailsz;
+	else if (page == 0)
+		*len = rqst->rq_pagesz - rqst->rq_offset; }

This subroutine does what patch 5 does inline. Why not push this patch up in the sequence and use the helper?

This is actually a little different. This function returns the length and offset

for a given page in the request. There might be multiple pages in a request.

Ok, but I still think there is quite a bit of inline computation of this stuff that would more clearly and more robustly be done in a set of common functions. If someone ever touches the code to support a new upper layer, or even integrate with more complex compounding, things will get ugly fast.

Quoted 1 line

The other function calculates the total length of all the pages in a request.

Again, a great candidate for a common set of subroutines, IMO.

From: Long Li <[email protected]>

The RDMA send function needs to look at offset in the request pages, and send data starting from there.

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

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

fs/cifs/smbdirect.c +6 -2

index c62f7c9..6141e3c 100644
@@ -17,6 +17,7 @@
  #include <linux/highmem.h>
  #include "smbdirect.h"
  #include "cifs_debug.h"
+#include "cifsproto.h"
 
  static struct smbd_response *get_empty_queue_buffer(
  		struct smbd_connection *info);

@@ -2082,7 +2083,7 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
  	struct kvec vec;
  	int nvecs;
  	int size;
-	int buflen = 0, remaining_data_length;
+	unsigned int buflen = 0, remaining_data_length;
  	int start, i, j;
  	int max_iov_size =
  		info->max_send_size - sizeof(struct smbd_data_transfer);

@@ -2113,10 +2114,17 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
  		buflen += iov[i].iov_len;
  	}
 
-	/* add in the page array if there is one */
+	/*
+	 * Add in the page array if there is one. The caller needs to set
+	 * rq_tailsz to PAGE_SIZE when the buffer has multiple pages and
+	 * ends at page boundary

fs/cifs/smbdirect.c +7 -4

  	if (buflen + sizeof(struct smbd_data_transfer) >

@@ -2213,8 +2221,9 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
 
  	/* now sending pages if there are any */
  	for (i = 0; i < rqst->rq_npages; i++) {
-		buflen = (i == rqst->rq_npages-1) ?
-			rqst->rq_tailsz : rqst->rq_pagesz;
+		unsigned int offset;
+
+		rqst_page_get_length(rqst, i, &buflen, &offset);
  		nvecs = (buflen + max_iov_size - 1) / max_iov_size;
  		log_write(INFO, "sending pages buflen=%d nvecs=%d\n",
  			buflen, nvecs);

@@ -2225,9 +2234,11 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
  			remaining_data_length -= size;
  			log_write(INFO, "sending pages i=%d offset=%d size=%d"
  				" remaining_data_length=%d\n",
-				i, j*max_iov_size, size, remaining_data_length);
+				i, j*max_iov_size+offset, size,
+				remaining_data_length);
  			rc = smbd_post_send_page(
-				info, rqst->rq_pages[i], j*max_iov_size,
+				info, rqst->rq_pages[i],
+				j*max_iov_size + offset,
  				size, remaining_data_length);
  			if (rc)
  				goto done;

Subject: Re: [Patch v2 08/15] CIFS: SMBD: Support page offset in RDMA send

fs/cifs/smbdirect.c int smbd_send(struct smbd_connection *info,

Quoted 23 lines · Long Li

From: Long Li <[email protected]>

The RDMA send function needs to look at offset in the request pages, and send data starting from there.

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

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

fs/cifs/smbdirect.c index renamed from fs/cifs/smbdirect.c +0 -0

c62f7c9..6141e3c 100644

fs/cifs/smbdirect.c +1 -0

@@ -17,6 +17,7 @@
  #include <linux/highmem.h>
  #include "smbdirect.h"
  #include "cifs_debug.h"
+#include "cifsproto.h"
 
  static struct smbd_response *get_empty_queue_buffer(
  		struct smbd_connection *info);

@@ -2082,7 +2083,7 @@ int smbd_send(struct smbd_connection *info,

struct smb_rqst *rqst)

fs/cifs/smbdirect.c int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst) · line 2089

Quoted 8 lines

fs/cifs/smbdirect.c +1 -1

  	struct kvec vec;
  	int nvecs;
  	int size;
-	int buflen = 0, remaining_data_length;
+	unsigned int buflen = 0, remaining_data_length;
  	int start, i, j;
  	int max_iov_size =
  		info->max_send_size - sizeof(struct smbd_data_transfer);

@@

Quoted 1 line

-2113,10 +2114,17 @@ int smbd_send(struct smbd_connection *info,

struct smb_rqst *rqst)

Quoted 14 lines

  		buflen += iov[i].iov_len;
  	}

fs/cifs/smbdirect.c +7 -3

-	/* add in the page array if there is one */
+	/*
+	 * Add in the page array if there is one. The caller needs to set
+	 * rq_tailsz to PAGE_SIZE when the buffer has multiple pages and
+	 * ends at page boundary
+	 */
  	if (rqst->rq_npages) {
-		buflen += rqst->rq_pagesz * (rqst->rq_npages - 1);
-		buflen += rqst->rq_tailsz;
+		if (rqst->rq_npages == 1)
+			buflen += rqst->rq_tailsz;

fs/cifs/smbdirect.c int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst) · line 2128

Quoted 4 lines

fs/cifs/smbdirect.c +3 -0

+		else
+			buflen += rqst->rq_pagesz * (rqst->rq_npages - 1) -
+					rqst->rq_offset + rqst->rq_tailsz;
  	}

This code is really confusing and redundant. It tests npages > 0, then tests npages == 1, then does an else. Why not call the helper like the following smbd_send()?

Tom.

fs/cifs/smbdirect.c int smbd_send(struct smbd_connection *info,

Quoted 15 lines

if (buflen + sizeof(struct smbd_data_transfer) > @@ -2213,8 +2221,9
@@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)

/* now sending pages if there are any */
for (i = 0; i < rqst->rq_npages; i++) {

fs/cifs/smbdirect.c +3 -2

-		buflen = (i == rqst->rq_npages-1) ?
-			rqst->rq_tailsz : rqst->rq_pagesz;
+		unsigned int offset;
+
+		rqst_page_get_length(rqst, i, &buflen, &offset);
  		nvecs = (buflen + max_iov_size - 1) / max_iov_size;
  		log_write(INFO, "sending pages buflen=%d nvecs=%d\n",
  			buflen, nvecs);

@@ -2225,9 +2234,11 @@ int smbd_send(struct smbd_connection *info,

struct smb_rqst *rqst)

fs/cifs/smbdirect.c int smbd_send(struct smbd_connection *info, · line 2235

Quoted 2 lines

fs/cifs/smbdirect.c +0 -0

  			remaining_data_length -= size;
  			log_write(INFO, "sending pages i=%d offset=%d

size=%d"

fs/cifs/smbdirect.c int smbd_send(struct smbd_connection *info, · line 2228

Quoted 2 lines

fs/cifs/smbdirect.c +0 -1

  				" remaining_data_length=%d\n",
-				i, j*max_iov_size, size,

remaining_data_length);

Quoted 10 lines

fs/cifs/smbdirect.c +4 -1

+				i, j*max_iov_size+offset, size,
+				remaining_data_length);
  			rc = smbd_post_send_page(
-				info, rqst->rq_pages[i], j*max_iov_size,
+				info, rqst->rq_pages[i],
+				j*max_iov_size + offset,
  				size, remaining_data_length);
  			if (rc)
  				goto done;

From: Long Li <[email protected]>

RDMA recv function needs to place data to the correct place starting at page offset.

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

---
  fs/cifs/smbdirect.c | 18 +++++++++++-------
  1 file changed, 11 insertions(+), 7 deletions(-)

fs/cifs/smbdirect.c +4 -2

index 6141e3c..ba53c52 100644
@@ -2004,10 +2004,12 @@ static int smbd_recv_buf(struct smbd_connection *info, char *buf,
   * return value: actual data read
   */
  static int smbd_recv_page(struct smbd_connection *info,
-		struct page *page, unsigned int to_read)
+		struct page *page, unsigned int page_offset,
+		unsigned int to_read)
  {
  	int ret;
  	char *to_address;
+	void *page_address;
 
  	/* make sure we have the page ready for read */
  	ret = wait_event_interruptible(

@@ -2015,16 +2017,17 @@ static int smbd_recv_page(struct smbd_connection *info,
  		info->reassembly_data_length >= to_read ||
  			info->transport_status != SMBD_CONNECTED);
  	if (ret)
-		return 0;
+		return ret;
 
  	/* now we can read from reassembly queue and not sleep */

fs/cifs/smbdirect.c +4 -3

  	return ret;
  }

@@ -2038,7 +2041,7 @@ int smbd_recv(struct smbd_connection *info, struct msghdr *msg)
  {
  	char *buf;
  	struct page *page;
-	unsigned int to_read;
+	unsigned int to_read, page_offset;
  	int rc;
 
  	info->smbd_recv_pending++;

@@ -2052,15 +2055,16 @@ int smbd_recv(struct smbd_connection *info, struct msghdr *msg)
 
  	case READ | ITER_BVEC:
  		page = msg->msg_iter.bvec->bv_page;
+		page_offset = msg->msg_iter.bvec->bv_offset;
  		to_read = msg->msg_iter.bvec->bv_len;
-		rc = smbd_recv_page(info, page, to_read);
+		rc = smbd_recv_page(info, page, page_offset, to_read);
  		break;
 
  	default:
  		/* It's a bug in upper layer to get there */
  		cifs_dbg(VFS, "CIFS: invalid msg type %d\n",
  			msg->msg_iter.type);
-		rc = -EIO;
+		rc = -EINVAL;
  	}
 
  	info->smbd_recv_pending--;

Subject: Re: [Patch v2 09/15] CIFS: SMBD: Support page offset in RDMA recv

fs/cifs/smbdirect.c static int smbd_recv_buf(struct

Quoted 15 lines · Long Li

From: Long Li <[email protected]>

RDMA recv function needs to place data to the correct place starting at page offset.

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

---
  fs/cifs/smbdirect.c | 18 +++++++++++-------
  1 file changed, 11 insertions(+), 7 deletions(-)

fs/cifs/smbdirect.c index renamed from fs/cifs/smbdirect.c +0 -0

6141e3c..ba53c52 100644

fs/cifs/smbdirect.c +0 -0

@@ -2004,10 +2004,12 @@ static int smbd_recv_buf(struct

smbd_connection *info, char *buf,

fs/cifs/smbdirect.c static int smbd_recv_page(struct

Quoted 14 lines

fs/cifs/smbdirect.c +3 -1

   * return value: actual data read
   */
  static int smbd_recv_page(struct smbd_connection *info,
-		struct page *page, unsigned int to_read)
+		struct page *page, unsigned int page_offset,
+		unsigned int to_read)
  {
  	int ret;
  	char *to_address;
+	void *page_address;
 
  	/* make sure we have the page ready for read */
  	ret = wait_event_interruptible(

@@ -2015,16 +2017,17 @@ static int smbd_recv_page(struct

smbd_connection *info,

Quoted 14 lines

fs/cifs/smbdirect.c +3 -2

  		info->reassembly_data_length >= to_read ||
  			info->transport_status != SMBD_CONNECTED);
  	if (ret)
-		return 0;
+		return ret;
 
  	/* now we can read from reassembly queue and not sleep */
-	to_address = kmap_atomic(page);
+	page_address = kmap_atomic(page);
+	to_address = (char *) page_address + page_offset;
 
  	log_read(INFO, "reading from page=%p address=%p to_read=%d\n",
  		page, to_address, to_read);
 

fs/cifs/smbdirect.c static int smbd_recv_page(struct smbd_connection *info, · line 2030

Quoted 3 lines

fs/cifs/smbdirect.c +1 -1

  	ret = smbd_recv_buf(info, to_address, to_read);
-	kunmap_atomic(to_address);
+	kunmap_atomic(page_address);

Is "page" truly not mapped? This kmap/kunmap for each received 4KB is very expensive. Is there not a way to keep a kva for the reassembly queue segments?

fs/cifs/smbdirect.c int smbd_recv(struct smbd_connection *info,

Quoted 5 lines

fs/cifs/smbdirect.c +0 -0

  	return ret;
  }

@@ -2038,7 +2041,7 @@ int smbd_recv(struct smbd_connection *info,

struct msghdr *msg)

Quoted 29 lines

fs/cifs/smbdirect.c +4 -3

  {
  	char *buf;
  	struct page *page;
-	unsigned int to_read;
+	unsigned int to_read, page_offset;
  	int rc;
 
  	info->smbd_recv_pending++;

@@ -2052,15 +2055,16 @@ int smbd_recv(struct smbd_connection *info,
 struct msghdr *msg)
 
  	case READ | ITER_BVEC:
  		page = msg->msg_iter.bvec->bv_page;
+		page_offset = msg->msg_iter.bvec->bv_offset;
  		to_read = msg->msg_iter.bvec->bv_len;
-		rc = smbd_recv_page(info, page, to_read);
+		rc = smbd_recv_page(info, page, page_offset, to_read);
  		break;
 
  	default:
  		/* It's a bug in upper layer to get there */
  		cifs_dbg(VFS, "CIFS: invalid msg type %d\n",
  			msg->msg_iter.type);
-		rc = -EIO;
+		rc = -EINVAL;
  	}
 

info->smbd_recv_pending--;

From: Long Li <[email protected]>

Change code to pass the correct page offset during memory registration for RDMA read/write.

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

---
  fs/cifs/smb2pdu.c   | 18 ++++++++-----
  fs/cifs/smbdirect.c | 76 +++++++++++++++++++++++++++++++----------------------
  fs/cifs/smbdirect.h |  2 +-
  3 files changed, 58 insertions(+), 38 deletions(-)

fs/cifs/smb2pdu.c +4 -4

index f603fbe..fc30774 100644
@@ -2623,8 +2623,8 @@ smb2_new_read_req(void **buf, unsigned int *total_len,
 
  		rdata->mr = smbd_register_mr(
  				server->smbd_conn, rdata->pages,
-				rdata->nr_pages, rdata->tailsz,
-				true, need_invalidate);
+				rdata->nr_pages, rdata->page_offset,
+				rdata->tailsz, true, need_invalidate);
  		if (!rdata->mr)
  			return -ENOBUFS;
 

@@ -3013,16 +3013,22 @@ smb2_async_writev(struct cifs_writedata *wdata,
 
  		wdata->mr = smbd_register_mr(
  				server->smbd_conn, wdata->pages,
-				wdata->nr_pages, wdata->tailsz,
-				false, need_invalidate);
+				wdata->nr_pages, wdata->page_offset,
+				wdata->tailsz, false, need_invalidate);
  		if (!wdata->mr) {
  			rc = -ENOBUFS;
  			goto async_writev_out;
  		}
  		req->Length = 0;
  		req->DataOffset = 0;

fs/cifs/smb2pdu.c +0 -0

  		req->Channel = SMB2_CHANNEL_RDMA_V1_INVALIDATE;
  		if (need_invalidate)
  			req->Channel = SMB2_CHANNEL_RDMA_V1;

fs/cifs/smbdirect.c +0 -0

index ba53c52..e459c97 100644
@@ -2299,37 +2299,37 @@ static void smbd_mr_recovery_work(struct work_struct *work)
  		if (smbdirect_mr->state == MR_INVALIDATED ||
  			smbdirect_mr->state == MR_ERROR) {
 

fs/cifs/smbdirect.c +4 -0

+
+			smbdirect_mr->mr = ib_alloc_mr(
+				info->pd, info->mr_type,
+				info->max_frmr_depth);

fs/cifs/smbdirect.c +2 -14

-				smbdirect_mr->mr = ib_alloc_mr(
-					info->pd, info->mr_type,
-					info->max_frmr_depth);
-				if (IS_ERR(smbdirect_mr->mr)) {
-					log_rdma_mr(ERR,
-						"ib_alloc_mr failed mr_type=%x "
-						"max_frmr_depth=%x\n",
-						info->mr_type,
-						info->max_frmr_depth);
-					smbd_disconnect_rdma_connection(info);
-				}
+			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

@@ -2475,7 +2475,7 @@ static struct smbd_mr *get_mr(struct smbd_connection *info)
   */
  struct smbd_mr *smbd_register_mr(
  	struct smbd_connection *info, struct page *pages[], int num_pages,
-	int tailsz, bool writing, bool need_invalidate)
+	int offset, int tailsz, bool writing, bool need_invalidate)
  {
  	struct smbd_mr *smbdirect_mr;
  	int rc, i;

@@ -2498,17 +2498,31 @@ struct smbd_mr *smbd_register_mr(
  	smbdirect_mr->sgl_count = num_pages;

fs/cifs/smbdirect.c +14 -3

+	}
+
+	/* We have at least two pages to register */
+	sg_set_page(
+		&smbdirect_mr->sgl[0], pages[0], PAGE_SIZE - offset, offset);
+	i = 1;
+	while (i < num_pages - 1) {
+		sg_set_page(&smbdirect_mr->sgl[i], pages[i], PAGE_SIZE, 0);
+		i++;
+	}
  	sg_set_page(&smbdirect_mr->sgl[i], pages[i],
  		tailsz ? tailsz : PAGE_SIZE, 0);
 
+skip_multiple_pages:
  	dir = writing ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
  	smbdirect_mr->dir = dir;
  	rc = ib_dma_map_sg(info->id->device, smbdirect_mr->sgl, num_pages, dir);
  	if (!rc) {
-		log_rdma_mr(INFO, "ib_dma_map_sg num_pages=%x dir=%x rc=%x\n",
+		log_rdma_mr(ERR, "ib_dma_map_sg num_pages=%x dir=%x rc=%x\n",
  			num_pages, dir, rc);
  		goto dma_map_error;
  	}

@@ -2516,8 +2530,8 @@ struct smbd_mr *smbd_register_mr(
  	rc = ib_map_mr_sg(smbdirect_mr->mr, smbdirect_mr->sgl, num_pages,
  		NULL, PAGE_SIZE);
  	if (rc != num_pages) {
-		log_rdma_mr(INFO,
-			"ib_map_mr_sg failed rc = %x num_pages = %x\n",
+		log_rdma_mr(ERR,
+			"ib_map_mr_sg failed rc = %d num_pages = %x\n",
  			rc, num_pages);
  		goto map_mr_error;
  	}

fs/cifs/smbdirect.h +1 -1

index f9038da..1e419c2 100644
@@ -321,7 +321,7 @@ struct smbd_mr {
  /* Interfaces to register and deregister MR for RDMA read/write */
  struct smbd_mr *smbd_register_mr(
  	struct smbd_connection *info, struct page *pages[], int num_pages,
-	int tailsz, bool writing, bool need_invalidate);
+	int offset, int tailsz, bool writing, bool need_invalidate);
  int smbd_deregister_mr(struct smbd_mr *mr);
 
  #else

From: Long Li <[email protected]>

When calculating signature for the packet, it needs to read into the correct page offset for the data.

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

---
  fs/cifs/cifsencrypt.c | 9 +++++----
  1 file changed, 5 insertions(+), 4 deletions(-)

fs/cifs/cifsencrypt.c +0 -0

index a6ef088..e88303c 100644
@@ -68,11 +68,12 @@ int __cifs_calc_signature(struct smb_rqst *rqst,
 
  	/* now hash over the rq_pages array */
  	for (i = 0; i < rqst->rq_npages; i++) {

fs/cifs/cifsencrypt.c +0 -0

  		crypto_shash_update(shash, kaddr, len);
 
 

Subject: Re: [Patch v2 11/15] CIFS: Pass page offset for calculating signature

Quoted 23 lines · Long Li

From: Long Li <[email protected]>

When calculating signature for the packet, it needs to read into the correct page offset for the data.

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

---
  fs/cifs/cifsencrypt.c | 9 +++++----
  1 file changed, 5 insertions(+), 4 deletions(-)

fs/cifs/cifsencrypt.c index renamed from fs/cifs/cifsencrypt.c +0 -0

a6ef088..e88303c 100644

fs/cifs/cifsencrypt.c +2 -2

@@ -68,11 +68,12 @@ int __cifs_calc_signature(struct smb_rqst *rqst,
 
  	/* now hash over the rq_pages array */
  	for (i = 0; i < rqst->rq_npages; i++) {
-		void *kaddr = kmap(rqst->rq_pages[i]);
-		size_t len = rqst->rq_pagesz;
+		void *kaddr;
+		unsigned int len, offset;
 

fs/cifs/cifsencrypt.c int __cifs_calc_signature(struct smb_rqst *rqst, · line 76

Quoted 5 lines

fs/cifs/cifsencrypt.c +3 -2

-		if (i == rqst->rq_npages - 1)
-			len = rqst->rq_tailsz;
+		rqst_page_get_length(rqst, i, &len, &offset);
+
+		kaddr = (char *) kmap(rqst->rq_pages[i]) + offset;

I suppose it's more robust to map a page at a time, but it's pretty expensive. Is this the only way to iterate over a potentially very large block of data? For example, a 1MB segment means 256 kmap/kunmaps.

Tom.

Quoted 4 lines

fs/cifs/cifsencrypt.c +0 -0

  		crypto_shash_update(shash, kaddr, len);
 
 

From: Long Li <[email protected]>

Encryption function needs to read data starting page offset from input buffer.

This doesn't affect decryption path since it allocates its own page buffers.

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

---
  fs/cifs/smb2ops.c | 20 +++++++++++++-------
  1 file changed, 13 insertions(+), 7 deletions(-)

fs/cifs/smb2ops.c +5 -4

index 1fa1c29..38d19b6 100644
@@ -2189,9 +2189,10 @@ init_sg(struct smb_rqst *rqst, u8 *sign)
  		smb2_sg_set_buf(&sg[i], rqst->rq_iov[i].iov_base,
  						rqst->rq_iov[i].iov_len);
  	for (j = 0; i < sg_len - 1; i++, j++) {
-		unsigned int len = (j < rqst->rq_npages - 1) ? rqst->rq_pagesz
-							: rqst->rq_tailsz;
-		sg_set_page(&sg[i], rqst->rq_pages[j], len, 0);
+		unsigned int len, offset;
+
+		rqst_page_get_length(rqst, j, &len, &offset);
+		sg_set_page(&sg[i], rqst->rq_pages[j], len, offset);
  	}
  	smb2_sg_set_buf(&sg[sg_len - 1], sign, SMB2_SIGNATURE_SIZE);
  	return sg;

@@ -2332,6 +2333,7 @@ smb3_init_transform_rq(struct TCP_Server_Info *server, struct smb_rqst *new_rq,
  		return rc;
 
  	new_rq->rq_pages = pages;
+	new_rq->rq_offset = old_rq->rq_offset;
  	new_rq->rq_npages = old_rq->rq_npages;
  	new_rq->rq_pagesz = old_rq->rq_pagesz;
  	new_rq->rq_tailsz = old_rq->rq_tailsz;

@@ -2363,10 +2365,14 @@ smb3_init_transform_rq(struct TCP_Server_Info *server, struct smb_rqst *new_rq,
 
  	/* copy pages form the old */
  	for (i = 0; i < npages; i++) {
-		char *dst = kmap(new_rq->rq_pages[i]);

fs/cifs/smb2ops.c +1 -0

+
  		memcpy(dst, src, len);
  		kunmap(new_rq->rq_pages[i]);
  		kunmap(old_rq->rq_pages[i]);

From: Long Li <[email protected]>

Implement the function for direct I/O read. It doesn't support AIO, which will be implemented in a follow up patch.

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

---
  fs/cifs/cifsfs.h |   1 +
  fs/cifs/file.c   | 149 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 150 insertions(+)

fs/cifs/cifsfs.h +1 -0

index 5f02318..7fba9aa 100644
@@ -102,6 +102,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/file.c +55 -0

index 87eece6..e6e6f24 100644
@@ -2955,6 +2955,18 @@ cifs_read_allocate_pages(struct cifs_readdata *rdata, unsigned int nr_pages)
  	return rc;
  }
 
+static void cifs_direct_readdata_release(struct kref *refcount)
+{
+	struct cifs_readdata *rdata = container_of(refcount,
+					struct cifs_readdata, refcount);
+	unsigned int i;
+
+	for (i = 0; i < rdata->nr_pages; i++)
+		put_page(rdata->pages[i]);
+
+	cifs_readdata_release(refcount);
+}
+
  static void
  cifs_uncached_readdata_release(struct kref *refcount)
  {

@@ -3267,6 +3279,143 @@ collect_uncached_read_data(struct cifs_aio_ctx *ctx)
  		complete(&ctx->done);
  }
 
+static void cifs_direct_readv_complete(struct work_struct *work)
+{
+	struct cifs_readdata *rdata =
+		container_of(work, struct cifs_readdata, work);
+
+	complete(&rdata->done);
+	kref_put(&rdata->refcount, cifs_direct_readdata_release);
+}
+
+ssize_t cifs_direct_readv(struct kiocb *iocb, struct iov_iter *to)
+{
+	size_t len, cur_len, start;
+	unsigned int npages, rsize, credits;
+	struct file *file;
+	struct cifs_sb_info *cifs_sb;
+	struct cifsFileInfo *cfile;
+	struct cifs_tcon *tcon;
+	struct page **pagevec;
+	ssize_t rc, total_read = 0;
+	struct TCP_Server_Info *server;
+	loff_t offset = iocb->ki_pos;
+	pid_t pid;
+	struct cifs_readdata *rdata;
+
+	/*
+	 * iov_iter_get_pages_alloc() doesn't work with ITER_KVEC,
+	 * fall back to data copy read path
+	 */
+	if (to->type & ITER_KVEC) {
+		cifs_dbg(FYI, "use non-direct cifs_user_readv for kvec I/O\n");
+		return cifs_user_readv(iocb, to);
+	}
+
+	len = iov_iter_count(to);
+	if (!len)
+		return 0;
+
+	file = iocb->ki_filp;
+	cifs_sb = CIFS_FILE_SB(file);
+	cfile = file->private_data;
+	tcon = tlink_tcon(cfile->tlink);
+	server = tcon->ses->server;
+