lkml.org 
[lkml]   [2011]   [Aug]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] fs / ext3: Always unlock updates in ext3_freeze()
      Hello,

    On Mon 15-08-11 20:09:13, Rafael J. Wysocki wrote:
    > On Monday, August 15, 2011, Jan Kara wrote:
    > > Hi,
    > > On Thu 11-08-11 23:31:13, Rafael J. Wysocki wrote:
    > > > From: Rafael J. Wysocki <rjw@sisk.pl>
    > > >
    > > > In analogy with ext4 make ext3_freeze() always call
    > > > journal_unlock_updates() to prevent it from leaving a locked mutex
    > > > behind. Accordingly, modify ext3_unfreeze() so that it doesn't
    > > > call journal_unlock_updates() any more.
    > > >
    > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
    > > > ---
    > > >
    > > > Sorry for the duplicate, the previous one was sent too early.
    > > >
    > > > ---
    > > > fs/ext3/super.c | 39 ++++++++++++++++++---------------------
    > > > 1 file changed, 18 insertions(+), 21 deletions(-)
    > > >
    > > > Index: linux/fs/ext3/super.c
    > > > ===================================================================
    > > > --- linux.orig/fs/ext3/super.c
    > > > +++ linux/fs/ext3/super.c
    > > > @@ -2535,30 +2535,28 @@ static int ext3_sync_fs(struct super_blo
    > > > */
    > > > static int ext3_freeze(struct super_block *sb)
    > > > {
    > > > - int error = 0;
    > > > + int error;
    > > > journal_t *journal;
    > > >
    > > > - if (!(sb->s_flags & MS_RDONLY)) {
    > > > - journal = EXT3_SB(sb)->s_journal;
    > > > + if (sb->s_flags & MS_RDONLY)
    > > > + return 0;
    > > >
    > > > - /* Now we set up the journal barrier. */
    > > > - journal_lock_updates(journal);
    > > > + journal = EXT3_SB(sb)->s_journal;
    > > >
    > > > - /*
    > > > - * We don't want to clear needs_recovery flag when we failed
    > > > - * to flush the journal.
    > > > - */
    > > > - error = journal_flush(journal);
    > > > - if (error < 0)
    > > > - goto out;
    > > > -
    > > > - /* Journal blocked and flushed, clear needs_recovery flag. */
    > > > - EXT3_CLEAR_INCOMPAT_FEATURE(sb, EXT3_FEATURE_INCOMPAT_RECOVER);
    > > > - error = ext3_commit_super(sb, EXT3_SB(sb)->s_es, 1);
    > > > - if (error)
    > > > - goto out;
    > > > - }
    > > > - return 0;
    > > > + /* Now we set up the journal barrier. */
    > > > + journal_lock_updates(journal);
    > > > +
    > > > + /*
    > > > + * We don't want to clear needs_recovery flag when we failed
    > > > + * to flush the journal.
    > > > + */
    > > > + error = journal_flush(journal);
    > > > + if (error < 0)
    > > > + goto out;
    > > > +
    > > > + /* Journal blocked and flushed, clear needs_recovery flag. */
    > > > + EXT3_CLEAR_INCOMPAT_FEATURE(sb, EXT3_FEATURE_INCOMPAT_RECOVER);
    > > > + error = ext3_commit_super(sb, EXT3_SB(sb)->s_es, 1);
    > > >
    > > > out:
    > > > journal_unlock_updates(journal);
    > > > @@ -2577,7 +2575,6 @@ static int ext3_unfreeze(struct super_bl
    > > > EXT3_SET_INCOMPAT_FEATURE(sb, EXT3_FEATURE_INCOMPAT_RECOVER);
    > > > ext3_commit_super(sb, EXT3_SB(sb)->s_es, 1);
    > > > unlock_super(sb);
    > > > - journal_unlock_updates(EXT3_SB(sb)->s_journal);
    > > > }
    > > > return 0;
    > > > }
    > > It's not so simple as this. Ext3 relies on the mutex (the one hidden in
    > > journal_lock_updates()) to make sure that new transaction cannot be started
    > > while the filesystem is frozen - that's essentially what makes the
    > > filesystem frozen. So if we want to get rid of the mutex we have to achieve
    > > blocking by something else - ext4 uses vfs_check_frozen() in
    > > ext4_journal_start().
    >
    > I see. Still, freeze_bdev() may be called by user space through a syscall,
    > as far as I can say, so it shouldn't leave the mutex locked.
    Yes, I agree with you. That's an ugliness left over from a long time ago.
    I'll have a look at fixing this...

    > > BTW, filesystem freezing never really worked for mmaped writes under
    > > ext3 - ext3 would have to implement page_mkwrite() callback for that - so
    > > if you want to rely on it for suspending, this will be non-trivial.
    >
    > At this point the purpose of freezing filesystems is basically to
    > prevent XFS from deadlocking with hibernation's memory preallocation.
    > For other filesystems it may or may not make a difference depending on
    > their implementation of freeze/unfreeze_super().
    What's exactly the problem? Memory preallocation enters direct reclaim
    and that deadlocks in the filesystem?

    Honza


    \
     
     \ /
      Last update: 2011-08-15 23:01    [W:0.026 / U:32.296 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site