diff mbox series

[1/9] btrfs: get rid of trivial __btrfs_lookup_bio_sums() wrappers

Message ID af5aefd84186419ead73107895ddd6aba02ef8b6.1575336815.git.osandov@fb.com (mailing list archive)
State New, archived
Headers show
Series btrfs: miscellaneous cleanups | expand

Commit Message

Omar Sandoval Dec. 3, 2019, 1:34 a.m. UTC 
From: Omar Sandoval <osandov@fb.com>

Currently, we have two wrappers for __btrfs_lookup_bio_sums():
btrfs_lookup_bio_sums_dio(), which is used for direct I/O, and
btrfs_lookup_bio_sums(), which is used everywhere else. The only
difference is that the _dio variant looks up csums starting at the given
offset instead of using the page index, which isn't actually direct
I/O-specific. Let's clean up the signature and return value of
__btrfs_lookup_bio_sums(), rename it to btrfs_lookup_bio_sums(), and get
rid of the trivial helpers.

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/compression.c |  4 ++--
 fs/btrfs/ctree.h       |  4 +---
 fs/btrfs/file-item.c   | 35 +++++++++++++++++------------------
 fs/btrfs/inode.c       |  6 +++---
 4 files changed, 23 insertions(+), 26 deletions(-)

Comments

Johannes Thumshirn Dec. 3, 2019, 8:36 a.m. UTC | #1 
Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
David Sterba Dec. 10, 2019, 5:12 p.m. UTC | #2 
On Mon, Dec 02, 2019 at 05:34:17PM -0800, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Currently, we have two wrappers for __btrfs_lookup_bio_sums():
> btrfs_lookup_bio_sums_dio(), which is used for direct I/O, and
> btrfs_lookup_bio_sums(), which is used everywhere else. The only
> difference is that the _dio variant looks up csums starting at the given
> offset instead of using the page index, which isn't actually direct
> I/O-specific. Let's clean up the signature and return value of
> __btrfs_lookup_bio_sums(), rename it to btrfs_lookup_bio_sums(), and get
> rid of the trivial helpers.
> 
>  				ret = btrfs_lookup_bio_sums(inode, comp_bio,
> -							    sums);
> +							    false, 0, sums);

> -		ret = btrfs_lookup_bio_sums(inode, comp_bio, sums);
> +		ret = btrfs_lookup_bio_sums(inode, comp_bio, false, 0, sums);

> -			ret = btrfs_lookup_bio_sums(inode, bio, NULL);
> +			ret = btrfs_lookup_bio_sums(inode, bio, false, 0, NULL);

> -		ret = btrfs_lookup_bio_sums_dio(inode, dip->orig_bio,
> -						file_offset);
> +		ret = btrfs_lookup_bio_sums(inode, dip->orig_bio, true,
> +					    file_offset, NULL);

Can't we also get rid of the at_offset parameter? Encoding that into
file_offset itself where at_offset=true would be some special
placeholder like (u64)-1 that can never be a valid file offset.
Omar Sandoval Dec. 10, 2019, 6:24 p.m. UTC | #3 
On Tue, Dec 10, 2019 at 06:12:10PM +0100, David Sterba wrote:
> On Mon, Dec 02, 2019 at 05:34:17PM -0800, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > Currently, we have two wrappers for __btrfs_lookup_bio_sums():
> > btrfs_lookup_bio_sums_dio(), which is used for direct I/O, and
> > btrfs_lookup_bio_sums(), which is used everywhere else. The only
> > difference is that the _dio variant looks up csums starting at the given
> > offset instead of using the page index, which isn't actually direct
> > I/O-specific. Let's clean up the signature and return value of
> > __btrfs_lookup_bio_sums(), rename it to btrfs_lookup_bio_sums(), and get
> > rid of the trivial helpers.
> > 
> >  				ret = btrfs_lookup_bio_sums(inode, comp_bio,
> > -							    sums);
> > +							    false, 0, sums);
> 
> > -		ret = btrfs_lookup_bio_sums(inode, comp_bio, sums);
> > +		ret = btrfs_lookup_bio_sums(inode, comp_bio, false, 0, sums);
> 
> > -			ret = btrfs_lookup_bio_sums(inode, bio, NULL);
> > +			ret = btrfs_lookup_bio_sums(inode, bio, false, 0, NULL);
> 
> > -		ret = btrfs_lookup_bio_sums_dio(inode, dip->orig_bio,
> > -						file_offset);
> > +		ret = btrfs_lookup_bio_sums(inode, dip->orig_bio, true,
> > +					    file_offset, NULL);
> 
> Can't we also get rid of the at_offset parameter? Encoding that into
> file_offset itself where at_offset=true would be some special
> placeholder like (u64)-1 that can never be a valid file offset.

Yeah Nikolay mentioned this as well but I was on the fence about whether
it would look any nicer. I'll go ahead and make that change.
David Sterba Dec. 10, 2019, 6:26 p.m. UTC | #4 
On Tue, Dec 10, 2019 at 10:24:30AM -0800, Omar Sandoval wrote:
> On Tue, Dec 10, 2019 at 06:12:10PM +0100, David Sterba wrote:
> > On Mon, Dec 02, 2019 at 05:34:17PM -0800, Omar Sandoval wrote:
> > > From: Omar Sandoval <osandov@fb.com>
> > > 
> > > Currently, we have two wrappers for __btrfs_lookup_bio_sums():
> > > btrfs_lookup_bio_sums_dio(), which is used for direct I/O, and
> > > btrfs_lookup_bio_sums(), which is used everywhere else. The only
> > > difference is that the _dio variant looks up csums starting at the given
> > > offset instead of using the page index, which isn't actually direct
> > > I/O-specific. Let's clean up the signature and return value of
> > > __btrfs_lookup_bio_sums(), rename it to btrfs_lookup_bio_sums(), and get
> > > rid of the trivial helpers.
> > > 
> > >  				ret = btrfs_lookup_bio_sums(inode, comp_bio,
> > > -							    sums);
> > > +							    false, 0, sums);
> > 
> > > -		ret = btrfs_lookup_bio_sums(inode, comp_bio, sums);
> > > +		ret = btrfs_lookup_bio_sums(inode, comp_bio, false, 0, sums);
> > 
> > > -			ret = btrfs_lookup_bio_sums(inode, bio, NULL);
> > > +			ret = btrfs_lookup_bio_sums(inode, bio, false, 0, NULL);
> > 
> > > -		ret = btrfs_lookup_bio_sums_dio(inode, dip->orig_bio,
> > > -						file_offset);
> > > +		ret = btrfs_lookup_bio_sums(inode, dip->orig_bio, true,
> > > +					    file_offset, NULL);
> > 
> > Can't we also get rid of the at_offset parameter? Encoding that into
> > file_offset itself where at_offset=true would be some special
> > placeholder like (u64)-1 that can never be a valid file offset.
> 
> Yeah Nikolay mentioned this as well but I was on the fence about whether
> it would look any nicer. I'll go ahead and make that change.

Ok, let's do that as another patch so it's not mixed to the helper
removal. Thanks.
diff mbox series

Patch

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index ee834ef7beb4..03eb50727038 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -758,7 +758,7 @@  blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 
 			if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) {
 				ret = btrfs_lookup_bio_sums(inode, comp_bio,
-							    sums);
+							    false, 0, sums);
 				BUG_ON(ret); /* -ENOMEM */
 			}
 
@@ -786,7 +786,7 @@  blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	BUG_ON(ret); /* -ENOMEM */
 
 	if (!(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) {
-		ret = btrfs_lookup_bio_sums(inode, comp_bio, sums);
+		ret = btrfs_lookup_bio_sums(inode, comp_bio, false, 0, sums);
 		BUG_ON(ret); /* -ENOMEM */
 	}
 
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index b2e8fd8a8e59..5ad45171e482 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2789,9 +2789,7 @@  struct btrfs_dio_private;
 int btrfs_del_csums(struct btrfs_trans_handle *trans,
 		    struct btrfs_fs_info *fs_info, u64 bytenr, u64 len);
 blk_status_t btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio,
-				   u8 *dst);
-blk_status_t btrfs_lookup_bio_sums_dio(struct inode *inode, struct bio *bio,
-			      u64 logical_offset);
+				   bool at_offset, u64 offset, u8 *dst);
 int btrfs_insert_file_extent(struct btrfs_trans_handle *trans,
 			     struct btrfs_root *root,
 			     u64 objectid, u64 pos,
diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 3270a40b0777..b001ad073d16 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -148,8 +148,21 @@  int btrfs_lookup_file_extent(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-static blk_status_t __btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio,
-				   u64 logical_offset, u8 *dst, int dio)
+/**
+ * btrfs_lookup_bio_sums - Look up checksums for a bio.
+ * @inode: inode that the bio is for.
+ * @bio: bio embedded in btrfs_io_bio.
+ * @at_offset: If true, look up checksums for the extent at @offset.
+ *             If false, use the page offsets from the bio.
+ * @offset: If @at_offset is true, offset in file to look up checksums for.
+ *          Ignored otherwise.
+ * @dst: Buffer of size btrfs_super_csum_size() used to return checksum. If
+ *       NULL, the checksum is returned in btrfs_io_bio(bio)->csum instead.
+ *
+ * Return: BLK_STS_RESOURCE if allocating memory fails, BLK_STS_OK otherwise.
+ */
+blk_status_t btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio,
+				   bool at_offset, u64 offset, u8 *dst)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct bio_vec bvec;
@@ -159,7 +172,6 @@  static blk_status_t __btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio
 	struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
 	struct btrfs_path *path;
 	u8 *csum;
-	u64 offset = 0;
 	u64 item_start_offset = 0;
 	u64 item_last_offset = 0;
 	u64 disk_bytenr;
@@ -205,15 +217,13 @@  static blk_status_t __btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio
 	}
 
 	disk_bytenr = (u64)bio->bi_iter.bi_sector << 9;
-	if (dio)
-		offset = logical_offset;
 
 	bio_for_each_segment(bvec, bio, iter) {
 		page_bytes_left = bvec.bv_len;
 		if (count)
 			goto next;
 
-		if (!dio)
+		if (!at_offset)
 			offset = page_offset(bvec.bv_page) + bvec.bv_offset;
 		count = btrfs_find_ordered_sum(inode, offset, disk_bytenr,
 					       csum, nblocks);
@@ -285,18 +295,7 @@  static blk_status_t __btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio
 
 	WARN_ON_ONCE(count);
 	btrfs_free_path(path);
-	return 0;
-}
-
-blk_status_t btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio,
-				   u8 *dst)
-{
-	return __btrfs_lookup_bio_sums(inode, bio, 0, dst, 0);
-}
-
-blk_status_t btrfs_lookup_bio_sums_dio(struct inode *inode, struct bio *bio, u64 offset)
-{
-	return __btrfs_lookup_bio_sums(inode, bio, offset, NULL, 1);
+	return BLK_STS_OK;
 }
 
 int btrfs_lookup_csums_range(struct btrfs_root *root, u64 start, u64 end,
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e3c76645cad7..1fe4e5ec7907 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2128,7 +2128,7 @@  static blk_status_t btrfs_submit_bio_hook(struct inode *inode, struct bio *bio,
 							   bio_flags);
 			goto out;
 		} else if (!skip_sum) {
-			ret = btrfs_lookup_bio_sums(inode, bio, NULL);
+			ret = btrfs_lookup_bio_sums(inode, bio, false, 0, NULL);
 			if (ret)
 				goto out;
 		}
@@ -8356,8 +8356,8 @@  static inline blk_status_t btrfs_lookup_and_bind_dio_csum(struct inode *inode,
 	 * contention.
 	 */
 	if (dip->logical_offset == file_offset) {
-		ret = btrfs_lookup_bio_sums_dio(inode, dip->orig_bio,
-						file_offset);
+		ret = btrfs_lookup_bio_sums(inode, dip->orig_bio, true,
+					    file_offset, NULL);
 		if (ret)
 			return ret;
 	}