Messages in this thread Patch in this message | | | Date | Wed, 21 Feb 2024 10:18:22 +0800 | Subject | Re: [PATCH v1] erofs: fix refcount on the metabuf used for inode lookup | From | Gao Xiang <> |
| |
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
| |