| Message ID | 20240424033409.2735257-6-libaokun@huaweicloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cachefiles: some bugfixes for clean object/send req/poll | expand |
Hi Baokun, On 2024/4/24 11:34, libaokun@huaweicloud.com wrote: > From: Jingbo Xu <jefflexu@linux.alibaba.com> > > Add missing lock protection in poll routine when iterating xarray, > otherwise: > > Even with RCU read lock held, only the slot of the radix tree is > ensured to be pinned there, while the data structure (e.g. struct > cachefiles_req) stored in the slot has no such guarantee. The poll > routine will iterate the radix tree and dereference cachefiles_req > accordingly. Thus RCU read lock is not adequate in this case and > spinlock is needed here. > > Fixes: b817e22b2e91 ("cachefiles: narrow the scope of triggering EPOLLIN events in ondemand mode") > Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com> > Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com> > Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com> I'm not sure why this patch didn't send upstream, https://gitee.com/anolis/cloud-kernel/commit/324ecaaa10fefb0e3d94b547e3170e40b90cda1f But since we're now working on upstreaming, so let's drop the previous in-house review tags.. Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com> Thanks, Gao Xiang > Signed-off-by: Baokun Li <libaokun1@huawei.com> > --- > fs/cachefiles/daemon.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c > index 6465e2574230..73ed2323282a 100644 > --- a/fs/cachefiles/daemon.c > +++ b/fs/cachefiles/daemon.c > @@ -365,14 +365,14 @@ static __poll_t cachefiles_daemon_poll(struct file *file, > > if (cachefiles_in_ondemand_mode(cache)) { > if (!xa_empty(&cache->reqs)) { > - rcu_read_lock(); > + xas_lock(&xas); > xas_for_each_marked(&xas, req, ULONG_MAX, CACHEFILES_REQ_NEW) { > if (!cachefiles_ondemand_is_reopening_read(req)) { > mask |= EPOLLIN; > break; > } > } > - rcu_read_unlock(); > + xas_unlock(&xas); > } > } else { > if (test_bit(CACHEFILES_STATE_CHANGED, &cache->flags))
在 2024/4/24 11:34, libaokun@huaweicloud.com 写道: > From: Jingbo Xu <jefflexu@linux.alibaba.com> > > Add missing lock protection in poll routine when iterating xarray, > otherwise: > > Even with RCU read lock held, only the slot of the radix tree is > ensured to be pinned there, while the data structure (e.g. struct > cachefiles_req) stored in the slot has no such guarantee. The poll > routine will iterate the radix tree and dereference cachefiles_req > accordingly. Thus RCU read lock is not adequate in this case and > spinlock is needed here. > > Fixes: b817e22b2e91 ("cachefiles: narrow the scope of triggering EPOLLIN events in ondemand mode") > Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com> > Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com> > Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com> > Signed-off-by: Baokun Li <libaokun1@huawei.com> Thanks for catching this. Reviewed-by: Jia Zhu <zhujia.zj@bytedance.com> > --- > fs/cachefiles/daemon.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c > index 6465e2574230..73ed2323282a 100644 > --- a/fs/cachefiles/daemon.c > +++ b/fs/cachefiles/daemon.c > @@ -365,14 +365,14 @@ static __poll_t cachefiles_daemon_poll(struct file *file, > > if (cachefiles_in_ondemand_mode(cache)) { > if (!xa_empty(&cache->reqs)) { > - rcu_read_lock(); > + xas_lock(&xas); > xas_for_each_marked(&xas, req, ULONG_MAX, CACHEFILES_REQ_NEW) { > if (!cachefiles_ondemand_is_reopening_read(req)) { > mask |= EPOLLIN; > break; > } > } > - rcu_read_unlock(); > + xas_unlock(&xas); > } > } else { > if (test_bit(CACHEFILES_STATE_CHANGED, &cache->flags))
Hi Xiang, On 2024/4/24 12:29, Gao Xiang wrote: > Hi Baokun, > > On 2024/4/24 11:34, libaokun@huaweicloud.com wrote: >> From: Jingbo Xu <jefflexu@linux.alibaba.com> >> >> Add missing lock protection in poll routine when iterating xarray, >> otherwise: >> >> Even with RCU read lock held, only the slot of the radix tree is >> ensured to be pinned there, while the data structure (e.g. struct >> cachefiles_req) stored in the slot has no such guarantee. The poll >> routine will iterate the radix tree and dereference cachefiles_req >> accordingly. Thus RCU read lock is not adequate in this case and >> spinlock is needed here. >> >> Fixes: b817e22b2e91 ("cachefiles: narrow the scope of triggering >> EPOLLIN events in ondemand mode") >> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com> >> Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com> >> Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com> > > I'm not sure why this patch didn't send upstream, > https://gitee.com/anolis/cloud-kernel/commit/324ecaaa10fefb0e3d94b547e3170e40b90cda1f > > Yes, this issue blocks our tests, so this commit is adapted to upstream here. > But since we're now working on upstreaming, so let's drop > the previous in-house review tags.. > > Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com> > > Thanks, > Gao Xiang Ok, thanks for the review! Cheers, Baokun > >> Signed-off-by: Baokun Li <libaokun1@huawei.com> >> --- >> fs/cachefiles/daemon.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c >> index 6465e2574230..73ed2323282a 100644 >> --- a/fs/cachefiles/daemon.c >> +++ b/fs/cachefiles/daemon.c >> @@ -365,14 +365,14 @@ static __poll_t cachefiles_daemon_poll(struct >> file *file, >> if (cachefiles_in_ondemand_mode(cache)) { >> if (!xa_empty(&cache->reqs)) { >> - rcu_read_lock(); >> + xas_lock(&xas); >> xas_for_each_marked(&xas, req, ULONG_MAX, >> CACHEFILES_REQ_NEW) { >> if (!cachefiles_ondemand_is_reopening_read(req)) { >> mask |= EPOLLIN; >> break; >> } >> } >> - rcu_read_unlock(); >> + xas_unlock(&xas); >> } >> } else { >> if (test_bit(CACHEFILES_STATE_CHANGED, &cache->flags))
diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c index 6465e2574230..73ed2323282a 100644 --- a/fs/cachefiles/daemon.c +++ b/fs/cachefiles/daemon.c @@ -365,14 +365,14 @@ static __poll_t cachefiles_daemon_poll(struct file *file, if (cachefiles_in_ondemand_mode(cache)) { if (!xa_empty(&cache->reqs)) { - rcu_read_lock(); + xas_lock(&xas); xas_for_each_marked(&xas, req, ULONG_MAX, CACHEFILES_REQ_NEW) { if (!cachefiles_ondemand_is_reopening_read(req)) { mask |= EPOLLIN; break; } } - rcu_read_unlock(); + xas_unlock(&xas); } } else { if (test_bit(CACHEFILES_STATE_CHANGED, &cache->flags))