diff mbox series

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

Message ID 21a1f2a6155398965f79ed64f0bd23bf38a50367.1700220277.git.bcodding@redhat.com (mailing list archive)
State New
Headers show
Series [v2,1/2] pNFS: Fix the pnfs block driver's calculation of layoutget size | expand

Commit Message

Benjamin Coddington Nov. 17, 2023, 11:25 a.m. UTC
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(-)

Comments

Christoph Hellwig Nov. 17, 2023, 1:04 p.m. UTC | #1
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>
diff mbox series

Patch

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),