public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
* Re: [BUG REPORT] ext4: “errors=remount-ro” has become “errors=shutdown”?
       [not found]   ` <17108cad-efa8-46b4-a320-70d7b696f75b@huawei.com>
@ 2025-01-03 10:42     ` Jan Kara
  2025-01-03 13:19       ` Baokun Li
  2025-01-03 15:35       ` Theodore Ts'o
  0 siblings, 2 replies; 11+ messages in thread
From: Jan Kara @ 2025-01-03 10:42 UTC (permalink / raw)
  To: Baokun Li
  Cc: Jan Kara, linux-ext4@vger.kernel.org, Theodore Ts'o,
	Christian Brauner, sunyongjian1, Yang Erkun, linux-fsdevel,
	linux-xfs

[CCed XFS and fsdevel list in case people have opinion what would be the
best interface to know the fs has shutdown]
 
On Fri 03-01-25 17:26:58, Baokun Li wrote:
> Hi Honza,
> 
> Happy New Year!

Thanks!

> On 2025/1/2 23:58, Jan Kara wrote:
> > On Mon 30-12-24 15:27:01, Baokun Li wrote:
> > > We reported similar issues to the community in 2020,
> > > https://lore.kernel.org/all/20210104160457.GG4018@quack2.suse.cz/
> > > Jan Kara provides a simple and effective patch. This patch somehow
> > > didn't end up merged into upstream, but this patch has been merged into
> > > our internal version for a couple years now and it works fine, is it
> > > possible to revert the patch that no longer sets SB_RDONLY and use
> > > the patch in the link above?
> > Well, the problem with filesystem freezing was just the immediate trigger
> > for the changes. But the setting of SB_RDONLY bit by ext4 behind the VFS'
> > back (as VFS is generally in charge of manipulating that bit and you must
> > hold s_umount for that which we cannot get in ext4 when handling errors)
> > was always problematic and I'm almost sure syzbot would find more problems
> > with that than just fs freezing. As such I don't think we should really
> > return to doing that in ext4 but we need to find other ways how to make
> > your error injection to work...
> I believe this is actually a bug in the evolution of the freeze
> functionality. In v2.6.35-rc1 with commit 18e9e5104fcd ("Introduce
> freeze_super and thaw_super for the fsfreeze ioctl"), the introduction
> of freeze_super/thaw_super did not cause any problems when setting the
> irreversible read-only (ro) bit without locking, because at that time
> we used the flag in sb->s_frozen to determine the file system's state.
> It was not until v4.3-rc1 with commit 8129ed29644b ("change sb_writers
> to use percpu_rw_semaphore") introduced locking into
> freeze_super/thaw_super that setting the irreversible ro without locking
> caused thaw_super to fail to release the locks that should have been
> released, eventually leading to hangs or other issues.
> 
> Therefore, I believe that the patch discussed in the previous link is
> the correct one, and it has a smaller impact and does not introduce any
> mechanism changes. Furthermore, after roughly reviewing the code using
> SB_RDONLY, I did not find any logic where setting the irreversible ro
> without locking could cause problems. If I have overlooked anything,
> please let me know.

Well, I don't remember the details but I think there were issues when
remount between ro/rw state raced with ext4 error setting the filesystem
read-only. ext4_remount() isn't really prepared for SB_RDONLY changing
under it and VFS could possibly get confused as well. Also if nothing else
the unlocked update (or the properly locked one!) to sb->s_flags can get
lost due to a racing read-modify-write cycle of sb->s_flags from other
process.

All these could possibly be fixed but in general it is a rather fragile
design that the SB_RDONLY flag can change under you at any moment.
Basically two following sb_rdonly() checks have to be prepared to get
different results regardless of locks they hold. And this is very easy to
forget. So I still think moving away from that is a good direction.

> > > What's worse is that after commit
> > >    95257987a638 ("ext4: drop EXT4_MF_FS_ABORTED flag")
> > > was merged in v6.6-rc1, the EXT4_FLAGS_SHUTDOWN bit is set in
> > > ext4_handle_error(). This causes the file system to not be read-only
> > > when an error is triggered in "errors=remount-ro" mode, because
> > > EXT4_FLAGS_SHUTDOWN prevents both writing and reading.
> > Here I don't understand what is really the problem with EXT4_MF_FS_ABORTED
> > removal. What do you exactly mean by "causes the file system to not be
> > read-only"? We still return EROFS where we used to, we disallow writing as
> > we used to. Can you perhaps give an example what changed with this commit?
> Sorry for the lack of clarity in my previous explanation. The key point
> is not about removing EXT4_MF_FS_ABORTED, but rather we will set
> EXT4_FLAGS_SHUTDOWN bit, which not only prevents writes but also prevents
> reads. Therefore, saying it's not read-only actually means it's completely
> unreadable.

Ah, I see. I didn't think about that. Is it that you really want reading to
work from a filesystem after error? Can you share why (I'm just trying to
understand the usecase)? Or is this mostly a theoretical usecase?

I think we could introduce "shutdown modifications" state which would still
allow pure reads to succeed if there's a usecase for such functionality.

> > So how does your framework detect that the filesystem has failed with
> > errors=remount-ro? By parsing /proc/mounts or otherwise querying current
> > filesystem mount options?
> In most cases, run the mount command and filter related options.
> > Would it be acceptable for you to look at some
> > other mount option (e.g. "shutdown") to detect that state? We could easily
> > implement that.
> We do need to add a shutdown hint, but that's not the point.
> 
> We've discussed this internally, and now if the logs are flushed,
> we have no way of knowing if the current filesystem is shutdown. We don't
> know if the -EIO from the filesystem is a hardware problem or if the
> filesystem is already shutdown. So even if there is no current problem,
> we should add some kind of hint to let the user know that the current
> filesystem is shutdown.
> 
> The changes to display shutdown are as follows, so that we can see if the
> current filesystem has been shutdown in the mount command.

Yes, I think this would be a good addition regardless of other changes we
might need to do. It would be preferable to be able to come up with
something that's acceptable for querying of shutdown state also for other
filesystems - I've CCed fsdevel and XFS in particular since it has much
longer history of fs shutdown implementation.

								Honza

> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 3955bec9245d..ba28ef0f662e 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3157,6 +3157,9 @@ static int _ext4_show_options(struct seq_file *seq,
> struct super_block *sb,
>         if (nodefs && !test_opt(sb, NO_PREFETCH_BLOCK_BITMAPS))
>                 SEQ_OPTS_PUTS("prefetch_block_bitmaps");
> 
> +       if (!nodefs && ext4_forced_shutdown(sb))
> +               SEQ_OPTS_PUTS("shutdown");
> +
>         ext4_show_quota_options(seq, sb);
>         return 0;
>  }
> > I'm sorry again for causing you trouble.
> 
> Never mind, thank you for your reply!
> 
> 
> Regards,
> Baokun
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BUG REPORT] ext4: “errors=remount-ro” has become “errors=shutdown”?
  2025-01-03 10:42     ` [BUG REPORT] ext4: “errors=remount-ro” has become “errors=shutdown”? Jan Kara
@ 2025-01-03 13:19       ` Baokun Li
  2025-01-03 14:34         ` Jan Kara
  2025-01-03 15:35       ` Theodore Ts'o
  1 sibling, 1 reply; 11+ messages in thread
From: Baokun Li @ 2025-01-03 13:19 UTC (permalink / raw)
  To: Jan Kara
  Cc: linux-ext4@vger.kernel.org, Theodore Ts'o, Christian Brauner,
	sunyongjian1, Yang Erkun, linux-fsdevel, linux-xfs

On 2025/1/3 18:42, Jan Kara wrote:
> [CCed XFS and fsdevel list in case people have opinion what would be the
> best interface to know the fs has shutdown]
>   
> On Fri 03-01-25 17:26:58, Baokun Li wrote:
>> Hi Honza,
>>
>> Happy New Year!
> Thanks!
>
>> On 2025/1/2 23:58, Jan Kara wrote:
>>> On Mon 30-12-24 15:27:01, Baokun Li wrote:
>>>> We reported similar issues to the community in 2020,
>>>> https://lore.kernel.org/all/20210104160457.GG4018@quack2.suse.cz/
>>>> Jan Kara provides a simple and effective patch. This patch somehow
>>>> didn't end up merged into upstream, but this patch has been merged into
>>>> our internal version for a couple years now and it works fine, is it
>>>> possible to revert the patch that no longer sets SB_RDONLY and use
>>>> the patch in the link above?
>>> Well, the problem with filesystem freezing was just the immediate trigger
>>> for the changes. But the setting of SB_RDONLY bit by ext4 behind the VFS'
>>> back (as VFS is generally in charge of manipulating that bit and you must
>>> hold s_umount for that which we cannot get in ext4 when handling errors)
>>> was always problematic and I'm almost sure syzbot would find more problems
>>> with that than just fs freezing. As such I don't think we should really
>>> return to doing that in ext4 but we need to find other ways how to make
>>> your error injection to work...
>> I believe this is actually a bug in the evolution of the freeze
>> functionality. In v2.6.35-rc1 with commit 18e9e5104fcd ("Introduce
>> freeze_super and thaw_super for the fsfreeze ioctl"), the introduction
>> of freeze_super/thaw_super did not cause any problems when setting the
>> irreversible read-only (ro) bit without locking, because at that time
>> we used the flag in sb->s_frozen to determine the file system's state.
>> It was not until v4.3-rc1 with commit 8129ed29644b ("change sb_writers
>> to use percpu_rw_semaphore") introduced locking into
>> freeze_super/thaw_super that setting the irreversible ro without locking
>> caused thaw_super to fail to release the locks that should have been
>> released, eventually leading to hangs or other issues.
>>
>> Therefore, I believe that the patch discussed in the previous link is
>> the correct one, and it has a smaller impact and does not introduce any
>> mechanism changes. Furthermore, after roughly reviewing the code using
>> SB_RDONLY, I did not find any logic where setting the irreversible ro
>> without locking could cause problems. If I have overlooked anything,
>> please let me know.
> Well, I don't remember the details but I think there were issues when
> remount between ro/rw state raced with ext4 error setting the filesystem
> read-only. ext4_remount() isn't really prepared for SB_RDONLY changing
> under it and VFS could possibly get confused as well. Also if nothing else
> the unlocked update (or the properly locked one!) to sb->s_flags can get
> lost due to a racing read-modify-write cycle of sb->s_flags from other
> process.
With journaling enabled, we abort the journal upon encountering an error.

If the transition is from rw (read-write) to ro (read-only), the change
to read-only due to the error itself is not a problem.

When transitioning from ro back to rw, even if s_flags is lost, the ro
state will be re-established upon detecting the aborted journal when
metadata modifications are attempted. Therefore, the data on disk is not
affected.

However, similar to how freeze works, some locks that depend on the ro/rw
state might cause a hang. Therefore, I haven't found any code where a race
condition could cause problems, but I might have missed something.
> All these could possibly be fixed but in general it is a rather fragile
> design that the SB_RDONLY flag can change under you at any moment.
> Basically two following sb_rdonly() checks have to be prepared to get
> different results regardless of locks they hold. And this is very easy to
> forget. So I still think moving away from that is a good direction.
If you dislike the deep coupling of these codes with the VFS, we can
maintain a read-only flag internally within ext4, similar to
BCH_FS_emergency_ro in bcachefs.
>>>> What's worse is that after commit
>>>>     95257987a638 ("ext4: drop EXT4_MF_FS_ABORTED flag")
>>>> was merged in v6.6-rc1, the EXT4_FLAGS_SHUTDOWN bit is set in
>>>> ext4_handle_error(). This causes the file system to not be read-only
>>>> when an error is triggered in "errors=remount-ro" mode, because
>>>> EXT4_FLAGS_SHUTDOWN prevents both writing and reading.
>>> Here I don't understand what is really the problem with EXT4_MF_FS_ABORTED
>>> removal. What do you exactly mean by "causes the file system to not be
>>> read-only"? We still return EROFS where we used to, we disallow writing as
>>> we used to. Can you perhaps give an example what changed with this commit?
>> Sorry for the lack of clarity in my previous explanation. The key point
>> is not about removing EXT4_MF_FS_ABORTED, but rather we will set
>> EXT4_FLAGS_SHUTDOWN bit, which not only prevents writes but also prevents
>> reads. Therefore, saying it's not read-only actually means it's completely
>> unreadable.
> Ah, I see. I didn't think about that. Is it that you really want reading to
> work from a filesystem after error? Can you share why (I'm just trying to
> understand the usecase)? Or is this mostly a theoretical usecase?
Switching to read-only mode after an error is a common practice for most
file systems (ext4/btrfs/affs/fat/jfs/nilfs/nilfs2/ocfs2/ubifs/ufs, etc.).
There are two main benefits to doing this:
  * Read-only processes can continue to run unaffected after the error.
  * Shutting down after an error would lose some data in memory that has
    not been written to disk. If the file system is read-only, we can back
    up these data to another location in time and then exit gracefully.
> I think we could introduce "shutdown modifications" state which would still
> allow pure reads to succeed if there's a usecase for such functionality.
I agree that maintaining a flag like EXT4_FLAGS_RDONLY within ext4 seems
to be a good solution at this point. It avoids both introducing mechanism
changes and VFS coupling. If no one has a better idea, I will implement it.


Cheers,
Baokun
>>> So how does your framework detect that the filesystem has failed with
>>> errors=remount-ro? By parsing /proc/mounts or otherwise querying current
>>> filesystem mount options?
>> In most cases, run the mount command and filter related options.
>>> Would it be acceptable for you to look at some
>>> other mount option (e.g. "shutdown") to detect that state? We could easily
>>> implement that.
>> We do need to add a shutdown hint, but that's not the point.
>>
>> We've discussed this internally, and now if the logs are flushed,
>> we have no way of knowing if the current filesystem is shutdown. We don't
>> know if the -EIO from the filesystem is a hardware problem or if the
>> filesystem is already shutdown. So even if there is no current problem,
>> we should add some kind of hint to let the user know that the current
>> filesystem is shutdown.
>>
>> The changes to display shutdown are as follows, so that we can see if the
>> current filesystem has been shutdown in the mount command.
> Yes, I think this would be a good addition regardless of other changes we
> might need to do. It would be preferable to be able to come up with
> something that's acceptable for querying of shutdown state also for other
> filesystems - I've CCed fsdevel and XFS in particular since it has much
> longer history of fs shutdown implementation.
>
> 								Honza
>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 3955bec9245d..ba28ef0f662e 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -3157,6 +3157,9 @@ static int _ext4_show_options(struct seq_file *seq,
>> struct super_block *sb,
>>          if (nodefs && !test_opt(sb, NO_PREFETCH_BLOCK_BITMAPS))
>>                  SEQ_OPTS_PUTS("prefetch_block_bitmaps");
>>
>> +       if (!nodefs && ext4_forced_shutdown(sb))
>> +               SEQ_OPTS_PUTS("shutdown");
>> +
>>          ext4_show_quota_options(seq, sb);
>>          return 0;
>>   }
>>> I'm sorry again for causing you trouble.
>> Never mind, thank you for your reply!
>>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BUG REPORT] ext4: “errors=remount-ro” has become “errors=shutdown”?
  2025-01-03 13:19       ` Baokun Li
@ 2025-01-03 14:34         ` Jan Kara
  2025-01-04  3:15           ` Baokun Li
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Kara @ 2025-01-03 14:34 UTC (permalink / raw)
  To: Baokun Li
  Cc: Jan Kara, linux-ext4@vger.kernel.org, Theodore Ts'o,
	Christian Brauner, sunyongjian1, Yang Erkun, linux-fsdevel,
	linux-xfs

On Fri 03-01-25 21:19:27, Baokun Li wrote:
> On 2025/1/3 18:42, Jan Kara wrote:
> > > > > What's worse is that after commit
> > > > >     95257987a638 ("ext4: drop EXT4_MF_FS_ABORTED flag")
> > > > > was merged in v6.6-rc1, the EXT4_FLAGS_SHUTDOWN bit is set in
> > > > > ext4_handle_error(). This causes the file system to not be read-only
> > > > > when an error is triggered in "errors=remount-ro" mode, because
> > > > > EXT4_FLAGS_SHUTDOWN prevents both writing and reading.
> > > > Here I don't understand what is really the problem with EXT4_MF_FS_ABORTED
> > > > removal. What do you exactly mean by "causes the file system to not be
> > > > read-only"? We still return EROFS where we used to, we disallow writing as
> > > > we used to. Can you perhaps give an example what changed with this commit?
> > > Sorry for the lack of clarity in my previous explanation. The key point
> > > is not about removing EXT4_MF_FS_ABORTED, but rather we will set
> > > EXT4_FLAGS_SHUTDOWN bit, which not only prevents writes but also prevents
> > > reads. Therefore, saying it's not read-only actually means it's completely
> > > unreadable.
> > Ah, I see. I didn't think about that. Is it that you really want reading to
> > work from a filesystem after error? Can you share why (I'm just trying to
> > understand the usecase)? Or is this mostly a theoretical usecase?
> Switching to read-only mode after an error is a common practice for most
> file systems (ext4/btrfs/affs/fat/jfs/nilfs/nilfs2/ocfs2/ubifs/ufs, etc.).
> There are two main benefits to doing this:
>  * Read-only processes can continue to run unaffected after the error.
>  * Shutting down after an error would lose some data in memory that has
>    not been written to disk. If the file system is read-only, we can back
>    up these data to another location in time and then exit gracefully.
> > I think we could introduce "shutdown modifications" state which would still
> > allow pure reads to succeed if there's a usecase for such functionality.
> I agree that maintaining a flag like EXT4_FLAGS_RDONLY within ext4 seems
> to be a good solution at this point. It avoids both introducing mechanism
> changes and VFS coupling. If no one has a better idea, I will implement it.

Yeah, let's go with a separate "emergency RO" ext4 flag then. I think we
could just enhance the ext4_forced_shutdown() checks to take a flag whether
the operation is a modification or not and when it is a modification, it
would additionally trigger also when EMERGENCY_RO flag is set (which would
get set by ext4_handle_error()).

Thanks for having a look into this.

								Honza

> > > > So how does your framework detect that the filesystem has failed with
> > > > errors=remount-ro? By parsing /proc/mounts or otherwise querying current
> > > > filesystem mount options?
> > > In most cases, run the mount command and filter related options.
> > > > Would it be acceptable for you to look at some
> > > > other mount option (e.g. "shutdown") to detect that state? We could easily
> > > > implement that.
> > > We do need to add a shutdown hint, but that's not the point.
> > > 
> > > We've discussed this internally, and now if the logs are flushed,
> > > we have no way of knowing if the current filesystem is shutdown. We don't
> > > know if the -EIO from the filesystem is a hardware problem or if the
> > > filesystem is already shutdown. So even if there is no current problem,
> > > we should add some kind of hint to let the user know that the current
> > > filesystem is shutdown.
> > > 
> > > The changes to display shutdown are as follows, so that we can see if the
> > > current filesystem has been shutdown in the mount command.
> > Yes, I think this would be a good addition regardless of other changes we
> > might need to do. It would be preferable to be able to come up with
> > something that's acceptable for querying of shutdown state also for other
> > filesystems - I've CCed fsdevel and XFS in particular since it has much
> > longer history of fs shutdown implementation.
> > 
> > 								Honza
> > 
> > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > > index 3955bec9245d..ba28ef0f662e 100644
> > > --- a/fs/ext4/super.c
> > > +++ b/fs/ext4/super.c
> > > @@ -3157,6 +3157,9 @@ static int _ext4_show_options(struct seq_file *seq,
> > > struct super_block *sb,
> > >          if (nodefs && !test_opt(sb, NO_PREFETCH_BLOCK_BITMAPS))
> > >                  SEQ_OPTS_PUTS("prefetch_block_bitmaps");
> > > 
> > > +       if (!nodefs && ext4_forced_shutdown(sb))
> > > +               SEQ_OPTS_PUTS("shutdown");
> > > +
> > >          ext4_show_quota_options(seq, sb);
> > >          return 0;
> > >   }
> > > > I'm sorry again for causing you trouble.
> > > Never mind, thank you for your reply!
> > > 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BUG REPORT] ext4: “errors=remount-ro” has become “errors=shutdown”?
  2025-01-03 10:42     ` [BUG REPORT] ext4: “errors=remount-ro” has become “errors=shutdown”? Jan Kara
  2025-01-03 13:19       ` Baokun Li
@ 2025-01-03 15:35       ` Theodore Ts'o
  2025-01-03 15:54         ` Theodore Ts'o
  1 sibling, 1 reply; 11+ messages in thread
From: Theodore Ts'o @ 2025-01-03 15:35 UTC (permalink / raw)
  To: Jan Kara
  Cc: Baokun Li, linux-ext4@vger.kernel.org, Christian Brauner,
	sunyongjian1, Yang Erkun, linux-fsdevel, linux-xfs

On Fri, Jan 03, 2025 at 11:42:13AM +0100, Jan Kara wrote:
> [CCed XFS and fsdevel list in case people have opinion what would be the
> best interface to know the fs has shutdown]
>  
> > Sorry for the lack of clarity in my previous explanation. The key point
> > is not about removing EXT4_MF_FS_ABORTED, but rather we will set
> > EXT4_FLAGS_SHUTDOWN bit, which not only prevents writes but also prevents
> > reads. Therefore, saying it's not read-only actually means it's completely
> > unreadable.
> 
> Ah, I see. I didn't think about that. Is it that you really want reading to
> work from a filesystem after error? Can you share why (I'm just trying to
> understand the usecase)? Or is this mostly a theoretical usecase?

I don't see how setting the shutdown flag causes reads to fail.  That
was true in an early version of the ext4 patch which implemented
shutdown support, but one of the XFS developers (I don't remember if
it was Dave or Cristoph) objected because XFS did not cause the
read_pages function to fail.  Are you seeing this with an upstream
kernel, or with a patched kernel?  The upstream kernel does *not* have
the check in ext4_readpages() or ext4_read_folio() (post folio
conversion).

					- Ted

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BUG REPORT] ext4: “errors=remount-ro” has become “errors=shutdown”?
  2025-01-03 15:35       ` Theodore Ts'o
@ 2025-01-03 15:54         ` Theodore Ts'o
  2025-01-04  2:41           ` Baokun Li
  0 siblings, 1 reply; 11+ messages in thread
From: Theodore Ts'o @ 2025-01-03 15:54 UTC (permalink / raw)
  To: Jan Kara
  Cc: Baokun Li, linux-ext4@vger.kernel.org, Christian Brauner,
	sunyongjian1, Yang Erkun, linux-fsdevel, linux-xfs

On Fri, Jan 03, 2025 at 10:35:17AM -0500, Theodore Ts'o wrote:
> I don't see how setting the shutdown flag causes reads to fail.  That
> was true in an early version of the ext4 patch which implemented
> shutdown support, but one of the XFS developers (I don't remember if
> it was Dave or Cristoph) objected because XFS did not cause the
> read_pages function to fail.  Are you seeing this with an upstream
> kernel, or with a patched kernel?  The upstream kernel does *not* have
> the check in ext4_readpages() or ext4_read_folio() (post folio
> conversion).

OK, that's weird.  Testing on 6.13-rc4, I don't see the problem simulating an ext4 error:

root@kvm-xfstests:~# mke2fs -t ext4 -Fq /dev/vdc
/dev/vdc contains a ext4 file system
	last mounted on /vdc on Fri Jan  3 10:38:21 2025
root@kvm-xfstests:~# mount -t ext4 -o errors=continue /dev/vdc /vdc
[   24.780982] EXT4-fs (vdc): mounted filesystem f8595206-fe57-486c-80dd-48b03d41ebdb r/w with ordered data mode. Quota mode: none.
root@kvm-xfstests:~# cp /etc/motd /vdc/motd
root@kvm-xfstests:~# echo testing > /sys/fs/ext4/vdc/trigger_fs_error 
[   42.943141] EXT4-fs error (device vdc): trigger_test_error:129: comm bash: testing
root@kvm-xfstests:~# cat /vdc/motd 

The programs included with the Debian GNU/Linux system are free software;
the exact distribution terms for each program are described in the
individual files in /usr/share/doc/*/copyright.

Debian GNU/Linux comes with ABSOLUTELY NO WARRANTY, to the extent
permitted by applicable law.
root@kvm-xfstests:~# 


HOWEVER, testing with shutdown ioctl, both ext4 and xfs are failing with EIO:

root@kvm-xfstests:~# mount /dev/vdc /vdc
[    7.969168] XFS (vdc): Mounting V5 Filesystem 7834ea96-eab0-46c5-9b18-c8f054fa9cf4
[    7.978539] XFS (vdc): Ending clean mount
root@kvm-xfstests:~# cp /etc/motd /vdc
root@kvm-xfstests:~# /root/xfstests/src/godown -v /vdc
Opening "/vdc"
Calling XFS_IOC_GOINGDOWN
[   29.354609] XFS (vdc): User initiated shutdown received.
[   29.356123] XFS (vdc): Log I/O Error (0x6) detected at xfs_fs_goingdown+0x55/0xb0 (fs/xfs/xfs_fsops.c:452).  Shutting down filesystem.
[   29.357092] XFS (vdc): Please unmount the filesystem and rectify the problem(s)
root@kvm-xfstests:~# cat /vdc/motd
cat: /vdc/motd: Input/output error
root@kvm-xfstests:~#

So I take back what I said earlier, but I am a bit confused why it
worked after simulating an file system error using "echo testing >
/sys/fs/ext4/vdc/trigger_fs_error".

						- Ted

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BUG REPORT] ext4: “errors=remount-ro” has become “errors=shutdown”?
  2025-01-03 15:54         ` Theodore Ts'o
@ 2025-01-04  2:41           ` Baokun Li
  2025-01-06 23:49             ` Darrick J. Wong
  0 siblings, 1 reply; 11+ messages in thread
From: Baokun Li @ 2025-01-04  2:41 UTC (permalink / raw)
  To: Theodore Ts'o
  Cc: Jan Kara, linux-ext4@vger.kernel.org, Christian Brauner,
	sunyongjian1, Yang Erkun, linux-fsdevel, linux-xfs, Baokun Li

Hi Ted,

On 2025/1/3 23:54, Theodore Ts'o wrote:
> On Fri, Jan 03, 2025 at 10:35:17AM -0500, Theodore Ts'o wrote:
>> I don't see how setting the shutdown flag causes reads to fail.  That
>> was true in an early version of the ext4 patch which implemented
>> shutdown support, but one of the XFS developers (I don't remember if
>> it was Dave or Cristoph) objected because XFS did not cause the
>> read_pages function to fail.  Are you seeing this with an upstream
>> kernel, or with a patched kernel?  The upstream kernel does *not* have
>> the check in ext4_readpages() or ext4_read_folio() (post folio
>> conversion).
> OK, that's weird.  Testing on 6.13-rc4, I don't see the problem simulating an ext4 error:
>
> root@kvm-xfstests:~# mke2fs -t ext4 -Fq /dev/vdc
> /dev/vdc contains a ext4 file system
> 	last mounted on /vdc on Fri Jan  3 10:38:21 2025
> root@kvm-xfstests:~# mount -t ext4 -o errors=continue /dev/vdc /vdc
We are discussing "errors=remount-ro," as the title states, not the
continue mode. The key code leading to the behavior change is as follows,
therefore the continue mode is not affected.

--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -657,7 +657,7 @@ static void ext4_handle_error(struct super_block 
*sb, bool force_ro, int error,
                 WARN_ON_ONCE(1);

         if (!continue_fs && !sb_rdonly(sb)) {
-               ext4_set_mount_flag(sb, EXT4_MF_FS_ABORTED);
+               set_bit(EXT4_FLAGS_SHUTDOWN, &EXT4_SB(sb)->s_ext4_flags);
                 if (journal)
                         jbd2_journal_abort(journal, -EIO);
         }

See the end for problem reproduction.

> [   24.780982] EXT4-fs (vdc): mounted filesystem f8595206-fe57-486c-80dd-48b03d41ebdb r/w with ordered data mode. Quota mode: none.
> root@kvm-xfstests:~# cp /etc/motd /vdc/motd
> root@kvm-xfstests:~# echo testing > /sys/fs/ext4/vdc/trigger_fs_error
> [   42.943141] EXT4-fs error (device vdc): trigger_test_error:129: comm bash: testing
> root@kvm-xfstests:~# cat /vdc/motd
>
> The programs included with the Debian GNU/Linux system are free software;
> the exact distribution terms for each program are described in the
> individual files in /usr/share/doc/*/copyright.
>
> Debian GNU/Linux comes with ABSOLUTELY NO WARRANTY, to the extent
> permitted by applicable law.
> root@kvm-xfstests:~#
>
>
> HOWEVER, testing with shutdown ioctl, both ext4 and xfs are failing with EIO:
Yes, this is as expected.
> root@kvm-xfstests:~# mount /dev/vdc /vdc
> [    7.969168] XFS (vdc): Mounting V5 Filesystem 7834ea96-eab0-46c5-9b18-c8f054fa9cf4
> [    7.978539] XFS (vdc): Ending clean mount
> root@kvm-xfstests:~# cp /etc/motd /vdc
> root@kvm-xfstests:~# /root/xfstests/src/godown -v /vdc
> Opening "/vdc"
> Calling XFS_IOC_GOINGDOWN
> [   29.354609] XFS (vdc): User initiated shutdown received.
> [   29.356123] XFS (vdc): Log I/O Error (0x6) detected at xfs_fs_goingdown+0x55/0xb0 (fs/xfs/xfs_fsops.c:452).  Shutting down filesystem.
> [   29.357092] XFS (vdc): Please unmount the filesystem and rectify the problem(s)
> root@kvm-xfstests:~# cat /vdc/motd
> cat: /vdc/motd: Input/output error
> root@kvm-xfstests:~#
>
> So I take back what I said earlier, but I am a bit confused why it
> worked after simulating an file system error using "echo testing >
> /sys/fs/ext4/vdc/trigger_fs_error".
>
It's because "errors=remount-ro" wasn't used when mounting...

Here's a replication:

root@kvm-xfstests:~# mount -o errors=remount-ro /dev/vdc /mnt/test
[  115.731007] EXT4-fs (vdc): mounted filesystem 
0838f08f-c04e-440c-a9a5-417677efb03e r/w with ordered data mode. Quota 
mode: none.
root@kvm-xfstests:~# echo test > /mnt/test/file
root@kvm-xfstests:~# cat /mnt/test/file
test
root@kvm-xfstests:~# echo 1 > /sys/fs/ext4/vdc/trigger_fs_error
[  131.537649] EXT4-fs error (device vdc): trigger_test_error:129: comm 
bash: 1
[  131.538226] Aborting journal on device vdc-8.
[  131.538844] EXT4-fs (vdc): Remounting filesystem read-only
root@kvm-xfstests:~# cat /mnt/test/file
cat: /mnt/test/file: Input/output error
root@kvm-xfstests:~# uname -a
Linux kvm-xfstests 6.13.0-rc4-xfstests-g6cfe3548f8f5-dirty #284 SMP 
PREEMPT_DYNAMIC Fri Dec 27 10:39:02 CST 2024 x86_64 GNU/Linux


Regards,
Baokun


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BUG REPORT] ext4: “errors=remount-ro” has become “errors=shutdown”?
  2025-01-03 14:34         ` Jan Kara
@ 2025-01-04  3:15           ` Baokun Li
  0 siblings, 0 replies; 11+ messages in thread
From: Baokun Li @ 2025-01-04  3:15 UTC (permalink / raw)
  To: Jan Kara
  Cc: linux-ext4@vger.kernel.org, Theodore Ts'o, Christian Brauner,
	sunyongjian1, Yang Erkun, linux-fsdevel, linux-xfs, Baokun Li

On 2025/1/3 22:34, Jan Kara wrote:
> On Fri 03-01-25 21:19:27, Baokun Li wrote:
>> On 2025/1/3 18:42, Jan Kara wrote:
>>>>>> What's worse is that after commit
>>>>>>      95257987a638 ("ext4: drop EXT4_MF_FS_ABORTED flag")
>>>>>> was merged in v6.6-rc1, the EXT4_FLAGS_SHUTDOWN bit is set in
>>>>>> ext4_handle_error(). This causes the file system to not be read-only
>>>>>> when an error is triggered in "errors=remount-ro" mode, because
>>>>>> EXT4_FLAGS_SHUTDOWN prevents both writing and reading.
>>>>> Here I don't understand what is really the problem with EXT4_MF_FS_ABORTED
>>>>> removal. What do you exactly mean by "causes the file system to not be
>>>>> read-only"? We still return EROFS where we used to, we disallow writing as
>>>>> we used to. Can you perhaps give an example what changed with this commit?
>>>> Sorry for the lack of clarity in my previous explanation. The key point
>>>> is not about removing EXT4_MF_FS_ABORTED, but rather we will set
>>>> EXT4_FLAGS_SHUTDOWN bit, which not only prevents writes but also prevents
>>>> reads. Therefore, saying it's not read-only actually means it's completely
>>>> unreadable.
>>> Ah, I see. I didn't think about that. Is it that you really want reading to
>>> work from a filesystem after error? Can you share why (I'm just trying to
>>> understand the usecase)? Or is this mostly a theoretical usecase?
>> Switching to read-only mode after an error is a common practice for most
>> file systems (ext4/btrfs/affs/fat/jfs/nilfs/nilfs2/ocfs2/ubifs/ufs, etc.).
>> There are two main benefits to doing this:
>>   * Read-only processes can continue to run unaffected after the error.
>>   * Shutting down after an error would lose some data in memory that has
>>     not been written to disk. If the file system is read-only, we can back
>>     up these data to another location in time and then exit gracefully.
>>> I think we could introduce "shutdown modifications" state which would still
>>> allow pure reads to succeed if there's a usecase for such functionality.
>> I agree that maintaining a flag like EXT4_FLAGS_RDONLY within ext4 seems
>> to be a good solution at this point. It avoids both introducing mechanism
>> changes and VFS coupling. If no one has a better idea, I will implement it.
> Yeah, let's go with a separate "emergency RO" ext4 flag then. I think we
> could just enhance the ext4_forced_shutdown() checks to take a flag whether
> the operation is a modification or not and when it is a modification, it
> would additionally trigger also when EMERGENCY_RO flag is set (which would
> get set by ext4_handle_error()).
>
> Thanks for having a look into this.
>
> 								Honza
Sounds very nice, this way the changes will be minimal and the code won't
look messy.Thank you for your suggestion!

Cheers,
Baokun
>>>>> So how does your framework detect that the filesystem has failed with
>>>>> errors=remount-ro? By parsing /proc/mounts or otherwise querying current
>>>>> filesystem mount options?
>>>> In most cases, run the mount command and filter related options.
>>>>> Would it be acceptable for you to look at some
>>>>> other mount option (e.g. "shutdown") to detect that state? We could easily
>>>>> implement that.
>>>> We do need to add a shutdown hint, but that's not the point.
>>>>
>>>> We've discussed this internally, and now if the logs are flushed,
>>>> we have no way of knowing if the current filesystem is shutdown. We don't
>>>> know if the -EIO from the filesystem is a hardware problem or if the
>>>> filesystem is already shutdown. So even if there is no current problem,
>>>> we should add some kind of hint to let the user know that the current
>>>> filesystem is shutdown.
>>>>
>>>> The changes to display shutdown are as follows, so that we can see if the
>>>> current filesystem has been shutdown in the mount command.
>>> Yes, I think this would be a good addition regardless of other changes we
>>> might need to do. It would be preferable to be able to come up with
>>> something that's acceptable for querying of shutdown state also for other
>>> filesystems - I've CCed fsdevel and XFS in particular since it has much
>>> longer history of fs shutdown implementation.
>>>
>>> 								Honza
>>>
>>>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>>>> index 3955bec9245d..ba28ef0f662e 100644
>>>> --- a/fs/ext4/super.c
>>>> +++ b/fs/ext4/super.c
>>>> @@ -3157,6 +3157,9 @@ static int _ext4_show_options(struct seq_file *seq,
>>>> struct super_block *sb,
>>>>           if (nodefs && !test_opt(sb, NO_PREFETCH_BLOCK_BITMAPS))
>>>>                   SEQ_OPTS_PUTS("prefetch_block_bitmaps");
>>>>
>>>> +       if (!nodefs && ext4_forced_shutdown(sb))
>>>> +               SEQ_OPTS_PUTS("shutdown");
>>>> +
>>>>           ext4_show_quota_options(seq, sb);
>>>>           return 0;
>>>>    }
>>>>> I'm sorry again for causing you trouble.
>>>> Never mind, thank you for your reply!
>>>>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BUG REPORT] ext4: “errors=remount-ro” has become “errors=shutdown”?
  2025-01-04  2:41           ` Baokun Li
@ 2025-01-06 23:49             ` Darrick J. Wong
  2025-01-07  2:01               ` Baokun Li
  0 siblings, 1 reply; 11+ messages in thread
From: Darrick J. Wong @ 2025-01-06 23:49 UTC (permalink / raw)
  To: Baokun Li
  Cc: Theodore Ts'o, Jan Kara, linux-ext4@vger.kernel.org,
	Christian Brauner, sunyongjian1, Yang Erkun, linux-fsdevel,
	linux-xfs

On Sat, Jan 04, 2025 at 10:41:28AM +0800, Baokun Li wrote:
> Hi Ted,
> 
> On 2025/1/3 23:54, Theodore Ts'o wrote:
> > On Fri, Jan 03, 2025 at 10:35:17AM -0500, Theodore Ts'o wrote:
> > > I don't see how setting the shutdown flag causes reads to fail.  That
> > > was true in an early version of the ext4 patch which implemented
> > > shutdown support, but one of the XFS developers (I don't remember if
> > > it was Dave or Cristoph) objected because XFS did not cause the
> > > read_pages function to fail.  Are you seeing this with an upstream
> > > kernel, or with a patched kernel?  The upstream kernel does *not* have
> > > the check in ext4_readpages() or ext4_read_folio() (post folio
> > > conversion).
> > OK, that's weird.  Testing on 6.13-rc4, I don't see the problem simulating an ext4 error:
> > 
> > root@kvm-xfstests:~# mke2fs -t ext4 -Fq /dev/vdc
> > /dev/vdc contains a ext4 file system
> > 	last mounted on /vdc on Fri Jan  3 10:38:21 2025
> > root@kvm-xfstests:~# mount -t ext4 -o errors=continue /dev/vdc /vdc
> We are discussing "errors=remount-ro," as the title states, not the
> continue mode. The key code leading to the behavior change is as follows,
> therefore the continue mode is not affected.

Hmm.  On the one hand, XFS has generally returned EIO (or ESHUTDOWN in a
couple of specialty cases) when the fs has been shut down.

OTOH XFS also doesn't have errors=remount-ro; it just dies, which I
think has been its behavior for a long time.

To me, it doesn't sound unreasonable for ext* to allow reads after a
shutdown when errors=remount-ro since it's always had that behavior.

Bonus Q: do you want an errors=fail variant to shut things down fast?

--D

> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -657,7 +657,7 @@ static void ext4_handle_error(struct super_block *sb,
> bool force_ro, int error,
>                 WARN_ON_ONCE(1);
> 
>         if (!continue_fs && !sb_rdonly(sb)) {
> -               ext4_set_mount_flag(sb, EXT4_MF_FS_ABORTED);
> +               set_bit(EXT4_FLAGS_SHUTDOWN, &EXT4_SB(sb)->s_ext4_flags);
>                 if (journal)
>                         jbd2_journal_abort(journal, -EIO);
>         }
> 
> See the end for problem reproduction.
> 
> > [   24.780982] EXT4-fs (vdc): mounted filesystem f8595206-fe57-486c-80dd-48b03d41ebdb r/w with ordered data mode. Quota mode: none.
> > root@kvm-xfstests:~# cp /etc/motd /vdc/motd
> > root@kvm-xfstests:~# echo testing > /sys/fs/ext4/vdc/trigger_fs_error
> > [   42.943141] EXT4-fs error (device vdc): trigger_test_error:129: comm bash: testing
> > root@kvm-xfstests:~# cat /vdc/motd
> > 
> > The programs included with the Debian GNU/Linux system are free software;
> > the exact distribution terms for each program are described in the
> > individual files in /usr/share/doc/*/copyright.
> > 
> > Debian GNU/Linux comes with ABSOLUTELY NO WARRANTY, to the extent
> > permitted by applicable law.
> > root@kvm-xfstests:~#
> > 
> > 
> > HOWEVER, testing with shutdown ioctl, both ext4 and xfs are failing with EIO:
> Yes, this is as expected.
> > root@kvm-xfstests:~# mount /dev/vdc /vdc
> > [    7.969168] XFS (vdc): Mounting V5 Filesystem 7834ea96-eab0-46c5-9b18-c8f054fa9cf4
> > [    7.978539] XFS (vdc): Ending clean mount
> > root@kvm-xfstests:~# cp /etc/motd /vdc
> > root@kvm-xfstests:~# /root/xfstests/src/godown -v /vdc
> > Opening "/vdc"
> > Calling XFS_IOC_GOINGDOWN
> > [   29.354609] XFS (vdc): User initiated shutdown received.
> > [   29.356123] XFS (vdc): Log I/O Error (0x6) detected at xfs_fs_goingdown+0x55/0xb0 (fs/xfs/xfs_fsops.c:452).  Shutting down filesystem.
> > [   29.357092] XFS (vdc): Please unmount the filesystem and rectify the problem(s)
> > root@kvm-xfstests:~# cat /vdc/motd
> > cat: /vdc/motd: Input/output error
> > root@kvm-xfstests:~#
> > 
> > So I take back what I said earlier, but I am a bit confused why it
> > worked after simulating an file system error using "echo testing >
> > /sys/fs/ext4/vdc/trigger_fs_error".
> > 
> It's because "errors=remount-ro" wasn't used when mounting...
> 
> Here's a replication:
> 
> root@kvm-xfstests:~# mount -o errors=remount-ro /dev/vdc /mnt/test
> [  115.731007] EXT4-fs (vdc): mounted filesystem
> 0838f08f-c04e-440c-a9a5-417677efb03e r/w with ordered data mode. Quota mode:
> none.
> root@kvm-xfstests:~# echo test > /mnt/test/file
> root@kvm-xfstests:~# cat /mnt/test/file
> test
> root@kvm-xfstests:~# echo 1 > /sys/fs/ext4/vdc/trigger_fs_error
> [  131.537649] EXT4-fs error (device vdc): trigger_test_error:129: comm
> bash: 1
> [  131.538226] Aborting journal on device vdc-8.
> [  131.538844] EXT4-fs (vdc): Remounting filesystem read-only
> root@kvm-xfstests:~# cat /mnt/test/file
> cat: /mnt/test/file: Input/output error
> root@kvm-xfstests:~# uname -a
> Linux kvm-xfstests 6.13.0-rc4-xfstests-g6cfe3548f8f5-dirty #284 SMP
> PREEMPT_DYNAMIC Fri Dec 27 10:39:02 CST 2024 x86_64 GNU/Linux
> 
> 
> Regards,
> Baokun
> 
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BUG REPORT] ext4: “errors=remount-ro” has become “errors=shutdown”?
  2025-01-06 23:49             ` Darrick J. Wong
@ 2025-01-07  2:01               ` Baokun Li
  2025-01-07  7:08                 ` Darrick J. Wong
  0 siblings, 1 reply; 11+ messages in thread
From: Baokun Li @ 2025-01-07  2:01 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Theodore Ts'o, Jan Kara, linux-ext4@vger.kernel.org,
	Christian Brauner, sunyongjian1, Yang Erkun, linux-fsdevel,
	linux-xfs, Baokun Li

On 2025/1/7 7:49, Darrick J. Wong wrote:
> On Sat, Jan 04, 2025 at 10:41:28AM +0800, Baokun Li wrote:
>> Hi Ted,
>>
>> On 2025/1/3 23:54, Theodore Ts'o wrote:
>>> On Fri, Jan 03, 2025 at 10:35:17AM -0500, Theodore Ts'o wrote:
>>>> I don't see how setting the shutdown flag causes reads to fail.  That
>>>> was true in an early version of the ext4 patch which implemented
>>>> shutdown support, but one of the XFS developers (I don't remember if
>>>> it was Dave or Cristoph) objected because XFS did not cause the
>>>> read_pages function to fail.  Are you seeing this with an upstream
>>>> kernel, or with a patched kernel?  The upstream kernel does *not* have
>>>> the check in ext4_readpages() or ext4_read_folio() (post folio
>>>> conversion).
>>> OK, that's weird.  Testing on 6.13-rc4, I don't see the problem simulating an ext4 error:
>>>
>>> root@kvm-xfstests:~# mke2fs -t ext4 -Fq /dev/vdc
>>> /dev/vdc contains a ext4 file system
>>> 	last mounted on /vdc on Fri Jan  3 10:38:21 2025
>>> root@kvm-xfstests:~# mount -t ext4 -o errors=continue /dev/vdc /vdc
>> We are discussing "errors=remount-ro," as the title states, not the
>> continue mode. The key code leading to the behavior change is as follows,
>> therefore the continue mode is not affected.
> Hmm.  On the one hand, XFS has generally returned EIO (or ESHUTDOWN in a
> couple of specialty cases) when the fs has been shut down.
Indeed, this is the intended behavior during shutdown.
>
> OTOH XFS also doesn't have errors=remount-ro; it just dies, which I
> think has been its behavior for a long time.
Yes. As an aside, is there any way for xfs to determine if -EIO is
originating from a hardware error or if the filesystem has been shutdown?

Or would you consider it useful to have the mount command display
"shutdown" when the file system is being shut down?
> To me, it doesn't sound unreasonable for ext* to allow reads after a
> shutdown when errors=remount-ro since it's always had that behavior.
Yes, a previous bug fix inadvertently changed the behavior of 
errors=remount-ro,
and the patch to correct this is coming.

Additionally, ext4 now allows directory reads even after shutdown, is this
expected behavior?
> Bonus Q: do you want an errors=fail variant to shut things down fast?
>
> --D

In my opinion, I have not yet seen a scenario where the file system needs
to be shut down after an error occurs. Therefore, using errors=remount-ro
to prevent modifications after an error is sufficient. Of course, if
customers have such needs, implementing this mode is also very simple.


Thanks,
Baokun


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BUG REPORT] ext4: “errors=remount-ro” has become “errors=shutdown”?
  2025-01-07  2:01               ` Baokun Li
@ 2025-01-07  7:08                 ` Darrick J. Wong
  2025-01-08  2:08                   ` Baokun Li
  0 siblings, 1 reply; 11+ messages in thread
From: Darrick J. Wong @ 2025-01-07  7:08 UTC (permalink / raw)
  To: Baokun Li
  Cc: Theodore Ts'o, Jan Kara, linux-ext4@vger.kernel.org,
	Christian Brauner, sunyongjian1, Yang Erkun, linux-fsdevel,
	linux-xfs

On Tue, Jan 07, 2025 at 10:01:32AM +0800, Baokun Li wrote:
> On 2025/1/7 7:49, Darrick J. Wong wrote:
> > On Sat, Jan 04, 2025 at 10:41:28AM +0800, Baokun Li wrote:
> > > Hi Ted,
> > > 
> > > On 2025/1/3 23:54, Theodore Ts'o wrote:
> > > > On Fri, Jan 03, 2025 at 10:35:17AM -0500, Theodore Ts'o wrote:
> > > > > I don't see how setting the shutdown flag causes reads to fail.  That
> > > > > was true in an early version of the ext4 patch which implemented
> > > > > shutdown support, but one of the XFS developers (I don't remember if
> > > > > it was Dave or Cristoph) objected because XFS did not cause the
> > > > > read_pages function to fail.  Are you seeing this with an upstream
> > > > > kernel, or with a patched kernel?  The upstream kernel does *not* have
> > > > > the check in ext4_readpages() or ext4_read_folio() (post folio
> > > > > conversion).
> > > > OK, that's weird.  Testing on 6.13-rc4, I don't see the problem simulating an ext4 error:
> > > > 
> > > > root@kvm-xfstests:~# mke2fs -t ext4 -Fq /dev/vdc
> > > > /dev/vdc contains a ext4 file system
> > > > 	last mounted on /vdc on Fri Jan  3 10:38:21 2025
> > > > root@kvm-xfstests:~# mount -t ext4 -o errors=continue /dev/vdc /vdc
> > > We are discussing "errors=remount-ro," as the title states, not the
> > > continue mode. The key code leading to the behavior change is as follows,
> > > therefore the continue mode is not affected.
> > Hmm.  On the one hand, XFS has generally returned EIO (or ESHUTDOWN in a
> > couple of specialty cases) when the fs has been shut down.
> Indeed, this is the intended behavior during shutdown.
> > 
> > OTOH XFS also doesn't have errors=remount-ro; it just dies, which I
> > think has been its behavior for a long time.
> Yes. As an aside, is there any way for xfs to determine if -EIO is
> originating from a hardware error or if the filesystem has been shutdown?

XFS knows the difference, but nothing above it does.

> Or would you consider it useful to have the mount command display
> "shutdown" when the file system is being shut down?

Trouble is, will mount get confused and try to pass ",shutdown" as part
of a remount operation?  I suppose the fs is dead so what does it
matter...

> > To me, it doesn't sound unreasonable for ext* to allow reads after a
> > shutdown when errors=remount-ro since it's always had that behavior.
> Yes, a previous bug fix inadvertently changed the behavior of
> errors=remount-ro,
> and the patch to correct this is coming.
> 
> Additionally, ext4 now allows directory reads even after shutdown, is this
> expected behavior?

There's no formal specification for what shutdown means, so ... it's not
unexpected.  XFS doesn't allow that.

> > Bonus Q: do you want an errors=fail variant to shut things down fast?
> > 
> > --D
> 
> In my opinion, I have not yet seen a scenario where the file system needs
> to be shut down after an error occurs. Therefore, using errors=remount-ro
> to prevent modifications after an error is sufficient. Of course, if
> customers have such needs, implementing this mode is also very simple.

IO errors, sure.  Metadata errors?  No, we want to stop the world
immediately, either so the sysadmin can go run xfs_repair, or the clod
manager can just kill the node and deploy another.

--D

> 
> 
> Thanks,
> Baokun
> 
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BUG REPORT] ext4: “errors=remount-ro” has become “errors=shutdown”?
  2025-01-07  7:08                 ` Darrick J. Wong
@ 2025-01-08  2:08                   ` Baokun Li
  0 siblings, 0 replies; 11+ messages in thread
From: Baokun Li @ 2025-01-08  2:08 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Theodore Ts'o, Jan Kara, linux-ext4@vger.kernel.org,
	Christian Brauner, sunyongjian1, Yang Erkun, linux-fsdevel,
	linux-xfs, Baokun Li

On 2025/1/7 15:08, Darrick J. Wong wrote:
> On Tue, Jan 07, 2025 at 10:01:32AM +0800, Baokun Li wrote:
>> On 2025/1/7 7:49, Darrick J. Wong wrote:
>>> On Sat, Jan 04, 2025 at 10:41:28AM +0800, Baokun Li wrote:
>>>> Hi Ted,
>>>>
>>>> On 2025/1/3 23:54, Theodore Ts'o wrote:
>>>>> On Fri, Jan 03, 2025 at 10:35:17AM -0500, Theodore Ts'o wrote:
>>>>>> I don't see how setting the shutdown flag causes reads to fail.  That
>>>>>> was true in an early version of the ext4 patch which implemented
>>>>>> shutdown support, but one of the XFS developers (I don't remember if
>>>>>> it was Dave or Cristoph) objected because XFS did not cause the
>>>>>> read_pages function to fail.  Are you seeing this with an upstream
>>>>>> kernel, or with a patched kernel?  The upstream kernel does *not* have
>>>>>> the check in ext4_readpages() or ext4_read_folio() (post folio
>>>>>> conversion).
>>>>> OK, that's weird.  Testing on 6.13-rc4, I don't see the problem simulating an ext4 error:
>>>>>
>>>>> root@kvm-xfstests:~# mke2fs -t ext4 -Fq /dev/vdc
>>>>> /dev/vdc contains a ext4 file system
>>>>> 	last mounted on /vdc on Fri Jan  3 10:38:21 2025
>>>>> root@kvm-xfstests:~# mount -t ext4 -o errors=continue /dev/vdc /vdc
>>>> We are discussing "errors=remount-ro," as the title states, not the
>>>> continue mode. The key code leading to the behavior change is as follows,
>>>> therefore the continue mode is not affected.
>>> Hmm.  On the one hand, XFS has generally returned EIO (or ESHUTDOWN in a
>>> couple of specialty cases) when the fs has been shut down.
>> Indeed, this is the intended behavior during shutdown.
>>> OTOH XFS also doesn't have errors=remount-ro; it just dies, which I
>>> think has been its behavior for a long time.
>> Yes. As an aside, is there any way for xfs to determine if -EIO is
>> originating from a hardware error or if the filesystem has been shutdown?
> XFS knows the difference, but nothing above it does.
Okay.
>> Or would you consider it useful to have the mount command display
>> "shutdown" when the file system is being shut down?
> Trouble is, will mount get confused and try to pass ",shutdown" as part
> of a remount operation?
The ",shutdown" string is only displayed by show_options when specific
flags are set; it's not actually parsed by remount. Unless the sysadmin
sees it in the mount command output and then mounts with this option.
> I suppose the fs is dead so what does it
> matter...
Since XFS is typically already shut down when it returns EIO, this prompt
may not be important for xfs. However, it's not as straightforward to
distinguish between EIO and shutdown for file systems that support a
continue mode or allow some operations even after shutdown.
>>> To me, it doesn't sound unreasonable for ext* to allow reads after a
>>> shutdown when errors=remount-ro since it's always had that behavior.
>> Yes, a previous bug fix inadvertently changed the behavior of
>> errors=remount-ro,
>> and the patch to correct this is coming.
>>
>> Additionally, ext4 now allows directory reads even after shutdown, is this
>> expected behavior?
> There's no formal specification for what shutdown means, so ... it's not
> unexpected.  XFS doesn't allow that.
Okay.
>>> Bonus Q: do you want an errors=fail variant to shut things down fast?
>>>
>>> --D
>> In my opinion, I have not yet seen a scenario where the file system needs
>> to be shut down after an error occurs. Therefore, using errors=remount-ro
>> to prevent modifications after an error is sufficient. Of course, if
>> customers have such needs, implementing this mode is also very simple.
> IO errors, sure.  Metadata errors?  No, we want to stop the world
> immediately, either so the sysadmin can go run xfs_repair, or the clod
> manager can just kill the node and deploy another.
>
> --D

The remount-ro mode generally only becomes read-only when metadata errors
occur,too. I think if users have no need to read after an error, there is
basically no difference between read-only and shutdown. In fact,
errors=remount-ro does more; it allows users to back up some potentially
lost data after an error and then exit gracefully. However, reading
corrupted metadata does have some risks, for which we have done a lot of
work.


Regards,
Baokun


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-01-08  2:08 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <22d652f6-cb3c-43f5-b2fe-0a4bb6516a04@huawei.com>
     [not found] ` <z52ea53du2k66du24ju4yetqm72e6pvtcbwkrjf4oomw2feffq@355vymdndrxn>
     [not found]   ` <17108cad-efa8-46b4-a320-70d7b696f75b@huawei.com>
2025-01-03 10:42     ` [BUG REPORT] ext4: “errors=remount-ro” has become “errors=shutdown”? Jan Kara
2025-01-03 13:19       ` Baokun Li
2025-01-03 14:34         ` Jan Kara
2025-01-04  3:15           ` Baokun Li
2025-01-03 15:35       ` Theodore Ts'o
2025-01-03 15:54         ` Theodore Ts'o
2025-01-04  2:41           ` Baokun Li
2025-01-06 23:49             ` Darrick J. Wong
2025-01-07  2:01               ` Baokun Li
2025-01-07  7:08                 ` Darrick J. Wong
2025-01-08  2:08                   ` Baokun Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox