[PATCH 1/4] ext4: Fix fsync error handling after filesysteb abort.

[PATCH 1/4] ext4: Fix fsync error handling after filesysteb abort.

[Dmitry Monakhov]

If filesystem was aborted after inode's write back complete
but before it's metadata was updated we may return success
due to (sb->s_flags & MS_RDONLY) which is incorrect and
result in data loss.
In order to handle fs abort correctly we have to check
fs state once we discover that it is in MS_RDONLY state

Test case: http://patchwork.ozlabs.org/patch/244297/

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/fsync.c |    8 ++++++--
 fs/ext4/super.c |   13 ++++++++++++-
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index e0ba8a4..d7df2f1 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -129,9 +129,13 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 		return ret;
 	mutex_lock(&inode->i_mutex);
 
-	if (inode->i_sb->s_flags & MS_RDONLY)
+	if (inode->i_sb->s_flags & MS_RDONLY) {
+		/* Make shure that we read updated s_mount_flags value */
+		smp_rmb();
+		if (EXT4_SB(inode->i_sb)->s_mount_flags & EXT4_MF_FS_ABORTED)
+			ret = -EROFS;
 		goto out;
-
+	}
 	ret = ext4_flush_unwritten_io(inode);
 	if (ret < 0)
 		goto out;
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index dbc7c09..6c91c8e 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -398,6 +398,11 @@ static void ext4_handle_error(struct super_block *sb)
 	}
 	if (test_opt(sb, ERRORS_RO)) {
 		ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
+		/*
+		 * Make shure updated value of ->s_mount_flags will be visiable
+		 * before ->s_flags update
+		 */
+		smp_wmb();
 		sb->s_flags |= MS_RDONLY;
 	}
 	if (test_opt(sb, ERRORS_PANIC))
@@ -552,6 +557,7 @@ void __ext4_std_error(struct super_block *sb, const char *function,
  *
  * We unconditionally force the filesystem into an ABORT|READONLY state,
  * unless the error response on the fs has been set to panic in which
+
  * case we take the easy way out and panic immediately.
  */
 
@@ -570,8 +576,13 @@ void __ext4_abort(struct super_block *sb, const char *function,
 
 	if ((sb->s_flags & MS_RDONLY) == 0) {
 		ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
-		sb->s_flags |= MS_RDONLY;
 		EXT4_SB(sb)->s_mount_flags |= EXT4_MF_FS_ABORTED;
+		/*
+		 * Make shure updated value of ->s_mount_flags will be visiable
+		 * before ->s_flags update
+		 */
+		smp_wmb();
+		sb->s_flags |= MS_RDONLY;
 		if (EXT4_SB(sb)->s_journal)
 			jbd2_journal_abort(EXT4_SB(sb)->s_journal, -EIO);
 		save_error_info(sb, function, line);
-- 
1.7.1

[PATCH 2/4] ext3: Fix fsync error handling after filesysteb abort.

[Dmitry Monakhov]

If filesystem was aborted we will return success
due to (sb->s_flags & MS_RDONLY) which is incorrect and
result in data loss.
In order to handle fs abort correctly we have to check
fs state once we discover that it is in MS_RDONLY state

Test case: http://patchwork.ozlabs.org/patch/244297/

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext3/fsync.c |    8 ++++++--
 fs/ext3/super.c |   13 ++++++++++++-
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/fs/ext3/fsync.c b/fs/ext3/fsync.c
index b31dbd4..5412916 100644
--- a/fs/ext3/fsync.c
+++ b/fs/ext3/fsync.c
@@ -48,9 +48,13 @@ int ext3_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 
 	trace_ext3_sync_file_enter(file, datasync);
 
-	if (inode->i_sb->s_flags & MS_RDONLY)
+	if (inode->i_sb->s_flags & MS_RDONLY) {
+		/* Make shure that we read updated state */
+		smp_rmb();
+		if (EXT3_SB(inode->i_sb)->s_mount_state & EXT3_ERROR_FS)
+			return -EROFS;
 		return 0;
-
+	}
 	ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
 	if (ret)
 		goto out;
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index fb5120a..5a32670 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -172,6 +172,11 @@ static void ext3_handle_error(struct super_block *sb)
 			journal_abort(journal, -EIO);
 	}
 	if (test_opt (sb, ERRORS_RO)) {
+		/*
+		 * Make shure updated value of ->s_mount_state will be visiable
+		 * before ->s_flags update.
+		 */
+		smp_wmb();
 		ext3_msg(sb, KERN_CRIT,
 			"error: remounting filesystem read-only");
 		sb->s_flags |= MS_RDONLY;
@@ -291,8 +296,14 @@ void ext3_abort(struct super_block *sb, const char *function,
 	ext3_msg(sb, KERN_CRIT,
 		"error: remounting filesystem read-only");
 	EXT3_SB(sb)->s_mount_state |= EXT3_ERROR_FS;
-	sb->s_flags |= MS_RDONLY;
 	set_opt(EXT3_SB(sb)->s_mount_opt, ABORT);
+	/*
+	 * Make shure updated value of ->s_mount_state will be visiable
+	 * before ->s_flags update.
+	 */
+	smp_wmb();
+	sb->s_flags |= MS_RDONLY;
+
 	if (EXT3_SB(sb)->s_journal)
 		journal_abort(EXT3_SB(sb)->s_journal, -EIO);
 }
-- 
1.7.1

[PATCH 3/4] jbd2: make shure that we do not miss aborted state

[Dmitry Monakhov]

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/jbd2/journal.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 886ec2f..57b9120 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -700,12 +700,11 @@ int jbd2_log_wait_commit(journal_t *journal, tid_t tid)
 				!tid_gt(tid, journal->j_commit_sequence));
 		read_lock(&journal->j_state_lock);
 	}
-	read_unlock(&journal->j_state_lock);

[PATCH 4/4] jbd: make shure that we do not miss aborted state

[Dmitry Monakhov]

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/jbd/journal.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/fs/jbd/journal.c b/fs/jbd/journal.c
index 81cc7ea..dc00240 100644
--- a/fs/jbd/journal.c
+++ b/fs/jbd/journal.c
@@ -575,12 +575,11 @@ int log_wait_commit(journal_t *journal, tid_t tid)
 				!tid_gt(tid, journal->j_commit_sequence));
 		spin_lock(&journal->j_state_lock);
 	}
-	spin_unlock(&journal->j_state_lock);

Re: [PATCH 3/4] jbd2: make shure that we do not miss aborted state

[Wang Shilong]

Nothing serious. It seems you miswrite patch title 'sure'  to 'shure'..

Thanks,
Wang

> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
> fs/jbd2/journal.c |    3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 886ec2f..57b9120 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -700,12 +700,11 @@ int jbd2_log_wait_commit(journal_t *journal, tid_t tid)
> 				!tid_gt(tid, journal->j_commit_sequence));
> 		read_lock(&journal->j_state_lock);
> 	}
> -	read_unlock(&journal->j_state_lock);
> -
> 	if (unlikely(is_journal_aborted(journal))) {
> 		printk(KERN_EMERG "journal commit I/O error\n");
> 		err = -EIO;
> 	}
> +	read_unlock(&journal->j_state_lock);
> 	return err;
> }
> 
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] ext4: Fix fsync error handling after filesysteb abort.

[Jan Kara]

On Thu 16-05-13 16:28:07, Dmitry Monakhov wrote:
  The patch looks OK but I have some typo fixes below. But you can add
Reviewed-by: Jan Kara <jack@suse.cz>

> If filesystem was aborted after inode's write back complete
						    ^^ is
> but before it's metadata was updated we may return success
             ^^^ its

> due to (sb->s_flags & MS_RDONLY) which is incorrect and
> result in data loss.
   ^^^ results

> In order to handle fs abort correctly we have to check
> fs state once we discover that it is in MS_RDONLY state
> 
> Test case: http://patchwork.ozlabs.org/patch/244297/
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  fs/ext4/fsync.c |    8 ++++++--
>  fs/ext4/super.c |   13 ++++++++++++-
>  2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
> index e0ba8a4..d7df2f1 100644
> --- a/fs/ext4/fsync.c
> +++ b/fs/ext4/fsync.c
> @@ -129,9 +129,13 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
>  		return ret;
>  	mutex_lock(&inode->i_mutex);
>  
> -	if (inode->i_sb->s_flags & MS_RDONLY)
> +	if (inode->i_sb->s_flags & MS_RDONLY) {
> +		/* Make shure that we read updated s_mount_flags value */
                        ^^^ sure
> +		smp_rmb();
> +		if (EXT4_SB(inode->i_sb)->s_mount_flags & EXT4_MF_FS_ABORTED)
> +			ret = -EROFS;
>  		goto out;
> -
> +	}
>  	ret = ext4_flush_unwritten_io(inode);
>  	if (ret < 0)
>  		goto out;
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index dbc7c09..6c91c8e 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -398,6 +398,11 @@ static void ext4_handle_error(struct super_block *sb)
>  	}
>  	if (test_opt(sb, ERRORS_RO)) {
>  		ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
> +		/*
> +		 * Make shure updated value of ->s_mount_flags will be visiable
                        ^^^ sure                                  visible ^^
> +		 * before ->s_flags update
> +		 */
> +		smp_wmb();
>  		sb->s_flags |= MS_RDONLY;
>  	}
>  	if (test_opt(sb, ERRORS_PANIC))
> @@ -552,6 +557,7 @@ void __ext4_std_error(struct super_block *sb, const char *function,
>   *
>   * We unconditionally force the filesystem into an ABORT|READONLY state,
>   * unless the error response on the fs has been set to panic in which
> +
  ^^ Stray empty line.

>   * case we take the easy way out and panic immediately.
>   */
>  
> @@ -570,8 +576,13 @@ void __ext4_abort(struct super_block *sb, const char *function,
>  
>  	if ((sb->s_flags & MS_RDONLY) == 0) {
>  		ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
> -		sb->s_flags |= MS_RDONLY;
>  		EXT4_SB(sb)->s_mount_flags |= EXT4_MF_FS_ABORTED;
> +		/*
> +		 * Make shure updated value of ->s_mount_flags will be visiable
                         ^^ sure                                 visible ^^
> +		 * before ->s_flags update
> +		 */
> +		smp_wmb();
> +		sb->s_flags |= MS_RDONLY;
>  		if (EXT4_SB(sb)->s_journal)
>  			jbd2_journal_abort(EXT4_SB(sb)->s_journal, -EIO);
>  		save_error_info(sb, function, line);
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 3/4] jbd2: make shure that we do not miss aborted state

[Jan Kara]

On Thu 16-05-13 16:28:09, Dmitry Monakhov wrote:
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
  Can you elaborate a bit more on how the aborted state could be missed so
that it would cause any problems?

								Honza
> ---
>  fs/jbd2/journal.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 886ec2f..57b9120 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -700,12 +700,11 @@ int jbd2_log_wait_commit(journal_t *journal, tid_t tid)
>  				!tid_gt(tid, journal->j_commit_sequence));
>  		read_lock(&journal->j_state_lock);
>  	}
> -	read_unlock(&journal->j_state_lock);
> -
>  	if (unlikely(is_journal_aborted(journal))) {
>  		printk(KERN_EMERG "journal commit I/O error\n");
>  		err = -EIO;
>  	}
> +	read_unlock(&journal->j_state_lock);
>  	return err;
>  }
>  
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 3/4] jbd2: make shure that we do not miss aborted state

[Dmitry Monakhov]

On Tue, 21 May 2013 18:21:14 +0200, Jan Kara <jack@suse.cz> wrote:
> On Thu 16-05-13 16:28:09, Dmitry Monakhov wrote:
> > 
> > Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
>   Can you elaborate a bit more on how the aborted state could be missed so
> that it would cause any problems?
Fairly to say this was fix of hypothetical issue.
Since we  j_state_lock goes trough lock/unlock after wait_event()
we should not miss an error for the certain tid we are interested in.
But this patch makes code looks cleaner. 
> 
> 								Honza
> > ---
> >  fs/jbd2/journal.c |    3 +--
> >  1 files changed, 1 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> > index 886ec2f..57b9120 100644
> > --- a/fs/jbd2/journal.c
> > +++ b/fs/jbd2/journal.c
> > @@ -700,12 +700,11 @@ int jbd2_log_wait_commit(journal_t *journal, tid_t tid)
> >  				!tid_gt(tid, journal->j_commit_sequence));
> >  		read_lock(&journal->j_state_lock);
> >  	}
> > -	read_unlock(&journal->j_state_lock);
> > -
> >  	if (unlikely(is_journal_aborted(journal))) {
> >  		printk(KERN_EMERG "journal commit I/O error\n");
> >  		err = -EIO;
> >  	}
> > +	read_unlock(&journal->j_state_lock);
> >  	return err;
> >  }
> >  
> > -- 
> > 1.7.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> -- 
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] jbd2: make shure that we do not miss aborted state

[Jan Kara]

On Wed 22-05-13 13:02:10, Dmitry Monakhov wrote:
> On Tue, 21 May 2013 18:21:14 +0200, Jan Kara <jack@suse.cz> wrote:
> > On Thu 16-05-13 16:28:09, Dmitry Monakhov wrote:
> > > 
> > > Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> >   Can you elaborate a bit more on how the aborted state could be missed so
> > that it would cause any problems?
> Fairly to say this was fix of hypothetical issue.
> Since we  j_state_lock goes trough lock/unlock after wait_event()
> we should not miss an error for the certain tid we are interested in.
> But this patch makes code looks cleaner. 
  Well, we often check is_journal_aborted() without holding any lock so
moving it inside the lock in this case doesn't make much sence to me. So at
least for jbd I'm not inclined to take it as it seems as an unnecessary
churn. For jbd2 it's Ted's choice...

								Honza

> > > ---
> > >  fs/jbd2/journal.c |    3 +--
> > >  1 files changed, 1 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> > > index 886ec2f..57b9120 100644
> > > --- a/fs/jbd2/journal.c
> > > +++ b/fs/jbd2/journal.c
> > > @@ -700,12 +700,11 @@ int jbd2_log_wait_commit(journal_t *journal, tid_t tid)
> > >  				!tid_gt(tid, journal->j_commit_sequence));
> > >  		read_lock(&journal->j_state_lock);
> > >  	}
> > > -	read_unlock(&journal->j_state_lock);
> > > -
> > >  	if (unlikely(is_journal_aborted(journal))) {
> > >  		printk(KERN_EMERG "journal commit I/O error\n");
> > >  		err = -EIO;
> > >  	}
> > > +	read_unlock(&journal->j_state_lock);
> > >  	return err;
> > >  }
> > >  
> > > -- 
> > > 1.7.1
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > -- 
> > Jan Kara <jack@suse.cz>
> > SUSE Labs, CR
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR