From: Madhuparna Bhowmik <[email protected]>
The space_info list is rcu protected.
Hence, it should be traversed with rcu_read_lock held.
Warning:
[ 29.104591] =============================
[ 29.104756] WARNING: suspicious RCU usage
[ 29.105046] 5.6.0-rc4-next-20200305 #1 Not tainted
[ 29.105231] -----------------------------
[ 29.105401] fs/btrfs/block-group.c:2011 RCU-list traversed in non-reader section!!
Reported-by: Guenter Roeck <[email protected]>
Signed-off-by: Madhuparna Bhowmik <[email protected]>
---
fs/btrfs/block-group.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 404e050ce8ee..9cabeef66f5b 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1987,6 +1987,7 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
btrfs_release_path(path);
}
+ rcu_read_lock();
list_for_each_entry_rcu(space_info, &info->space_info, list) {
if (!(btrfs_get_alloc_profile(info, space_info->flags) &
(BTRFS_BLOCK_GROUP_RAID10 |
@@ -2007,7 +2008,8 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
list)
inc_block_group_ro(cache, 1);
}
-
+ rcu_read_unlock();
+
btrfs_init_global_block_rsv(info);
ret = check_chunk_block_group_mappings(info);
error:
--
2.17.1
On 2020/3/6 下午2:52, [email protected] wrote:
> From: Madhuparna Bhowmik <[email protected]>
>
> The space_info list is rcu protected.
> Hence, it should be traversed with rcu_read_lock held.
>
> Warning:
> [ 29.104591] =============================
> [ 29.104756] WARNING: suspicious RCU usage
> [ 29.105046] 5.6.0-rc4-next-20200305 #1 Not tainted
> [ 29.105231] -----------------------------
> [ 29.105401] fs/btrfs/block-group.c:2011 RCU-list traversed in non-reader section!!
>
> Reported-by: Guenter Roeck <[email protected]>
> Signed-off-by: Madhuparna Bhowmik <[email protected]>
> ---
> fs/btrfs/block-group.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 404e050ce8ee..9cabeef66f5b 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -1987,6 +1987,7 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
This function is only triggered at mount time, where no other rcu
operation can happen.
Thanks,
Qu
> btrfs_release_path(path);
> }
>
> + rcu_read_lock();
> list_for_each_entry_rcu(space_info, &info->space_info, list) {
> if (!(btrfs_get_alloc_profile(info, space_info->flags) &
> (BTRFS_BLOCK_GROUP_RAID10 |
> @@ -2007,7 +2008,8 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
> list)
> inc_block_group_ro(cache, 1);
> }
> -
> + rcu_read_unlock();
> +
> btrfs_init_global_block_rsv(info);
> ret = check_chunk_block_group_mappings(info);
> error:
>
On Fri, Mar 06, 2020 at 03:16:53PM +0800, Qu Wenruo wrote:
>
>
> On 2020/3/6 下午2:52, [email protected] wrote:
> > From: Madhuparna Bhowmik <[email protected]>
> >
> > The space_info list is rcu protected.
> > Hence, it should be traversed with rcu_read_lock held.
> >
> > Warning:
> > [ 29.104591] =============================
> > [ 29.104756] WARNING: suspicious RCU usage
> > [ 29.105046] 5.6.0-rc4-next-20200305 #1 Not tainted
> > [ 29.105231] -----------------------------
> > [ 29.105401] fs/btrfs/block-group.c:2011 RCU-list traversed in non-reader section!!
> >
> > Reported-by: Guenter Roeck <[email protected]>
> > Signed-off-by: Madhuparna Bhowmik <[email protected]>
> > ---
> > fs/btrfs/block-group.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> > index 404e050ce8ee..9cabeef66f5b 100644
> > --- a/fs/btrfs/block-group.c
> > +++ b/fs/btrfs/block-group.c
> > @@ -1987,6 +1987,7 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
>
> This function is only triggered at mount time, where no other rcu
> operation can happen.
>
Thanks Qu.
Joel and Paul, what should we do in this case?
Should we just pass cond = true or use list_for_each_entry instead?
Thank you,
Madhuparna
> Thanks,
> Qu
> > btrfs_release_path(path);
> > }
> >
> > + rcu_read_lock();
> > list_for_each_entry_rcu(space_info, &info->space_info, list) {
> > if (!(btrfs_get_alloc_profile(info, space_info->flags) &
> > (BTRFS_BLOCK_GROUP_RAID10 |
> > @@ -2007,7 +2008,8 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
> > list)
> > inc_block_group_ro(cache, 1);
> > }
> > -
> > + rcu_read_unlock();
> > +
> > btrfs_init_global_block_rsv(info);
> > ret = check_chunk_block_group_mappings(info);
> > error:
> >
>
On Fri, Mar 06, 2020 at 07:30:24PM +0530, Madhuparna Bhowmik wrote:
> On Fri, Mar 06, 2020 at 03:16:53PM +0800, Qu Wenruo wrote:
> >
> >
> > On 2020/3/6 下午2:52, [email protected] wrote:
> > > From: Madhuparna Bhowmik <[email protected]>
> > >
> > > The space_info list is rcu protected.
> > > Hence, it should be traversed with rcu_read_lock held.
> > >
> > > Warning:
> > > [ 29.104591] =============================
> > > [ 29.104756] WARNING: suspicious RCU usage
> > > [ 29.105046] 5.6.0-rc4-next-20200305 #1 Not tainted
> > > [ 29.105231] -----------------------------
> > > [ 29.105401] fs/btrfs/block-group.c:2011 RCU-list traversed in non-reader section!!
> > >
> > > Reported-by: Guenter Roeck <[email protected]>
> > > Signed-off-by: Madhuparna Bhowmik <[email protected]>
> > > ---
> > > fs/btrfs/block-group.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> > > index 404e050ce8ee..9cabeef66f5b 100644
> > > --- a/fs/btrfs/block-group.c
> > > +++ b/fs/btrfs/block-group.c
> > > @@ -1987,6 +1987,7 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
> >
> > This function is only triggered at mount time, where no other rcu
> > operation can happen.
> >
> Thanks Qu.
>
> Joel and Paul, what should we do in this case?
> Should we just pass cond = true or use list_for_each_entry instead?
I think we can afford to add rcu lock/unlock, even if it's not strictly
necessary due to the single threaded context where the function is run.
There are some lightweight operations inside and inc_block_group starts
with two spin locks so there's nothing we'd be losing with disabled
preemption from the caller.
On Fri, Mar 06, 2020 at 12:22:43PM +0530, [email protected] wrote:
> From: Madhuparna Bhowmik <[email protected]>
>
> The space_info list is rcu protected.
> Hence, it should be traversed with rcu_read_lock held.
>
> Warning:
> [ 29.104591] =============================
> [ 29.104756] WARNING: suspicious RCU usage
> [ 29.105046] 5.6.0-rc4-next-20200305 #1 Not tainted
> [ 29.105231] -----------------------------
> [ 29.105401] fs/btrfs/block-group.c:2011 RCU-list traversed in non-reader section!!
>
> Reported-by: Guenter Roeck <[email protected]>
> Signed-off-by: Madhuparna Bhowmik <[email protected]>
I've updated the changelog based on comments from the mail thread and
applied the patch. Thanks.
On Fri, Mar 06, 2020 at 04:25:27PM +0100, David Sterba wrote:
> On Fri, Mar 06, 2020 at 07:30:24PM +0530, Madhuparna Bhowmik wrote:
> > On Fri, Mar 06, 2020 at 03:16:53PM +0800, Qu Wenruo wrote:
> > >
> > >
> > > On 2020/3/6 下午2:52, [email protected] wrote:
> > > > From: Madhuparna Bhowmik <[email protected]>
> > > >
> > > > The space_info list is rcu protected.
> > > > Hence, it should be traversed with rcu_read_lock held.
> > > >
> > > > Warning:
> > > > [ 29.104591] =============================
> > > > [ 29.104756] WARNING: suspicious RCU usage
> > > > [ 29.105046] 5.6.0-rc4-next-20200305 #1 Not tainted
> > > > [ 29.105231] -----------------------------
> > > > [ 29.105401] fs/btrfs/block-group.c:2011 RCU-list traversed in non-reader section!!
> > > >
> > > > Reported-by: Guenter Roeck <[email protected]>
> > > > Signed-off-by: Madhuparna Bhowmik <[email protected]>
> > > > ---
> > > > fs/btrfs/block-group.c | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> > > > index 404e050ce8ee..9cabeef66f5b 100644
> > > > --- a/fs/btrfs/block-group.c
> > > > +++ b/fs/btrfs/block-group.c
> > > > @@ -1987,6 +1987,7 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
> > >
> > > This function is only triggered at mount time, where no other rcu
> > > operation can happen.
> > >
> > Thanks Qu.
> >
> > Joel and Paul, what should we do in this case?
> > Should we just pass cond = true or use list_for_each_entry instead?
>
> I think we can afford to add rcu lock/unlock, even if it's not strictly
> necessary due to the single threaded context where the function is run.
> There are some lightweight operations inside and inc_block_group starts
> with two spin locks so there's nothing we'd be losing with disabled
> preemption from the caller.
I think use list_for_each_entry().
thanks,
- Joel