[PATCH] ext4: do not try to write superblock on journal-less readonly remount

[PATCH] ext4: do not try to write superblock on journal-less readonly remount

[Michael Tokarev]

When a journal-less ext4 filesystem is mounted on a read-only block
device (blockdev --setro will do), each remount (for other, unrelated,
flags, like suid=>nosuid etc) results in a series of scary messages
from kernel telling about I/O errors on the device.

This is becauese of the following code ext4_remount():

       if (sbi->s_journal == NULL)
                ext4_commit_super(sb, 1);

at the end of remount procedure, which forces writing (flushing) of
a superblock regardless whenever it is dirty or not, if the filesystem
is readonly or not, and whenever the device itself is readonly or not.

The proposed fix tests whenever both old mount flags and new mount
flags does not include MS_READONLY, and only in this case calls
ext4_commit_super().

Maybe it is sufficient to check for MS_READONLY just in old mount
options (old_sb_flags).  Note this is journal-less mode, so, for
example, we weren't have journal replay operation, so if old flags
include MS_REASONLY, we shuold have no dirty blocks at all, and
there's no reason to call ext4_commit_super().

But only in case both old and new flags include MS_READONLY we're
certain we will not write anything - if new flag does not include
this bit, we will write sooner or later anyway, so preventing just
one commit_super() at the _beginning_ of mount is not really necessary.

This change probably applicable to -stable, -- not because it fixes
a serious bug, but because the messages printed by the kernel are
rather scary for an average user.  On the other hand, actual usage
of ext4 in nojournal mode on a read-only medium is very rare.

Thanks to Eric Sandeen for help in diagnosing this issue.

Signed-off-By: Michael Tokarev <mjt@tls.msk.ru>
---
 fs/ext4/super.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 3e0851e..2e896fd 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4687,7 +4687,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 	}
 
 	ext4_setup_system_zone(sb);
-	if (sbi->s_journal == NULL)
+	if (sbi->s_journal == NULL && !(sb->s_flags & old_sb_flags & MS_RDONLY))
 		ext4_commit_super(sb, 1);
 
 	unlock_super(sb);
-- 
1.7.10.4

Re: [PATCH] ext4: do not try to write superblock on journal-less readonly remount

[Lukáš Czerner]

On Thu, 25 Oct 2012, Michael Tokarev wrote:

> Date: Thu, 25 Oct 2012 12:39:57 +0400
> From: Michael Tokarev <mjt@tls.msk.ru>
> To: linux-ext4@vger.kernel.org
> Cc: sandeen@redhat.com, Michael Tokarev <mjt@tls.msk.ru>
> Subject: [PATCH] ext4: do not try to write superblock on journal-less readonly
>      remount
> 
> When a journal-less ext4 filesystem is mounted on a read-only block
> device (blockdev --setro will do), each remount (for other, unrelated,
> flags, like suid=>nosuid etc) results in a series of scary messages
> from kernel telling about I/O errors on the device.

Hi Michael,

I am not able to reproduce the problem you're seeing:

mkfs.ext4 /dev/sdd1
tune2fs -O ^has_journal /dev/sdd1
blockdev --setro /dev/sdd1
mount /dev/sdd1 /mnt/test

and then

mount -o remount,suid /dev/sdd1
mount -o remount,nosuid /dev/sdd1
mount -o remount,noatime /dev/sdd1
mount -o remount,relatime /dev/sdd1
mount -o remount,relatime,commit=20 /dev/sdd1

just does not produce any errors. Both /var/log/messages and dmesg
are clear.

mount shows
...
/dev/sdd1 on /mnt/test type ext4 (ro,nosuid,noatime,relatime,commit=20)
...


This is on 3.7.0-rc2

Am I missing something ?

Thanks!
-Lukas

> 
> This is becauese of the following code ext4_remount():
> 
>        if (sbi->s_journal == NULL)
>                 ext4_commit_super(sb, 1);
> 
> at the end of remount procedure, which forces writing (flushing) of
> a superblock regardless whenever it is dirty or not, if the filesystem
> is readonly or not, and whenever the device itself is readonly or not.
> 
> The proposed fix tests whenever both old mount flags and new mount
> flags does not include MS_READONLY, and only in this case calls
> ext4_commit_super().
> 
> Maybe it is sufficient to check for MS_READONLY just in old mount
> options (old_sb_flags).  Note this is journal-less mode, so, for
> example, we weren't have journal replay operation, so if old flags
> include MS_REASONLY, we shuold have no dirty blocks at all, and
> there's no reason to call ext4_commit_super().
> 
> But only in case both old and new flags include MS_READONLY we're
> certain we will not write anything - if new flag does not include
> this bit, we will write sooner or later anyway, so preventing just
> one commit_super() at the _beginning_ of mount is not really necessary.
> 
> This change probably applicable to -stable, -- not because it fixes
> a serious bug, but because the messages printed by the kernel are
> rather scary for an average user.  On the other hand, actual usage
> of ext4 in nojournal mode on a read-only medium is very rare.
> 
> Thanks to Eric Sandeen for help in diagnosing this issue.
> 
> Signed-off-By: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  fs/ext4/super.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 3e0851e..2e896fd 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4687,7 +4687,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>  	}
>  
>  	ext4_setup_system_zone(sb);
> -	if (sbi->s_journal == NULL)
> +	if (sbi->s_journal == NULL && !(sb->s_flags & old_sb_flags & MS_RDONLY))
>  		ext4_commit_super(sb, 1);
>  
>  	unlock_super(sb);
> 

Re: [PATCH] ext4: do not try to write superblock on journal-less readonly remount

[Michael Tokarev]

On 25.10.2012 16:43, Lukáš Czerner wrote:
> On Thu, 25 Oct 2012, Michael Tokarev wrote:
> 
>> Date: Thu, 25 Oct 2012 12:39:57 +0400
>> From: Michael Tokarev <mjt@tls.msk.ru>
>> To: linux-ext4@vger.kernel.org
>> Cc: sandeen@redhat.com, Michael Tokarev <mjt@tls.msk.ru>
>> Subject: [PATCH] ext4: do not try to write superblock on journal-less readonly
>>      remount
>>
>> When a journal-less ext4 filesystem is mounted on a read-only block
>> device (blockdev --setro will do), each remount (for other, unrelated,
>> flags, like suid=>nosuid etc) results in a series of scary messages
>> from kernel telling about I/O errors on the device.
> 
> Hi Michael,
> 
> I am not able to reproduce the problem you're seeing:
> 
> mkfs.ext4 /dev/sdd1
> tune2fs -O ^has_journal /dev/sdd1
> blockdev --setro /dev/sdd1
> mount /dev/sdd1 /mnt/test
> 
> and then
> 
> mount -o remount,suid /dev/sdd1
> mount -o remount,nosuid /dev/sdd1
> mount -o remount,noatime /dev/sdd1
> mount -o remount,relatime /dev/sdd1
> mount -o remount,relatime,commit=20 /dev/sdd1
> 
> just does not produce any errors. Both /var/log/messages and dmesg
> are clear.

Interesting.

Actual situation where I observed this issue was when the device
really was read-only.  In my case it was a virtual machine (kvm)
with a read-only virtio drive (-drive file=foo,if=virtio,readonly=on).
I played with a "live CD"-type system.

Now when I look at it, I'm not sure if I were really able to
reproduce it with regular /dev/sdNN and blockdev --setro.  I
*think* it was reproducible, but actually I can't.  So it looks
like blockdev --setro does not do what it claims to do -- the
actual device isn't really set read-only.

The errors produced at remount are real, when the device in question
really dislikes (reject) writes.  Apparently --setro isn't enough --
somewhere at kernel level write for such device are actually succeeded
instead of being errored out, when the device itself does not reject
writes.  So the impact is even less severe when I initially thought.

Thanks,

/mjt
--
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: do not try to write superblock on journal-less readonly remount

[Michael Tokarev]

Ping?  Almost 2 months has passed since initial patch...

Thanks,

/mjt

On 25.10.2012 12:39, Michael Tokarev wrote:
> When a journal-less ext4 filesystem is mounted on a read-only block
> device (blockdev --setro will do), each remount (for other, unrelated,
> flags, like suid=>nosuid etc) results in a series of scary messages
> from kernel telling about I/O errors on the device.
> 
> This is becauese of the following code ext4_remount():
> 
>        if (sbi->s_journal == NULL)
>                 ext4_commit_super(sb, 1);
> 
> at the end of remount procedure, which forces writing (flushing) of
> a superblock regardless whenever it is dirty or not, if the filesystem
> is readonly or not, and whenever the device itself is readonly or not.
> 
> The proposed fix tests whenever both old mount flags and new mount
> flags does not include MS_READONLY, and only in this case calls
> ext4_commit_super().
> 
> Maybe it is sufficient to check for MS_READONLY just in old mount
> options (old_sb_flags).  Note this is journal-less mode, so, for
> example, we weren't have journal replay operation, so if old flags
> include MS_REASONLY, we shuold have no dirty blocks at all, and
> there's no reason to call ext4_commit_super().
> 
> But only in case both old and new flags include MS_READONLY we're
> certain we will not write anything - if new flag does not include
> this bit, we will write sooner or later anyway, so preventing just
> one commit_super() at the _beginning_ of mount is not really necessary.
> 
> This change probably applicable to -stable, -- not because it fixes
> a serious bug, but because the messages printed by the kernel are
> rather scary for an average user.  On the other hand, actual usage
> of ext4 in nojournal mode on a read-only medium is very rare.
> 
> Thanks to Eric Sandeen for help in diagnosing this issue.
> 
> Signed-off-By: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  fs/ext4/super.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 3e0851e..2e896fd 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4687,7 +4687,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>  	}
>  
>  	ext4_setup_system_zone(sb);
> -	if (sbi->s_journal == NULL)
> +	if (sbi->s_journal == NULL && !(sb->s_flags & old_sb_flags & MS_RDONLY))
>  		ext4_commit_super(sb, 1);
>  
>  	unlock_super(sb);

Re: [PATCH] ext4: do not try to write superblock on journal-less readonly remount

[Eric Sandeen]

On 12/18/12 2:14 AM, Michael Tokarev wrote:
> Ping?  Almost 2 months has passed since initial patch...
> 
> Thanks,
> 
> /mjt

Michael, Lukas commented a while ago (10/25) that he was unable to reproduce
the problem.  Do you have any comment on that?  TBH it's long enough
ago that I've forgotten the issue ;)

But Lukas' question may be what's holding Ted up.

-Eric

> On 25.10.2012 12:39, Michael Tokarev wrote:
>> When a journal-less ext4 filesystem is mounted on a read-only block
>> device (blockdev --setro will do), each remount (for other, unrelated,
>> flags, like suid=>nosuid etc) results in a series of scary messages
>> from kernel telling about I/O errors on the device.
>>
>> This is becauese of the following code ext4_remount():
>>
>>        if (sbi->s_journal == NULL)
>>                 ext4_commit_super(sb, 1);
>>
>> at the end of remount procedure, which forces writing (flushing) of
>> a superblock regardless whenever it is dirty or not, if the filesystem
>> is readonly or not, and whenever the device itself is readonly or not.
>>
>> The proposed fix tests whenever both old mount flags and new mount
>> flags does not include MS_READONLY, and only in this case calls
>> ext4_commit_super().
>>
>> Maybe it is sufficient to check for MS_READONLY just in old mount
>> options (old_sb_flags).  Note this is journal-less mode, so, for
>> example, we weren't have journal replay operation, so if old flags
>> include MS_REASONLY, we shuold have no dirty blocks at all, and
>> there's no reason to call ext4_commit_super().
>>
>> But only in case both old and new flags include MS_READONLY we're
>> certain we will not write anything - if new flag does not include
>> this bit, we will write sooner or later anyway, so preventing just
>> one commit_super() at the _beginning_ of mount is not really necessary.
>>
>> This change probably applicable to -stable, -- not because it fixes
>> a serious bug, but because the messages printed by the kernel are
>> rather scary for an average user.  On the other hand, actual usage
>> of ext4 in nojournal mode on a read-only medium is very rare.
>>
>> Thanks to Eric Sandeen for help in diagnosing this issue.
>>
>> Signed-off-By: Michael Tokarev <mjt@tls.msk.ru>
>> ---
>>  fs/ext4/super.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 3e0851e..2e896fd 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -4687,7 +4687,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>>  	}
>>  
>>  	ext4_setup_system_zone(sb);
>> -	if (sbi->s_journal == NULL)
>> +	if (sbi->s_journal == NULL && !(sb->s_flags & old_sb_flags & MS_RDONLY))
>>  		ext4_commit_super(sb, 1);
>>  
>>  	unlock_super(sb);
> 

Re: [PATCH] ext4: do not try to write superblock on journal-less readonly remount

[Michael Tokarev]

On 18.12.2012 19:20, Eric Sandeen wrote:
> On 12/18/12 2:14 AM, Michael Tokarev wrote:
>> Ping?  Almost 2 months has passed since initial patch...
>>
>> Thanks,
>>
>> /mjt
> 
> Michael, Lukas commented a while ago (10/25) that he was unable to reproduce
> the problem.  Do you have any comment on that?  TBH it's long enough
> ago that I've forgotten the issue ;)

Yeah okay.

The two reproducers I've found so far are both about using true read-only
media.  One original where I've hit it was a virtual machine (KVM) with
a read-only virtio drive:

  kvm ... -drive file=guest.img,if=virtio,readonly=yes

(It does not work with IDE emulation because there's no way on IDE to pass
the "readonly" flag).

Another way I found is to use an SD card in an USB card reader with the
"read-only" jumper in "on" position (or a micro-SD to SD adaptor with
such a jumper).

In both cases mount -o remount in guest results in a series of error
messages from kernel - it complains about write errors.

My initial comment that it is enough to set block device to be read-only
using blockdev --setro is wrong, -- apparently ext4fs uses write paths
that bypasses the block-level RO checks -- which is, apparenlty, also
wrong, but it's a different matter.

Thanks,

/mjt

>> On 25.10.2012 12:39, Michael Tokarev wrote:
>>> When a journal-less ext4 filesystem is mounted on a read-only block
>>> device (blockdev --setro will do), each remount (for other, unrelated,
>>> flags, like suid=>nosuid etc) results in a series of scary messages
>>> from kernel telling about I/O errors on the device.
>>>
>>> This is becauese of the following code ext4_remount():
>>>
>>>        if (sbi->s_journal == NULL)
>>>                 ext4_commit_super(sb, 1);
>>>
>>> at the end of remount procedure, which forces writing (flushing) of
>>> a superblock regardless whenever it is dirty or not, if the filesystem
>>> is readonly or not, and whenever the device itself is readonly or not.
>>>
>>> The proposed fix tests whenever both old mount flags and new mount
>>> flags does not include MS_READONLY, and only in this case calls
>>> ext4_commit_super().
>>>
>>> Maybe it is sufficient to check for MS_READONLY just in old mount
>>> options (old_sb_flags).  Note this is journal-less mode, so, for
>>> example, we weren't have journal replay operation, so if old flags
>>> include MS_REASONLY, we shuold have no dirty blocks at all, and
>>> there's no reason to call ext4_commit_super().
>>>
>>> But only in case both old and new flags include MS_READONLY we're
>>> certain we will not write anything - if new flag does not include
>>> this bit, we will write sooner or later anyway, so preventing just
>>> one commit_super() at the _beginning_ of mount is not really necessary.
>>>
>>> This change probably applicable to -stable, -- not because it fixes
>>> a serious bug, but because the messages printed by the kernel are
>>> rather scary for an average user.  On the other hand, actual usage
>>> of ext4 in nojournal mode on a read-only medium is very rare.
>>>
>>> Thanks to Eric Sandeen for help in diagnosing this issue.
>>>
>>> Signed-off-By: Michael Tokarev <mjt@tls.msk.ru>
>>> ---
>>>  fs/ext4/super.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>>> index 3e0851e..2e896fd 100644
>>> --- a/fs/ext4/super.c
>>> +++ b/fs/ext4/super.c
>>> @@ -4687,7 +4687,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>>>  	}
>>>  
>>>  	ext4_setup_system_zone(sb);
>>> -	if (sbi->s_journal == NULL)
>>> +	if (sbi->s_journal == NULL && !(sb->s_flags & old_sb_flags & MS_RDONLY))
>>>  		ext4_commit_super(sb, 1);
>>>  
>>>  	unlock_super(sb);
>>
> 

Re: [PATCH] ext4: do not try to write superblock on journal-less readonly remount

[Theodore Ts'o]

OK, I'll take this; I'm going to take your suggestion and only call
ext4_commit_super() when we are going from a read-write to read-only
mount, since that's the only time when we need to force that the
superblock be written out.

					- Ted

commit 86fffe43a74af16e306896eba8210bebd33bfd1f
Author: Michael Tokarev <mjt@tls.msk.ru>
Date:   Tue Dec 25 14:08:16 2012 -0500

    ext4: do not try to write superblock on ro remount w/o journal
    
    When a journal-less ext4 filesystem is mounted on a read-only block
    device (blockdev --setro will do), each remount (for other, unrelated,
    flags, like suid=>nosuid etc) results in a series of scary messages
    from kernel telling about I/O errors on the device.
    
    This is becauese of the following code ext4_remount():
    
           if (sbi->s_journal == NULL)
                    ext4_commit_super(sb, 1);
    
    at the end of remount procedure, which forces writing (flushing) of
    a superblock regardless whenever it is dirty or not, if the filesystem
    is readonly or not, and whenever the device itself is readonly or not.
    
    We only need call ext4_commit_super when the file system had been
    previously mounted read/write.
    
    Thanks to Eric Sandeen for help in diagnosing this issue.
    
    Cc: stable@vger.kernel.org
    Signed-off-By: Michael Tokarev <mjt@tls.msk.ru>
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 4969167..183ae34 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4729,7 +4729,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 	}
 
 	ext4_setup_system_zone(sb);
-	if (sbi->s_journal == NULL)
+	if (sbi->s_journal == NULL && !(old_sb_flags & MS_RDONLY))
 		ext4_commit_super(sb, 1);
 
 #ifdef CONFIG_QUOTA