locking typo in ext4_mb_add_n_trim()

locking typo in ext4_mb_add_n_trim()

[Dan Carpenter]

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 <error27@gmail.com>

regards,
dan carpenter

Re: locking typo in ext4_mb_add_n_trim()

[Eric Sandeen]

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 <error27@gmail.com>

Seems right to me,

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

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

Re: locking typo in ext4_mb_add_n_trim()

[Aneesh Kumar K.V]

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 <error27@gmail.com>
> 

Good catch. Can you send a proper patch with signed-off-by. You can add

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@gmail.com>

-aneesh

Re: locking typo in ext4_mb_add_n_trim()

[Theodore Tso]

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

Re: locking typo in ext4_mb_add_n_trim()

[Theodore Tso]

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

Re: locking typo in ext4_mb_add_n_trim()

[Jan Kara]

> 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 <jack@suse.cz>
SuSE CR Labs