All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] sunrpc: clean-up cache downcall
@ 2020-11-27 16:14 Roberto Bergantinos Corpas
  2020-11-27 16:50 ` Chuck Lever
  0 siblings, 1 reply; 3+ messages in thread
From: Roberto Bergantinos Corpas @ 2020-11-27 16:14 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs

We can simplifly code around cache_downcall unifying memory
allocations using kvmalloc, this have the benefit of getting rid of
cache_slow_downcall (and queue_io_mutex), and also matches userland
allocation size and limits

Signed-off-by: Roberto Bergantinos Corpas <rbergant@redhat.com>
---
 net/sunrpc/cache.c | 41 +++++++++++------------------------------
 1 file changed, 11 insertions(+), 30 deletions(-)

diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index baef5ee43dbb..1347ecae9c84 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -777,7 +777,6 @@ void cache_clean_deferred(void *owner)
  */
 
 static DEFINE_SPINLOCK(queue_lock);
-static DEFINE_MUTEX(queue_io_mutex);
 
 struct cache_queue {
 	struct list_head	list;
@@ -905,44 +904,26 @@ static ssize_t cache_do_downcall(char *kaddr, const char __user *buf,
 	return ret;
 }
 
-static ssize_t cache_slow_downcall(const char __user *buf,
-				   size_t count, struct cache_detail *cd)
-{
-	static char write_buf[8192]; /* protected by queue_io_mutex */
-	ssize_t ret = -EINVAL;
-
-	if (count >= sizeof(write_buf))
-		goto out;
-	mutex_lock(&queue_io_mutex);
-	ret = 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 = -ENOMEM;
 
-	if (count >= PAGE_SIZE)
-		goto out_slow;
+	if (count >= 32768) { /* 32k is max userland buffer, lets check anyway */
+		ret = -EINVAL;
+		goto out;
+	}
 
-	page = find_or_create_page(mapping, 0, GFP_KERNEL);
-	if (!page)
-		goto out_slow;
+	write_buf = kvmalloc(count + 1, GFP_KERNEL);
+	if (!write_buf)
+		goto out;
 
-	kaddr = kmap(page);
-	ret = cache_do_downcall(kaddr, buf, count, cd);
-	kunmap(page);
-	unlock_page(page);
-	put_page(page);
+	ret = cache_do_downcall(write_buf, buf, count, cd);
+	kvfree(write_buf);
+out:
 	return ret;
-out_slow:
-	return cache_slow_downcall(buf, count, cd);
 }
 
 static ssize_t cache_write(struct file *filp, const char __user *buf,
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] sunrpc: clean-up cache downcall
  2020-11-27 16:14 [PATCH] sunrpc: clean-up cache downcall Roberto Bergantinos Corpas
@ 2020-11-27 16:50 ` Chuck Lever
  2020-11-27 17:14   ` Roberto Bergantinos Corpas
  0 siblings, 1 reply; 3+ messages in thread
From: Chuck Lever @ 2020-11-27 16:50 UTC (permalink / raw)
  To: Roberto Bergantinos Corpas; +Cc: Bruce Fields, Linux NFS Mailing List

Hi Roberto-

I spotted some mechanical problems.


> On Nov 27, 2020, at 11:14 AM, Roberto Bergantinos Corpas <rbergant@redhat.com> wrote:
> 
> We can simplifly code around cache_downcall unifying memory

^simplifly^simplify

> allocations using kvmalloc, this have the benefit of getting rid of

^, this have^. This has

> cache_slow_downcall (and queue_io_mutex), and also matches userland
> allocation size and limits
> 
> Signed-off-by: Roberto Bergantinos Corpas <rbergant@redhat.com>

Assuming Bruce is copacetic with this patch, the change looks
appropriate for the v5.11 merge window. However, this patch
doesn't appear to apply to v5.10-rc5. Might be because
27a1e8a0f79e ("sunrpc: raise kernel RPC channel buffer size")
was already merged?


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

--
Chuck Lever




^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] sunrpc: clean-up cache downcall
  2020-11-27 16:50 ` Chuck Lever
@ 2020-11-27 17:14   ` Roberto Bergantinos Corpas
  0 siblings, 0 replies; 3+ messages in thread
From: Roberto Bergantinos Corpas @ 2020-11-27 17:14 UTC (permalink / raw)
  To: Chuck Lever; +Cc: Bruce Fields, Linux NFS Mailing List

Hi Bruce!

 Thanks for comments!, i'll send a v2 with mechanical errors fixed
based on v5.10-rc5.

rgds
roberto

On Fri, Nov 27, 2020 at 5:52 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> Hi Roberto-
>
> I spotted some mechanical problems.
>
>
> > On Nov 27, 2020, at 11:14 AM, Roberto Bergantinos Corpas <rbergant@redhat.com> wrote:
> >
> > We can simplifly code around cache_downcall unifying memory
>
> ^simplifly^simplify
>
> > allocations using kvmalloc, this have the benefit of getting rid of
>
> ^, this have^. This has
>
> > cache_slow_downcall (and queue_io_mutex), and also matches userland
> > allocation size and limits
> >
> > Signed-off-by: Roberto Bergantinos Corpas <rbergant@redhat.com>
>
> Assuming Bruce is copacetic with this patch, the change looks
> appropriate for the v5.11 merge window. However, this patch
> doesn't appear to apply to v5.10-rc5. Might be because
> 27a1e8a0f79e ("sunrpc: raise kernel RPC channel buffer size")
> was already merged?
>
>
> > ---
> > net/sunrpc/cache.c | 41 +++++++++++------------------------------
> > 1 file changed, 11 insertions(+), 30 deletions(-)
> >
> > diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> > index baef5ee43dbb..1347ecae9c84 100644
> > --- a/net/sunrpc/cache.c
> > +++ b/net/sunrpc/cache.c
> > @@ -777,7 +777,6 @@ void cache_clean_deferred(void *owner)
> >  */
> >
> > static DEFINE_SPINLOCK(queue_lock);
> > -static DEFINE_MUTEX(queue_io_mutex);
> >
> > struct cache_queue {
> >       struct list_head        list;
> > @@ -905,44 +904,26 @@ static ssize_t cache_do_downcall(char *kaddr, const char __user *buf,
> >       return ret;
> > }
> >
> > -static ssize_t cache_slow_downcall(const char __user *buf,
> > -                                size_t count, struct cache_detail *cd)
> > -{
> > -     static char write_buf[8192]; /* protected by queue_io_mutex */
> > -     ssize_t ret = -EINVAL;
> > -
> > -     if (count >= sizeof(write_buf))
> > -             goto out;
> > -     mutex_lock(&queue_io_mutex);
> > -     ret = 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 = -ENOMEM;
> >
> > -     if (count >= PAGE_SIZE)
> > -             goto out_slow;
> > +     if (count >= 32768) { /* 32k is max userland buffer, lets check anyway */
> > +             ret = -EINVAL;
> > +             goto out;
> > +     }
> >
> > -     page = find_or_create_page(mapping, 0, GFP_KERNEL);
> > -     if (!page)
> > -             goto out_slow;
> > +     write_buf = kvmalloc(count + 1, GFP_KERNEL);
> > +     if (!write_buf)
> > +             goto out;
> >
> > -     kaddr = kmap(page);
> > -     ret = cache_do_downcall(kaddr, buf, count, cd);
> > -     kunmap(page);
> > -     unlock_page(page);
> > -     put_page(page);
> > +     ret = cache_do_downcall(write_buf, buf, count, cd);
> > +     kvfree(write_buf);
> > +out:
> >       return ret;
> > -out_slow:
> > -     return cache_slow_downcall(buf, count, cd);
> > }
> >
> > static ssize_t cache_write(struct file *filp, const char __user *buf,
> > --
> > 2.21.0
> >
>
> --
> Chuck Lever
>
>
>


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-11-27 17:14 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-27 16:14 [PATCH] sunrpc: clean-up cache downcall Roberto Bergantinos Corpas
2020-11-27 16:50 ` Chuck Lever
2020-11-27 17:14   ` Roberto Bergantinos Corpas

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.