diff mbox

[SRU,TRUSTY] Btrfs: fix transaction abortion when remounting btrfs from RW to RO

Message ID 1421342592-22166-1-git-send-email-colin.king@canonical.com
State New
Headers show

Commit Message

Colin Ian King Jan. 15, 2015, 5:23 p.m. UTC
From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>

BugLink: http://bugs.launchpad.net/bugs/1411320

Steps to reproduce:
 # mkfs.btrfs -f /dev/sda8
 # mount /dev/sda8 /mnt -o flushoncommit
 # dd if=/dev/zero of=/mnt/data bs=4k count=102400 &
 # mount /dev/sda8 /mnt -o remount, ro

When remounting RW to RO, the logic is to firstly set flag
to RO and then commit transaction, however with option
flushoncommit enabled,we will do RO check within committing
transaction, so we get a transaction abortion here.

Actually,here check is wrong, we should check if FS_STATE_ERROR
is set, fix it.

Reported-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
Suggested-by: Miao Xie <miaox@cn.fujitsu.com>
Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
Signed-off-by: Josef Bacik <jbacik@fb.com>
Signed-off-by: Chris Mason <clm@fb.com>
(cherry picked from commit 2c21b4d733d6e50514e30ffd87110364ddda695b)
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 fs/btrfs/inode.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Chris J Arges Jan. 15, 2015, 7:59 p.m. UTC | #1
I see
$ git describe --contains 2c21b4d733d6e50514e30ffd87110364ddda695b
v3.14-rc2~3^2~35
Its a clean cherry-pick and fixes a very testable issue.

Acked-by: Chris J Arges <chris.j.arges@canonical.com>

--chris

On 01/15/2015 11:23 AM, Colin King wrote:
> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/1411320
> 
> Steps to reproduce:
>  # mkfs.btrfs -f /dev/sda8
>  # mount /dev/sda8 /mnt -o flushoncommit
>  # dd if=/dev/zero of=/mnt/data bs=4k count=102400 &
>  # mount /dev/sda8 /mnt -o remount, ro
> 
> When remounting RW to RO, the logic is to firstly set flag
> to RO and then commit transaction, however with option
> flushoncommit enabled,we will do RO check within committing
> transaction, so we get a transaction abortion here.
> 
> Actually,here check is wrong, we should check if FS_STATE_ERROR
> is set, fix it.
> 
> Reported-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> Suggested-by: Miao Xie <miaox@cn.fujitsu.com>
> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> Signed-off-by: Chris Mason <clm@fb.com>
> (cherry picked from commit 2c21b4d733d6e50514e30ffd87110364ddda695b)
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  fs/btrfs/inode.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index f4a870a..387b6fb 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8290,7 +8290,7 @@ int btrfs_start_delalloc_inodes(struct btrfs_root *root, int delay_iput)
>  {
>  	int ret;
>  
> -	if (root->fs_info->sb->s_flags & MS_RDONLY)
> +	if (test_bit(BTRFS_FS_STATE_ERROR, &root->fs_info->fs_state))
>  		return -EROFS;
>  
>  	ret = __start_delalloc_inodes(root, delay_iput);
> @@ -8316,7 +8316,7 @@ int btrfs_start_delalloc_roots(struct btrfs_fs_info *fs_info, int delay_iput)
>  	struct list_head splice;
>  	int ret;
>  
> -	if (fs_info->sb->s_flags & MS_RDONLY)
> +	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
>  		return -EROFS;
>  
>  	INIT_LIST_HEAD(&splice);
>
Brad Figg Jan. 15, 2015, 8:10 p.m. UTC | #2
On Thu, Jan 15, 2015 at 05:23:12PM +0000, Colin King wrote:
> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/1411320
> 
> Steps to reproduce:
>  # mkfs.btrfs -f /dev/sda8
>  # mount /dev/sda8 /mnt -o flushoncommit
>  # dd if=/dev/zero of=/mnt/data bs=4k count=102400 &
>  # mount /dev/sda8 /mnt -o remount, ro
> 
> When remounting RW to RO, the logic is to firstly set flag
> to RO and then commit transaction, however with option
> flushoncommit enabled,we will do RO check within committing
> transaction, so we get a transaction abortion here.
> 
> Actually,here check is wrong, we should check if FS_STATE_ERROR
> is set, fix it.
> 
> Reported-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> Suggested-by: Miao Xie <miaox@cn.fujitsu.com>
> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> Signed-off-by: Chris Mason <clm@fb.com>
> (cherry picked from commit 2c21b4d733d6e50514e30ffd87110364ddda695b)
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  fs/btrfs/inode.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index f4a870a..387b6fb 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8290,7 +8290,7 @@ int btrfs_start_delalloc_inodes(struct btrfs_root *root, int delay_iput)
>  {
>  	int ret;
>  
> -	if (root->fs_info->sb->s_flags & MS_RDONLY)
> +	if (test_bit(BTRFS_FS_STATE_ERROR, &root->fs_info->fs_state))
>  		return -EROFS;
>  
>  	ret = __start_delalloc_inodes(root, delay_iput);
> @@ -8316,7 +8316,7 @@ int btrfs_start_delalloc_roots(struct btrfs_fs_info *fs_info, int delay_iput)
>  	struct list_head splice;
>  	int ret;
>  
> -	if (fs_info->sb->s_flags & MS_RDONLY)
> +	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
>  		return -EROFS;
>  
>  	INIT_LIST_HEAD(&splice);
> -- 
> 1.7.9.5
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

Looks reasonable
Andy Whitcroft Jan. 16, 2015, 11:18 a.m. UTC | #3
Applied to Trusty.

-apw
diff mbox

Patch

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index f4a870a..387b6fb 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8290,7 +8290,7 @@  int btrfs_start_delalloc_inodes(struct btrfs_root *root, int delay_iput)
 {
 	int ret;
 
-	if (root->fs_info->sb->s_flags & MS_RDONLY)
+	if (test_bit(BTRFS_FS_STATE_ERROR, &root->fs_info->fs_state))
 		return -EROFS;
 
 	ret = __start_delalloc_inodes(root, delay_iput);
@@ -8316,7 +8316,7 @@  int btrfs_start_delalloc_roots(struct btrfs_fs_info *fs_info, int delay_iput)
 	struct list_head splice;
 	int ret;
 
-	if (fs_info->sb->s_flags & MS_RDONLY)
+	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
 		return -EROFS;
 
 	INIT_LIST_HEAD(&splice);