Skip to contentSkip to conversation

erofs: fix refcount on the metabuf used for inode lookup

PATCH In review Sandeep Dhavale 4 participants 4 messages Feb 21, 2024 – Feb 22, 2024

Subsystems (2)
lore
Share & tools
lore

Sandeep Dhavale

author reviewer
#0 lore raw

PATCH v2 1/1 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")

---
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(-)
fs/erofs/namei.c +14 -14
index d4f631d39f0f..f0110a78acb2 100644
@@ -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 */
Signature · 1 line
2.44.0.rc0.258.g7320e95886-goog
Reviewed-by on v2 1/1
#1 lore raw
Quoted 10 lines · Sandeep Dhavale

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.

Quoted 3 lines
Fixes 500edd095648 ("erofs: use meta buffers for inode lookup")

Thanks,
Gao Xiang

Reviewed-by on v2 1/1
#2 lore raw
In reply to Sandeep Dhavale
Quoted 10 lines · Sandeep Dhavale

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.

Quoted 3 lines
Fixes 500edd095648 ("erofs: use meta buffers for inode lookup")

LGTM.

Quoted 51 lines

---
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(-)
fs/erofs/namei.c +14 -14
index d4f631d39f0f..f0110a78acb2 100644
@@ -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 */
Signature · 2 lines
Thanks,
Jingbo
maintainer Reviewed-by on v2 1/1
#3 lore raw
In reply to Sandeep Dhavale
Quoted 10 lines · Sandeep Dhavale

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.

Quoted 3 lines
Fixes 500edd095648 ("erofs: use meta buffers for inode lookup")

Thanks,