[PATCH 0/4] ceph: convert to netfs_direct_read_iter for DIO reads

[PATCH 0/4] ceph: convert to netfs_direct_read_iter for DIO reads

[Jeff Layton]

This patch series is based on top of David Howells' netfs-lib branch.

I previously sent an RFC set for this back in early April, but David has
revised his netfs-lib branch since then. This converts ceph to use the
new DIO read helper netfs_direct_read_iter instead of using our own
routine.

With this change we ought to be able to rip out a large swath of
ceph_direct_read_write, but I've decided to wait on that until the write
helpers are in place and we can just remove that function wholesale.

David, do you mind carrying these patches in your tree? Given that they
depend on your netfs-lib branch, it's probably simpler to do it that way
rather than have us base the ceph-client master branch on yours. If
conflicts crop up, we can revisit that approach though.

David Howells (1):
  ceph: Use the provided iterator in ceph_netfs_issue_op()

Jeff Layton (3):
  netfs: fix sense of DIO test on short read
  ceph: enhance dout messages in issue_read codepaths
  ceph: switch to netfs_direct_read_iter

 fs/ceph/addr.c | 55 +++++++++++++++++++++++++++++++++-----------------
 fs/ceph/file.c |  3 +--
 fs/netfs/io.c  |  2 +-
 3 files changed, 38 insertions(+), 22 deletions(-)

-- 
2.36.1

[PATCH 1/4] netfs: fix sense of DIO test on short read

[Jeff Layton]

The sense of this test is reversed. There's nothing that prevents
userland from requesting a DIO read that is longer than the available
data. Conversely, we don't expect a buffered read to be short unless it
hits the EOF.

Suggested-by: David Howells <dhowells@redhat.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/netfs/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

David, feel free to fold this into the patch that adds the condition
so we can avoid the regression.

diff --git a/fs/netfs/io.c b/fs/netfs/io.c
index e5a15a924fc7..8188d43e8044 100644
--- a/fs/netfs/io.c
+++ b/fs/netfs/io.c
@@ -728,7 +728,7 @@ ssize_t netfs_begin_read(struct netfs_io_request *rreq, bool sync)
 
 		ret = rreq->error;
 		if (ret == 0 && rreq->submitted < rreq->len &&
-		    rreq->origin == NETFS_DIO_READ) {
+		    rreq->origin != NETFS_DIO_READ) {
 			trace_netfs_failure(rreq, NULL, ret, netfs_fail_short_read);
 			ret = -EIO;
 		}
-- 
2.36.1

[PATCH 2/4] ceph: Use the provided iterator in ceph_netfs_issue_op()

[Jeff Layton]

From: David Howells <dhowells@redhat.com>

The netfs_read_subrequest struct now contains a persistent iterator
representing the destination buffer for a read that the network filesystem
should use.  Make ceph use this.

Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/addr.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 8e6a931f3a0f..d14a9378d120 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -233,7 +233,6 @@ static bool ceph_netfs_issue_op_inline(struct netfs_io_subrequest *subreq)
 	struct ceph_mds_request *req;
 	struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(inode->i_sb);
 	struct ceph_inode_info *ci = ceph_inode(inode);
-	struct iov_iter iter;
 	ssize_t err = 0;
 	size_t len;
 	int mode;
@@ -268,8 +267,7 @@ static bool ceph_netfs_issue_op_inline(struct netfs_io_subrequest *subreq)
 	}
 
 	len = min_t(size_t, iinfo->inline_len - subreq->start, subreq->len);
-	iov_iter_xarray(&iter, READ, &rreq->mapping->i_pages, subreq->start, len);
-	err = copy_to_iter(iinfo->inline_data + subreq->start, len, &iter);
+	err = copy_to_iter(iinfo->inline_data + subreq->start, len, &subreq->iter);
 	if (err == 0)
 		err = -EFAULT;
 
@@ -287,7 +285,6 @@ static void ceph_netfs_issue_read(struct netfs_io_subrequest *subreq)
 	struct ceph_fs_client *fsc = ceph_inode_to_client(inode);
 	struct ceph_osd_request *req;
 	struct ceph_vino vino = ceph_vino(inode);
-	struct iov_iter iter;
 	struct page **pages;
 	size_t page_off;
 	int err = 0;
@@ -308,15 +305,14 @@ static void ceph_netfs_issue_read(struct netfs_io_subrequest *subreq)
 	}
 
 	dout("%s: pos=%llu orig_len=%zu len=%llu\n", __func__, subreq->start, subreq->len, len);
-	iov_iter_xarray(&iter, READ, &rreq->mapping->i_pages, subreq->start, len);
-	err = iov_iter_get_pages_alloc(&iter, &pages, len, &page_off);
+
+	err = iov_iter_get_pages_alloc(&subreq->iter, &pages, len, &page_off);
 	if (err < 0) {
 		dout("%s: iov_ter_get_pages_alloc returned %d\n", __func__, err);
 		goto out;
 	}
 
-	/* should always give us a page-aligned read */
-	WARN_ON_ONCE(page_off);
+	/* FIXME: adjust the len in req downward if necessary */
 	len = err;
 
 	osd_req_op_extent_osd_data_pages(req, 0, pages, len, 0, false, false);
-- 
2.36.1

[PATCH 3/4] ceph: enhance dout messages in issue_read codepaths

[Jeff Layton]

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/addr.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index d14a9378d120..475df4efd2c7 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -190,6 +190,8 @@ static bool ceph_netfs_clamp_length(struct netfs_io_subrequest *subreq)
 	/* Truncate the extent at the end of the current block */
 	ceph_calc_file_object_mapping(&ci->i_layout, subreq->start, subreq->len,
 				      &objno, &objoff, &xlen);
+	dout("%s: subreq->len=0x%zx xlen=0x%x rsize=0x%x",
+		__func__, subreq->len, xlen, fsc->mount_options->rsize);
 	subreq->len = min(xlen, fsc->mount_options->rsize);
 	return true;
 }
@@ -304,7 +306,9 @@ static void ceph_netfs_issue_read(struct netfs_io_subrequest *subreq)
 		goto out;
 	}
 
-	dout("%s: pos=%llu orig_len=%zu len=%llu\n", __func__, subreq->start, subreq->len, len);
+	dout("%s: pos=%llu orig_len=%zu len=%llu debug_id=%x debug_idx=%hx iter->count=%zx\n",
+		__func__, subreq->start, subreq->len, len, rreq->debug_id,
+		subreq->debug_index, iov_iter_count(&subreq->iter));
 
 	err = iov_iter_get_pages_alloc(&subreq->iter, &pages, len, &page_off);
 	if (err < 0) {
-- 
2.36.1

[PATCH 4/4] ceph: switch to netfs_direct_read_iter

[Jeff Layton]

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/addr.c | 41 +++++++++++++++++++++++++++++------------
 fs/ceph/file.c |  3 +--
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 475df4efd2c7..938679a7a1e3 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -201,7 +201,6 @@ static void finish_netfs_read(struct ceph_osd_request *req)
 	struct ceph_fs_client *fsc = ceph_inode_to_client(req->r_inode);
 	struct ceph_osd_data *osd_data = osd_req_op_extent_osd_data(req, 0);
 	struct netfs_io_subrequest *subreq = req->r_priv;
-	int num_pages;
 	int err = req->r_result;
 
 	ceph_update_read_metrics(&fsc->mdsc->metric, req->r_start_latency,
@@ -216,13 +215,18 @@ static void finish_netfs_read(struct ceph_osd_request *req)
 	else if (err == -EBLOCKLISTED)
 		fsc->blocklisted = true;
 
-	if (err >= 0 && err < subreq->len)
-		__set_bit(NETFS_SREQ_CLEAR_TAIL, &subreq->flags);
+	if (err >= 0) {
+		if (err < subreq->len)
+			__set_bit(NETFS_SREQ_CLEAR_TAIL, &subreq->flags);
+		iov_iter_advance(&subreq->iter, err);
+	}
+	if (!iov_iter_is_bvec(&subreq->iter))
+		ceph_put_page_vector(osd_data->pages,
+				     calc_pages_for(osd_data->alignment,
+				     osd_data->length),
+				     false);
 
 	netfs_subreq_terminated(subreq, err, true);
-
-	num_pages = calc_pages_for(osd_data->alignment, osd_data->length);
-	ceph_put_page_vector(osd_data->pages, num_pages, false);
 	iput(req->r_inode);
 }
 
@@ -287,6 +291,7 @@ static void ceph_netfs_issue_read(struct netfs_io_subrequest *subreq)
 	struct ceph_fs_client *fsc = ceph_inode_to_client(inode);
 	struct ceph_osd_request *req;
 	struct ceph_vino vino = ceph_vino(inode);
+	struct iov_iter *iter = &subreq->iter;
 	struct page **pages;
 	size_t page_off;
 	int err = 0;
@@ -310,16 +315,28 @@ static void ceph_netfs_issue_read(struct netfs_io_subrequest *subreq)
 		__func__, subreq->start, subreq->len, len, rreq->debug_id,
 		subreq->debug_index, iov_iter_count(&subreq->iter));
 
-	err = iov_iter_get_pages_alloc(&subreq->iter, &pages, len, &page_off);
-	if (err < 0) {
-		dout("%s: iov_ter_get_pages_alloc returned %d\n", __func__, err);
-		goto out;
+	if (iov_iter_is_bvec(iter)) {
+		/*
+		 * FIXME: remove force cast, ideally by plumbing an IOV_ITER osd_data
+		 * 	  variant.
+		 */
+		osd_req_op_extent_osd_data_bvecs(req, 0, (__force struct bio_vec *)iter->bvec,
+				iter->nr_segs, len);
+		goto submit;
 	}
 
-	/* FIXME: adjust the len in req downward if necessary */
-	len = err;
+	err = iov_iter_get_pages_alloc(&subreq->iter, &pages, len, &page_off);
+	if (err < len) {
+		if (err < 0) {
+			dout("%s: iov_ter_get_pages_alloc returned %d\n", __func__, err);
+			goto out;
+		}
+		len = err;
+		req->r_ops[0].extent.length = err;
+	}
 
 	osd_req_op_extent_osd_data_pages(req, 0, pages, len, 0, false, false);
+submit:
 	req->r_callback = finish_netfs_read;
 	req->r_priv = subreq;
 	req->r_inode = inode;
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 8c8226c0feac..81ce6753fa67 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -1634,8 +1634,7 @@ static ssize_t ceph_read_iter(struct kiocb *iocb, struct iov_iter *to)
 
 		if (ci->i_inline_version == CEPH_INLINE_NONE) {
 			if (!retry_op && (iocb->ki_flags & IOCB_DIRECT)) {
-				ret = ceph_direct_read_write(iocb, to,
-							     NULL, NULL);
+				ret = netfs_direct_read_iter(iocb, to);
 				if (ret >= 0 && ret < len)
 					retry_op = CHECK_EOF;
 			} else {
-- 
2.36.1

Re: [PATCH 0/4] ceph: convert to netfs_direct_read_iter for DIO reads

[Xiubo Li]

On 5/18/22 11:11 PM, Jeff Layton wrote:
> This patch series is based on top of David Howells' netfs-lib branch.
>
> I previously sent an RFC set for this back in early April, but David has
> revised his netfs-lib branch since then. This converts ceph to use the
> new DIO read helper netfs_direct_read_iter instead of using our own
> routine.
>
> With this change we ought to be able to rip out a large swath of
> ceph_direct_read_write, but I've decided to wait on that until the write
> helpers are in place and we can just remove that function wholesale.
>
> David, do you mind carrying these patches in your tree? Given that they
> depend on your netfs-lib branch, it's probably simpler to do it that way
> rather than have us base the ceph-client master branch on yours. If
> conflicts crop up, we can revisit that approach though.
>
> David Howells (1):
>    ceph: Use the provided iterator in ceph_netfs_issue_op()
>
> Jeff Layton (3):
>    netfs: fix sense of DIO test on short read
>    ceph: enhance dout messages in issue_read codepaths
>    ceph: switch to netfs_direct_read_iter
>
>   fs/ceph/addr.c | 55 +++++++++++++++++++++++++++++++++-----------------
>   fs/ceph/file.c |  3 +--
>   fs/netfs/io.c  |  2 +-
>   3 files changed, 38 insertions(+), 22 deletions(-)
>
This series LGTM.

Reviewed-by: Xiubo Li <xiubli@redhat.com>