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 | 4 ----
fs/nfs/internal.h | 1 -
fs/nfs/nfstrace.h | 6 ++----
3 files changed, 2 insertions(+), 9 deletions(-)
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 5918c67dae0d..7167f588b1fc 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,6 @@ 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->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 +872,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 +978,6 @@ 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->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
Hi Ben,
On Wed, Nov 15, 2023 at 4:34 PM 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.
I've been having problems with xfstests generic/465 on all NFS
versions after applying this patch. Looking at wireshark, the client
appears to be resending the same reads over and over again. Have you
seen anything like this in your testing?
Thanks,
Anna
>
> Signed-off-by: Benjamin Coddington <[email protected]>
> ---
> fs/nfs/direct.c | 4 ----
> fs/nfs/internal.h | 1 -
> fs/nfs/nfstrace.h | 6 ++----
> 3 files changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
> index 5918c67dae0d..7167f588b1fc 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,6 @@ 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->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 +872,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 +978,6 @@ 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->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 16 Nov 2023, at 16:44, Anna Schumaker wrote:
> Hi Ben,
>
> On Wed, Nov 15, 2023 at 4:34 PM 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.
>
> I've been having problems with xfstests generic/465 on all NFS
> versions after applying this patch. Looking at wireshark, the client
> appears to be resending the same reads over and over again. Have you
> seen anything like this in your testing?
I have generic/465 failing before and after these two patches on pNFS SCSI..
but at least it completes. If I run it without pNFS I can see the same
thing.. it just sends the same reads over and over. I'll figure out why.
Ben
On Thu, Nov 16, 2023 at 5:03 PM Benjamin Coddington <[email protected]> wrote:
>
> On 16 Nov 2023, at 16:44, Anna Schumaker wrote:
>
> > Hi Ben,
> >
> > On Wed, Nov 15, 2023 at 4:34 PM 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.
> >
> > I've been having problems with xfstests generic/465 on all NFS
> > versions after applying this patch. Looking at wireshark, the client
> > appears to be resending the same reads over and over again. Have you
> > seen anything like this in your testing?
>
> I have generic/465 failing before and after these two patches on pNFS SCSI..
> but at least it completes. If I run it without pNFS I can see the same
> thing.. it just sends the same reads over and over. I'll figure out why.
Thanks! I have it failing normally as well, so that's expected. It's
the hanging forever that's not :)
Anna
>
> Ben
>
On 16 Nov 2023, at 17:11, Anna Schumaker wrote:
> On Thu, Nov 16, 2023 at 5:03 PM Benjamin Coddington <[email protected]> wrote:
>>
>> On 16 Nov 2023, at 16:44, Anna Schumaker wrote:
>>
>>> Hi Ben,
>>>
>>> On Wed, Nov 15, 2023 at 4:34 PM 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.
>>>
>>> I've been having problems with xfstests generic/465 on all NFS
>>> versions after applying this patch. Looking at wireshark, the client
>>> appears to be resending the same reads over and over again. Have you
>>> seen anything like this in your testing?
>>
>> I have generic/465 failing before and after these two patches on pNFS SCSI..
>> but at least it completes. If I run it without pNFS I can see the same
>> thing.. it just sends the same reads over and over. I'll figure out why.
>
> Thanks! I have it failing normally as well, so that's expected. It's
> the hanging forever that's not :)
The direct read is returning 0 when there's data on the device.
Oh, the problem is probably that patch drops the update of dreq->max_count,
which I overlooked because of the double assignment. Shame on me.
Ben
On 17 Nov 2023, at 6:21, Benjamin Coddington wrote:
> On 16 Nov 2023, at 17:11, Anna Schumaker wrote:
>
>> On Thu, Nov 16, 2023 at 5:03 PM Benjamin Coddington <[email protected]> wrote:
>>>
>>> On 16 Nov 2023, at 16:44, Anna Schumaker wrote:
>>>
>>>> Hi Ben,
>>>>
>>>> On Wed, Nov 15, 2023 at 4:34 PM 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.
>>>>
>>>> I've been having problems with xfstests generic/465 on all NFS
>>>> versions after applying this patch. Looking at wireshark, the client
>>>> appears to be resending the same reads over and over again. Have you
>>>> seen anything like this in your testing?
>>>
>>> I have generic/465 failing before and after these two patches on pNFS SCSI..
>>> but at least it completes. If I run it without pNFS I can see the same
>>> thing.. it just sends the same reads over and over. I'll figure out why.
>>
>> Thanks! I have it failing normally as well, so that's expected. It's
>> the hanging forever that's not :)
>
> The direct read is returning 0 when there's data on the device.
>
> Oh, the problem is probably that patch drops the update of dreq->max_count,
> which I overlooked because of the double assignment. Shame on me.
BTW - I think generic/465 makes bad assumptions about what read() should
return for O_DIRECT, and that's why it fails on NFS. Basically it does a
bunch of WRITEs and then READs and expects the same data coming back in the
READs, but doesn't use O_SYNC. On the wire, the client is interleaving the
READs and WRITEs.
Ben