On Thursday 01 Oct 2015 10:39:29 Josef Bacik wrote:
> On 09/30/2015 06:28 AM, Chandan Rajendra wrote:
> > Checksums are applicable to sectorsize units. The current code uses
> > bio->bv_len units to compute and look up checksums. This works on machines
> > where sectorsize == PAGE_SIZE. This patch makes the checksum computation
> > and look up code to work with sectorsize units.
> >
> > Reviewed-by: Liu Bo <[email protected]>
> > Reviewed-by: Josef Bacik <[email protected]>
> > Signed-off-by: Chandan Rajendra <[email protected]>
> > ---
> >
> > fs/btrfs/file-item.c | 93
> > +++++++++++++++++++++++++++++++++------------------- 1 file changed, 59
> > insertions(+), 34 deletions(-)
> >
> > diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> > index 58ece65..818c859 100644
> > --- a/fs/btrfs/file-item.c
> > +++ b/fs/btrfs/file-item.c
> > @@ -172,6 +172,7 @@ static int __btrfs_lookup_bio_sums(struct btrfs_root
> > *root,>
> > u64 item_start_offset = 0;
> > u64 item_last_offset = 0;
> > u64 disk_bytenr;
> >
> > + u64 page_bytes_left;
> >
> > u32 diff;
> > int nblocks;
> > int bio_index = 0;
> >
> > @@ -220,6 +221,8 @@ static int __btrfs_lookup_bio_sums(struct btrfs_root
> > *root,>
> > disk_bytenr = (u64)bio->bi_iter.bi_sector << 9;
> > if (dio)
> >
> > offset = logical_offset;
> >
> > +
> > + page_bytes_left = bvec->bv_len;
> >
> > while (bio_index < bio->bi_vcnt) {
> >
> > if (!dio)
> >
> > offset = page_offset(bvec->bv_page) + bvec->bv_offset;
> >
> > @@ -243,7 +246,7 @@ static int __btrfs_lookup_bio_sums(struct btrfs_root
> > *root,>
> > if (BTRFS_I(inode)->root->root_key.objectid ==
> >
> > BTRFS_DATA_RELOC_TREE_OBJECTID) {
> >
> > set_extent_bits(io_tree, offset,
> >
> > - offset + bvec->bv_len - 1,
> > + offset + root->sectorsize - 1,
> >
> > EXTENT_NODATASUM, GFP_NOFS);
> >
> > } else {
> >
> > btrfs_info(BTRFS_I(inode)->root-
>fs_info,
> >
> > @@ -281,11 +284,17 @@ static int __btrfs_lookup_bio_sums(struct btrfs_root
> > *root,>
> > found:
> > csum += count * csum_size;
> > nblocks -= count;
> >
> > - bio_index += count;
> > +
> >
> > while (count--) {
> >
> > - disk_bytenr += bvec->bv_len;
> > - offset += bvec->bv_len;
> > - bvec++;
> > + disk_bytenr += root->sectorsize;
> > + offset += root->sectorsize;
> > + page_bytes_left -= root->sectorsize;
> > + if (!page_bytes_left) {
> > + bio_index++;
> > + bvec++;
> > + page_bytes_left = bvec->bv_len;
> > + }
> > +
> >
> I don't understand why this needs to be changed, bv_len is still the
> amount we're copying, irrespective of the page size.
Josef, assume bvec[0] has 2 blocks worth of data and bvec[1] has 4 blocks of
worth of data. For the first iteration of the loop, assume that
btrfs_find_ordered_sum() returned 4 csums i.e. csums associated with first 4
blocks of the bio. In such a scenario, the first of the several csums returned
during the second iteration of the loop applies to the the 3rd block mapped by
bvec[1]. Knowing this wouldn't be possible by only using bvec->bv_len. Hence
page_bytes_left helps us figure out the block inside a bvec for which the
first of the new set of csums found applies and also to decide whether to move
to the next bvec or not.
> >
> > }
> >
> > }
> > btrfs_free_path(path);
> >
> > @@ -432,6 +441,8 @@ int btrfs_csum_one_bio(struct btrfs_root *root, struct
> > inode *inode,>
> > struct bio_vec *bvec = bio->bi_io_vec;
> > int bio_index = 0;
> > int index;
> >
> > + int nr_sectors;
> > + int i;
> >
> > unsigned long total_bytes = 0;
> > unsigned long this_sum_bytes = 0;
> > u64 offset;
> >
> > @@ -451,7 +462,7 @@ int btrfs_csum_one_bio(struct btrfs_root *root, struct
> > inode *inode,>
> > offset = page_offset(bvec->bv_page) + bvec->bv_offset;
> >
> > ordered = btrfs_lookup_ordered_extent(inode, offset);
> >
> > - BUG_ON(!ordered); /* Logic error */
> > + ASSERT(ordered); /* Logic error */
> >
>
> Don't worry about converting existing BUG_ON()'s, just don't add new ones.
Ok.
> > sums->bytenr = (u64)bio->bi_iter.bi_sector << 9;
> > index = 0;
> >
> > @@ -459,41 +470,55 @@ int btrfs_csum_one_bio(struct btrfs_root *root,
> > struct inode *inode,>
> > if (!contig)
> >
> > offset = page_offset(bvec->bv_page) + bvec->bv_offset;
> >
> > - if (offset >= ordered->file_offset + ordered->len ||
> > - offset < ordered->file_offset) {
> > - unsigned long bytes_left;
> > - sums->len = this_sum_bytes;
> > - this_sum_bytes = 0;
> > - btrfs_add_ordered_sum(inode, ordered, sums);
> > - btrfs_put_ordered_extent(ordered);
> > + data = kmap_atomic(bvec->bv_page);
> >
> > - bytes_left = bio->bi_iter.bi_size - total_bytes;
> > + nr_sectors = (bvec->bv_len + root->sectorsize - 1)
> > + >> inode->i_blkbits;
> > +
> So I've seen similar sort of math in the previous patch for this as
> well, lets make this into a helper.
I agree. I will add a helper function to do that and invoke it in appropriate
place.
> > + for (i = 0; i < nr_sectors; i++) {
> > + if (offset >= ordered->file_offset + ordered->len ||
> > + offset < ordered->file_offset) {
> > + unsigned long bytes_left;
> > +
> > + kunmap_atomic(data);
> > + sums->len = this_sum_bytes;
> > + this_sum_bytes = 0;
> > + btrfs_add_ordered_sum(inode, ordered, sums);
> > + btrfs_put_ordered_extent(ordered);
> > +
> > + bytes_left = bio->bi_iter.bi_size -
total_bytes;
> > +
> > + sums = kzalloc(btrfs_ordered_sum_size(root,
bytes_left),
> > + GFP_NOFS);
> > + BUG_ON(!sums); /* -ENOMEM */
> > + sums->len = bytes_left;
> > + ordered = btrfs_lookup_ordered_extent(inode,
> > + offset);
> > + ASSERT(ordered); /* Logic error */
> > + sums->bytenr = ((u64)bio->bi_iter.bi_sector <<
9)
> > + + total_bytes;
> > + index = 0;
> > +
> > + data = kmap_atomic(bvec->bv_page);
> > + }
> >
> > - sums = kzalloc(btrfs_ordered_sum_size(root,
bytes_left),
> > - GFP_NOFS);
> > - BUG_ON(!sums); /* -ENOMEM */
> > - sums->len = bytes_left;
> > - ordered = btrfs_lookup_ordered_extent(inode, offset);
> > - BUG_ON(!ordered); /* Logic error */
> > - sums->bytenr = ((u64)bio->bi_iter.bi_sector << 9) +
> > - total_bytes;
> > - index = 0;
> > + sums->sums[index] = ~(u32)0;
> > + sums->sums[index]
> > + = btrfs_csum_data(data + bvec->bv_offset
> > + + (i * root->sectorsize),
> > + sums->sums[index],
> > + root->sectorsize);
> > + btrfs_csum_final(sums->sums[index],
> > + (char *)(sums->sums + index));
> > + index++;
> > + offset += root->sectorsize;
> > + this_sum_bytes += root->sectorsize;
> > + total_bytes += root->sectorsize;
> >
> > }
>
> What I said about this area in the other email I sent just ignore, I
> misread the patch. The other stuff is still valid tho. Thanks,
>
> Josef
--
chandan
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
