[PATCHSET 0/4] vfs: actually return fs errors from ->sync_fs

[PATCHSET 0/4] vfs: actually return fs errors from ->sync_fs

[Darrick J. Wong]

Hi all,

While auditing the VFS code, I noticed that while ->sync_fs is allowed
to return error codes to reflect some sort of internal filesystem error,
none of the callers actually check the return value.  Back when this
callout was introduced for sync_filesystem in 2.5 this didn't matter
because we only did it prior to rebooting, and best-effort was enough.

Nowadays, we've grown other callers that mandate persistence of fs
metadata, like syncfs(2) and quota sync.  Reporting internal fs errors
is critical for these functions, and we drop the errors.  Fix them, and
also fix FIFREEZE, so that userspace can't freeze broken filesystems.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=return-sync-fs-errors-5.17
---
 fs/quota/dquot.c   |   11 ++++++++---
 fs/super.c         |   19 ++++++++++++-------
 fs/sync.c          |   18 ++++++++++++------
 fs/xfs/xfs_super.c |    6 +++++-
 4 files changed, 37 insertions(+), 17 deletions(-)

[PATCH 1/4] vfs: make freeze_super abort when sync_filesystem returns error

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

If we fail to synchronize the filesystem while preparing to freeze the
fs, abort the freeze.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/super.c |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)


diff --git a/fs/super.c b/fs/super.c
index 7af820ba5ad5..f1d4a193602d 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1616,11 +1616,9 @@ static void lockdep_sb_freeze_acquire(struct super_block *sb)
 		percpu_rwsem_acquire(sb->s_writers.rw_sem + level, 0, _THIS_IP_);
 }
 
-static void sb_freeze_unlock(struct super_block *sb)
+static void sb_freeze_unlock(struct super_block *sb, int level)
 {
-	int level;
-
-	for (level = SB_FREEZE_LEVELS - 1; level >= 0; level--)
+	for (level--; level >= 0; level--)
 		percpu_up_write(sb->s_writers.rw_sem + level);
 }
 
@@ -1691,7 +1689,14 @@ int freeze_super(struct super_block *sb)
 	sb_wait_write(sb, SB_FREEZE_PAGEFAULT);
 
 	/* All writers are done so after syncing there won't be dirty data */
-	sync_filesystem(sb);
+	ret = sync_filesystem(sb);
+	if (ret) {
+		sb->s_writers.frozen = SB_UNFROZEN;
+		sb_freeze_unlock(sb, SB_FREEZE_PAGEFAULT);
+		wake_up(&sb->s_writers.wait_unfrozen);
+		deactivate_locked_super(sb);
+		return ret;
+	}
 
 	/* Now wait for internal filesystem counter */
 	sb->s_writers.frozen = SB_FREEZE_FS;
@@ -1703,7 +1708,7 @@ int freeze_super(struct super_block *sb)
 			printk(KERN_ERR
 				"VFS:Filesystem freeze failed\n");
 			sb->s_writers.frozen = SB_UNFROZEN;
-			sb_freeze_unlock(sb);
+			sb_freeze_unlock(sb, SB_FREEZE_FS);
 			wake_up(&sb->s_writers.wait_unfrozen);
 			deactivate_locked_super(sb);
 			return ret;
@@ -1748,7 +1753,7 @@ static int thaw_super_locked(struct super_block *sb)
 	}
 
 	sb->s_writers.frozen = SB_UNFROZEN;
-	sb_freeze_unlock(sb);
+	sb_freeze_unlock(sb, SB_FREEZE_FS);
 out:
 	wake_up(&sb->s_writers.wait_unfrozen);
 	deactivate_locked_super(sb);

Re: [PATCH 1/4] vfs: make freeze_super abort when sync_filesystem returns error

[Jan Kara]

On Tue 25-01-22 18:18:15, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> If we fail to synchronize the filesystem while preparing to freeze the
> fs, abort the freeze.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/super.c |   19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> 
> 
> diff --git a/fs/super.c b/fs/super.c
> index 7af820ba5ad5..f1d4a193602d 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1616,11 +1616,9 @@ static void lockdep_sb_freeze_acquire(struct super_block *sb)
>  		percpu_rwsem_acquire(sb->s_writers.rw_sem + level, 0, _THIS_IP_);
>  }
>  
> -static void sb_freeze_unlock(struct super_block *sb)
> +static void sb_freeze_unlock(struct super_block *sb, int level)
>  {
> -	int level;
> -
> -	for (level = SB_FREEZE_LEVELS - 1; level >= 0; level--)
> +	for (level--; level >= 0; level--)
>  		percpu_up_write(sb->s_writers.rw_sem + level);
>  }
>  
> @@ -1691,7 +1689,14 @@ int freeze_super(struct super_block *sb)
>  	sb_wait_write(sb, SB_FREEZE_PAGEFAULT);
>  
>  	/* All writers are done so after syncing there won't be dirty data */
> -	sync_filesystem(sb);
> +	ret = sync_filesystem(sb);
> +	if (ret) {
> +		sb->s_writers.frozen = SB_UNFROZEN;
> +		sb_freeze_unlock(sb, SB_FREEZE_PAGEFAULT);
> +		wake_up(&sb->s_writers.wait_unfrozen);
> +		deactivate_locked_super(sb);
> +		return ret;
> +	}
>  
>  	/* Now wait for internal filesystem counter */
>  	sb->s_writers.frozen = SB_FREEZE_FS;
> @@ -1703,7 +1708,7 @@ int freeze_super(struct super_block *sb)
>  			printk(KERN_ERR
>  				"VFS:Filesystem freeze failed\n");
>  			sb->s_writers.frozen = SB_UNFROZEN;
> -			sb_freeze_unlock(sb);
> +			sb_freeze_unlock(sb, SB_FREEZE_FS);
>  			wake_up(&sb->s_writers.wait_unfrozen);
>  			deactivate_locked_super(sb);
>  			return ret;
> @@ -1748,7 +1753,7 @@ static int thaw_super_locked(struct super_block *sb)
>  	}
>  
>  	sb->s_writers.frozen = SB_UNFROZEN;
> -	sb_freeze_unlock(sb);
> +	sb_freeze_unlock(sb, SB_FREEZE_FS);
>  out:
>  	wake_up(&sb->s_writers.wait_unfrozen);
>  	deactivate_locked_super(sb);
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/4] vfs: make freeze_super abort when sync_filesystem returns error

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/4] vfs: make sync_filesystem return errors from ->sync_fs

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Strangely, sync_filesystem ignores the return code from the ->sync_fs
call, which means that syscalls like syncfs(2) never see the error.
This doesn't seem right, so fix that.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/sync.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)


diff --git a/fs/sync.c b/fs/sync.c
index 3ce8e2137f31..c7690016453e 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -29,7 +29,7 @@
  */
 int sync_filesystem(struct super_block *sb)
 {
-	int ret;
+	int ret = 0;
 
 	/*
 	 * We need to be protected against the filesystem going from
@@ -52,15 +52,21 @@ int sync_filesystem(struct super_block *sb)
 	 * at a time.
 	 */
 	writeback_inodes_sb(sb, WB_REASON_SYNC);
-	if (sb->s_op->sync_fs)
-		sb->s_op->sync_fs(sb, 0);
+	if (sb->s_op->sync_fs) {
+		ret = sb->s_op->sync_fs(sb, 0);
+		if (ret)
+			return ret;
+	}
 	ret = sync_blockdev_nowait(sb->s_bdev);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	sync_inodes_sb(sb);
-	if (sb->s_op->sync_fs)
-		sb->s_op->sync_fs(sb, 1);
+	if (sb->s_op->sync_fs) {
+		ret = sb->s_op->sync_fs(sb, 1);
+		if (ret)
+			return ret;
+	}
 	return sync_blockdev(sb->s_bdev);
 }
 EXPORT_SYMBOL(sync_filesystem);

Re: [PATCH 2/4] vfs: make sync_filesystem return errors from ->sync_fs

[Jan Kara]

On Tue 25-01-22 18:18:20, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Strangely, sync_filesystem ignores the return code from the ->sync_fs
> call, which means that syscalls like syncfs(2) never see the error.
> This doesn't seem right, so fix that.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/sync.c |   18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> 
> diff --git a/fs/sync.c b/fs/sync.c
> index 3ce8e2137f31..c7690016453e 100644
> --- a/fs/sync.c
> +++ b/fs/sync.c
> @@ -29,7 +29,7 @@
>   */
>  int sync_filesystem(struct super_block *sb)
>  {
> -	int ret;
> +	int ret = 0;
>  
>  	/*
>  	 * We need to be protected against the filesystem going from
> @@ -52,15 +52,21 @@ int sync_filesystem(struct super_block *sb)
>  	 * at a time.
>  	 */
>  	writeback_inodes_sb(sb, WB_REASON_SYNC);
> -	if (sb->s_op->sync_fs)
> -		sb->s_op->sync_fs(sb, 0);
> +	if (sb->s_op->sync_fs) {
> +		ret = sb->s_op->sync_fs(sb, 0);
> +		if (ret)
> +			return ret;
> +	}
>  	ret = sync_blockdev_nowait(sb->s_bdev);
> -	if (ret < 0)
> +	if (ret)
>  		return ret;
>  
>  	sync_inodes_sb(sb);
> -	if (sb->s_op->sync_fs)
> -		sb->s_op->sync_fs(sb, 1);
> +	if (sb->s_op->sync_fs) {
> +		ret = sb->s_op->sync_fs(sb, 1);
> +		if (ret)
> +			return ret;
> +	}
>  	return sync_blockdev(sb->s_bdev);
>  }
>  EXPORT_SYMBOL(sync_filesystem);
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/4] vfs: make sync_filesystem return errors from ->sync_fs

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 3/4] quota: make dquot_quota_sync return errors from ->sync_fs

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Strangely, dquot_quota_sync ignores the return code from the ->sync_fs
call, which means that quotacalls like Q_SYNC never see the error.  This
doesn't seem right, so fix that.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/quota/dquot.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)


diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 22d904bde6ab..a74aef99bd3d 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -690,9 +690,14 @@ int dquot_quota_sync(struct super_block *sb, int type)
 	/* This is not very clever (and fast) but currently I don't know about
 	 * any other simple way of getting quota data to disk and we must get
 	 * them there for userspace to be visible... */
-	if (sb->s_op->sync_fs)
-		sb->s_op->sync_fs(sb, 1);
-	sync_blockdev(sb->s_bdev);
+	if (sb->s_op->sync_fs) {
+		ret = sb->s_op->sync_fs(sb, 1);
+		if (ret)
+			return ret;
+	}
+	ret = sync_blockdev(sb->s_bdev);
+	if (ret)
+		return ret;
 
 	/*
 	 * Now when everything is written we can discard the pagecache so

Re: [PATCH 3/4] quota: make dquot_quota_sync return errors from ->sync_fs

[Jan Kara]

On Tue 25-01-22 18:18:26, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Strangely, dquot_quota_sync ignores the return code from the ->sync_fs
> call, which means that quotacalls like Q_SYNC never see the error.  This
> doesn't seem right, so fix that.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/quota/dquot.c |   11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> 
> 
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index 22d904bde6ab..a74aef99bd3d 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -690,9 +690,14 @@ int dquot_quota_sync(struct super_block *sb, int type)
>  	/* This is not very clever (and fast) but currently I don't know about
>  	 * any other simple way of getting quota data to disk and we must get
>  	 * them there for userspace to be visible... */
> -	if (sb->s_op->sync_fs)
> -		sb->s_op->sync_fs(sb, 1);
> -	sync_blockdev(sb->s_bdev);
> +	if (sb->s_op->sync_fs) {
> +		ret = sb->s_op->sync_fs(sb, 1);
> +		if (ret)
> +			return ret;
> +	}
> +	ret = sync_blockdev(sb->s_bdev);
> +	if (ret)
> +		return ret;
>  
>  	/*
>  	 * Now when everything is written we can discard the pagecache so
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/4] quota: make dquot_quota_sync return errors from ->sync_fs

[Christoph Hellwig]

On Tue, Jan 25, 2022 at 06:18:26PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Strangely, dquot_quota_sync ignores the return code from the ->sync_fs
> call, which means that quotacalls like Q_SYNC never see the error.  This
> doesn't seem right, so fix that.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 4/4] xfs: return errors in xfs_fs_sync_fs

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Now that the VFS will do something with the return values from
->sync_fs, make ours pass on error codes.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_super.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index e8f37bdc8354..4c0dee78b2f8 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -735,6 +735,7 @@ xfs_fs_sync_fs(
 	int			wait)
 {
 	struct xfs_mount	*mp = XFS_M(sb);
+	int			error;
 
 	trace_xfs_fs_sync_fs(mp, __return_address);
 
@@ -744,7 +745,10 @@ xfs_fs_sync_fs(
 	if (!wait)
 		return 0;
 
-	xfs_log_force(mp, XFS_LOG_SYNC);
+	error = xfs_log_force(mp, XFS_LOG_SYNC);
+	if (error)
+		return error;
+
 	if (laptop_mode) {
 		/*
 		 * The disk must be active because we're syncing.

Re: [PATCH 4/4] xfs: return errors in xfs_fs_sync_fs

[Jan Kara]

On Tue 25-01-22 18:18:31, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Now that the VFS will do something with the return values from
> ->sync_fs, make ours pass on error codes.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_super.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Makes sence. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> 
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index e8f37bdc8354..4c0dee78b2f8 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -735,6 +735,7 @@ xfs_fs_sync_fs(
>  	int			wait)
>  {
>  	struct xfs_mount	*mp = XFS_M(sb);
> +	int			error;
>  
>  	trace_xfs_fs_sync_fs(mp, __return_address);
>  
> @@ -744,7 +745,10 @@ xfs_fs_sync_fs(
>  	if (!wait)
>  		return 0;
>  
> -	xfs_log_force(mp, XFS_LOG_SYNC);
> +	error = xfs_log_force(mp, XFS_LOG_SYNC);
> +	if (error)
> +		return error;
> +
>  	if (laptop_mode) {
>  		/*
>  		 * The disk must be active because we're syncing.
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/4] xfs: return errors in xfs_fs_sync_fs

[Christoph Hellwig]

On Tue, Jan 25, 2022 at 06:18:31PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Now that the VFS will do something with the return values from
> ->sync_fs, make ours pass on error codes.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHSET 0/4] vfs: actually return fs errors from ->sync_fs

[Christian Brauner]

On Tue, Jan 25, 2022 at 06:18:09PM -0800, Darrick J. Wong wrote:
> Hi all,
> 
> While auditing the VFS code, I noticed that while ->sync_fs is allowed
> to return error codes to reflect some sort of internal filesystem error,
> none of the callers actually check the return value.  Back when this
> callout was introduced for sync_filesystem in 2.5 this didn't matter

(Also, it looks like that most(/none?) of the filesystems that
implemented ->sync_fs around 2.5/2.6 (ext3, jfs, jffs2, reiserfs etc.)
actually did return an error?
In fact, 5.8 seems to be the first kernel to report other errors than
-EBADF since commit 735e4ae5ba28 ("vfs: track per-sb writeback errors
and report them to syncfs"?)

Re: [PATCHSET 0/4] vfs: actually return fs errors from ->sync_fs

[Darrick J. Wong]

On Wed, Jan 26, 2022 at 09:21:53AM +0100, Christian Brauner wrote:
> On Tue, Jan 25, 2022 at 06:18:09PM -0800, Darrick J. Wong wrote:
> > Hi all,
> > 
> > While auditing the VFS code, I noticed that while ->sync_fs is allowed
> > to return error codes to reflect some sort of internal filesystem error,
> > none of the callers actually check the return value.  Back when this
> > callout was introduced for sync_filesystem in 2.5 this didn't matter
> 
> (Also, it looks like that most(/none?) of the filesystems that
> implemented ->sync_fs around 2.5/2.6 (ext3, jfs, jffs2, reiserfs etc.)
> actually did return an error?

Yes, some of them do -- ext4 will bubble up jbd2 errors and the results
of flushing the bdev write cache.

> In fact, 5.8 seems to be the first kernel to report other errors than
> -EBADF since commit 735e4ae5ba28 ("vfs: track per-sb writeback errors
> and report them to syncfs"?)

Yeah.  I think the bdev pagecache flush might occasionally return errors
if there happened to be dirty pages, but (a) that doesn't help XFS which
has its own buffer cache and (b) that doesn't capture the state "fs has
errored out but media is fine".

As it is I think the ext4 syncfs needs to start returning EIO if someone
forced a shutdown, and probably some auditing for dropped error codes
due to the 'traditional' vfs behavior.  btrfs probably ought to return
the result of filemap_flush too.

--D

Re: [PATCHSET 0/4] vfs: actually return fs errors from ->sync_fs

[Christian Brauner]

On Wed, Jan 26, 2022 at 10:05:07AM -0800, Darrick J. Wong wrote:
> On Wed, Jan 26, 2022 at 09:21:53AM +0100, Christian Brauner wrote:
> > On Tue, Jan 25, 2022 at 06:18:09PM -0800, Darrick J. Wong wrote:
> > > Hi all,
> > > 
> > > While auditing the VFS code, I noticed that while ->sync_fs is allowed
> > > to return error codes to reflect some sort of internal filesystem error,
> > > none of the callers actually check the return value.  Back when this
> > > callout was introduced for sync_filesystem in 2.5 this didn't matter
> > 
> > (Also, it looks like that most(/none?) of the filesystems that
> > implemented ->sync_fs around 2.5/2.6 (ext3, jfs, jffs2, reiserfs etc.)
> > actually did return an error?
> 
> Yes, some of them do -- ext4 will bubble up jbd2 errors and the results
> of flushing the bdev write cache.
> 
> > In fact, 5.8 seems to be the first kernel to report other errors than
> > -EBADF since commit 735e4ae5ba28 ("vfs: track per-sb writeback errors
> > and report them to syncfs"?)
> 
> Yeah.  I think the bdev pagecache flush might occasionally return errors
> if there happened to be dirty pages, but (a) that doesn't help XFS which
> has its own buffer cache and (b) that doesn't capture the state "fs has
> errored out but media is fine".
> 
> As it is I think the ext4 syncfs needs to start returning EIO if someone
> forced a shutdown, and probably some auditing for dropped error codes
> due to the 'traditional' vfs behavior.  btrfs probably ought to return
> the result of filemap_flush too.

Makes sense. Fwiw,
Acked-by: Christian Brauner <brauner@kernel.org>