Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
MIME-Version: 1.0
References: <21a1f2a6155398965f79ed64f0bd23bf38a50367.1700083991.git.bcodding@redhat.com>
 <952eea7e97246870f83e7a4592e9338007dfe625.1700083991.git.bcodding@redhat.com>
In-Reply-To: <952eea7e97246870f83e7a4592e9338007dfe625.1700083991.git.bcodding@redhat.com>
From:   Anna Schumaker <anna@kernel.org>
Date:   Thu, 16 Nov 2023 16:44:44 -0500
Message-ID: <CAFX2Jfkc7p+22aK0KvN4yUerS1HuKDC+Njo_AJV1=5pWW0sUYQ@mail.gmail.com>
Subject: Re: [PATCH 2/2] NFS: drop unused nfs_direct_req bytes_left
To:     Benjamin Coddington <bcodding@redhat.com>
Cc:     trond.myklebust@hammerspace.com, linux-nfs@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Ben,

On Wed, Nov 15, 2023 at 4:34=E2=80=AFPM Benjamin Coddington <bcodding@redha=
t.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.

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 <bcodding@redhat.com>
> ---
>  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 -=3D req_len;
>                         requested_bytes +=3D req_len;
>                         pos +=3D req_len;
> -                       dreq->bytes_left -=3D req_len;
>                 }
>                 nfs_direct_release_pages(pagevec, npages);
>                 kvfree(pagevec);
> @@ -441,7 +440,6 @@ ssize_t nfs_file_direct_read(struct kiocb *iocb, stru=
ct iov_iter *iter,
>                 goto out;
>
>         dreq->inode =3D inode;
> -       dreq->bytes_left =3D dreq->max_count =3D count;
>         dreq->io_start =3D iocb->ki_pos;
>         dreq->ctx =3D get_nfs_open_context(nfs_file_open_context(iocb->ki=
_filp));
>         l_ctx =3D nfs_get_lock_context(dreq->ctx);
> @@ -874,7 +872,6 @@ static ssize_t nfs_direct_write_schedule_iovec(struct=
 nfs_direct_req *dreq,
>                         bytes -=3D req_len;
>                         requested_bytes +=3D req_len;
>                         pos +=3D req_len;
> -                       dreq->bytes_left -=3D 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, str=
uct iov_iter *iter,
>                 goto out;
>
>         dreq->inode =3D inode;
> -       dreq->bytes_left =3D dreq->max_count =3D count;
>         dreq->io_start =3D pos;
>         dreq->ctx =3D get_nfs_open_context(nfs_file_open_context(iocb->ki=
_filp));
>         l_ctx =3D 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 process=
ed */
>                                 max_count,      /* max expected count */
> -                               bytes_left,     /* bytes left to be sent =
*/
>                                 error;          /* any reported error */
>         struct completion       completion;     /* wait for i/o completio=
n */
>
> 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 =3D nfs_fhandle_hash(fh);
>                         __entry->offset =3D dreq->io_start;
>                         __entry->count =3D dreq->count;
> -                       __entry->bytes_left =3D dreq->bytes_left;
>                         __entry->error =3D dreq->error;
>                         __entry->flags =3D dreq->flags;
>                 ),
>
>                 TP_printk(
>                         "error=3D%zd fileid=3D%02x:%02x:%llu fhandle=3D0x=
%08x "
> -                       "offset=3D%lld count=3D%zd bytes_left=3D%zd flags=
=3D%s",
> +                       "offset=3D%lld count=3D%zd flags=3D%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
>