diff mbox

[v8,16/18] ext4: use errseq_t based error handling for reporting data writeback errors

Message ID 20170629131954.28733-17-jlayton@kernel.org (mailing list archive)
State New, archived
Headers show

Commit Message

Jeff Layton June 29, 2017, 1:19 p.m. UTC 
From: Jeff Layton <jlayton@redhat.com>

Add a call to filemap_report_wb_err at the end of ext4_sync_file. This
will ensure that we check and advance the errseq_t in the file, which
allows us to track and report errors on all open fds when they occur.

Note that metadata writeback errors are not yet reported on all fds at
this point. That will be added in a later patch.

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

Comments

Christoph Hellwig June 29, 2017, 2:12 p.m. UTC | #1 
> -	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
> -		return -EIO;
> +	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) {
> +		ret = -EIO;
> +		goto out;
> +	}

This just seems to add a call to trace_ext4_sync_file_exit for this
case, which seems unrelated to the patch.

>  	if (ret)
> -		return ret;
> +		goto out;
> +

Same here.

>  	/*
>  	 * data=writeback,ordered:
>  	 *  The caller's filemap_fdatawrite()/wait will sync the data.
> @@ -152,7 +155,7 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
>  		needs_barrier = true;
>  	ret = jbd2_complete_transaction(journal, commit_tid);
>  	if (needs_barrier) {
> -	issue_flush:
> +issue_flush:
>  		err = blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);

And while I much prefer your new label placement it also doesn't
seem to belong into this patch.
Jeff Layton June 29, 2017, 8:26 p.m. UTC | #2 
On Thu, 2017-06-29 at 07:12 -0700, Christoph Hellwig wrote:
> > -	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
> > -		return -EIO;
> > +	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) {
> > +		ret = -EIO;
> > +		goto out;
> > +	}
> 
> This just seems to add a call to trace_ext4_sync_file_exit for this
> case, which seems unrelated to the patch.
> 
> >  	if (ret)
> > -		return ret;
> > +		goto out;
> > +
> 
> Same here.
> 
> >  	/*
> >  	 * data=writeback,ordered:
> >  	 *  The caller's filemap_fdatawrite()/wait will sync the data.
> > @@ -152,7 +155,7 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
> >  		needs_barrier = true;
> >  	ret = jbd2_complete_transaction(journal, commit_tid);
> >  	if (needs_barrier) {
> > -	issue_flush:
> > +issue_flush:
> >  		err = blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
> 
> And while I much prefer your new label placement it also doesn't
> seem to belong into this patch.

I revised the patch description earlier to say:

    While we're at it, ensure we always "goto out" instead of just
    returning directly, so that we always hit the exit tracepoint.

...but I'm fine with taking that out if you prefer.
diff mbox

Patch

diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index 9d549608fd30..d7cf0934c71c 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -100,8 +100,10 @@  int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	tid_t commit_tid;
 	bool needs_barrier = false;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
-		return -EIO;
+	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) {
+		ret = -EIO;
+		goto out;
+	}
 
 	J_ASSERT(ext4_journal_current_handle() == NULL);
 
@@ -124,9 +126,10 @@  int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 		goto out;
 	}
 
-	ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
+	ret = file_write_and_wait_range(file, start, end);
 	if (ret)
-		return ret;
+		goto out;
+
 	/*
 	 * data=writeback,ordered:
 	 *  The caller's filemap_fdatawrite()/wait will sync the data.
@@ -152,7 +155,7 @@  int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 		needs_barrier = true;
 	ret = jbd2_complete_transaction(journal, commit_tid);
 	if (needs_barrier) {
-	issue_flush:
+issue_flush:
 		err = blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
 		if (!ret)
 			ret = err;