Re: [PATCH V2 2/2] Btrfs: remove btrfs_sector_sum structure
On Fri, 26 Apr 2013 16:58:18 +0800, Miao Xie wrote:
> On tue, 23 Apr 2013 16:54:35 -0400, Josef Bacik wrote:
>> On Wed, Apr 03, 2013 at 03:14:56AM -0600, Miao Xie wrote:
>>> Using the structure btrfs_sector_sum to keep the checksum value is
>>> unnecessary, because the extents that btrfs_sector_sum points to are
>>> continuous, we can find out the expected checksums by btrfs_ordered_sum's
>>> bytenr and the offset, so we can remove btrfs_sector_sum's bytenr. After
>>> removing bytenr, there is only one member in the structure, so it makes
>>> no sense to keep the structure, just remove it, and use a u32 array to
>>> store the checksum value.
>>>
>>> By this change, we don't use the while loop to get the checksums one by
>>> one. Now, we can get several checksum value at one time, it improved the
>>> performance by ~74% on my SSD (31MB/s -> 54MB/s).
>>>
>>> test command:
>>> # dd if=/dev/zero of=/mnt/btrfs/file0 bs=1M count=1024 oflag=sync
>>>
>>> Signed-off-by: Miao Xie
>>> Reviewed-by: Liu Bo
> [SNIP]
>>> next_offset = (u64)-1;
>>> found_next = 0;
>>> + bytenr = sums->bytenr + total_bytes;
>>> file_key.objectid = BTRFS_EXTENT_CSUM_OBJECTID;
>>> - file_key.offset = sector_sum->bytenr;
>>> - bytenr = sector_sum->bytenr;
>>> + file_key.offset = bytenr;
>>> btrfs_set_key_type(&file_key, BTRFS_EXTENT_CSUM_KEY);
>>>
>>> - item = btrfs_lookup_csum(trans, root, path, sector_sum->bytenr, 1);
>>> + item = btrfs_lookup_csum(trans, root, path, bytenr, 1);
>>> if (!IS_ERR(item)) {
>>> - leaf = path->nodes[0];
>>> - ret = 0;
>>> - goto found;
>>> + csum_offset = 0;
>>> + goto csum;
>>
>> Ok I've just spent the last 3 hours tracking down an fsync() problem that
>> turned
>> out to be because of this patch. btrfs_lookup_csum() assumes you are just
>> going
>> in 4k chunks, but we could be going in larger chunks. So as long as the
>> bytenr
>> falls inside of this csum item it thinks its good. So what I'm seeing is
>> this,
>> we have item
>>
>> [0-8k]
>>
>> and we are csumming
>>
>> [4k-12k]
>>
>> and then we're adding our new csum into the old one, the sizes match but the
>> bytenrs don't match. If you want a reproducer just run my fsync xfstest
>> that I
>> just posted. I'm dropping this patch for now and I'll wait for you to fix
>> it.
>> Thanks,
>
> Is the reproducer is the 311th case of xfstests?
> ([PATCH] xfstests 311: test fsync with dm flakey V2)
>
> If yes, I'm so sorry that we didn't reproduce the problem you said above.
> Could you
> give me your test option?
please ignore the attached patch, I sent it out by mistake.
Thanks
Miao
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 2/2] Btrfs: remove btrfs_sector_sum structure
On tue, 23 Apr 2013 16:54:35 -0400, Josef Bacik wrote:
> On Wed, Apr 03, 2013 at 03:14:56AM -0600, Miao Xie wrote:
>> Using the structure btrfs_sector_sum to keep the checksum value is
>> unnecessary, because the extents that btrfs_sector_sum points to are
>> continuous, we can find out the expected checksums by btrfs_ordered_sum's
>> bytenr and the offset, so we can remove btrfs_sector_sum's bytenr. After
>> removing bytenr, there is only one member in the structure, so it makes
>> no sense to keep the structure, just remove it, and use a u32 array to
>> store the checksum value.
>>
>> By this change, we don't use the while loop to get the checksums one by
>> one. Now, we can get several checksum value at one time, it improved the
>> performance by ~74% on my SSD (31MB/s -> 54MB/s).
>>
>> test command:
>> # dd if=/dev/zero of=/mnt/btrfs/file0 bs=1M count=1024 oflag=sync
>>
>> Signed-off-by: Miao Xie
>> Reviewed-by: Liu Bo
[SNIP]
>> next_offset = (u64)-1;
>> found_next = 0;
>> + bytenr = sums->bytenr + total_bytes;
>> file_key.objectid = BTRFS_EXTENT_CSUM_OBJECTID;
>> - file_key.offset = sector_sum->bytenr;
>> - bytenr = sector_sum->bytenr;
>> + file_key.offset = bytenr;
>> btrfs_set_key_type(&file_key, BTRFS_EXTENT_CSUM_KEY);
>>
>> - item = btrfs_lookup_csum(trans, root, path, sector_sum->bytenr, 1);
>> + item = btrfs_lookup_csum(trans, root, path, bytenr, 1);
>> if (!IS_ERR(item)) {
>> - leaf = path->nodes[0];
>> - ret = 0;
>> - goto found;
>> + csum_offset = 0;
>> + goto csum;
>
> Ok I've just spent the last 3 hours tracking down an fsync() problem that
> turned
> out to be because of this patch. btrfs_lookup_csum() assumes you are just
> going
> in 4k chunks, but we could be going in larger chunks. So as long as the
> bytenr
> falls inside of this csum item it thinks its good. So what I'm seeing is
> this,
> we have item
>
> [0-8k]
>
> and we are csumming
>
> [4k-12k]
>
> and then we're adding our new csum into the old one, the sizes match but the
> bytenrs don't match. If you want a reproducer just run my fsync xfstest that
> I
> just posted. I'm dropping this patch for now and I'll wait for you to fix it.
> Thanks,
Is the reproducer is the 311th case of xfstests?
([PATCH] xfstests 311: test fsync with dm flakey V2)
If yes, I'm so sorry that we didn't reproduce the problem you said above. Could
you
give me your test option?
Thanks
Miao
>From d8b9c06ecb4aa5cb2aca5be96a8b65af1afb1992 Mon Sep 17 00:00:00 2001
From: Miao Xie
Date: Sat, 16 Mar 2013 01:06:03 +0800
Subject: [PATCH] Btrfs: remove btrfs_sector_sum structure
Using the structure btrfs_sector_sum to keep the checksum value is
unnecessary, because the extents that btrfs_sector_sum points to are
continuous, we can find out the expected checksums by btrfs_ordered_sum's
bytenr and the offset, so we can remove btrfs_sector_sum's bytenr. After
removing bytenr, there is only one member in the structure, so it makes
no sense to keep the structure, just remove it, and use a u32 array to
store the checksum value.
By this change, we don't use the while loop to get the checksums one by
one. Now, we can get several checksum value at one time, it improved the
performance by ~74% on my SSD (31MB/s -> 54MB/s).
test command:
# dd if=/dev/zero of=/mnt/btrfs/file0 bs=1M count=1024 oflag=sync
Signed-off-by: Miao Xie
---
fs/btrfs/file-item.c| 142 ++--
fs/btrfs/ordered-data.c | 19 +++
fs/btrfs/ordered-data.h | 25 ++---
fs/btrfs/relocation.c | 10
fs/btrfs/scrub.c| 16 ++
5 files changed, 70 insertions(+), 142 deletions(-)
diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 769eb86..f5f6629 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -34,8 +34,7 @@
#define MAX_ORDERED_SUM_BYTES(r) ((PAGE_SIZE - \
sizeof(struct btrfs_ordered_sum)) / \
- sizeof(struct btrfs_sector_sum) * \
- (r)->sectorsize - (r)->sectorsize)
+ sizeof(u32) * (r)->sectorsize)
int btrfs_insert_file_extent(struct btrfs_trans_handle *trans,
struct btrfs_root *root,
@@ -317,7 +316,6 @@ int btrfs_lookup_csums_range(struct btrfs_root *root, u64 start, u64 end,
struct btrfs_path *path;
struct extent_buffer *leaf;
struct btrfs_ordered_sum *sums;
- struct btrfs_sector_sum *sector_sum;
struct btrfs_csum_item *item;
LIST_HEAD(tmplist);
unsigned long offset;
@@ -388,34 +386,28 @@ int btrfs_lookup_csums_range(struct btrfs_root *root, u64 start, u64 end,
struct btrfs_csum_item);
while (start < csum_end) {
size = min_t(size_t, csum_end - start,
- MAX_ORDERED_SUM_BYTES(root));
+ MAX_ORDERED_SUM_BYTES(root));
sums = kzalloc(btrfs_ordered_sum_size(root, size),
- GFP_NOFS);
+ GFP_NOFS);
if (!sums) {
ret = -ENOMEM;
goto fail;
Re: [PATCH V2 2/2] Btrfs: remove btrfs_sector_sum structure
On Wed, Apr 03, 2013 at 03:14:56AM -0600, Miao Xie wrote:
> Using the structure btrfs_sector_sum to keep the checksum value is
> unnecessary, because the extents that btrfs_sector_sum points to are
> continuous, we can find out the expected checksums by btrfs_ordered_sum's
> bytenr and the offset, so we can remove btrfs_sector_sum's bytenr. After
> removing bytenr, there is only one member in the structure, so it makes
> no sense to keep the structure, just remove it, and use a u32 array to
> store the checksum value.
>
> By this change, we don't use the while loop to get the checksums one by
> one. Now, we can get several checksum value at one time, it improved the
> performance by ~74% on my SSD (31MB/s -> 54MB/s).
>
> test command:
> # dd if=/dev/zero of=/mnt/btrfs/file0 bs=1M count=1024 oflag=sync
>
> Signed-off-by: Miao Xie
> Reviewed-by: Liu Bo
> ---
> Changelog v1 -> v2:
> - modify the changelog and the title which can not explain this patch clearly
> - fix the 64bit division problem on 32bit machine
> ---
> fs/btrfs/file-item.c| 144
> ++--
> fs/btrfs/ordered-data.c | 19 +++
> fs/btrfs/ordered-data.h | 25 ++---
> fs/btrfs/relocation.c | 10
> fs/btrfs/scrub.c| 16 ++
> 5 files changed, 71 insertions(+), 143 deletions(-)
>
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index 484017a..8d653c2 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -34,8 +34,7 @@
>
> #define MAX_ORDERED_SUM_BYTES(r) ((PAGE_SIZE - \
>sizeof(struct btrfs_ordered_sum)) / \
> - sizeof(struct btrfs_sector_sum) * \
> - (r)->sectorsize - (r)->sectorsize)
> + sizeof(u32) * (r)->sectorsize)
>
> int btrfs_insert_file_extent(struct btrfs_trans_handle *trans,
> struct btrfs_root *root,
> @@ -313,7 +312,6 @@ int btrfs_lookup_csums_range(struct btrfs_root *root, u64
> start, u64 end,
> struct btrfs_path *path;
> struct extent_buffer *leaf;
> struct btrfs_ordered_sum *sums;
> - struct btrfs_sector_sum *sector_sum;
> struct btrfs_csum_item *item;
> LIST_HEAD(tmplist);
> unsigned long offset;
> @@ -387,34 +385,28 @@ int btrfs_lookup_csums_range(struct btrfs_root *root,
> u64 start, u64 end,
> struct btrfs_csum_item);
> while (start < csum_end) {
> size = min_t(size_t, csum_end - start,
> - MAX_ORDERED_SUM_BYTES(root));
> +MAX_ORDERED_SUM_BYTES(root));
> sums = kzalloc(btrfs_ordered_sum_size(root, size),
> - GFP_NOFS);
> + GFP_NOFS);
> if (!sums) {
> ret = -ENOMEM;
> goto fail;
> }
>
> - sector_sum = sums->sums;
> sums->bytenr = start;
> - sums->len = size;
> + sums->len = (int)size;
>
> offset = (start - key.offset) >>
> root->fs_info->sb->s_blocksize_bits;
> offset *= csum_size;
> + size >>= root->fs_info->sb->s_blocksize_bits;
>
> - while (size > 0) {
> - read_extent_buffer(path->nodes[0],
> - §or_sum->sum,
> - ((unsigned long)item) +
> - offset, csum_size);
> - sector_sum->bytenr = start;
> -
> - size -= root->sectorsize;
> - start += root->sectorsize;
> - offset += csum_size;
> - sector_sum++;
> - }
> + read_extent_buffer(path->nodes[0],
> + sums->sums,
> + ((unsigned long)item) + offset,
> + csum_size * size);
> +
> + start += root->sectorsize * size;
> list_add_tail(&sums->list, &tmplist);
> }
> path->slots[0]++;
> @@ -436,23 +428,20 @@ int btrfs_csum_one_bio(struct btrfs_root *root, struct
> inode *inode,
>struct bio *bio, u64 file_start, int contig)
> {
> struct btrfs_ordered_sum *sums;
> - struct btrfs_sector_sum *sector_sum;
> struct btrfs_ordered_extent *ordered;
> char *data;
> struct bio_vec *bvec = bio
[PATCH V2 2/2] Btrfs: remove btrfs_sector_sum structure
Using the structure btrfs_sector_sum to keep the checksum value is
unnecessary, because the extents that btrfs_sector_sum points to are
continuous, we can find out the expected checksums by btrfs_ordered_sum's
bytenr and the offset, so we can remove btrfs_sector_sum's bytenr. After
removing bytenr, there is only one member in the structure, so it makes
no sense to keep the structure, just remove it, and use a u32 array to
store the checksum value.
By this change, we don't use the while loop to get the checksums one by
one. Now, we can get several checksum value at one time, it improved the
performance by ~74% on my SSD (31MB/s -> 54MB/s).
test command:
# dd if=/dev/zero of=/mnt/btrfs/file0 bs=1M count=1024 oflag=sync
Signed-off-by: Miao Xie
Reviewed-by: Liu Bo
---
Changelog v1 -> v2:
- modify the changelog and the title which can not explain this patch clearly
- fix the 64bit division problem on 32bit machine
---
fs/btrfs/file-item.c| 144 ++--
fs/btrfs/ordered-data.c | 19 +++
fs/btrfs/ordered-data.h | 25 ++---
fs/btrfs/relocation.c | 10
fs/btrfs/scrub.c| 16 ++
5 files changed, 71 insertions(+), 143 deletions(-)
diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 484017a..8d653c2 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -34,8 +34,7 @@
#define MAX_ORDERED_SUM_BYTES(r) ((PAGE_SIZE - \
sizeof(struct btrfs_ordered_sum)) / \
- sizeof(struct btrfs_sector_sum) * \
- (r)->sectorsize - (r)->sectorsize)
+ sizeof(u32) * (r)->sectorsize)
int btrfs_insert_file_extent(struct btrfs_trans_handle *trans,
struct btrfs_root *root,
@@ -313,7 +312,6 @@ int btrfs_lookup_csums_range(struct btrfs_root *root, u64
start, u64 end,
struct btrfs_path *path;
struct extent_buffer *leaf;
struct btrfs_ordered_sum *sums;
- struct btrfs_sector_sum *sector_sum;
struct btrfs_csum_item *item;
LIST_HEAD(tmplist);
unsigned long offset;
@@ -387,34 +385,28 @@ int btrfs_lookup_csums_range(struct btrfs_root *root, u64
start, u64 end,
struct btrfs_csum_item);
while (start < csum_end) {
size = min_t(size_t, csum_end - start,
- MAX_ORDERED_SUM_BYTES(root));
+MAX_ORDERED_SUM_BYTES(root));
sums = kzalloc(btrfs_ordered_sum_size(root, size),
- GFP_NOFS);
+ GFP_NOFS);
if (!sums) {
ret = -ENOMEM;
goto fail;
}
- sector_sum = sums->sums;
sums->bytenr = start;
- sums->len = size;
+ sums->len = (int)size;
offset = (start - key.offset) >>
root->fs_info->sb->s_blocksize_bits;
offset *= csum_size;
+ size >>= root->fs_info->sb->s_blocksize_bits;
- while (size > 0) {
- read_extent_buffer(path->nodes[0],
- §or_sum->sum,
- ((unsigned long)item) +
- offset, csum_size);
- sector_sum->bytenr = start;
-
- size -= root->sectorsize;
- start += root->sectorsize;
- offset += csum_size;
- sector_sum++;
- }
+ read_extent_buffer(path->nodes[0],
+ sums->sums,
+ ((unsigned long)item) + offset,
+ csum_size * size);
+
+ start += root->sectorsize * size;
list_add_tail(&sums->list, &tmplist);
}
path->slots[0]++;
@@ -436,23 +428,20 @@ int btrfs_csum_one_bio(struct btrfs_root *root, struct
inode *inode,
struct bio *bio, u64 file_start, int contig)
{
struct btrfs_ordered_sum *sums;
- struct btrfs_sector_sum *sector_sum;
struct btrfs_ordered_extent *ordered;
char *data;
struct bio_vec *bvec = bio->bi_io_vec;
int bio_index = 0;
+ int index;
unsigned long total_bytes = 0;
unsigned long this_sum_bytes = 0;
u64 offset;
- u64 disk_bytenr;
WARN_ON(bio->bi_vcnt <= 0);
sums = kzalloc(btrfs_ordered_sum_size(r
