Btrfs: cancel the scrub when remounting a fs to ro

PATCH In review Miao Xie 4 participants 5 messages May 17, 2012 – May 21, 2012 Btrfs file system

lore

Share & tools

lore

Miao Xie

author

2012-05-17 11:59 UTC #0 lore raw

PATCH 4/5 Btrfs: cancel the scrub when remounting a fs to ro

If the filesystem is mounted to readonly, we should not run scrub.

Signed-off-by Miao Xie <[email protected]>

---
 fs/btrfs/super.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

fs/btrfs/super.c +2 -0

index ea17f0a..817b3a7 100644
@@ -1151,6 +1151,8 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 		/* pause restriper - we want to resume on remount to r/w */
 		btrfs_pause_balance(root->fs_info);
 
+		btrfs_scrub_cancel(root);
+
 		ret = btrfs_commit_super(root);
 		if (ret)
 			goto restore;

Signature · 1 line

1.7.6.5

David Sterba

2012-05-18 12:52 UTC #1 lore raw

fs/btrfs/super.c static int btrfs_remount(struct super_block *sb, int *flags, char *data) · line 1154

Quoted 7 lines · Miao Xie

fs/btrfs/super.c +1 -0

@@ -1151,6 +1151,8 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 		/* pause restriper - we want to resume on remount to r/w */
 		btrfs_pause_balance(root->fs_info);
 
+		btrfs_scrub_cancel(root);

Can we possibly switch scrub to readonly instead ? I'm not sure what's the 'least surprise here', whether to cancel everything on the filesystem upon ro-remount or just the minimal set of operations (and leave the rest running if possible).

Looking at the scrub code, if dev->readonly is set, no repairs are done, so the only concern is to wait for any outstanding IOs and then switch to RO.

david

Miao Xie

author

2012-05-21 05:35 UTC #2 lore raw

Quoted 7 lines · David Sterba

Quoted 6 lines · Miao Xie

fs/btrfs/super.c +0 -0

@@ -1151,6 +1151,8 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 		/* pause restriper - we want to resume on remount to r/w */
 		btrfs_pause_balance(root->fs_info);

Quoted 6 lines

fs/btrfs/super.c static int btrfs_remount(struct super_block *sb, int *flags, char *data) · line 1154
Quoted 1 line
fs/btrfs/super.c +1 -0
+		btrfs_scrub_cancel(root);
Can we possibly switch scrub to readonly instead ? I'm not sure what's the 'least surprise here', whether to cancel everything on the filesystem upon ro-remount or just the minimal set of operations (and leave the rest running if possible).

I don't think it is better to switch scrub to readonly soundlessly, because it is not the operation that the users expect if they don't choose the readonly mode.

BTW, I think we needn't cancel the readonly scrub on the filesystem upon ro-remount.

Thanks
Miao

Quoted 8 lines

Looking at the scrub code, if dev->readonly is set, no repairs are done, so the only concern is to wait for any outstanding IOs and then switch to RO.
david

Stefan Behrens

2012-05-21 10:29 UTC #3 lore raw

Quoted 15 lines · Miao Xie

Quoted 7 lines · David Sterba
Quoted 6 lines · Miao Xie
fs/btrfs/super.c +0 -0
@@ -1151,6 +1151,8 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 		/* pause restriper - we want to resume on remount to r/w */
 		btrfs_pause_balance(root->fs_info);
 
Quoted 6 lines
fs/btrfs/super.c static int btrfs_remount(struct super_block *sb, int *flags, char *data) · line 1154
Quoted 1 line
fs/btrfs/super.c +1 -0
+		btrfs_scrub_cancel(root);
Can we possibly switch scrub to readonly instead ? I'm not sure what's the 'least surprise here', whether to cancel everything on the filesystem upon ro-remount or just the minimal set of operations (and leave the rest running if possible).

Quoted 3 lines

I don't think it is better to switch scrub to readonly soundlessly, because it is not the operation that the users expect if they don't choose the readonly mode.

Since David put me on CC for this thread, I state my humble opinion:
If I manually remount read-only, I would expect that scrub is switched into read-only mode as well, not canceled. Plus a log message about this change.

Quoted 12 lines

BTW, I think we needn't cancel the readonly scrub on the filesystem upon ro-remount.
Thanks
Miao
Quoted 7 lines
Looking at the scrub code, if dev->readonly is set, no repairs are done, so the only concern is to wait for any outstanding IOs and then switch to RO.
david

Arne Jansen

2012-05-21 10:29 UTC #4 lore raw

In reply to Miao Xie

Quoted 15 lines · Miao Xie

Quoted 7 lines · David Sterba
Quoted 6 lines · Miao Xie
fs/btrfs/super.c +0 -0
@@ -1151,6 +1151,8 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 		/* pause restriper - we want to resume on remount to r/w */
 		btrfs_pause_balance(root->fs_info);
 
Quoted 6 lines
fs/btrfs/super.c static int btrfs_remount(struct super_block *sb, int *flags, char *data) · line 1154
Quoted 1 line
fs/btrfs/super.c +1 -0
+		btrfs_scrub_cancel(root);
Can we possibly switch scrub to readonly instead ? I'm not sure what's the 'least surprise here', whether to cancel everything on the filesystem upon ro-remount or just the minimal set of operations (and leave the rest running if possible).

Quoted 3 lines

I don't think it is better to switch scrub to readonly soundlessly, because it is not the operation that the users expect if they don't choose the readonly mode.

I'm not even sure the user expects any changes here. I'd opt for letting the scrub continue r/w.

-Arne

Quoted 19 lines

BTW, I think we needn't cancel the readonly scrub on the filesystem upon ro-remount.
Thanks
Miao
Quoted 8 lines
Looking at the scrub code, if dev->readonly is set, no repairs are done, so the only concern is to wait for any outstanding IOs and then switch to RO.
david
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html