fs: sync_filesystem should not discard sync_fs return code
diff mbox

Message ID 20170530155036.17072-1-jlayton@redhat.com
State New
Headers show

Commit Message

Jeff Layton May 30, 2017, 3:50 p.m. UTC
sync_filesystem is called from several kernel subsystems to write out
a superblock. After writing out the inodes, it calls the superblock
sync_fs operation, which is an int return operation. That error code
is then discarded. It then calls __sync_blockdev and returns an error
if that fails.

Most filesystems that have a sync_fs operation do most of the work
inside that operation and then return an error if it fails. With those
filesystems, the sync_blockdev is effectively a no-op and may very well
not return an error even though writeback failed.

Have __sync_filesystem preserve the error from sync_fs and return that
if it fails, or the return from __sync_blockdev if it succeeds.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/sync.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Jan Kara May 31, 2017, 8:17 a.m. UTC | #1
On Tue 30-05-17 11:50:36, Jeff Layton wrote:
> sync_filesystem is called from several kernel subsystems to write out
> a superblock. After writing out the inodes, it calls the superblock
> sync_fs operation, which is an int return operation. That error code
> is then discarded. It then calls __sync_blockdev and returns an error
> if that fails.
> 
> Most filesystems that have a sync_fs operation do most of the work
> inside that operation and then return an error if it fails. With those
> filesystems, the sync_blockdev is effectively a no-op and may very well
> not return an error even though writeback failed.
> 
> Have __sync_filesystem preserve the error from sync_fs and return that
> if it fails, or the return from __sync_blockdev if it succeeds.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

Makes sense. Although this will likely have only a small impact since most
of the errors are likely to happen in the inode writeback where they just
get ignored. Anyway you can add:

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

> ---
>  fs/sync.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/sync.c b/fs/sync.c
> index 45ecf6854595..ec93aac4feb9 100644
> --- a/fs/sync.c
> +++ b/fs/sync.c
> @@ -29,14 +29,18 @@
>   */
>  static int __sync_filesystem(struct super_block *sb, int wait)
>  {
> +	int fs_ret = 0, bd_ret;
> +
>  	if (wait)
>  		sync_inodes_sb(sb);
>  	else
>  		writeback_inodes_sb(sb, WB_REASON_SYNC);
>  
>  	if (sb->s_op->sync_fs)
> -		sb->s_op->sync_fs(sb, wait);
> -	return __sync_blockdev(sb->s_bdev, wait);
> +		fs_ret = sb->s_op->sync_fs(sb, wait);
> +	bd_ret = __sync_blockdev(sb->s_bdev, wait);
> +
> +	return fs_ret ? fs_ret : bd_ret;
>  }
>  
>  /*
> -- 
> 2.9.4
>

Patch
diff mbox

diff --git a/fs/sync.c b/fs/sync.c
index 45ecf6854595..ec93aac4feb9 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -29,14 +29,18 @@ 
  */
 static int __sync_filesystem(struct super_block *sb, int wait)
 {
+	int fs_ret = 0, bd_ret;
+
 	if (wait)
 		sync_inodes_sb(sb);
 	else
 		writeback_inodes_sb(sb, WB_REASON_SYNC);
 
 	if (sb->s_op->sync_fs)
-		sb->s_op->sync_fs(sb, wait);
-	return __sync_blockdev(sb->s_bdev, wait);
+		fs_ret = sb->s_op->sync_fs(sb, wait);
+	bd_ret = __sync_blockdev(sb->s_bdev, wait);
+
+	return fs_ret ? fs_ret : bd_ret;
 }
 
 /*