On Wed 16-02-11 22:13:53, Andreas Dilger wrote:
> On 2011-02-16, at 20:50, Toshiyuki Okajima wrote:
> > (2011/02/16 23:56), Jan Kara wrote:
> >>
> >>> 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);
>
> This seems like a very low-risk fix.
>
> >> 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...?
>
> It seems worthwhile to implement the low-risk fix that covers the common
> case, and if/when someone hits the rare 3-process case and/or submits a
> patch for it then that one will be fixed also.
Yes, the fix is simple enough that I won't oppose it getting in as a
band aid and if we add this band aid to fs/sync.c:sync_one_sb(), it would
even be a reasonably reliable band aid. But that doesn't change the fact
that the locking is simply broken ;).
Honza
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
