diff mbox

btrfs: btrfs_wait_tree_block_writeback can be void return

Message ID 20170525103952.7173-1-jlayton@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jeff Layton May 25, 2017, 10:39 a.m. UTC 
Nothing checks its return value.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/disk-io.c | 6 +++---
 fs/btrfs/disk-io.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

David Sterba May 26, 2017, 5:08 p.m. UTC | #1 
On Thu, May 25, 2017 at 06:39:52AM -0400, Jeff Layton wrote:
> Nothing checks its return value.

I think we don't need to check the return value of
filemap_fdatawait_range, because the errors are tracked by other means
(attached to the btree_inode mapping that represents the metadata), in
set_btree_ioerr.

> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>

To Liu Bo:

May I ask if you did a review that matches the above understanding? It's
trivial to see that nothing really checks the return value of
btrfs_wait_tree_block_writeback but my question is if it's really safe
not to so. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Liu Bo May 26, 2017, 7:49 p.m. UTC | #2 
On Fri, May 26, 2017 at 07:08:31PM +0200, David Sterba wrote:
> On Thu, May 25, 2017 at 06:39:52AM -0400, Jeff Layton wrote:
> > Nothing checks its return value.
> 
> I think we don't need to check the return value of
> filemap_fdatawait_range, because the errors are tracked by other means
> (attached to the btree_inode mapping that represents the metadata), in
> set_btree_ioerr.
> 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > Reviewed-by: Jan Kara <jack@suse.cz>
> > Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
> 
> To Liu Bo:
> 
> May I ask if you did a review that matches the above understanding? It's
> trivial to see that nothing really checks the return value of
> btrfs_wait_tree_block_writeback but my question is if it's really safe
> not to so. Thanks.

About the question if it's safe not to do so, I think yes, it's used in
walk_log_tree which is called in two places, free_log_tree and log replay.  For
free_log_tree, it waits for any running writeback of the extent buffer under
freeing to finish in case we need to access the eb pointer from page->private,
and it's OK to not check the return value, while for log replay, it's doesn't
wait because wc->wait is not set. So neither cares about the writeback error.

thanks,
-liubo
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba May 29, 2017, 1:30 p.m. UTC | #3 
On Fri, May 26, 2017 at 12:49:25PM -0700, Liu Bo wrote:
> On Fri, May 26, 2017 at 07:08:31PM +0200, David Sterba wrote:
> > On Thu, May 25, 2017 at 06:39:52AM -0400, Jeff Layton wrote:
> > > Nothing checks its return value.
> > 
> > I think we don't need to check the return value of
> > filemap_fdatawait_range, because the errors are tracked by other means
> > (attached to the btree_inode mapping that represents the metadata), in
> > set_btree_ioerr.
> > 
> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > Reviewed-by: Jan Kara <jack@suse.cz>
> > > Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
> > 
> > To Liu Bo:
> > 
> > May I ask if you did a review that matches the above understanding? It's
> > trivial to see that nothing really checks the return value of
> > btrfs_wait_tree_block_writeback but my question is if it's really safe
> > not to so. Thanks.
> 
> About the question if it's safe not to do so, I think yes, it's used in
> walk_log_tree which is called in two places, free_log_tree and log replay.  For
> free_log_tree, it waits for any running writeback of the extent buffer under
> freeing to finish in case we need to access the eb pointer from page->private,
> and it's OK to not check the return value, while for log replay, it's doesn't
> wait because wc->wait is not set. So neither cares about the writeback error.

Thanks, I'll update the changelog.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 8685d67185d0..17acb72fed0f 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1222,10 +1222,10 @@  int btrfs_write_tree_block(struct extent_buffer *buf)
 					buf->start + buf->len - 1);
 }
 
-int btrfs_wait_tree_block_writeback(struct extent_buffer *buf)
+void btrfs_wait_tree_block_writeback(struct extent_buffer *buf)
 {
-	return filemap_fdatawait_range(buf->pages[0]->mapping,
-				       buf->start, buf->start + buf->len - 1);
+	filemap_fdatawait_range(buf->pages[0]->mapping,
+			        buf->start, buf->start + buf->len - 1);
 }
 
 struct extent_buffer *read_tree_block(struct btrfs_fs_info *fs_info, u64 bytenr,
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index 21f1ceb85b76..f92f3e177d70 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -127,7 +127,7 @@  int btrfs_wq_submit_bio(struct btrfs_fs_info *fs_info, struct inode *inode,
 			extent_submit_bio_hook_t *submit_bio_done);
 unsigned long btrfs_async_submit_limit(struct btrfs_fs_info *info);
 int btrfs_write_tree_block(struct extent_buffer *buf);
-int btrfs_wait_tree_block_writeback(struct extent_buffer *buf);
+void btrfs_wait_tree_block_writeback(struct extent_buffer *buf);
 int btrfs_init_log_root_tree(struct btrfs_trans_handle *trans,
 			     struct btrfs_fs_info *fs_info);
 int btrfs_add_log_tree(struct btrfs_trans_handle *trans,