Received-SPF: pass (google.com: domain of linux-nfs+bounces-3687-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Mike Snitzer" <snitzer@kernel.org>
Cc: linux-nfs@vger.kernel.org, "Jeff Layton" <jlayton@kernel.org>,
 "Chuck Lever" <chuck.lever@oracle.com>,
 "Trond Myklebust" <trondmy@hammerspace.com>, snitzer@hammerspace.com
Subject: Re: [RFC PATCH v2 09/15] NFS: Enable localio for non-pNFS I/O
In-reply-to: <20240612030752.31754-10-snitzer@kernel.org>
References: <20240612030752.31754-1-snitzer@kernel.org>,
 <20240612030752.31754-10-snitzer@kernel.org>
Date: Wed, 12 Jun 2024 15:12:28 +1000
Message-id: <171816914830.14261.13610752851419538459@noble.neil.brown.name>

On Wed, 12 Jun 2024, Mike Snitzer wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>=20
> Try a local open of the file we're writing to, and if it succeeds, then
> do local I/O.
>=20
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> ---
>  fs/nfs/pagelist.c | 19 ++++++++++---------
>  fs/nfs/write.c    |  7 ++++++-
>  2 files changed, 16 insertions(+), 10 deletions(-)
>=20
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index b08420b8e664..3ee78da5ebc4 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -1063,6 +1063,7 @@ EXPORT_SYMBOL_GPL(nfs_generic_pgio);
>  static int nfs_generic_pg_pgios(struct nfs_pageio_descriptor *desc)
>  {
>  	struct nfs_pgio_header *hdr;
> +	struct file *filp;
>  	int ret;
>  	unsigned short task_flags =3D 0;
> =20
> @@ -1074,18 +1075,18 @@ static int nfs_generic_pg_pgios(struct nfs_pageio_d=
escriptor *desc)
>  	nfs_pgheader_init(desc, hdr, nfs_pgio_header_free);
>  	ret =3D nfs_generic_pgio(desc, hdr);
>  	if (ret =3D=3D 0) {
> +		struct nfs_client *clp =3D NFS_SERVER(hdr->inode)->nfs_client;
> +
> +		filp =3D nfs_local_file_open(clp, hdr->cred, hdr->args.fh,
> +					   hdr->args.context);
> +
>  		if (NFS_SERVER(hdr->inode)->nfs_client->cl_minorversion)
>  			task_flags =3D RPC_TASK_MOVEABLE;
> -		ret =3D nfs_initiate_pgio(desc,
> -					NFS_SERVER(hdr->inode)->nfs_client,
> -					NFS_CLIENT(hdr->inode),
> -					hdr,
> -					hdr->cred,
> -					NFS_PROTO(hdr->inode),
> -					desc->pg_rpc_callops,
> -					desc->pg_ioflags,
> +		ret =3D nfs_initiate_pgio(desc, clp, NFS_CLIENT(hdr->inode),
> +					hdr, hdr->cred, NFS_PROTO(hdr->inode),
> +					desc->pg_rpc_callops, desc->pg_ioflags,
>  					RPC_TASK_CRED_NOREF | task_flags,
> -					NULL);
> +					filp);

Is this rearrangement really an improvement?  I guess it is personal
taste question, but in this case it makes the patch a little harder to
read.

At first glance it looks like filp doesn't get closed, but I guess it is
getting stored in 'desc' and gets closed when 'desc' is freed.

NeilBrown