[PATCH v2 1/2] pNFS: Fix the pnfs block driver's calculation of layoutget size

[PATCH v2 1/2] pNFS: Fix the pnfs block driver's calculation of layoutget size

[Benjamin Coddington]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Instead of relying on the value of the 'bytes_left' field, we should
calculate the layout size based on the offset of the request that is
being written out.

Reported-by: Benjamin Coddington <bcodding@redhat.com>
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
Fixes: 954998b60caa ("NFS: Fix error handling for O_DIRECT write scheduling")
Reviewed-by: Benjamin Coddington <bcodding@redhat.com>
Tested-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfs/blocklayout/blocklayout.c | 5 ++---
 fs/nfs/direct.c                  | 5 +++--
 fs/nfs/internal.h                | 2 +-
 fs/nfs/pnfs.c                    | 3 ++-
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/blocklayout/blocklayout.c b/fs/nfs/blocklayout/blocklayout.c
index 943aeea1eb16..c1cc9fe93dd4 100644
--- a/fs/nfs/blocklayout/blocklayout.c
+++ b/fs/nfs/blocklayout/blocklayout.c
@@ -893,10 +893,9 @@ bl_pg_init_write(struct nfs_pageio_descriptor *pgio, struct nfs_page *req)
 	}
 
 	if (pgio->pg_dreq == NULL)
-		wb_size = pnfs_num_cont_bytes(pgio->pg_inode,
-					      req->wb_index);
+		wb_size = pnfs_num_cont_bytes(pgio->pg_inode, req->wb_index);
 	else
-		wb_size = nfs_dreq_bytes_left(pgio->pg_dreq);
+		wb_size = nfs_dreq_bytes_left(pgio->pg_dreq, req_offset(req));
 
 	pnfs_generic_pg_init_write(pgio, req, wb_size);
 
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index f6c74f424691..5918c67dae0d 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -205,9 +205,10 @@ static void nfs_direct_req_release(struct nfs_direct_req *dreq)
 	kref_put(&dreq->kref, nfs_direct_req_free);
 }
 
-ssize_t nfs_dreq_bytes_left(struct nfs_direct_req *dreq)
+ssize_t nfs_dreq_bytes_left(struct nfs_direct_req *dreq, loff_t offset)
 {
-	return dreq->bytes_left;
+	loff_t start = offset - dreq->io_start;
+	return dreq->max_count - start;
 }
 EXPORT_SYMBOL_GPL(nfs_dreq_bytes_left);
 
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 9c9cf764f600..b1fa81c9dff6 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -655,7 +655,7 @@ extern int nfs_sillyrename(struct inode *dir, struct dentry *dentry);
 /* direct.c */
 void nfs_init_cinfo_from_dreq(struct nfs_commit_info *cinfo,
 			      struct nfs_direct_req *dreq);
-extern ssize_t nfs_dreq_bytes_left(struct nfs_direct_req *dreq);
+extern ssize_t nfs_dreq_bytes_left(struct nfs_direct_req *dreq, loff_t offset);
 
 /* nfs4proc.c */
 extern struct nfs_client *nfs4_init_client(struct nfs_client *clp,
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 84343aefbbd6..9084f156d67b 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -2729,7 +2729,8 @@ pnfs_generic_pg_init_read(struct nfs_pageio_descriptor *pgio, struct nfs_page *r
 		if (pgio->pg_dreq == NULL)
 			rd_size = i_size_read(pgio->pg_inode) - req_offset(req);
 		else
-			rd_size = nfs_dreq_bytes_left(pgio->pg_dreq);
+			rd_size = nfs_dreq_bytes_left(pgio->pg_dreq,
+						      req_offset(req));
 
 		pgio->pg_lseg =
 			pnfs_update_layout(pgio->pg_inode, nfs_req_openctx(req),
-- 
2.41.0

[PATCH v2 2/2] NFS: drop unused nfs_direct_req bytes_left

[Benjamin Coddington]

Now that we're calculating how large a remaining IO should be based
on the current request's offset, we no longer need to track bytes_left on
each struct nfs_direct_req.  Drop the field, and clean up the direct
request tracepoints.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfs/direct.c   | 6 ++----
 fs/nfs/internal.h | 1 -
 fs/nfs/nfstrace.h | 6 ++----
 3 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 5918c67dae0d..c03926a1cc73 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -369,7 +369,6 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq,
 			bytes -= req_len;
 			requested_bytes += req_len;
 			pos += req_len;
-			dreq->bytes_left -= req_len;
 		}
 		nfs_direct_release_pages(pagevec, npages);
 		kvfree(pagevec);
@@ -441,7 +440,7 @@ ssize_t nfs_file_direct_read(struct kiocb *iocb, struct iov_iter *iter,
 		goto out;
 
 	dreq->inode = inode;
-	dreq->bytes_left = dreq->max_count = count;
+	dreq->max_count = count;
 	dreq->io_start = iocb->ki_pos;
 	dreq->ctx = get_nfs_open_context(nfs_file_open_context(iocb->ki_filp));
 	l_ctx = nfs_get_lock_context(dreq->ctx);
@@ -874,7 +873,6 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
 			bytes -= req_len;
 			requested_bytes += req_len;
 			pos += req_len;
-			dreq->bytes_left -= req_len;
 
 			if (defer) {
 				nfs_mark_request_commit(req, NULL, &cinfo, 0);
@@ -981,7 +979,7 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb, struct iov_iter *iter,
 		goto out;
 
 	dreq->inode = inode;
-	dreq->bytes_left = dreq->max_count = count;
+	dreq->max_count = count;
 	dreq->io_start = pos;
 	dreq->ctx = get_nfs_open_context(nfs_file_open_context(iocb->ki_filp));
 	l_ctx = nfs_get_lock_context(dreq->ctx);
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index b1fa81c9dff6..e3722ce6722e 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -936,7 +936,6 @@ struct nfs_direct_req {
 	loff_t			io_start;	/* Start offset for I/O */
 	ssize_t			count,		/* bytes actually processed */
 				max_count,	/* max expected count */
-				bytes_left,	/* bytes left to be sent */
 				error;		/* any reported error */
 	struct completion	completion;	/* wait for i/o completion */
 
diff --git a/fs/nfs/nfstrace.h b/fs/nfs/nfstrace.h
index 4e90ca531176..03cbc3893cef 100644
--- a/fs/nfs/nfstrace.h
+++ b/fs/nfs/nfstrace.h
@@ -1539,7 +1539,6 @@ DECLARE_EVENT_CLASS(nfs_direct_req_class,
 			__field(u32, fhandle)
 			__field(loff_t, offset)
 			__field(ssize_t, count)
-			__field(ssize_t, bytes_left)
 			__field(ssize_t, error)
 			__field(int, flags)
 		),
@@ -1554,19 +1553,18 @@ DECLARE_EVENT_CLASS(nfs_direct_req_class,
 			__entry->fhandle = nfs_fhandle_hash(fh);
 			__entry->offset = dreq->io_start;
 			__entry->count = dreq->count;
-			__entry->bytes_left = dreq->bytes_left;
 			__entry->error = dreq->error;
 			__entry->flags = dreq->flags;
 		),
 
 		TP_printk(
 			"error=%zd fileid=%02x:%02x:%llu fhandle=0x%08x "
-			"offset=%lld count=%zd bytes_left=%zd flags=%s",
+			"offset=%lld count=%zd flags=%s",
 			__entry->error, MAJOR(__entry->dev),
 			MINOR(__entry->dev),
 			(unsigned long long)__entry->fileid,
 			__entry->fhandle, __entry->offset,
-			__entry->count, __entry->bytes_left,
+			__entry->count,
 			nfs_show_direct_req_flags(__entry->flags)
 		)
 );
-- 
2.41.0

Re: [PATCH v2 2/2] NFS: drop unused nfs_direct_req bytes_left

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/2] NFS: drop unused nfs_direct_req bytes_left

[Anna Schumaker]

Hi Ben,

On Fri, Nov 17, 2023 at 6:25 AM Benjamin Coddington <bcodding@redhat.com> wrote:
>
> Now that we're calculating how large a remaining IO should be based
> on the current request's offset, we no longer need to track bytes_left on
> each struct nfs_direct_req.  Drop the field, and clean up the direct
> request tracepoints.

v2 works better for me! Thanks for fixing that up!

Anna

>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/direct.c   | 6 ++----
>  fs/nfs/internal.h | 1 -
>  fs/nfs/nfstrace.h | 6 ++----
>  3 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
> index 5918c67dae0d..c03926a1cc73 100644
> --- a/fs/nfs/direct.c
> +++ b/fs/nfs/direct.c
> @@ -369,7 +369,6 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq,
>                         bytes -= req_len;
>                         requested_bytes += req_len;
>                         pos += req_len;
> -                       dreq->bytes_left -= req_len;
>                 }
>                 nfs_direct_release_pages(pagevec, npages);
>                 kvfree(pagevec);
> @@ -441,7 +440,7 @@ ssize_t nfs_file_direct_read(struct kiocb *iocb, struct iov_iter *iter,
>                 goto out;
>
>         dreq->inode = inode;
> -       dreq->bytes_left = dreq->max_count = count;
> +       dreq->max_count = count;
>         dreq->io_start = iocb->ki_pos;
>         dreq->ctx = get_nfs_open_context(nfs_file_open_context(iocb->ki_filp));
>         l_ctx = nfs_get_lock_context(dreq->ctx);
> @@ -874,7 +873,6 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
>                         bytes -= req_len;
>                         requested_bytes += req_len;
>                         pos += req_len;
> -                       dreq->bytes_left -= req_len;
>
>                         if (defer) {
>                                 nfs_mark_request_commit(req, NULL, &cinfo, 0);
> @@ -981,7 +979,7 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb, struct iov_iter *iter,
>                 goto out;
>
>         dreq->inode = inode;
> -       dreq->bytes_left = dreq->max_count = count;
> +       dreq->max_count = count;
>         dreq->io_start = pos;
>         dreq->ctx = get_nfs_open_context(nfs_file_open_context(iocb->ki_filp));
>         l_ctx = nfs_get_lock_context(dreq->ctx);
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index b1fa81c9dff6..e3722ce6722e 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -936,7 +936,6 @@ struct nfs_direct_req {
>         loff_t                  io_start;       /* Start offset for I/O */
>         ssize_t                 count,          /* bytes actually processed */
>                                 max_count,      /* max expected count */
> -                               bytes_left,     /* bytes left to be sent */
>                                 error;          /* any reported error */
>         struct completion       completion;     /* wait for i/o completion */
>
> diff --git a/fs/nfs/nfstrace.h b/fs/nfs/nfstrace.h
> index 4e90ca531176..03cbc3893cef 100644
> --- a/fs/nfs/nfstrace.h
> +++ b/fs/nfs/nfstrace.h
> @@ -1539,7 +1539,6 @@ DECLARE_EVENT_CLASS(nfs_direct_req_class,
>                         __field(u32, fhandle)
>                         __field(loff_t, offset)
>                         __field(ssize_t, count)
> -                       __field(ssize_t, bytes_left)
>                         __field(ssize_t, error)
>                         __field(int, flags)
>                 ),
> @@ -1554,19 +1553,18 @@ DECLARE_EVENT_CLASS(nfs_direct_req_class,
>                         __entry->fhandle = nfs_fhandle_hash(fh);
>                         __entry->offset = dreq->io_start;
>                         __entry->count = dreq->count;
> -                       __entry->bytes_left = dreq->bytes_left;
>                         __entry->error = dreq->error;
>                         __entry->flags = dreq->flags;
>                 ),
>
>                 TP_printk(
>                         "error=%zd fileid=%02x:%02x:%llu fhandle=0x%08x "
> -                       "offset=%lld count=%zd bytes_left=%zd flags=%s",
> +                       "offset=%lld count=%zd flags=%s",
>                         __entry->error, MAJOR(__entry->dev),
>                         MINOR(__entry->dev),
>                         (unsigned long long)__entry->fileid,
>                         __entry->fhandle, __entry->offset,
> -                       __entry->count, __entry->bytes_left,
> +                       __entry->count,
>                         nfs_show_direct_req_flags(__entry->flags)
>                 )
>  );
> --
> 2.41.0
>

Re: [PATCH v2 1/2] pNFS: Fix the pnfs block driver's calculation of layoutget size

[Christoph Hellwig]

On Fri, Nov 17, 2023 at 06:25:13AM -0500, Benjamin Coddington wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> Instead of relying on the value of the 'bytes_left' field, we should
> calculate the layout size based on the offset of the request that is
> being written out.
> 
> Reported-by: Benjamin Coddington <bcodding@redhat.com>
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> Fixes: 954998b60caa ("NFS: Fix error handling for O_DIRECT write scheduling")
> Reviewed-by: Benjamin Coddington <bcodding@redhat.com>
> Tested-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/blocklayout/blocklayout.c | 5 ++---
>  fs/nfs/direct.c                  | 5 +++--
>  fs/nfs/internal.h                | 2 +-
>  fs/nfs/pnfs.c                    | 3 ++-
>  4 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/nfs/blocklayout/blocklayout.c b/fs/nfs/blocklayout/blocklayout.c
> index 943aeea1eb16..c1cc9fe93dd4 100644
> --- a/fs/nfs/blocklayout/blocklayout.c
> +++ b/fs/nfs/blocklayout/blocklayout.c
> @@ -893,10 +893,9 @@ bl_pg_init_write(struct nfs_pageio_descriptor *pgio, struct nfs_page *req)
>  	}
>  
>  	if (pgio->pg_dreq == NULL)
> -		wb_size = pnfs_num_cont_bytes(pgio->pg_inode,
> -					      req->wb_index);
> +		wb_size = pnfs_num_cont_bytes(pgio->pg_inode, req->wb_index);
>  	else
> -		wb_size = nfs_dreq_bytes_left(pgio->pg_dreq);
> +		wb_size = nfs_dreq_bytes_left(pgio->pg_dreq, req_offset(req));
>  
>  	pnfs_generic_pg_init_write(pgio, req, wb_size);
>  
> diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
> index f6c74f424691..5918c67dae0d 100644
> --- a/fs/nfs/direct.c
> +++ b/fs/nfs/direct.c
> @@ -205,9 +205,10 @@ static void nfs_direct_req_release(struct nfs_direct_req *dreq)
>  	kref_put(&dreq->kref, nfs_direct_req_free);
>  }
>  
> -ssize_t nfs_dreq_bytes_left(struct nfs_direct_req *dreq)
> +ssize_t nfs_dreq_bytes_left(struct nfs_direct_req *dreq, loff_t offset)
>  {
> -	return dreq->bytes_left;
> +	loff_t start = offset - dreq->io_start;
> +	return dreq->max_count - start;

We normally put an empty line after the variable declarations.  But
looking at this, thee local variables seems a bit pointless to me,
as does not simply making this an inline function.

> +extern ssize_t nfs_dreq_bytes_left(struct nfs_direct_req *dreq, loff_t offset);

and you might as well drop the pointless extern here while you're at it.

Otherwise this looks good to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>