[PATCH v1] ext4:Allow an active handle to be started when freezing.

[PATCH v1] ext4:Allow an active handle to be started when freezing.

[Yongqiang Yang]

v0->v1:
  check s_frozen in nojournal case only if there is no an active handle.
  get reference to an active handle by jbd2_journal_start().

ext4_journal_start_sb() should not prevent an active handle from being
started due to s_frozen.  Otherwise, deadlock is easy to happen, below
is a situation.

================================================
     freeze         |       truncate
================================================
                    |  ext4_ext_truncate()
    freeze_super()  |   starts a handle
    sets s_frozen   |
                    |  ext4_ext_truncate()
                    |  holds i_data_sem
  ext4_freeze()     |
  waits for updates |
                    |  ext4_free_blocks()
                    |  calls dquot_free_block()
                    |
                    |  dquot_free_blocks()
                    |  calls ext4_dirty_inode()
                    |
                    |  ext4_dirty_inode()
                    |  trys to start an active
                    |  handle
                    |
                    |  block due to s_frozen
================================================

Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
Reported-by: Amir Goldstein <amir73il@users.sf.net>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c |   45 ++++++++++++++++++++++++++++++++++-----------
 1 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index ccfa686..53920bd 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -242,27 +242,45 @@ static void ext4_put_nojournal(handle_t *handle)
  * journal_end calls result in the superblock being marked dirty, so
  * that sync() will call the filesystem's write_super callback if
  * appropriate.
+ *
+ * To avoid j_barrier hold in userspace when a user calls freeze(),
+ * ext4 prevents a new handle from being started by s_frozen, which
+ * is in an upper layer.
  */
 handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks)
 {
 	journal_t *journal;
+	handle_t  *handle;

 	if (sb->s_flags & MS_RDONLY)
 		return ERR_PTR(-EROFS);

-	vfs_check_frozen(sb, SB_FREEZE_TRANS);
-	/* Special case here: if the journal has aborted behind our
-	 * backs (eg. EIO in the commit thread), then we still need to
-	 * take the FS itself readonly cleanly. */
 	journal = EXT4_SB(sb)->s_journal;
-	if (journal) {
-		if (is_journal_aborted(journal)) {
-			ext4_abort(sb, "Detected aborted journal");
-			return ERR_PTR(-EROFS);
-		}
-		return jbd2_journal_start(journal, nblocks);
+	handle = ext4_journal_current_handle();
+
+	/*
+	 * Before vfs_check_frozen(), the current handle should be allowed
+	 * to finish, otherwise deadlock would happen when the filesystem
+	 * is freezed && active handles are not stopped.
+	 */
+	if (!journal) {
+		if (!handle)
+			vfs_check_frozen(sb, SB_FREEZE_TRANS);
+		return ext4_get_nojournal();
 	}
-	return ext4_get_nojournal();
+
+	if (!handle)
+		vfs_check_frozen(sb, SB_FREEZE_TRANS);
+	/*
+	 * Special case here: if the journal has aborted behind our
+	 * backs (eg. EIO in the commit thread), then we still need to
+	 * take the FS itself readonly cleanly.
+	 */
+	if (is_journal_aborted(journal)) {
+		ext4_abort(sb, "Detected aborted journal");
+		return ERR_PTR(-EROFS);
+	}
+	return jbd2_journal_start(journal, nblocks);
 }

 /*
@@ -4131,6 +4149,11 @@ static int ext4_sync_fs(struct super_block *sb, int wait)
 /*
  * LVM calls this function before a (read-only) snapshot is created.  This
  * gives us a chance to flush the journal completely and mark the fs clean.
+ *
+ * Note that only this function cannot bring a filesystem to be in a clean
+ * state independently, because ext4 prevents a new handle from being started
+ * by @sb->s_frozen, which stays in an upper layer.  It thus needs help from
+ * the upper layer.
  */
 static int ext4_freeze(struct super_block *sb)
 {
-- 
1.7.4

Re: [PATCH v1] ext4:Allow an active handle to be started when freezing.

[Andreas Dilger]

On 2011-04-04, at 9:47 PM, Yongqiang Yang wrote:
> handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks)
> {
> +	handle = ext4_journal_current_handle();
> +
> +	/*
> +	 * Before vfs_check_frozen(), the current handle should be allowed
> +	 * to finish, otherwise deadlock would happen when the filesystem
> +	 * is freezed && active handles are not stopped.
> +	 */
> +	if (!journal) {
> +		if (!handle)
> +			vfs_check_frozen(sb, SB_FREEZE_TRANS);
> +		return ext4_get_nojournal();
> 	}
> +
> +	if (!handle)
> +		vfs_check_frozen(sb, SB_FREEZE_TRANS);

This is probably easier to read by putting the !handle check/code together,
with a minor fixup to the comment as well:

	/*
	 * If a handle has already been started, it should be allowed to
	 * finish, otherwise deadlock could happen between freeze and
	 * truncate due to the restart of the journal handle if the
	 * filesystem is frozen and active handles are not stopped.
	 */
	if (!handle) {
		vfs_check_frozen(sb, SB_FREEZE_TRANS);
		if (!journal)
			return ext4_get_nojournal();
	}

	/*
	 * Special case here: if the journal has aborted behind our
	 * backs (eg. EIO in the commit thread), then we still need to
	 * take the FS itself readonly cleanly.
	 */
	if (is_journal_aborted(journal)) {
		ext4_abort(sb, "Detected aborted journal");
		return ERR_PTR(-EROFS);
	}
	return jbd2_journal_start(journal, nblocks);
}

> /*
> @@ -4131,6 +4149,11 @@ static int ext4_sync_fs(struct super_block *sb, int wait)
> /*
> * LVM calls this function before a (read-only) snapshot is created.  This
> * gives us a chance to flush the journal completely and mark the fs clean.
> + *
> + * Note that only this function cannot bring a filesystem to be in a clean
> + * state independently, because ext4 prevents a new handle from being started
> + * by @sb->s_frozen, which stays in an upper layer.  It thus needs help from
> + * the upper layer.
> */
> static int ext4_freeze(struct super_block *sb)
> {
> -- 
> 1.7.4
> --
> 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


Cheers, Andreas

Re: [PATCH v1] ext4:Allow an active handle to be started when freezing.

[Jan Kara]

On Tue 05-04-11 15:47:25, Yongqiang Yang wrote:
> v0->v1:
>   check s_frozen in nojournal case only if there is no an active handle.
>   get reference to an active handle by jbd2_journal_start().
> 
> ext4_journal_start_sb() should not prevent an active handle from being
> started due to s_frozen.  Otherwise, deadlock is easy to happen, below
> is a situation.
> 
> ================================================
>      freeze         |       truncate
> ================================================
>                     |  ext4_ext_truncate()
>     freeze_super()  |   starts a handle
>     sets s_frozen   |
>                     |  ext4_ext_truncate()
>                     |  holds i_data_sem
>   ext4_freeze()     |
>   waits for updates |
>                     |  ext4_free_blocks()
>                     |  calls dquot_free_block()
>                     |
>                     |  dquot_free_blocks()
>                     |  calls ext4_dirty_inode()
>                     |
>                     |  ext4_dirty_inode()
>                     |  trys to start an active
>                     |  handle
>                     |
>                     |  block due to s_frozen
> ================================================
> 
> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
> Reported-by: Amir Goldstein <amir73il@users.sf.net>
> Reviewed-by: Jan Kara <jack@suse.cz>
  The usual workflow is that you add 'Reviewed-by' tag to the patch only
after you send a version of a patch the person agrees to and explicitely
says you can add a tag - for example I write something like "You can add
Reviewed-by: Jan Kara <jack@suse.cz>"
when I agree to the tag being added.

> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index ccfa686..53920bd 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -242,27 +242,45 @@ static void ext4_put_nojournal(handle_t *handle)
>   * journal_end calls result in the superblock being marked dirty, so
>   * that sync() will call the filesystem's write_super callback if
>   * appropriate.
> + *
> + * To avoid j_barrier hold in userspace when a user calls freeze(),
> + * ext4 prevents a new handle from being started by s_frozen, which
> + * is in an upper layer.
>   */
>  handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks)
>  {
>  	journal_t *journal;
> +	handle_t  *handle;
> 
>  	if (sb->s_flags & MS_RDONLY)
>  		return ERR_PTR(-EROFS);
> 
> -	vfs_check_frozen(sb, SB_FREEZE_TRANS);
> -	/* Special case here: if the journal has aborted behind our
> -	 * backs (eg. EIO in the commit thread), then we still need to
> -	 * take the FS itself readonly cleanly. */
>  	journal = EXT4_SB(sb)->s_journal;
> -	if (journal) {
> -		if (is_journal_aborted(journal)) {
> -			ext4_abort(sb, "Detected aborted journal");
> -			return ERR_PTR(-EROFS);
> -		}
> -		return jbd2_journal_start(journal, nblocks);
> +	handle = ext4_journal_current_handle();
> +
> +	/*
> +	 * Before vfs_check_frozen(), the current handle should be allowed
> +	 * to finish, otherwise deadlock would happen when the filesystem
> +	 * is freezed && active handles are not stopped.
> +	 */
> +	if (!journal) {
> +		if (!handle)
> +			vfs_check_frozen(sb, SB_FREEZE_TRANS);
> +		return ext4_get_nojournal();
>  	}
> -	return ext4_get_nojournal();
> +
> +	if (!handle)
> +		vfs_check_frozen(sb, SB_FREEZE_TRANS);
  This test is the same for 'nojournal' case so you can move it before the
!journal test. Otherwise the patch looks OK to me. So now you can add
Reviewed-by: Jan Kara <jack@suse.cz>
line :)

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH v1] ext4:Allow an active handle to be started when freezing.

[Yongqiang Yang]

On Wed, Apr 6, 2011 at 6:26 AM, Jan Kara <jack@suse.cz> wrote:
> On Tue 05-04-11 15:47:25, Yongqiang Yang wrote:
>> v0->v1:
>>   check s_frozen in nojournal case only if there is no an active handle.
>>   get reference to an active handle by jbd2_journal_start().
>>
>> ext4_journal_start_sb() should not prevent an active handle from being
>> started due to s_frozen.  Otherwise, deadlock is easy to happen, below
>> is a situation.
>>
>> ================================================
>>      freeze         |       truncate
>> ================================================
>>                     |  ext4_ext_truncate()
>>     freeze_super()  |   starts a handle
>>     sets s_frozen   |
>>                     |  ext4_ext_truncate()
>>                     |  holds i_data_sem
>>   ext4_freeze()     |
>>   waits for updates |
>>                     |  ext4_free_blocks()
>>                     |  calls dquot_free_block()
>>                     |
>>                     |  dquot_free_blocks()
>>                     |  calls ext4_dirty_inode()
>>                     |
>>                     |  ext4_dirty_inode()
>>                     |  trys to start an active
>>                     |  handle
>>                     |
>>                     |  block due to s_frozen
>> ================================================
>>
>> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
>> Reported-by: Amir Goldstein <amir73il@users.sf.net>
>> Reviewed-by: Jan Kara <jack@suse.cz>
>  The usual workflow is that you add 'Reviewed-by' tag to the patch only
> after you send a version of a patch the person agrees to and explicitely
> says you can add a tag - for example I write something like "You can add
> Reviewed-by: Jan Kara <jack@suse.cz>"
> when I agree to the tag being added.
I see.  Thank you.
>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index ccfa686..53920bd 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -242,27 +242,45 @@ static void ext4_put_nojournal(handle_t *handle)
>>   * journal_end calls result in the superblock being marked dirty, so
>>   * that sync() will call the filesystem's write_super callback if
>>   * appropriate.
>> + *
>> + * To avoid j_barrier hold in userspace when a user calls freeze(),
>> + * ext4 prevents a new handle from being started by s_frozen, which
>> + * is in an upper layer.
>>   */
>>  handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks)
>>  {
>>       journal_t *journal;
>> +     handle_t  *handle;
>>
>>       if (sb->s_flags & MS_RDONLY)
>>               return ERR_PTR(-EROFS);
>>
>> -     vfs_check_frozen(sb, SB_FREEZE_TRANS);
>> -     /* Special case here: if the journal has aborted behind our
>> -      * backs (eg. EIO in the commit thread), then we still need to
>> -      * take the FS itself readonly cleanly. */
>>       journal = EXT4_SB(sb)->s_journal;
>> -     if (journal) {
>> -             if (is_journal_aborted(journal)) {
>> -                     ext4_abort(sb, "Detected aborted journal");
>> -                     return ERR_PTR(-EROFS);
>> -             }
>> -             return jbd2_journal_start(journal, nblocks);
>> +     handle = ext4_journal_current_handle();
>> +
>> +     /*
>> +      * Before vfs_check_frozen(), the current handle should be allowed
>> +      * to finish, otherwise deadlock would happen when the filesystem
>> +      * is freezed && active handles are not stopped.
>> +      */
>> +     if (!journal) {
>> +             if (!handle)
>> +                     vfs_check_frozen(sb, SB_FREEZE_TRANS);
>> +             return ext4_get_nojournal();
>>       }
>> -     return ext4_get_nojournal();
>> +
>> +     if (!handle)
>> +             vfs_check_frozen(sb, SB_FREEZE_TRANS);
>  This test is the same for 'nojournal' case so you can move it before the
> !journal test. Otherwise the patch looks OK to me. So now you can add
> Reviewed-by: Jan Kara <jack@suse.cz>
> line :)
>
>                                                                        Honza
> --
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR
>



-- 
Best Wishes
Yongqiang Yang
--
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