[PATCH] ext4: don't give the "disabling delalloc" if not explicitly specified

[PATCH] ext4: don't give the "disabling delalloc" if not explicitly specified

[Theodore Ts'o]

If both delalloc and data=journalled are enabled, then a warning
message that delalloc will be disabled is displayed at mount time.
Since delalloc is the default, this means this warning is always
printed which really isn't necessary.  So disable the warning message
unless the user explicitly asked for delalloc.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/ext4.h  |    2 ++
 fs/ext4/super.c |    6 ++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index be593d5..b98b5a1 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -916,6 +916,8 @@ struct ext4_inode_info {
 #define EXT4_MOUNT_DISCARD		0x40000000 /* Issue DISCARD requests */
 #define EXT4_MOUNT_INIT_INODE_TABLE	0x80000000 /* Initialize uninitialized itables */
 
+#define EXT4_MOUNT2_DELALLOC_EXPLICIT	0x00000001 /* specified by user */
+
 #define clear_opt(sb, opt)		EXT4_SB(sb)->s_mount_opt &= \
 						~EXT4_MOUNT_##opt
 #define set_opt(sb, opt)		EXT4_SB(sb)->s_mount_opt |= \
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 44d0c8d..447eb45 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1817,6 +1817,7 @@ set_qf_format:
 			break;
 		case Opt_delalloc:
 			set_opt(sb, DELALLOC);
+			set_opt2(sb, DELALLOC_EXPLICIT);
 			break;
 		case Opt_block_validity:
 			set_opt(sb, BLOCK_VALIDITY);
@@ -3681,8 +3682,9 @@ no_journal:
 
 	if (test_opt(sb, DELALLOC) &&
 	    (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA)) {
-		ext4_msg(sb, KERN_WARNING, "Ignoring delalloc option - "
-			 "requested data journaling mode");
+		if (test_opt2(sb, DELALLOC_EXPLICIT))
+			ext4_msg(sb, KERN_WARNING, "Ignoring delalloc option "
+				 "- requested data journaling mode");
 		clear_opt(sb, DELALLOC);
 	}
 	if (test_opt(sb, DIOREAD_NOLOCK)) {
-- 
1.7.4.1.22.gec8e1.dirty

Re: [PATCH] ext4: don't give the "disabling delalloc" if not explicitly specified

[Lukas Czerner]

On Sat, 13 Aug 2011, Theodore Ts'o wrote:

> If both delalloc and data=journalled are enabled, then a warning
> message that delalloc will be disabled is displayed at mount time.
> Since delalloc is the default, this means this warning is always
> printed which really isn't necessary.  So disable the warning message
> unless the user explicitly asked for delalloc.

Hi Ted,

that seems a bit confusing. Since delalloc is the default how would the
user know that it will be disabled if data=journalled mode is used ?
That said, if users are complaining about the warning so lets at least
add this information into mount man page, but if they are not I am not
sure that we really need to change that.

Thanks!
-Lukas


> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  fs/ext4/ext4.h  |    2 ++
>  fs/ext4/super.c |    6 ++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index be593d5..b98b5a1 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -916,6 +916,8 @@ struct ext4_inode_info {
>  #define EXT4_MOUNT_DISCARD		0x40000000 /* Issue DISCARD requests */
>  #define EXT4_MOUNT_INIT_INODE_TABLE	0x80000000 /* Initialize uninitialized itables */
>  
> +#define EXT4_MOUNT2_DELALLOC_EXPLICIT	0x00000001 /* specified by user */
> +
>  #define clear_opt(sb, opt)		EXT4_SB(sb)->s_mount_opt &= \
>  						~EXT4_MOUNT_##opt
>  #define set_opt(sb, opt)		EXT4_SB(sb)->s_mount_opt |= \
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 44d0c8d..447eb45 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1817,6 +1817,7 @@ set_qf_format:
>  			break;
>  		case Opt_delalloc:
>  			set_opt(sb, DELALLOC);
> +			set_opt2(sb, DELALLOC_EXPLICIT);
>  			break;
>  		case Opt_block_validity:
>  			set_opt(sb, BLOCK_VALIDITY);
> @@ -3681,8 +3682,9 @@ no_journal:
>  
>  	if (test_opt(sb, DELALLOC) &&
>  	    (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA)) {
> -		ext4_msg(sb, KERN_WARNING, "Ignoring delalloc option - "
> -			 "requested data journaling mode");
> +		if (test_opt2(sb, DELALLOC_EXPLICIT))
> +			ext4_msg(sb, KERN_WARNING, "Ignoring delalloc option "
> +				 "- requested data journaling mode");
>  		clear_opt(sb, DELALLOC);
>  	}
>  	if (test_opt(sb, DIOREAD_NOLOCK)) {
> 

Re: [PATCH] ext4: don't give the "disabling delalloc" if not explicitly specified

[Eric Sandeen]

On 8/15/11 5:14 AM, Lukas Czerner wrote:
> On Sat, 13 Aug 2011, Theodore Ts'o wrote:
> 
>> If both delalloc and data=journalled are enabled, then a warning
>> message that delalloc will be disabled is displayed at mount time.
>> Since delalloc is the default, this means this warning is always
>> printed which really isn't necessary.  So disable the warning message
>> unless the user explicitly asked for delalloc.
> 
> Hi Ted,
> 
> that seems a bit confusing. Since delalloc is the default how would the
> user know that it will be disabled if data=journalled mode is used ?
> That said, if users are complaining about the warning so lets at least
> add this information into mount man page, but if they are not I am not
> sure that we really need to change that.

I concur...

The giant behavior-options switch in ext4 is confusing enough; if enabling
one option disables another default option, I think that explicitly stating
it in the logs is useful.  Doing so silently just covers up the behavior.

If users are unhappy with the message, it's probably more because of 
the fact of the matter, and not because of the presentation of the fact.  :)

IOW, what does this patch really improve?

-Eric

> Thanks!
> -Lukas
> 
> 
>>
>> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
>> ---
>>  fs/ext4/ext4.h  |    2 ++
>>  fs/ext4/super.c |    6 ++++--
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>> index be593d5..b98b5a1 100644
>> --- a/fs/ext4/ext4.h
>> +++ b/fs/ext4/ext4.h
>> @@ -916,6 +916,8 @@ struct ext4_inode_info {
>>  #define EXT4_MOUNT_DISCARD		0x40000000 /* Issue DISCARD requests */
>>  #define EXT4_MOUNT_INIT_INODE_TABLE	0x80000000 /* Initialize uninitialized itables */
>>  
>> +#define EXT4_MOUNT2_DELALLOC_EXPLICIT	0x00000001 /* specified by user */
>> +
>>  #define clear_opt(sb, opt)		EXT4_SB(sb)->s_mount_opt &= \
>>  						~EXT4_MOUNT_##opt
>>  #define set_opt(sb, opt)		EXT4_SB(sb)->s_mount_opt |= \
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 44d0c8d..447eb45 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -1817,6 +1817,7 @@ set_qf_format:
>>  			break;
>>  		case Opt_delalloc:
>>  			set_opt(sb, DELALLOC);
>> +			set_opt2(sb, DELALLOC_EXPLICIT);
>>  			break;
>>  		case Opt_block_validity:
>>  			set_opt(sb, BLOCK_VALIDITY);
>> @@ -3681,8 +3682,9 @@ no_journal:
>>  
>>  	if (test_opt(sb, DELALLOC) &&
>>  	    (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA)) {
>> -		ext4_msg(sb, KERN_WARNING, "Ignoring delalloc option - "
>> -			 "requested data journaling mode");
>> +		if (test_opt2(sb, DELALLOC_EXPLICIT))
>> +			ext4_msg(sb, KERN_WARNING, "Ignoring delalloc option "
>> +				 "- requested data journaling mode");
>>  		clear_opt(sb, DELALLOC);
>>  	}
>>  	if (test_opt(sb, DIOREAD_NOLOCK)) {
>>
> --
> 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] ext4: don't give the "disabling delalloc" if not explicitly specified

[Ted Ts'o]

On Mon, Aug 15, 2011 at 10:59:02AM -0500, Eric Sandeen wrote:
> 
> The giant behavior-options switch in ext4 is confusing enough; if enabling
> one option disables another default option, I think that explicitly stating
> it in the logs is useful.  Doing so silently just covers up the behavior.
> 
> If users are unhappy with the message, it's probably more because of 
> the fact of the matter, and not because of the presentation of the fact.  :)

Most users probably have no idea what "delalloc" actually means.  So
when they get a message that saying that data=journalled has disabled
delalloc, it could easily be seen as noise.  I was moved to do it
because I got tired of seeing the message over, and over, and over
again when running xfstests.

Maybe an improvement would be (1) to document what data=journal
implies in the Documentation/filesystems/ext4.txt, (2) change the
message to explicitly say "delayed allocation" instead of "delalloc"
(although many people won't have any idea what "delayed allocation"
means either), and (3) make it a printk_once thing.

I guess I don't agree with the fundamental presumption which is that
users should be looking at the dmesg output to understand what various
things mean, and if they didn't explicitly specify delalloc, why
should we complain about the fact that both delalloc and data=journal
were specified (when in fact it wasn't specified).

						- Ted

Re: [PATCH] ext4: don't give the "disabling delalloc" if not explicitly specified

[Lukas Czerner]

On Mon, 15 Aug 2011, Ted Ts'o wrote:

> On Mon, Aug 15, 2011 at 10:59:02AM -0500, Eric Sandeen wrote:
> > 
> > The giant behavior-options switch in ext4 is confusing enough; if enabling
> > one option disables another default option, I think that explicitly stating
> > it in the logs is useful.  Doing so silently just covers up the behavior.
> > 
> > If users are unhappy with the message, it's probably more because of 
> > the fact of the matter, and not because of the presentation of the fact.  :)
> 
> Most users probably have no idea what "delalloc" actually means.  So
> when they get a message that saying that data=journalled has disabled
> delalloc, it could easily be seen as noise.  I was moved to do it
> because I got tired of seeing the message over, and over, and over
> again when running xfstests.

So the actual users of data=journal does not care all that much about
it apparently. If the information for some of the users is "noise" than
be it, there is a lot of "noise" in the logs but it is useful for people
who do understand it, or to people who can search for it.

> 
> Maybe an improvement would be (1) to document what data=journal
> implies in the Documentation/filesystems/ext4.txt, (2) change the
> message to explicitly say "delayed allocation" instead of "delalloc"
> (although many people won't have any idea what "delayed allocation"
> means either), and (3) make it a printk_once thing.

That information actually already is in
Documentation/filesystems/ext4.txt but not in the Options section.
However important is to have this information in mount man page, because
that is where usually users go for info right ?

I like the "delayed allocation" version of the warning, but I do not
think we should do it with printk_once as it seems even more confusing
to me.

> 
> I guess I don't agree with the fundamental presumption which is that
> users should be looking at the dmesg output to understand what various
> things mean, and if they didn't explicitly specify delalloc, why
> should we complain about the fact that both delalloc and data=journal
> were specified (when in fact it wasn't specified).

Yeah, we should probably say that we are "disabling delayed allocation".

> 
> 						- Ted
> 

Thanks!
-Lukas

Re: [PATCH] ext4: don't give the "disabling delalloc" if not explicitly specified

[Eric Sandeen]

On 8/15/11 12:54 PM, Ted Ts'o wrote:
> On Mon, Aug 15, 2011 at 10:59:02AM -0500, Eric Sandeen wrote:
>>
>> The giant behavior-options switch in ext4 is confusing enough; if enabling
>> one option disables another default option, I think that explicitly stating
>> it in the logs is useful.  Doing so silently just covers up the behavior.
>>
>> If users are unhappy with the message, it's probably more because of 
>> the fact of the matter, and not because of the presentation of the fact.  :)
> 
> Most users probably have no idea what "delalloc" actually means.  So
> when they get a message that saying that data=journalled has disabled
> delalloc, it could easily be seen as noise.  I was moved to do it
> because I got tired of seeing the message over, and over, and over
> again when running xfstests.
> 
> Maybe an improvement would be (1) to document what data=journal
> implies in the Documentation/filesystems/ext4.txt, (2) change the
> message to explicitly say "delayed allocation" instead of "delalloc"
> (although many people won't have any idea what "delayed allocation"
> means either), and (3) make it a printk_once thing.
> 
> I guess I don't agree with the fundamental presumption which is that
> users should be looking at the dmesg output to understand what various
> things mean, and if they didn't explicitly specify delalloc, why
> should we complain about the fact that both delalloc and data=journal
> were specified (when in fact it wasn't specified).

Well, just my $0.02, I won't fight it.

One thing I do want, though, is to be able to look at logs and know
what mode we're running in.  I guess we do print all specified options,
so those in the know, will know that delalloc is off if data=journal is on.

But I think we could use some consistency here at least:

[root@inode ~]# mount -o data=journal,dioread_nolock /dev/sdb5 /mnt/test
[root@inode ~]#  /* YAY it worked! */

[root@inode ~]# dmesg | tail
...
[269530.183245] EXT4-fs (sdb5): Ignoring delalloc option - requested data journaling mode
[269530.191170] EXT4-fs (sdb5): Ignoring dioread_nolock option - requested data journaling mode

/* boo it didn't work */

Better hit that one too, I guess, and any other option which disables
another default ...  (honestly, I think it'd be better to fail the mount
in cases like this if conflicting options are specified).

-Eric


-Eric

[PATCH] ext4: improve handling of conflincting mount options

[Theodore Ts'o]

If the user explicitly specifies conflicting mount options for
delalloc or dioread_nolock and data=journal, fail the mount, instead
of printing a warning and continuing (since many user's won't look at
dmesg and notice the warning).

Also, print a single warning that data=journal implies that delayed
allocation is not on by default (since it's not supported), and
furthermore that O_DIRECT is not supported.  Improve the text in
Documentation/filesystems/ext4.txt so this is clear there as well.

Similarly, if the dioread_nolock mount option is specified when the
file system block size != PAGE_SIZE, fail the mount instead of
printing a warning message and ignoring the mount option.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---

n.b.  I plan to follow this up with a patch to the mount man page in
util-linux-ng

 Documentation/filesystems/ext4.txt |    8 ++++--
 fs/ext4/ext4.h                     |    3 ++
 fs/ext4/super.c                    |   46 +++++++++++++++++++++---------------
 3 files changed, 35 insertions(+), 22 deletions(-)

diff --git a/Documentation/filesystems/ext4.txt b/Documentation/filesystems/ext4.txt
index 232a575..168242b 100644
--- a/Documentation/filesystems/ext4.txt
+++ b/Documentation/filesystems/ext4.txt
@@ -160,7 +160,9 @@ noload			if the filesystem was not unmounted cleanly,
                      	lead to any number of problems.
 
 data=journal		All data are committed into the journal prior to being
-			written into the main file system.
+			written into the main file system.  Enabling
+			this mode will disable delayed allocation and
+			O_DIRECT support.
 
 data=ordered	(*)	All data are forced directly out to the main file
 			system prior to its metadata being committed to the
@@ -419,8 +421,8 @@ written to the journal first, and then to its final location.
 In the event of a crash, the journal can be replayed, bringing both data and
 metadata into a consistent state.  This mode is the slowest except when data
 needs to be read from and written to disk at the same time where it
-outperforms all others modes.  Currently ext4 does not have delayed
-allocation support if this data journalling mode is selected.
+outperforms all others modes.  Enabling this mode will disable delayed
+allocation and O_DIRECT support.
 
 /proc entries
 =============
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index e717dfd..062c668 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -917,6 +917,9 @@ struct ext4_inode_info {
 #define EXT4_MOUNT_DISCARD		0x40000000 /* Issue DISCARD requests */
 #define EXT4_MOUNT_INIT_INODE_TABLE	0x80000000 /* Initialize uninitialized itables */
 
+#define EXT4_MOUNT2_EXPLICIT_DELALLOC	0x00000001 /* User explicitly
+						      specified delalloc */
+
 #define clear_opt(sb, opt)		EXT4_SB(sb)->s_mount_opt &= \
 						~EXT4_MOUNT_##opt
 #define set_opt(sb, opt)		EXT4_SB(sb)->s_mount_opt |= \
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 44d0c8d..c3e3a80 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1801,6 +1801,7 @@ set_qf_format:
 			break;
 		case Opt_nodelalloc:
 			clear_opt(sb, DELALLOC);
+			clear_opt2(sb, EXPLICIT_DELALLOC);
 			break;
 		case Opt_mblk_io_submit:
 			set_opt(sb, MBLK_IO_SUBMIT);
@@ -1817,6 +1818,7 @@ set_qf_format:
 			break;
 		case Opt_delalloc:
 			set_opt(sb, DELALLOC);
+			set_opt2(sb, EXPLICIT_DELALLOC);
 			break;
 		case Opt_block_validity:
 			set_opt(sb, BLOCK_VALIDITY);
@@ -3224,6 +3226,31 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 			   &journal_ioprio, NULL, 0))
 		goto failed_mount;
 
+	if (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA) {
+		printk_once(KERN_WARNING "EXT4-fs: Warning: mounting "
+			    "with data=journal disables delayed "
+			    "allocation and O_DIRECT support!\n");
+		if (test_opt2(sb, EXPLICIT_DELALLOC)) {
+			ext4_msg(sb, KERN_ERR, "can't mount with "
+				 "both data=journal and delalloc");
+			goto failed_mount;
+		}
+		if (test_opt(sb, DIOREAD_NOLOCK)) {
+			ext4_msg(sb, KERN_ERR, "can't mount with "
+				 "both data=journal and delalloc");
+			goto failed_mount;
+		}
+		if (test_opt(sb, DELALLOC))
+			clear_opt(sb, DELALLOC);
+	}
+	if (test_opt(sb, DIOREAD_NOLOCK)) {
+		if (sb->s_blocksize < PAGE_SIZE) {
+			ext4_msg(sb, KERN_ERR, "can't mount with "
+				 "dioread_nolock if block size != PAGE_SIZE");
+			goto failed_mount;
+		}
+	}
+
 	sb->s_flags = (sb->s_flags & ~MS_POSIXACL) |
 		(test_opt(sb, POSIX_ACL) ? MS_POSIXACL : 0);
 
@@ -3679,25 +3706,6 @@ no_journal:
 			 "available");
 	}
 
-	if (test_opt(sb, DELALLOC) &&
-	    (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA)) {
-		ext4_msg(sb, KERN_WARNING, "Ignoring delalloc option - "
-			 "requested data journaling mode");
-		clear_opt(sb, DELALLOC);
-	}
-	if (test_opt(sb, DIOREAD_NOLOCK)) {
-		if (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA) {
-			ext4_msg(sb, KERN_WARNING, "Ignoring dioread_nolock "
-				"option - requested data journaling mode");
-			clear_opt(sb, DIOREAD_NOLOCK);
-		}
-		if (sb->s_blocksize < PAGE_SIZE) {
-			ext4_msg(sb, KERN_WARNING, "Ignoring dioread_nolock "
-				"option - block size is too small");
-			clear_opt(sb, DIOREAD_NOLOCK);
-		}
-	}