[PATCH 1/2] ext4: fix handling of nodelalloc parameter

[PATCH 1/2] ext4: fix handling of nodelalloc parameter

[Piotr Sarna]

Commit 26092bf ("ext4: use a table-driven handler for mount options")
introduced buggy handling of nodelalloc parameter in mount command.

After explicitly using delalloc or nodelalloc parameter in mount command,
MOPT_EXPLICIT flag is set. After that, a test ensures that "data=journal"
and "delalloc" parameters are not simultaneously activated.
Unluckily, the mentioned test reports a bug in both situations:
- "data=journal,delalloc"
- "data=journal,nodelalloc"
whereas the second one is perfectly legal and acceptable.

A simple solution to this problem is in setting EXPLICIT_DELALLOC flag
properly. This patch ensures that EXPLICIT_DELALLOC flag is set only
if "delalloc" parameter was used, and not set in case of "nodelalloc".

Signed-off-by: Piotr Sarna <p.sarna@partner.samsung.com>
Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 fs/ext4/super.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 94cc84d..10f9bb0 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1467,7 +1467,8 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
 	if (args->from && (m->flags & MOPT_GTE0) && (arg < 0))
 		return -1;
 	if (m->flags & MOPT_EXPLICIT)
-		set_opt2(sb, EXPLICIT_DELALLOC);
+		if (m->flags & MOPT_SET)
+			set_opt2(sb, EXPLICIT_DELALLOC);
 	if (m->flags & MOPT_CLEAR_ERR)
 		clear_opt(sb, ERRORS_MASK);
 	if (token == Opt_noquota && sb_any_quota_loaded(sb)) {
-- 
1.7.9.5

[PATCH 2/2] ext4: improve mount/remount error handling

[Piotr Sarna]

Commit 5688978 ("ext4: improve handling of conflicting mount options")
introduced incorrect messages shown while choosing wrong mount options.

Firstly, both cases of incorrect mount options, "data=journal,delalloc"
and "data=journal,dioread_nolock" result in the same error message.

Secondly, the problem above isn't solved for remount option: the mismatched
parameter is simply ignored. Moreover, ext4_msg states that remount
with options "data=journal,delalloc" succeeded, which is not true.

To fix it up, I added a simple check after parse_options() call to ensure
that data=journal and delalloc/dioread_nolock parameters are not present
at the same time.

Signed-off-by: Piotr Sarna <p.sarna@partner.samsung.com>
Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 fs/ext4/super.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 10f9bb0..ab0b23b 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3452,7 +3452,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 		}
 		if (test_opt(sb, DIOREAD_NOLOCK)) {
 			ext4_msg(sb, KERN_ERR, "can't mount with "
-				 "both data=journal and delalloc");
+				 "both data=journal and dioread_nolock");
 			goto failed_mount;
 		}
 		if (test_opt(sb, DELALLOC))
@@ -4653,6 +4653,21 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 		goto restore_opts;
 	}
 
+	if (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA) {
+		if (test_opt2(sb, EXPLICIT_DELALLOC)) {
+			ext4_msg(sb, KERN_ERR, "can't mount with "
+				 "both data=journal and delalloc");
+			err = -EINVAL;
+			goto restore_opts;
+		}
+		if (test_opt(sb, DIOREAD_NOLOCK)) {
+			ext4_msg(sb, KERN_ERR, "can't mount with "
+				 "both data=journal and dioread_nolock");
+			err = -EINVAL;
+			goto restore_opts;
+		}
+	}
+
 	if (sbi->s_mount_flags & EXT4_MF_FS_ABORTED)
 		ext4_abort(sb, "Abort forced by user");
 
-- 
1.7.9.5

Re: [PATCH 1/2] ext4: fix handling of nodelalloc parameter

[Theodore Ts'o]

On Fri, Aug 02, 2013 at 02:03:46PM +0200, Piotr Sarna wrote:
> Commit 26092bf ("ext4: use a table-driven handler for mount options")
> introduced buggy handling of nodelalloc parameter in mount command.
> 
> After explicitly using delalloc or nodelalloc parameter in mount command,
> MOPT_EXPLICIT flag is set. After that, a test ensures that "data=journal"
> and "delalloc" parameters are not simultaneously activated.
> Unluckily, the mentioned test reports a bug in both situations:
> - "data=journal,delalloc"
> - "data=journal,nodelalloc"
> whereas the second one is perfectly legal and acceptable.
> 
> A simple solution to this problem is in setting EXPLICIT_DELALLOC flag
> properly. This patch ensures that EXPLICIT_DELALLOC flag is set only
> if "delalloc" parameter was used, and not set in case of "nodelalloc".

Thanks for this bug report and patch.  There is an even simpler way of
fixing this doesn't involve adding an additional check in the code,
though.  Just make the following change the table entry:

     {Opt_nodelalloc, EXT4_MOUNT_DELALLOC,
-      MOPT_EXT4_ONLY | MOPT_CLEAR | MOPT_EXPLICIT},
+      MOPT_EXT4_ONLY | MOPT_CLEAR},

I'll send out a patch which does this...

					- Ted

Re: [PATCH 2/2] ext4: improve mount/remount error handling

[Theodore Ts'o]

On Fri, Aug 02, 2013 at 02:03:47PM +0200, Piotr Sarna wrote:
> Commit 5688978 ("ext4: improve handling of conflicting mount options")
> introduced incorrect messages shown while choosing wrong mount options.
> 
> Firstly, both cases of incorrect mount options, "data=journal,delalloc"
> and "data=journal,dioread_nolock" result in the same error message.
> 
> Secondly, the problem above isn't solved for remount option: the mismatched
> parameter is simply ignored. Moreover, ext4_msg states that remount
> with options "data=journal,delalloc" succeeded, which is not true.
> 
> To fix it up, I added a simple check after parse_options() call to ensure
> that data=journal and delalloc/dioread_nolock parameters are not present
> at the same time.
> 
> Signed-off-by: Piotr Sarna <p.sarna@partner.samsung.com>
> Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Applied, thanks.

						- Ted