Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
Subject: Re: [PATCH v2] sunrpc: clean-up cache downcall
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <20201127183831.25012-1-rbergant@redhat.com>
Date:   Fri, 27 Nov 2020 14:14:44 -0500
Cc:     Bruce Fields <bfields@fieldses.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <1EE331A2-2E73-4B3F-8787-4DAC2EEAF078@oracle.com>
References: <20201127183831.25012-1-rbergant@redhat.com>
To:     Roberto Bergantinos Corpas <rbergant@redhat.com>
Precedence: bulk


> On Nov 27, 2020, at 1:38 PM, Roberto Bergantinos Corpas =
<rbergant@redhat.com> wrote:
>=20
> We can simplify code around cache_downcall unifying memory
> allocations using kvmalloc. This has the benefit of getting rid of
> cache_slow_downcall (and queue_io_mutex), and also matches userland
> allocation size and limits
>=20
> Signed-off-by: Roberto Bergantinos Corpas <rbergant@redhat.com>


Thanks, Roberto.

I see from linux-nfs history that Bruce explicitly asked for
this change, so applied for the next merge window. See the
cel-next topic branch in:

 git://git.linux-nfs.org/projects/cel/cel-2.6.git

or

 https://git.linux-nfs.org/?p=3Dcel/cel-2.6.git;a=3Dsummary


> ---
> net/sunrpc/cache.c | 41 +++++++++++------------------------------
> 1 file changed, 11 insertions(+), 30 deletions(-)
>=20
> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> index 20c93b68505e..1a2c1c44bb00 100644
> --- a/net/sunrpc/cache.c
> +++ b/net/sunrpc/cache.c
> @@ -778,7 +778,6 @@ void cache_clean_deferred(void *owner)
>  */
>=20
> static DEFINE_SPINLOCK(queue_lock);
> -static DEFINE_MUTEX(queue_io_mutex);
>=20
> struct cache_queue {
> 	struct list_head	list;
> @@ -906,44 +905,26 @@ static ssize_t cache_do_downcall(char *kaddr, =
const char __user *buf,
> 	return ret;
> }
>=20
> -static ssize_t cache_slow_downcall(const char __user *buf,
> -				   size_t count, struct cache_detail =
*cd)
> -{
> -	static char write_buf[32768]; /* protected by queue_io_mutex */
> -	ssize_t ret =3D -EINVAL;
> -
> -	if (count >=3D sizeof(write_buf))
> -		goto out;
> -	mutex_lock(&queue_io_mutex);
> -	ret =3D cache_do_downcall(write_buf, buf, count, cd);
> -	mutex_unlock(&queue_io_mutex);
> -out:
> -	return ret;
> -}
> -
> static ssize_t cache_downcall(struct address_space *mapping,
> 			      const char __user *buf,
> 			      size_t count, struct cache_detail *cd)
> {
> -	struct page *page;
> -	char *kaddr;
> +	char *write_buf;
> 	ssize_t ret =3D -ENOMEM;
>=20
> -	if (count >=3D PAGE_SIZE)
> -		goto out_slow;
> +	if (count >=3D 32768) { /* 32k is max userland buffer, lets =
check anyway */
> +		ret =3D -EINVAL;
> +		goto out;
> +	}
>=20
> -	page =3D find_or_create_page(mapping, 0, GFP_KERNEL);
> -	if (!page)
> -		goto out_slow;
> +	write_buf =3D kvmalloc(count + 1, GFP_KERNEL);
> +	if (!write_buf)
> +		goto out;
>=20
> -	kaddr =3D kmap(page);
> -	ret =3D cache_do_downcall(kaddr, buf, count, cd);
> -	kunmap(page);
> -	unlock_page(page);
> -	put_page(page);
> +	ret =3D cache_do_downcall(write_buf, buf, count, cd);
> +	kvfree(write_buf);
> +out:
> 	return ret;
> -out_slow:
> -	return cache_slow_downcall(buf, count, cd);
> }
>=20
> static ssize_t cache_write(struct file *filp, const char __user *buf,
> --=20
> 2.21.0
>=20

--
Chuck Lever