| Message ID | 1351154397-14743-1-git-send-email-mjt@msgid.tls.msk.ru |
|---|---|
| State | Accepted, archived |
| Headers | show |
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); > -- 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
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
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); -- 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
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); > -- 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
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); >> > -- 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
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);
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(-)