linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] tmpfs: change final i_blocks BUG to WARNING
@ 2012-11-06  1:34 Hugh Dickins
  2012-11-18  9:16 ` Jaegeuk Hanse
  0 siblings, 1 reply; 2+ messages in thread
From: Hugh Dickins @ 2012-11-06  1:34 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-mm, linux-kernel

Under a particular load on one machine, I have hit shmem_evict_inode()'s
BUG_ON(inode->i_blocks), enough times to narrow it down to a particular
race between swapout and eviction.

It comes from the "if (freed > 0)" asymmetry in shmem_recalc_inode(),
and the lack of coherent locking between mapping's nrpages and shmem's
swapped count.  There's a window in shmem_writepage(), between lowering
nrpages in shmem_delete_from_page_cache() and then raising swapped count,
when the freed count appears to be +1 when it should be 0, and then the
asymmetry stops it from being corrected with -1 before hitting the BUG.

One answer is coherent locking: using tree_lock throughout, without
info->lock; reasonable, but the raw_spin_lock in percpu_counter_add()
on used_blocks makes that messier than expected.  Another answer may be
a further effort to eliminate the weird shmem_recalc_inode() altogether,
but previous attempts at that failed.

So far undecided, but for now change the BUG_ON to WARN_ON:
in usual circumstances it remains a useful consistency check.

Signed-off-by: Hugh Dickins <hughd@google.com>
Cc: stable@vger.kernel.org
---

 mm/shmem.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- 3.7-rc4/mm/shmem.c	2012-10-14 16:16:58.361309122 -0700
+++ linux/mm/shmem.c	2012-11-01 14:31:04.288185742 -0700
@@ -643,7 +643,7 @@ static void shmem_evict_inode(struct ino
 		kfree(info->symlink);
 
 	simple_xattrs_free(&info->xattrs);
-	BUG_ON(inode->i_blocks);
+	WARN_ON(inode->i_blocks);
 	shmem_free_inode(inode->i_sb);
 	clear_inode(inode);
 }

--
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] 2+ messages in thread

* Re: [PATCH] tmpfs: change final i_blocks BUG to WARNING
  2012-11-06  1:34 [PATCH] tmpfs: change final i_blocks BUG to WARNING Hugh Dickins
@ 2012-11-18  9:16 ` Jaegeuk Hanse
  0 siblings, 0 replies; 2+ messages in thread
From: Jaegeuk Hanse @ 2012-11-18  9:16 UTC (permalink / raw)
  To: Hugh Dickins; +Cc: Andrew Morton, linux-mm, linux-kernel

On 11/06/2012 09:34 AM, Hugh Dickins wrote:
> Under a particular load on one machine, I have hit shmem_evict_inode()'s
> BUG_ON(inode->i_blocks), enough times to narrow it down to a particular
> race between swapout and eviction.
> 	
> It comes from the "if (freed > 0)" asymmetry in shmem_recalc_inode(),
> and the lack of coherent locking between mapping's nrpages and shmem's
> swapped count.  There's a window in shmem_writepage(), between lowering
> nrpages in shmem_delete_from_page_cache() and then raising swapped count,
> when the freed count appears to be +1 when it should be 0, and then the
> asymmetry stops it from being corrected with -1 before hitting the BUG.

Hi Hugh,

So if race happen, still have pages swapout after inode and radix tree 
destroied.
What will happen when the pages need be swapin in the scenacio like 
swapoff.

Regards,
Jaegeuk

>
> One answer is coherent locking: using tree_lock throughout, without
> info->lock; reasonable, but the raw_spin_lock in percpu_counter_add()
> on used_blocks makes that messier than expected.  Another answer may be
> a further effort to eliminate the weird shmem_recalc_inode() altogether,
> but previous attempts at that failed.
>
> So far undecided, but for now change the BUG_ON to WARN_ON:
> in usual circumstances it remains a useful consistency check.
>
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Cc: stable@vger.kernel.org
> ---
>
>   mm/shmem.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- 3.7-rc4/mm/shmem.c	2012-10-14 16:16:58.361309122 -0700
> +++ linux/mm/shmem.c	2012-11-01 14:31:04.288185742 -0700
> @@ -643,7 +643,7 @@ static void shmem_evict_inode(struct ino
>   		kfree(info->symlink);
>   
>   	simple_xattrs_free(&info->xattrs);
> -	BUG_ON(inode->i_blocks);
> +	WARN_ON(inode->i_blocks);
>   	shmem_free_inode(inode->i_sb);
>   	clear_inode(inode);
>   }
>
> --
> 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] 2+ messages in thread

end of thread, other threads:[~2012-11-18  9:16 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-06  1:34 [PATCH] tmpfs: change final i_blocks BUG to WARNING Hugh Dickins
2012-11-18  9:16 ` Jaegeuk Hanse

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).