2024-02-21 21:04:04

by Sandeep Dhavale

Subject: [PATCH v2] erofs: fix refcount on the metabuf used for inode lookup

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: [email protected]
Signed-off-by: Sandeep Dhavale <[email protected]>
---
Changes since v1
- Rearrange the cases as suggested by Gao so there is less duplication
of the code and it is more readable

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

diff --git a/fs/erofs/namei.c b/fs/erofs/namei.c
index d4f631d39f0f..f0110a78acb2 100644
--- a/fs/erofs/namei.c
+++ b/fs/erofs/namei.c
@@ -130,24 +130,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.44.0.rc0.258.g7320e95886-goog

2024-02-22 00:43:02

by Gao Xiang

Subject: Re: [PATCH v2] erofs: fix refcount on the metabuf used for inode lookup

On 2024/2/22 05:03, 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: [email protected]
> Signed-off-by: Sandeep Dhavale <[email protected]>

Reviewed-by: Gao Xiang <[email protected]>

Thanks,
Gao Xiang

2024-02-22 03:19:56

by Jingbo Xu

Subject: Re: [PATCH v2] erofs: fix refcount on the metabuf used for inode lookup

On 2/22/24 5:03 AM, 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: [email protected]
> Signed-off-by: Sandeep Dhavale <[email protected]>


LGTM.

Reviewed-by: Jingbo Xu <[email protected]>


> ---
> Changes since v1
> - Rearrange the cases as suggested by Gao so there is less duplication
> of the code and it is more readable
>
> fs/erofs/namei.c | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/fs/erofs/namei.c b/fs/erofs/namei.c
> index d4f631d39f0f..f0110a78acb2 100644
> --- a/fs/erofs/namei.c
> +++ b/fs/erofs/namei.c
> @@ -130,24 +130,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 */

--
Thanks,
Jingbo

2024-02-22 07:36:03

by Chao Yu

Subject: Re: [PATCH v2] erofs: fix refcount on the metabuf used for inode lookup

On 2024/2/22 5:03, 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: [email protected]
> Signed-off-by: Sandeep Dhavale <[email protected]>

Reviewed-by: Chao Yu <[email protected]>

Thanks,