diff mbox series

[v1] erofs: fix refcount on the metabuf used for inode lookup

Message ID 20240220191114.3272126-1-dhavale@google.com
State New
Headers
Series [v1] erofs: fix refcount on the metabuf used for inode lookup |

Commit Message

Sandeep Dhavale Feb. 20, 2024, 7:11 p.m. UTC 
  In erofs_find_target_block() when erofs_dirnamecmp() returns 0,
we do not assign the target metabuf. This causes the caller
erofs_namei()'s erofs_put_metabuf() at the end to be not effective
leaving the refcount on the page.
As the page from metabuf (buf->page) is never put, such page cannot be
migrated or reclaimed. Fix it now by putting the metabuf from
previous loop and assigning the current metabuf to target before
returning so caller erofs_namei() can do the final put as it was
intended.

Fixes: 500edd095648 ("erofs: use meta buffers for inode lookup")
Cc: stable@vger.kernel.org
Signed-off-by: Sandeep Dhavale <dhavale@google.com>
---
 fs/erofs/namei.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Gao Xiang Feb. 21, 2024, 2:18 a.m. UTC | #1 
Hi Sandeep,

On 2024/2/21 03:11, Sandeep Dhavale wrote:
> In erofs_find_target_block() when erofs_dirnamecmp() returns 0,
> we do not assign the target metabuf. This causes the caller
> erofs_namei()'s erofs_put_metabuf() at the end to be not effective
> leaving the refcount on the page.
> As the page from metabuf (buf->page) is never put, such page cannot be
> migrated or reclaimed. Fix it now by putting the metabuf from
> previous loop and assigning the current metabuf to target before
> returning so caller erofs_namei() can do the final put as it was
> intended.
> 
> Fixes: 500edd095648 ("erofs: use meta buffers for inode lookup")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sandeep Dhavale <dhavale@google.com>

Many thanks for the catch!

> ---
>   fs/erofs/namei.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/erofs/namei.c b/fs/erofs/namei.c
> index d4f631d39f0f..bfe1c926436b 100644
> --- a/fs/erofs/namei.c
> +++ b/fs/erofs/namei.c
> @@ -132,7 +132,10 @@ static void *erofs_find_target_block(struct erofs_buf *target,
>   
>   			if (!diff) {
>   				*_ndirents = 0;
> -				goto out;
> +				if (!IS_ERR(candidate))
> +					erofs_put_metabuf(target);
> +				*target = buf;
> +				return de;
>   			} else if (diff > 0) {
>   				head = mid + 1;
>   				startprfx = matched;

The fix is correct, yet I tend to try to reorganize this snippet for
simplicity, how about the following diff (untested)?

If it looks good to you, could you resend a formal patch? Thanks!

Thanks,
Gao Xiang

  fs/erofs/namei.c | 29 ++++++++++++++---------------
  1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/fs/erofs/namei.c b/fs/erofs/namei.c
index d4f631d39f0f..9fb2d627578e 100644
--- a/fs/erofs/namei.c
+++ b/fs/erofs/namei.c
@@ -129,25 +129,24 @@ static void *erofs_find_target_block(struct erofs_buf *target,

  			/* string comparison without already matched prefix */
  			diff = erofs_dirnamecmp(name, &dname, &matched);
-
-			if (!diff) {
-				*_ndirents = 0;
-				goto out;
-			} else if (diff > 0) {
-				head = mid + 1;
-				startprfx = matched;
-
-				if (!IS_ERR(candidate))
-					erofs_put_metabuf(target);
-				*target = buf;
-				candidate = de;
-				*_ndirents = ndirents;
-			} else {
+			if (diff < 0) {
  				erofs_put_metabuf(&buf);
-
  				back = mid - 1;
  				endprfx = matched;
+				continue;
  			}
+
+			if (!IS_ERR(candidate))
+				erofs_put_metabuf(target);
+			*target = buf;
+			if (!diff) {
+				*_ndirents = 0;
+				return de;
+			}
+			head = mid + 1;
+			startprfx = matched;
+			candidate = de;
+			*_ndirents = ndirents;
  			continue;
  		}
  out:		/* free if the candidate is valid */
--
2.39.3
Sandeep Dhavale Feb. 21, 2024, 7:07 a.m. UTC | #2 
>
> If it looks good to you, could you resend a formal patch? Thanks!
>
Hi Gao,
This looks better and more readable. I will send a v2.

Thanks,
Sandeep.

> Thanks,
> Gao Xiang
diff mbox series

Patch

diff --git a/fs/erofs/namei.c b/fs/erofs/namei.c
index d4f631d39f0f..bfe1c926436b 100644
--- a/fs/erofs/namei.c
+++ b/fs/erofs/namei.c
@@ -132,7 +132,10 @@  static void *erofs_find_target_block(struct erofs_buf *target,
 
 			if (!diff) {
 				*_ndirents = 0;
-				goto out;
+				if (!IS_ERR(candidate))
+					erofs_put_metabuf(target);
+				*target = buf;
+				return de;
 			} else if (diff > 0) {
 				head = mid + 1;
 				startprfx = matched;