Re: [PATCH v2 02/12] Btrfs: load checksum data once when submitting a direct read io
On Fri, 8 Aug 2014 01:32:00 +0100, Filipe David Manana wrote:
> On Tue, Jul 29, 2014 at 10:24 AM, Miao Xie wrote:
>> The current code would load checksum data for several times when we split
>> a whole direct read io because of the limit of the raid stripe, it would
>> make us search the csum tree for several times. In fact, it just wasted time,
>> and made the contention of the csum tree root be more serious. This patch
>> improves this problem by loading the data at once.
>>
>> Signed-off-by: Miao Xie
>> ---
>> Changelog v1 -> v2:
>> - Remove the __GFP_ZERO flag in btrfs_submit_direct because it would trigger
>> a WARNing. It is reported by Filipe David Manana, Thanks.
>
> Hi Miao,
>
> Without this flag (to zero the struct), I get a general protection fault at:
>
> (gdb) list *(btrfs_endio_direct_read+0xb8)
> 0x38378 is in btrfs_endio_direct_read (fs/btrfs/inode.c:7242).
> 7237 if (err)
> 7238 clear_bit(BIO_UPTODATE, &dio_bio->bi_flags);
> 7239 dio_end_io(dio_bio, err);
> 7240
> 7241 if (io_bio->end_io)
> 7242 io_bio->end_io(io_bio, err);
> 7243 bio_put(bio);
> 7244 }
>
> Happens when running xfstests/generic/091
Sorry, I sent out wrong version of this patchset, which has a bug
in btrfs_bio_clone.
I will send new version of this patch. The other patches are not changed,
and can be applied successfully after the new version of this patch, so
don't send them again. Surely, we also can pull the whole patchset from
https://github.com/miaoxie/linux-btrfs.git for-Chris
Thanks
Miao
>
>
>> ---
>> fs/btrfs/btrfs_inode.h | 1 -
>> fs/btrfs/ctree.h | 3 +--
>> fs/btrfs/extent_io.c | 13 +++--
>> fs/btrfs/file-item.c | 14 ++
>> fs/btrfs/inode.c | 38 +-
>> 5 files changed, 35 insertions(+), 34 deletions(-)
>>
>> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
>> index a0cf3e5..b69bf7e 100644
>> --- a/fs/btrfs/btrfs_inode.h
>> +++ b/fs/btrfs/btrfs_inode.h
>> @@ -263,7 +263,6 @@ struct btrfs_dio_private {
>>
>> /* dio_bio came from fs/direct-io.c */
>> struct bio *dio_bio;
>> - u8 csum[0];
>> };
>>
>> /*
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index be91397..40e9938 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -3739,8 +3739,7 @@ int btrfs_del_csums(struct btrfs_trans_handle *trans,
>> int btrfs_lookup_bio_sums(struct btrfs_root *root, struct inode *inode,
>> struct bio *bio, u32 *dst);
>> int btrfs_lookup_bio_sums_dio(struct btrfs_root *root, struct inode *inode,
>> - struct btrfs_dio_private *dip, struct bio *bio,
>> - u64 logical_offset);
>> + struct bio *bio, u64 logical_offset);
>> int btrfs_insert_file_extent(struct btrfs_trans_handle *trans,
>> struct btrfs_root *root,
>> u64 objectid, u64 pos,
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 23398ad..0fb63c4 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -2617,9 +2617,18 @@ btrfs_bio_alloc(struct block_device *bdev, u64
>> first_sector, int nr_vecs,
>>
>> struct bio *btrfs_bio_clone(struct bio *bio, gfp_t gfp_mask)
>> {
>> - return bio_clone_bioset(bio, gfp_mask, btrfs_bioset);
>> -}
>> + struct btrfs_io_bio *btrfs_bio;
>> + struct bio *new;
>>
>> + new = bio_clone_bioset(bio, gfp_mask, btrfs_bioset);
>> + if (new) {
>> + btrfs_bio = btrfs_io_bio(new);
>> + btrfs_bio->csum = NULL;
>> + btrfs_bio->csum_allocated = NULL;
>> + btrfs_bio->end_io = NULL;
>> + }
>> + return bio;
>> +}
>>
>> /* this also allocates from the btrfs_bioset */
>> struct bio *btrfs_io_bio_alloc(gfp_t gfp_mask, unsigned int nr_iovecs)
>> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
>> index f46cfe4..cf1b94f 100644
>> --- a/fs/btrfs/file-item.c
>> +++ b/fs/btrfs/file-item.c
>> @@ -299,19 +299,9 @@ int btrfs_lookup_bio_sums(struct btrfs_root *root,
>> struct inode *inode,
>> }
>>
>> int btrfs_lookup_bio_sums_dio(struct btrfs_root *root, struct inode *inode,
>> - struct btrfs_dio_private *dip, struct bio *bio,
>> - u64 offset)
>> + struct bio *bio, u64 offset)
>> {
>> - int len = (bio->bi_iter.bi_sector << 9) - dip->disk_bytenr;
>> - u16 csum_size = btrfs_super_csum_size(root->fs_info->super_copy);
>> - int ret;
>> -
>> - len >>= inode->i_sb->s_blocksize_bits;
>> - len *= csum_size;
>> -
>> - ret = __btrfs_lookup_bio_sums(root, inode, bio, offset,
>> - (u32 *)(dip->csum + len), 1);
>> - return ret;
>> + return __btrfs_lookup_bio_sums(root, inode, bio, offset, NULL, 1);
>> }
>>
>> int btrfs_lookup_csums_range(struct btrfs_root *root, u64 s
Re: [PATCH v2 02/12] Btrfs: load checksum data once when submitting a direct read io
On Tue, Jul 29, 2014 at 10:24 AM, Miao Xie wrote:
> The current code would load checksum data for several times when we split
> a whole direct read io because of the limit of the raid stripe, it would
> make us search the csum tree for several times. In fact, it just wasted time,
> and made the contention of the csum tree root be more serious. This patch
> improves this problem by loading the data at once.
>
> Signed-off-by: Miao Xie
> ---
> Changelog v1 -> v2:
> - Remove the __GFP_ZERO flag in btrfs_submit_direct because it would trigger
> a WARNing. It is reported by Filipe David Manana, Thanks.
Hi Miao,
Without this flag (to zero the struct), I get a general protection fault at:
(gdb) list *(btrfs_endio_direct_read+0xb8)
0x38378 is in btrfs_endio_direct_read (fs/btrfs/inode.c:7242).
7237 if (err)
7238 clear_bit(BIO_UPTODATE, &dio_bio->bi_flags);
7239 dio_end_io(dio_bio, err);
7240
7241 if (io_bio->end_io)
7242 io_bio->end_io(io_bio, err);
7243 bio_put(bio);
7244 }
Happens when running xfstests/generic/091
> ---
> fs/btrfs/btrfs_inode.h | 1 -
> fs/btrfs/ctree.h | 3 +--
> fs/btrfs/extent_io.c | 13 +++--
> fs/btrfs/file-item.c | 14 ++
> fs/btrfs/inode.c | 38 +-
> 5 files changed, 35 insertions(+), 34 deletions(-)
>
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index a0cf3e5..b69bf7e 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -263,7 +263,6 @@ struct btrfs_dio_private {
>
> /* dio_bio came from fs/direct-io.c */
> struct bio *dio_bio;
> - u8 csum[0];
> };
>
> /*
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index be91397..40e9938 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3739,8 +3739,7 @@ int btrfs_del_csums(struct btrfs_trans_handle *trans,
> int btrfs_lookup_bio_sums(struct btrfs_root *root, struct inode *inode,
> struct bio *bio, u32 *dst);
> int btrfs_lookup_bio_sums_dio(struct btrfs_root *root, struct inode *inode,
> - struct btrfs_dio_private *dip, struct bio *bio,
> - u64 logical_offset);
> + struct bio *bio, u64 logical_offset);
> int btrfs_insert_file_extent(struct btrfs_trans_handle *trans,
> struct btrfs_root *root,
> u64 objectid, u64 pos,
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 23398ad..0fb63c4 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2617,9 +2617,18 @@ btrfs_bio_alloc(struct block_device *bdev, u64
> first_sector, int nr_vecs,
>
> struct bio *btrfs_bio_clone(struct bio *bio, gfp_t gfp_mask)
> {
> - return bio_clone_bioset(bio, gfp_mask, btrfs_bioset);
> -}
> + struct btrfs_io_bio *btrfs_bio;
> + struct bio *new;
>
> + new = bio_clone_bioset(bio, gfp_mask, btrfs_bioset);
> + if (new) {
> + btrfs_bio = btrfs_io_bio(new);
> + btrfs_bio->csum = NULL;
> + btrfs_bio->csum_allocated = NULL;
> + btrfs_bio->end_io = NULL;
> + }
> + return bio;
> +}
>
> /* this also allocates from the btrfs_bioset */
> struct bio *btrfs_io_bio_alloc(gfp_t gfp_mask, unsigned int nr_iovecs)
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index f46cfe4..cf1b94f 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -299,19 +299,9 @@ int btrfs_lookup_bio_sums(struct btrfs_root *root,
> struct inode *inode,
> }
>
> int btrfs_lookup_bio_sums_dio(struct btrfs_root *root, struct inode *inode,
> - struct btrfs_dio_private *dip, struct bio *bio,
> - u64 offset)
> + struct bio *bio, u64 offset)
> {
> - int len = (bio->bi_iter.bi_sector << 9) - dip->disk_bytenr;
> - u16 csum_size = btrfs_super_csum_size(root->fs_info->super_copy);
> - int ret;
> -
> - len >>= inode->i_sb->s_blocksize_bits;
> - len *= csum_size;
> -
> - ret = __btrfs_lookup_bio_sums(root, inode, bio, offset,
> - (u32 *)(dip->csum + len), 1);
> - return ret;
> + return __btrfs_lookup_bio_sums(root, inode, bio, offset, NULL, 1);
> }
>
> 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 548489e..fd88126 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7081,7 +7081,8 @@ static void btrfs_endio_direct_read(struct bio *bio,
> int err)
> struct inode *inode = dip->inode;
> struct btrfs_root *root = BTRFS_I(inode)->root;
> struct bio *dio_bio;
> - u32 *csums = (u32 *)dip->csum;
> + struct btrfs_io_bio *io_bio = btrfs_io_bio(bio);
> + u32 *csums = (u32 *)io_bio->csum;
> u64 start;
> int i;
>
> @@ -7123,6 +7124,9
[PATCH v2 02/12] Btrfs: load checksum data once when submitting a direct read io
The current code would load checksum data for several times when we split
a whole direct read io because of the limit of the raid stripe, it would
make us search the csum tree for several times. In fact, it just wasted time,
and made the contention of the csum tree root be more serious. This patch
improves this problem by loading the data at once.
Signed-off-by: Miao Xie
---
Changelog v1 -> v2:
- Remove the __GFP_ZERO flag in btrfs_submit_direct because it would trigger
a WARNing. It is reported by Filipe David Manana, Thanks.
---
fs/btrfs/btrfs_inode.h | 1 -
fs/btrfs/ctree.h | 3 +--
fs/btrfs/extent_io.c | 13 +++--
fs/btrfs/file-item.c | 14 ++
fs/btrfs/inode.c | 38 +-
5 files changed, 35 insertions(+), 34 deletions(-)
diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index a0cf3e5..b69bf7e 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -263,7 +263,6 @@ struct btrfs_dio_private {
/* dio_bio came from fs/direct-io.c */
struct bio *dio_bio;
- u8 csum[0];
};
/*
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index be91397..40e9938 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3739,8 +3739,7 @@ int btrfs_del_csums(struct btrfs_trans_handle *trans,
int btrfs_lookup_bio_sums(struct btrfs_root *root, struct inode *inode,
struct bio *bio, u32 *dst);
int btrfs_lookup_bio_sums_dio(struct btrfs_root *root, struct inode *inode,
- struct btrfs_dio_private *dip, struct bio *bio,
- u64 logical_offset);
+ struct bio *bio, u64 logical_offset);
int btrfs_insert_file_extent(struct btrfs_trans_handle *trans,
struct btrfs_root *root,
u64 objectid, u64 pos,
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 23398ad..0fb63c4 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2617,9 +2617,18 @@ btrfs_bio_alloc(struct block_device *bdev, u64
first_sector, int nr_vecs,
struct bio *btrfs_bio_clone(struct bio *bio, gfp_t gfp_mask)
{
- return bio_clone_bioset(bio, gfp_mask, btrfs_bioset);
-}
+ struct btrfs_io_bio *btrfs_bio;
+ struct bio *new;
+ new = bio_clone_bioset(bio, gfp_mask, btrfs_bioset);
+ if (new) {
+ btrfs_bio = btrfs_io_bio(new);
+ btrfs_bio->csum = NULL;
+ btrfs_bio->csum_allocated = NULL;
+ btrfs_bio->end_io = NULL;
+ }
+ return bio;
+}
/* this also allocates from the btrfs_bioset */
struct bio *btrfs_io_bio_alloc(gfp_t gfp_mask, unsigned int nr_iovecs)
diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index f46cfe4..cf1b94f 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -299,19 +299,9 @@ int btrfs_lookup_bio_sums(struct btrfs_root *root, struct
inode *inode,
}
int btrfs_lookup_bio_sums_dio(struct btrfs_root *root, struct inode *inode,
- struct btrfs_dio_private *dip, struct bio *bio,
- u64 offset)
+ struct bio *bio, u64 offset)
{
- int len = (bio->bi_iter.bi_sector << 9) - dip->disk_bytenr;
- u16 csum_size = btrfs_super_csum_size(root->fs_info->super_copy);
- int ret;
-
- len >>= inode->i_sb->s_blocksize_bits;
- len *= csum_size;
-
- ret = __btrfs_lookup_bio_sums(root, inode, bio, offset,
- (u32 *)(dip->csum + len), 1);
- return ret;
+ return __btrfs_lookup_bio_sums(root, inode, bio, offset, NULL, 1);
}
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 548489e..fd88126 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7081,7 +7081,8 @@ static void btrfs_endio_direct_read(struct bio *bio, int
err)
struct inode *inode = dip->inode;
struct btrfs_root *root = BTRFS_I(inode)->root;
struct bio *dio_bio;
- u32 *csums = (u32 *)dip->csum;
+ struct btrfs_io_bio *io_bio = btrfs_io_bio(bio);
+ u32 *csums = (u32 *)io_bio->csum;
u64 start;
int i;
@@ -7123,6 +7124,9 @@ static void btrfs_endio_direct_read(struct bio *bio, int
err)
if (err)
clear_bit(BIO_UPTODATE, &dio_bio->bi_flags);
dio_end_io(dio_bio, err);
+
+ if (io_bio->end_io)
+ io_bio->end_io(io_bio, err);
bio_put(bio);
}
@@ -7261,13 +7265,20 @@ static inline int __btrfs_submit_dio_bio(struct bio
*bio, struct inode *inode,
ret = btrfs_csum_one_bio(root, inode, bio, file_offset, 1);
if (ret)
goto err;
- } else if (!skip_sum) {
- ret = btrfs_lookup_bio_sums_dio(root, inode, dip, bio,
+ } else {
+ /*
+* We have
