Re: [PATCH v10 03/14] btrfs: don't advance offset for compressed bios in btrfs_csum_one_bio()

[Omar Sandoval <osandov@osandov.com>]

On Fri, Aug 20, 2021 at 11:08:41AM +0300, Nikolay Borisov wrote:
> 
> 
> On 18.08.21 г. 0:06, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > btrfs_csum_one_bio() loops over each filesystem block in the bio while
> > keeping a cursor of its current logical position in the file in order to
> > look up the ordered extent to add the checksums to. However, this
> > doesn't make much sense for compressed extents, as a sector on disk does
> > not correspond to a sector of decompressed file data. It happens to work
> > because 1) the compressed bio always covers one ordered extent and 2)
> > the size of the bio is always less than the size of the ordered extent.
> > However, the second point will not always be true for encoded writes.
> > 
> > Let's add a boolean parameter to btrfs_csum_one_bio() to indicate that
> > it can assume that the bio only covers one ordered extent. Since we're
> > already changing the signature, let's get rid of the contig parameter
> > and make it implied by the offset parameter, similar to the change we
> > recently made to btrfs_lookup_bio_sums(). Additionally, let's rename
> > nr_sectors to blockcount to make it clear that it's the number of
> > filesystem blocks, not the number of 512-byte sectors.
> > 
> > Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > ---
> >  fs/btrfs/compression.c |  5 +++--
> >  fs/btrfs/ctree.h       |  2 +-
> >  fs/btrfs/file-item.c   | 35 ++++++++++++++++-------------------
> >  fs/btrfs/inode.c       |  8 ++++----
> >  4 files changed, 24 insertions(+), 26 deletions(-)
> > 
> > diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> > index 7869ad12bc6e..e645b3c2f09a 100644
> > --- a/fs/btrfs/compression.c
> > +++ b/fs/btrfs/compression.c
> > @@ -480,7 +480,8 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
> >  			BUG_ON(ret); /* -ENOMEM */
> >  
> >  			if (!skip_sum) {
> > -				ret = btrfs_csum_one_bio(inode, bio, start, 1);
> > +				ret = btrfs_csum_one_bio(inode, bio, start,
> > +							 true);
> >  				BUG_ON(ret); /* -ENOMEM */
> >  			}
> >  
> > @@ -516,7 +517,7 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
> >  	BUG_ON(ret); /* -ENOMEM */
> >  
> >  	if (!skip_sum) {
> > -		ret = btrfs_csum_one_bio(inode, bio, start, 1);
> > +		ret = btrfs_csum_one_bio(inode, bio, start, true);
> >  		BUG_ON(ret); /* -ENOMEM */
> >  	}
> >  
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index f07c82fafa04..be245b4b8efe 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -3111,7 +3111,7 @@ int btrfs_csum_file_blocks(struct btrfs_trans_handle *trans,
> >  			   struct btrfs_root *root,
> >  			   struct btrfs_ordered_sum *sums);
> >  blk_status_t btrfs_csum_one_bio(struct btrfs_inode *inode, struct bio *bio,
> > -				u64 file_start, int contig);
> > +				u64 offset, bool one_ordered);
> >  int btrfs_lookup_csums_range(struct btrfs_root *root, u64 start, u64 end,
> >  			     struct list_head *list, int search_commit);
> >  void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
> > diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> > index 2673c6ba7a4e..844fae923340 100644
> > --- a/fs/btrfs/file-item.c
> > +++ b/fs/btrfs/file-item.c
> > @@ -614,28 +614,28 @@ int btrfs_lookup_csums_range(struct btrfs_root *root, u64 start, u64 end,
> >   * btrfs_csum_one_bio - Calculates checksums of the data contained inside a bio
> >   * @inode:	 Owner of the data inside the bio
> >   * @bio:	 Contains the data to be checksummed
> > - * @file_start:  offset in file this bio begins to describe
> > - * @contig:	 Boolean. If true/1 means all bio vecs in this bio are
> > - *		 contiguous and they begin at @file_start in the file. False/0
> > - *		 means this bio can contain potentially discontiguous bio vecs
> > - *		 so the logical offset of each should be calculated separately.
> > + * @offset:      If (u64)-1, @bio may contain discontiguous bio vecs, so the
> > + *               file offsets are determined from the page offsets in the bio.
> > + *               Otherwise, this is the starting file offset of the bio vecs in
> > + *               @bio, which must be contiguous.
> > + * @one_ordered: If true, @bio only refers to one ordered extent.
> 
> nit: I don't have strong preference but my gut feeling tells me
> "single_ordered" might be more explicit/informative. But unless someone
> else thinks the same one_ordered would also make do
> 
> >   */
> >  blk_status_t btrfs_csum_one_bio(struct btrfs_inode *inode, struct bio *bio,
> > -		       u64 file_start, int contig)
> > +				u64 offset, bool one_ordered)
> >  {
> >  	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> >  	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
> >  	struct btrfs_ordered_sum *sums;
> >  	struct btrfs_ordered_extent *ordered = NULL;
> > +	const bool page_offsets = (offset == (u64)-1);
> 
> nit: Again, instead of page_offsets perhaps use_page_offsets, somewhat
> more explicit/informative.

Sure, that sounds better.

> >  	char *data;
> >  	struct bvec_iter iter;
> >  	struct bio_vec bvec;
> >  	int index;
> > -	int nr_sectors;
> > +	int blockcount;
> >  	unsigned long total_bytes = 0;
> >  	unsigned long this_sum_bytes = 0;
> >  	int i;
> > -	u64 offset;
> >  	unsigned nofs_flag;
> >  
> >  	nofs_flag = memalloc_nofs_save();
> > @@ -649,18 +649,13 @@ blk_status_t btrfs_csum_one_bio(struct btrfs_inode *inode, struct bio *bio,
> >  	sums->len = bio->bi_iter.bi_size;
> >  	INIT_LIST_HEAD(&sums->list);
> >  
> > -	if (contig)
> > -		offset = file_start;
> > -	else
> > -		offset = 0; /* shut up gcc */
> > -
> >  	sums->bytenr = bio->bi_iter.bi_sector << 9;
> >  	index = 0;
> >  
> >  	shash->tfm = fs_info->csum_shash;
> >  
> >  	bio_for_each_segment(bvec, bio, iter) {
> > -		if (!contig)
> > +		if (page_offsets)
> >  			offset = page_offset(bvec.bv_page) + bvec.bv_offset;
> >  
> >  		if (!ordered) {
> > @@ -668,13 +663,14 @@ blk_status_t btrfs_csum_one_bio(struct btrfs_inode *inode, struct bio *bio,
> >  			BUG_ON(!ordered); /* Logic error */
> >  		}
> >  
> > -		nr_sectors = BTRFS_BYTES_TO_BLKS(fs_info,
> > +		blockcount = BTRFS_BYTES_TO_BLKS(fs_info,
> >  						 bvec.bv_len + fs_info->sectorsize
> >  						 - 1);
> >  
> > -		for (i = 0; i < nr_sectors; i++) {
> > -			if (offset >= ordered->file_offset + ordered->num_bytes ||
> > -			    offset < ordered->file_offset) {
> > +		for (i = 0; i < blockcount; i++) {
> > +			if (!one_ordered &&
> > +			    (offset >= ordered->file_offset + ordered->num_bytes ||
> > +			     offset < ordered->file_offset)) {
> 
> Since you are changing this hunk, how about using the in_range macro:
> 
> if (!one_ordered && !in_range(offset, ordered->file_offset,
> ordered->num_bytes) { foo

Will do (although I don't like that that macro evaluates b and first
twice. Someone changing the code later might not notice that.)

> Though I think the "change the ordered extent now that we are working on
> a different range" code should be factored out in a separate function
> because currently it's somewhat breaking the flow of reading.
> 
> >  				unsigned long bytes_left;
> >  
> >  				sums->len = this_sum_bytes;
> > @@ -705,7 +701,8 @@ blk_status_t btrfs_csum_one_bio(struct btrfs_inode *inode, struct bio *bio,
> >  					    sums->sums + index);
> >  			kunmap_atomic(data);
> >  			index += fs_info->csum_size;
> > -			offset += fs_info->sectorsize;
> > +			if (!one_ordered)
> > +				offset += fs_info->sectorsize;
> 
> Instead of adding one additional conditional op can't offset always be
> incremented but in the case of one_ordered then the !in_range check
> should always be false i.e we won't be using the offset to lookup a new OE?

I did it conditionally so that it's clearer that the offset isn't needed
for the one_ordered case, but I can drop the check.

> >  			this_sum_bytes += fs_info->sectorsize;
> >  			total_bytes += fs_info->sectorsize;
> >  		}
> 
> 
> <snip>