All of lore.kernel.org
 help / color / mirror / Atom feed
* [kbuild] [mmotm:master 57/427] fs/ocfs2/journal.c:2204:9: sparse: context imbalance in 'ocfs2_recover_orphans' - different lock contexts for basic block
@ 2014-09-26 14:36 Dan Carpenter
  2014-09-29 21:08 ` Andrew Morton
  0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2014-09-26 14:36 UTC (permalink / raw)
  To: kbuild, WeiWei Wang
  Cc: Johannes Weiner, Andrew Morton, Linux Memory Management List

tree:   git://git.cmpxchg.org/linux-mmotm.git master
head:   065f1d86a58cc88249cd8371b29a57c97483753a
commit: 8a09937cacc099da21313223443237cbc84d5876 [57/427] ocfs2: add orphan recovery types in ocfs2_recover_orphans
reproduce:
  # apt-get install sparse
  git checkout 8a09937cacc099da21313223443237cbc84d5876
  make ARCH=x86_64 allmodconfig
  make C=1 CF=-D__CHECK_ENDIAN__

   fs/ocfs2/journal.c:111:6: sparse: symbol 'ocfs2_replay_map_set_state' was not declared. Should it be static?
   fs/ocfs2/journal.c:156:6: sparse: symbol 'ocfs2_queue_replay_slots' was not declared. Should it be static?
   fs/ocfs2/journal.c:176:6: sparse: symbol 'ocfs2_free_replay_slots' was not declared. Should it be static?
   fs/ocfs2/journal.c:1888:6: sparse: symbol 'ocfs2_queue_orphan_scan' was not declared. Should it be static?
   fs/ocfs2/journal.c:1937:6: sparse: symbol 'ocfs2_orphan_scan_work' was not declared. Should it be static?
>> fs/ocfs2/journal.c:2204:9: sparse: context imbalance in 'ocfs2_recover_orphans' - different lock contexts for basic block

git remote add mmotm git://git.cmpxchg.org/linux-mmotm.git
git remote update mmotm
git checkout 8a09937cacc099da21313223443237cbc84d5876
vim +/ocfs2_recover_orphans +2204 fs/ocfs2/journal.c

8a09937c WeiWei Wang 2014-09-26  2188  		 * if the orphan scan work, continue to process the
8a09937c WeiWei Wang 2014-09-26  2189  		 * next orphan.
8a09937c WeiWei Wang 2014-09-26  2190  		 */
8a09937c WeiWei Wang 2014-09-26  2191  		else if (orphan_reco_type == ORPHAN_SCAN_WORK) {
8a09937c WeiWei Wang 2014-09-26  2192  			spin_unlock(&oi->ip_lock);
8a09937c WeiWei Wang 2014-09-26  2193  			inode = iter;
8a09937c WeiWei Wang 2014-09-26  2194  			continue;
8a09937c WeiWei Wang 2014-09-26  2195  		}
ccd979bd Mark Fasheh 2005-12-15  2196  		spin_unlock(&oi->ip_lock);
ccd979bd Mark Fasheh 2005-12-15  2197  
ccd979bd Mark Fasheh 2005-12-15  2198  		iput(inode);
ccd979bd Mark Fasheh 2005-12-15  2199  
ccd979bd Mark Fasheh 2005-12-15  2200  		inode = iter;
ccd979bd Mark Fasheh 2005-12-15  2201  	}
ccd979bd Mark Fasheh 2005-12-15  2202  
8a09937c WeiWei Wang 2014-09-26  2203  out:

Sparse error messages are hard to understand.  It's saying that there is
a missing unlock if ocfs2_start_trans() fails and we "goto out;"

"out" labels are the worst btw.  The name is too vague.  Sometimes they
do something and sometimes they don't do anything but the name doesn't
give any clue what it does.  In theory, out labels future proof the code
from locking bugs but they don't work.  It just means you have to jump
around like a rabbit to follow all the goto paths.  A return statement
is easier to understand.

b4df6ed8 Mark Fasheh 2006-02-22 @2204  	return ret;
ccd979bd Mark Fasheh 2005-12-15  2205  }
ccd979bd Mark Fasheh 2005-12-15  2206  
19ece546 Jan Kara    2008-08-21  2207  static int __ocfs2_wait_on_mount(struct ocfs2_super *osb, int quota)
ccd979bd Mark Fasheh 2005-12-15  2208  {
ccd979bd Mark Fasheh 2005-12-15  2209  	/* This check is good because ocfs2 will wait on our recovery
ccd979bd Mark Fasheh 2005-12-15  2210  	 * thread before changing it to something other than MOUNTED
ccd979bd Mark Fasheh 2005-12-15  2211  	 * or DISABLED. */
ccd979bd Mark Fasheh 2005-12-15  2212  	wait_event(osb->osb_mount_event,

---
0-DAY kernel build testing backend              Open Source Technology Center
http://lists.01.org/mailman/listinfo/kbuild                 Intel Corporation
_______________________________________________
kbuild mailing list
kbuild@lists.01.org
https://lists.01.org/mailman/listinfo/kbuild

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [kbuild] [mmotm:master 57/427] fs/ocfs2/journal.c:2204:9: sparse: context imbalance in 'ocfs2_recover_orphans' - different lock contexts for basic block
  2014-09-26 14:36 [kbuild] [mmotm:master 57/427] fs/ocfs2/journal.c:2204:9: sparse: context imbalance in 'ocfs2_recover_orphans' - different lock contexts for basic block Dan Carpenter
@ 2014-09-29 21:08 ` Andrew Morton
  2014-09-30  1:46   ` Joseph Qi
  0 siblings, 1 reply; 3+ messages in thread
From: Andrew Morton @ 2014-09-29 21:08 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: kbuild, WeiWei Wang, Johannes Weiner, Linux Memory Management List

On Fri, 26 Sep 2014 17:36:37 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:

> commit: 8a09937cacc099da21313223443237cbc84d5876 [57/427] ocfs2: add orphan recovery types in ocfs2_recover_orphans
>
> ...
>
> >> fs/ocfs2/journal.c:2204:9: sparse: context imbalance in 'ocfs2_recover_orphans' - different lock contexts for basic block
> 

this?

--- a/fs/ocfs2/journal.c~ocfs2-add-orphan-recovery-types-in-ocfs2_recover_orphans-fix
+++ a/fs/ocfs2/journal.c
@@ -2160,8 +2160,7 @@ static int ocfs2_recover_orphans(struct
 			ret = ocfs2_inode_lock(inode, &di_bh, 1);
 			if (ret) {
 				mlog_errno(ret);
-				spin_unlock(&oi->ip_lock);
-				goto out;
+				goto out_unlock;
 			}
 			ocfs2_truncate_file(inode, di_bh, i_size_read(inode));
 			ocfs2_inode_unlock(inode, 1);
@@ -2173,14 +2172,13 @@ static int ocfs2_recover_orphans(struct
 					OCFS2_INODE_DEL_FROM_ORPHAN_CREDITS);
 			if (IS_ERR(handle)) {
 				ret = PTR_ERR(handle);
-				goto out;
+				goto out_unlock;
 			}
 			ret = ocfs2_del_inode_from_orphan(osb, handle, inode);
 			if (ret) {
 				mlog_errno(ret);
 				ocfs2_commit_trans(osb, handle);
-				spin_unlock(&oi->ip_lock);
-				goto out;
+				goto out_unlock;
 			}
 			ocfs2_commit_trans(osb, handle);
 		}
@@ -2200,7 +2198,10 @@ static int ocfs2_recover_orphans(struct
 		inode = iter;
 	}
 
-out:
+	return ret;
+
+out_unlock:
+	spin_unlock(&oi->ip_lock);
 	return ret;
 }
 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [kbuild] [mmotm:master 57/427] fs/ocfs2/journal.c:2204:9: sparse: context imbalance in 'ocfs2_recover_orphans' - different lock contexts for basic block
  2014-09-29 21:08 ` Andrew Morton
@ 2014-09-30  1:46   ` Joseph Qi
  0 siblings, 0 replies; 3+ messages in thread
From: Joseph Qi @ 2014-09-30  1:46 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Dan Carpenter, kbuild, WeiWei Wang, Johannes Weiner,
	Linux Memory Management List

On 2014/9/30 5:08, Andrew Morton wrote:
> On Fri, 26 Sep 2014 17:36:37 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
>> commit: 8a09937cacc099da21313223443237cbc84d5876 [57/427] ocfs2: add orphan recovery types in ocfs2_recover_orphans
>>
>> ...
>>
>>>> fs/ocfs2/journal.c:2204:9: sparse: context imbalance in 'ocfs2_recover_orphans' - different lock contexts for basic block
>>
> 
> this?

I think there is another deadlock case in ocfs2_recover_orphans.

*spin_lock(&oi->ip_lock)*
	ocfs2_inode_lock
		ocfs2_inode_lock_full_nested
			ocfs2_inode_lock_update
				*spin_lock(&oi->ip_lock)*

Since ip_lock only wants to protect ip_flags and the added new logic
ocfs2_del_inode_from_orphan has nothing to do with it, distinguish
them.

> 
> --- a/fs/ocfs2/journal.c~ocfs2-add-orphan-recovery-types-in-ocfs2_recover_orphans-fix
> +++ a/fs/ocfs2/journal.c
> @@ -2160,8 +2160,7 @@ static int ocfs2_recover_orphans(struct
>  			ret = ocfs2_inode_lock(inode, &di_bh, 1);
>  			if (ret) {
>  				mlog_errno(ret);
> -				spin_unlock(&oi->ip_lock);
> -				goto out;
> +				goto out_unlock;
>  			}
>  			ocfs2_truncate_file(inode, di_bh, i_size_read(inode));
>  			ocfs2_inode_unlock(inode, 1);
> @@ -2173,14 +2172,13 @@ static int ocfs2_recover_orphans(struct
>  					OCFS2_INODE_DEL_FROM_ORPHAN_CREDITS);
>  			if (IS_ERR(handle)) {
>  				ret = PTR_ERR(handle);
> -				goto out;
> +				goto out_unlock;
>  			}
>  			ret = ocfs2_del_inode_from_orphan(osb, handle, inode);
>  			if (ret) {
>  				mlog_errno(ret);
>  				ocfs2_commit_trans(osb, handle);
> -				spin_unlock(&oi->ip_lock);
> -				goto out;
> +				goto out_unlock;
>  			}
>  			ocfs2_commit_trans(osb, handle);
>  		}
> @@ -2200,7 +2198,10 @@ static int ocfs2_recover_orphans(struct
>  		inode = iter;
>  	}
>  
> -out:
> +	return ret;
> +
> +out_unlock:
> +	spin_unlock(&oi->ip_lock);
>  	return ret;
>  }
>  
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 
> 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-09-30  1:47 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-26 14:36 [kbuild] [mmotm:master 57/427] fs/ocfs2/journal.c:2204:9: sparse: context imbalance in 'ocfs2_recover_orphans' - different lock contexts for basic block Dan Carpenter
2014-09-29 21:08 ` Andrew Morton
2014-09-30  1:46   ` Joseph Qi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.