[PATCH] staging: erofs: remove rcu_read_lock() in erofs_try_to_free_cached_page

[PATCH] staging: erofs: remove rcu_read_lock() in erofs_try_to_free_cached_page

[Gao Xiang]

page_private(page) cannot be changed if page lock is taken.

Besides, the corresponding workgroup won't be freed
if the page is already protected by page lock, therefore
no need to take rcu read lock.

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 drivers/staging/erofs/unzip_vle.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index 2b5951f233db..a127d8db76d8 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -238,14 +238,9 @@ int erofs_try_to_free_cached_page(struct address_space *mapping,
 {
 	struct erofs_sb_info *const sbi = EROFS_SB(mapping->host->i_sb);
 	const unsigned int clusterpages = erofs_clusterpages(sbi);
-
-	struct z_erofs_vle_workgroup *grp;
+	struct z_erofs_vle_workgroup *const grp = (void *)page_private(page);
 	int ret = 0;	/* 0 - busy */
 
-	/* prevent the workgroup from being freed */
-	rcu_read_lock();
-	grp = (void *)page_private(page);
-
 	if (erofs_workgroup_try_to_freeze(&grp->obj, 1)) {
 		unsigned int i;
 
@@ -257,12 +252,11 @@ int erofs_try_to_free_cached_page(struct address_space *mapping,
 			}
 		}
 		erofs_workgroup_unfreeze(&grp->obj, 1);
-	}
-	rcu_read_unlock();
 
-	if (ret) {
-		ClearPagePrivate(page);
-		put_page(page);
+		if (ret) {
+			ClearPagePrivate(page);
+			put_page(page);
+		}
 	}
 	return ret;
 }
-- 
2.14.4

Re: [PATCH] staging: erofs: remove rcu_read_lock() in erofs_try_to_free_cached_page

[Chao Yu]

On 2019/2/16 16:46, Gao Xiang wrote:
> page_private(page) cannot be changed if page lock is taken.
> 
> Besides, the corresponding workgroup won't be freed
> if the page is already protected by page lock, therefore
> no need to take rcu read lock.
> 
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

Re: [PATCH] staging: erofs: remove rcu_read_lock() in erofs_try_to_free_cached_page

[Gao Xiang]

Hi Greg,

Could you consider take some time on these two patches for linux-5.1-rc1?
 1) staging: erofs: fix race of initializing xattrs of a inode at the same time
 2) staging: erofs: remove rcu_read_lock() in erofs_try_to_free_cached_page

It will be of great help, especially patch 1 (it could cause potential memleak
in the race condition without it).

If something (eg, resend these patches in a new patchset) is needed, I'm happy
to do that. :)

Thanks,
Gao Xiang

On 2019/2/16 16:46, Gao Xiang wrote:
> page_private(page) cannot be changed if page lock is taken.
> 
> Besides, the corresponding workgroup won't be freed
> if the page is already protected by page lock, therefore
> no need to take rcu read lock.
> 
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> ---
>  drivers/staging/erofs/unzip_vle.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
> index 2b5951f233db..a127d8db76d8 100644
> --- a/drivers/staging/erofs/unzip_vle.c
> +++ b/drivers/staging/erofs/unzip_vle.c
> @@ -238,14 +238,9 @@ int erofs_try_to_free_cached_page(struct address_space *mapping,
>  {
>  	struct erofs_sb_info *const sbi = EROFS_SB(mapping->host->i_sb);
>  	const unsigned int clusterpages = erofs_clusterpages(sbi);
> -
> -	struct z_erofs_vle_workgroup *grp;
> +	struct z_erofs_vle_workgroup *const grp = (void *)page_private(page);
>  	int ret = 0;	/* 0 - busy */
>  
> -	/* prevent the workgroup from being freed */
> -	rcu_read_lock();
> -	grp = (void *)page_private(page);
> -
>  	if (erofs_workgroup_try_to_freeze(&grp->obj, 1)) {
>  		unsigned int i;
>  
> @@ -257,12 +252,11 @@ int erofs_try_to_free_cached_page(struct address_space *mapping,
>  			}
>  		}
>  		erofs_workgroup_unfreeze(&grp->obj, 1);
> -	}
> -	rcu_read_unlock();
>  
> -	if (ret) {
> -		ClearPagePrivate(page);
> -		put_page(page);
> +		if (ret) {
> +			ClearPagePrivate(page);
> +			put_page(page);
> +		}
>  	}
>  	return ret;
>  }
> 

Re: [PATCH] staging: erofs: remove rcu_read_lock() in erofs_try_to_free_cached_page

[Greg Kroah-Hartman]

On Wed, Feb 20, 2019 at 10:38:53AM +0800, Gao Xiang wrote:
> Hi Greg,
> 
> Could you consider take some time on these two patches for linux-5.1-rc1?
>  1) staging: erofs: fix race of initializing xattrs of a inode at the same time
>  2) staging: erofs: remove rcu_read_lock() in erofs_try_to_free_cached_page
> 
> It will be of great help, especially patch 1 (it could cause potential memleak
> in the race condition without it).

Ok, I'll queue them up now, I was waiting for reviewed-by tags from
others to show up :)

greg k-h

Re: [PATCH] staging: erofs: remove rcu_read_lock() in erofs_try_to_free_cached_page

[Gao Xiang]

On 2019/2/20 18:20, Greg Kroah-Hartman wrote:
> On Wed, Feb 20, 2019 at 10:38:53AM +0800, Gao Xiang wrote:
>> Hi Greg,
>>
>> Could you consider take some time on these two patches for linux-5.1-rc1?
>>  1) staging: erofs: fix race of initializing xattrs of a inode at the same time
>>  2) staging: erofs: remove rcu_read_lock() in erofs_try_to_free_cached_page
>>
>> It will be of great help, especially patch 1 (it could cause potential memleak
>> in the race condition without it).
> 
> Ok, I'll queue them up now, I was waiting for reviewed-by tags from
> others to show up :)

Thanks! :)
Gao Xiang

> 
> greg k-h
>