Smatch (http://repo.or.cz/w/smatch.git/) complains about the locking in
ext4_mb_add_n_trim() from fs/ext4/mballoc.c
I think it's meant to be spin_unlock(&tmp_pa->pa_lock); on line 4442.
4438 list_for_each_entry_rcu(tmp_pa, &lg->lg_prealloc_list[order],
4439 pa_inode_list) {
4440 spin_lock(&tmp_pa->pa_lock);
4441 if (tmp_pa->pa_deleted) {
4442 spin_unlock(&pa->pa_lock);
4443 continue;
4444 }
I can send a patch if I'm right or you could just give me a:
Reported-by: Dan Carpenter <[email protected]>
regards,
dan carpenter
Dan Carpenter wrote:
> Smatch (http://repo.or.cz/w/smatch.git/) complains about the locking in
> ext4_mb_add_n_trim() from fs/ext4/mballoc.c
>
> I think it's meant to be spin_unlock(&tmp_pa->pa_lock); on line 4442.
>
> 4438 list_for_each_entry_rcu(tmp_pa, &lg->lg_prealloc_list[order],
> 4439 pa_inode_list) {
> 4440 spin_lock(&tmp_pa->pa_lock);
> 4441 if (tmp_pa->pa_deleted) {
> 4442 spin_unlock(&pa->pa_lock);
> 4443 continue;
> 4444 }
>
> I can send a patch if I'm right or you could just give me a:
> Reported-by: Dan Carpenter <[email protected]>
Seems right to me,
Reviewed-by: Eric Sandeen <[email protected]>
although I wonder why we don't trip over this in spinlock debugging
(seems like it'd lead to a double unlock at times) I wonder if we can
tie this to any other bugs we've seen.
Thanks,
-Eric
On Fri, Mar 27, 2009 at 01:27:04PM +0300, Dan Carpenter wrote:
> Smatch (http://repo.or.cz/w/smatch.git/) complains about the locking in
> ext4_mb_add_n_trim() from fs/ext4/mballoc.c
>
> I think it's meant to be spin_unlock(&tmp_pa->pa_lock); on line 4442.
>
> 4438 list_for_each_entry_rcu(tmp_pa, &lg->lg_prealloc_list[order],
> 4439 pa_inode_list) {
> 4440 spin_lock(&tmp_pa->pa_lock);
> 4441 if (tmp_pa->pa_deleted) {
> 4442 spin_unlock(&pa->pa_lock);
> 4443 continue;
> 4444 }
>
> I can send a patch if I'm right or you could just give me a:
> Reported-by: Dan Carpenter <[email protected]>
>
Good catch. Can you send a proper patch with signed-off-by. You can add
Reviewed-by: Aneesh Kumar K.V <[email protected]>
-aneesh
On Fri, Mar 27, 2009 at 10:05:17PM +0530, Aneesh Kumar K.V wrote:
>
> Good catch. Can you send a proper patch with signed-off-by. You can add
No need, I've already created a proper patch for this problem, and am
adding it to the ext4 patch queue.
Thanks, Dan!!
- Ted
On Fri, Mar 27, 2009 at 09:16:59AM -0500, Eric Sandeen wrote:
>
> although I wonder why we don't trip over this in spinlock debugging
> (seems like it'd lead to a double unlock at times) I wonder if we can
> tie this to any other bugs we've seen.
I was wondering if it could be tied to the "rm -rf" soft lockup hang....
I wonder if vendor kernels (specifically, Ubuntu in this case) disable
spinlock debugging, which is why we wouldn't have seen the double
unlock warning. (Or maybe it happened earlier in the log, and users
didn't notice it).
- Ted
> On Fri, Mar 27, 2009 at 09:16:59AM -0500, Eric Sandeen wrote:
> >
> > although I wonder why we don't trip over this in spinlock debugging
> > (seems like it'd lead to a double unlock at times) I wonder if we can
> > tie this to any other bugs we've seen.
>
> I was wondering if it could be tied to the "rm -rf" soft lockup hang....
>
> I wonder if vendor kernels (specifically, Ubuntu in this case) disable
> spinlock debugging, which is why we wouldn't have seen the double
> unlock warning. (Or maybe it happened earlier in the log, and users
> didn't notice it).
Don't know about Ubuntu but SUSE has definitely turned off spinlock
debugging (I guess because it may cost some performance) and most other
debug stuff.
Honza
--
Jan Kara <[email protected]>
SuSE CR Labs