From: Trond Myklebust <[email protected]>
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 <[email protected]>
Signed-off-by: Trond Myklebust <[email protected]>
Fixes: 954998b60caa ("NFS: Fix error handling for O_DIRECT write scheduling")
Reviewed-by: Benjamin Coddington <[email protected]>
Tested-by: Benjamin Coddington <[email protected]>
---
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
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 <[email protected]>
---
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
On Fri, Nov 17, 2023 at 06:25:13AM -0500, Benjamin Coddington wrote:
> From: Trond Myklebust <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Trond Myklebust <[email protected]>
> Fixes: 954998b60caa ("NFS: Fix error handling for O_DIRECT write scheduling")
> Reviewed-by: Benjamin Coddington <[email protected]>
> Tested-by: Benjamin Coddington <[email protected]>
> ---
> 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 <[email protected]>
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
Hi Ben,
On Fri, Nov 17, 2023 at 6:25 AM Benjamin Coddington <[email protected]> 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 <[email protected]>
> ---
> 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
>