Message-ID: <20110401140856.GA5311@quack.suse.cz>
Date:	Fri, 1 Apr 2011 16:08:56 +0200
From:	Jan Kara <jack@...e.cz>
To:	Dave Chinner <david@...morbit.com>
Cc:	Toshiyuki Okajima <toshi.okajima@...fujitsu.com>,
	Jan Kara <jack@...e.cz>, Ted Ts'o <tytso@....edu>,
	Masayoshi MIZUMA <m.mizuma@...fujitsu.com>,
	Andreas Dilger <adilger.kernel@...ger.ca>,
	linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [RFC][PATCH] Re: [BUG] ext4: cannot unfreeze a filesystem due
 to a deadlock

On Fri 01-04-11 10:40:50, Dave Chinner wrote:
> On Mon, Mar 28, 2011 at 05:06:28PM +0900, Toshiyuki Okajima wrote:
> > On Thu, 17 Feb 2011 11:45:52 +0100
> > Jan Kara <jack@...e.cz> wrote:
> > > On Thu 17-02-11 12:50:51, Toshiyuki Okajima wrote:
> > > > (2011/02/16 23:56), Jan Kara wrote:
> > > > >On Wed 16-02-11 08:17:46, Toshiyuki Okajima wrote:
> > > > >>On Tue, 15 Feb 2011 18:29:54 +0100
> > > > >>Jan Kara<jack@...e.cz>  wrote:
> > > > >>>On Tue 15-02-11 12:03:52, Ted Ts'o wrote:
> > > > >>>>On Tue, Feb 15, 2011 at 05:06:30PM +0100, Jan Kara wrote:
> > > > >>>>>Thanks for detailed analysis. Indeed this is a bug. Whenever we do IO
> > > > >>>>>under s_umount semaphore, we are prone to deadlock like the one you
> > > > >>>>>describe above.
> > > > >>>>
> > > > >>>>One of the fundamental problems here is that the freeze and thaw
> > > > >>>>routines are using down_write(&sb->s_umount) for two purposes.  The
> > > > >>>>first is to prevent the resume/thaw from racing with a umount (which
> > > > >>>>it could do just as well by taking a read lock), but the second is to
> > > > >>>>prevent the resume/thaw code from racing with itself.  That's the core
> > > > >>>>fundamental problem here.
> > > > >>>>
> > > > >>>>So I think we can solve this by introduce a new mutex, s_freeze, and
> > > > >>>>having the the resume/thaw first take the s_freeze mutex and then
> > > > >>>>second take a read lock on the s_umount.
> > > > >>>   Sadly this does not quite work because even down_read(&sb->s_umount)
> > > > >>>in thaw_super() can block if there is another process that tries to acquire
> > > > >>>s_umount for writing - a situation like:
> > > > >>>   TASK 1 (e.g. flusher)		TASK 2	(e.g. remount)		TASK 3 (unfreeze)
> > > > >>>down_read(&sb->s_umount)
> > > > >>>   block on s_frozen
> > > > >>>				down_write(&sb->s_umount)
> > > > >>>				  -blocked
> > > > >>>								down_read(&sb->s_umount)
> > > > >>>								  -blocked
> > > > >>>behind the write access...
> > > > >>>
> > > > >>>The only working solution I see is to check for frozen filesystem before
> > > > >>>taking s_umount semaphore which seems rather ugly (but might be bearable if
> > > > >>>we did so in some well described wrapper).
> > > > >>I created the patch that you imagine yesterday.
> > > > >>
> > > > >>I got a reproducer from Mizuma-san yesterday, and then I executed it on the kernel
> > > > >>without a fixed patch. After an hour, I confirmed that this deadlock happened.
> > > > >>
> > > > >>However, on the kernel with a fixed patch, this deadlock doesn't still happen
> > > > >>after 12 hours passed.
> > > > >>
> > > > >>The patch for linux-2.6.38-rc4 is as follows:
> > > > >>---
> > > > >>  fs/fs-writeback.c |    2 +-
> > > > >>  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > >>
> > > > >>diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > > > >>index 59c6e49..1c9a05e 100644
> > > > >>--- a/fs/fs-writeback.c
> > > > >>+++ b/fs/fs-writeback.c
> > > > >>@@ -456,7 +456,7 @@ static bool pin_sb_for_writeback(struct super_block *sb)
> > > > >>         spin_unlock(&sb_lock);
> > > > >>
> > > > >>         if (down_read_trylock(&sb->s_umount)) {
> > > > >>-               if (sb->s_root)
> > > > >>+               if (sb->s_frozen == SB_UNFROZEN&&  sb->s_root)
> > > > >>                         return true;
> > > > >>                 up_read(&sb->s_umount);
> > > > 
> > > > >   So this is something along the lines I thought but it actually won't work
> > > > >for example if sync(1) is run while the filesystem is frozen (that takes
> > > > >s_umount semaphore in a different place). And generally, I'm not convinced
> > > > >there are not other places that try to do IO while holding s_umount
> > > > >semaphore...
> > > > OK. I understand.
> > > > 
> > > > This code only fixes the case for the following path:
> > > > writeback_inodes_wb
> > > > -> ext4_da_writepages
> > > >    -> ext4_journal_start_sb
> > > >       -> vfs_check_frozen
> > > > But, the code doesn't fix the other cases.
> > > > 
> > > > We must modify the local filesystem part in order to fix all cases...?
> > >   Yes, possibly. But most importantly we should first find clear locking
> > > rules for frozen filesystem that avoid deadlocks like the one above. And
> > > the freezing / unfreezing code might become subtle for that reason, that's
> > > fine, but it would be really good to avoid any complicated things for the
> > > code in the rest of the VFS / filesystems.
> > I have deeply continued to examined the root cause of this problem, then 
> > I found it.
> > 
> > It is that we can write a memory which is mmaped to a file. Then the memory 
> > becomes "DIRTY" so then the flusher thread (ex. wb_do_writeback) tries to
> > "writeback" the memory. 
> 
> Then surely the issue is that .page_mkwrite is not checking that the
> filesystem is frozen before allowing the page fault to continue and
> dirty the page?
  And is this a bug? That isn't clear to me...

> > I think the best approach to fix this problem is to let users not to write
> > memory which is mapped to a certain file while the filesystem is freezing. 
> > However, it is very difficult to control users not to write memory which has 
> > been already mapped to the file.
> 
> If you don't allow the page to be dirtied in the fist place, then
> nothing needs to be done to the writeback path because there is
> nothing dirty for it to write back.
  Sure but that's only the problem he was able to hit. But generally,
there's a problem with needing s_umount for unfreezing because it isn't
clear there aren't other code paths which can block with s_umount held
waiting for fs to get unfrozen. And these code paths would cause the same
deadlock. That's why I chose to get rid of s_umount during thawing.

									Honza
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists