On Fri, Nov 17, 2023 at 06:25:13AM -0500, Benjamin Coddington wrote:
> From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> Fixes: 954998b60caa ("NFS: Fix error handling for O_DIRECT write scheduling")
> Reviewed-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
> Tested-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
> ---
> 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@xxxxxx>
