* [PATCH] btrfs: rework the order of btrfs_ordered_extent::flags
@ 2021-01-21 6:13 Qu Wenruo
2021-01-21 7:38 ` Nikolay Borisov
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Qu Wenruo @ 2021-01-21 6:13 UTC (permalink / raw)
To: linux-btrfs
[BUG]
There is a long existing bug in the last parameter of
btrfs_add_ordered_extent(), in commit 771ed689d2cd ("Btrfs: Optimize
compressed writeback and reads") back to 2008.
In that ancient commit btrfs_add_ordered_extent() expects the @type
parameter to be one of the following:
- BTRFS_ORDERED_REGULAR
- BTRFS_ORDERED_NOCOW
- BTRFS_ORDERED_PREALLOC
- BTRFS_ORDERED_COMPRESSED
But we pass 0 in cow_file_range(), which means BTRFS_ORDERED_IO_DONE.
Ironically extra check in __btrfs_add_ordered_extent() won't set the bit
if we're seeing (type == IO_DONE || type == IO_COMPLETE), and avoid any
obvious bug.
But this still leads to regular COW ordered extent having no bit to
indicate its type in various trace events, rendering REGULAR bit
useless.
[FIX]
This patch will change the following aspects to avoid such problem:
- Reorder btrfs_ordered_extent::flags
Now the type bits go first (REGULAR/NOCOW/PREALLCO/COMPRESSED), then
DIRECT bit, finally extra status bits like IO_DONE/COMPLETE/IOERR.
- Add extra ASSERT() for btrfs_add_ordered_extent_*()
- Remove @type parameter for btrfs_add_ordered_extent_compress()
As the only valid @type here is BTRFS_ORDERED_COMPRESSED.
- Remove the unnecessary special check for IO_DONE/COMPLETE in
__btrfs_add_ordered_extent()
This is just to make the code work, with extra ASSERT(), there are
limited values can be passed in.
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
fs/btrfs/inode.c | 4 ++--
fs/btrfs/ordered-data.c | 18 +++++++++++++-----
fs/btrfs/ordered-data.h | 37 +++++++++++++++++++++++-------------
include/trace/events/btrfs.h | 7 ++++---
4 files changed, 43 insertions(+), 23 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index ef6cb7b620d0..ea9056cc5559 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -917,7 +917,6 @@ static noinline void submit_compressed_extents(struct async_chunk *async_chunk)
ins.objectid,
async_extent->ram_size,
ins.offset,
- BTRFS_ORDERED_COMPRESSED,
async_extent->compress_type);
if (ret) {
btrfs_drop_extent_cache(inode, async_extent->start,
@@ -1127,7 +1126,8 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
free_extent_map(em);
ret = btrfs_add_ordered_extent(inode, start, ins.objectid,
- ram_size, cur_alloc_size, 0);
+ ram_size, cur_alloc_size,
+ BTRFS_ORDERED_REGULAR);
if (ret)
goto out_drop_extent_cache;
diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
index d5d326c674b1..bd7e187d9b16 100644
--- a/fs/btrfs/ordered-data.c
+++ b/fs/btrfs/ordered-data.c
@@ -199,8 +199,11 @@ static int __btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset
entry->compress_type = compress_type;
entry->truncated_len = (u64)-1;
entry->qgroup_rsv = ret;
- if (type != BTRFS_ORDERED_IO_DONE && type != BTRFS_ORDERED_COMPLETE)
- set_bit(type, &entry->flags);
+
+ ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
+ type == BTRFS_ORDERED_PREALLOC ||
+ type == BTRFS_ORDERED_COMPRESSED);
+ set_bit(type, &entry->flags);
if (dio) {
percpu_counter_add_batch(&fs_info->dio_bytes, num_bytes,
@@ -256,6 +259,8 @@ int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset,
u64 disk_bytenr, u64 num_bytes, u64 disk_num_bytes,
int type)
{
+ ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
+ type == BTRFS_ORDERED_PREALLOC);
return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
num_bytes, disk_num_bytes, type, 0,
BTRFS_COMPRESS_NONE);
@@ -265,6 +270,8 @@ int btrfs_add_ordered_extent_dio(struct btrfs_inode *inode, u64 file_offset,
u64 disk_bytenr, u64 num_bytes,
u64 disk_num_bytes, int type)
{
+ ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
+ type == BTRFS_ORDERED_PREALLOC);
return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
num_bytes, disk_num_bytes, type, 1,
BTRFS_COMPRESS_NONE);
@@ -272,11 +279,12 @@ int btrfs_add_ordered_extent_dio(struct btrfs_inode *inode, u64 file_offset,
int btrfs_add_ordered_extent_compress(struct btrfs_inode *inode, u64 file_offset,
u64 disk_bytenr, u64 num_bytes,
- u64 disk_num_bytes, int type,
- int compress_type)
+ u64 disk_num_bytes, int compress_type)
{
+ ASSERT(compress_type != BTRFS_COMPRESS_NONE);
return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
- num_bytes, disk_num_bytes, type, 0,
+ num_bytes, disk_num_bytes,
+ BTRFS_ORDERED_COMPRESSED, 0,
compress_type);
}
diff --git a/fs/btrfs/ordered-data.h b/fs/btrfs/ordered-data.h
index 46194c2c05d4..151ec6bba405 100644
--- a/fs/btrfs/ordered-data.h
+++ b/fs/btrfs/ordered-data.h
@@ -27,7 +27,7 @@ struct btrfs_ordered_sum {
};
/*
- * bits for the flags field:
+ * Bits for btrfs_ordered_extent::flags.
*
* BTRFS_ORDERED_IO_DONE is set when all of the blocks are written.
* It is used to make sure metadata is inserted into the tree only once
@@ -38,24 +38,36 @@ struct btrfs_ordered_sum {
* IO is done and any metadata is inserted into the tree.
*/
enum {
+ /*
+ * Different types for direct io, one and only one of the 4 type can
+ * be set when creating ordered extent.
+ *
+ * REGULAR: For regular non-compressed COW write
+ * NOCOW: For NOCOW write into existing non-hole extent
+ * PREALLOC: For NOCOW write into preallocated extent
+ * COMPRESSED: For compressed COW write
+ */
+ BTRFS_ORDERED_REGULAR,
+ BTRFS_ORDERED_NOCOW,
+ BTRFS_ORDERED_PREALLOC,
+ BTRFS_ORDERED_COMPRESSED,
+
+ /*
+ * Extra bit for DirectIO, can only be set for
+ * REGULAR/NOCOW/PREALLOC. No DIO for compressed extent.
+ */
+ BTRFS_ORDERED_DIRECT,
+
+ /* Extra status bits for ordered extents */
+
/* set when all the pages are written */
BTRFS_ORDERED_IO_DONE,
/* set when removed from the tree */
BTRFS_ORDERED_COMPLETE,
- /* set when we want to write in place */
- BTRFS_ORDERED_NOCOW,
- /* writing a zlib compressed extent */
- BTRFS_ORDERED_COMPRESSED,
- /* set when writing to preallocated extent */
- BTRFS_ORDERED_PREALLOC,
- /* set when we're doing DIO with this extent */
- BTRFS_ORDERED_DIRECT,
/* We had an io error when writing this out */
BTRFS_ORDERED_IOERR,
/* Set when we have to truncate an extent */
BTRFS_ORDERED_TRUNCATED,
- /* Regular IO for COW */
- BTRFS_ORDERED_REGULAR,
/* Used during fsync to track already logged extents */
BTRFS_ORDERED_LOGGED,
/* We have already logged all the csums of the ordered extent */
@@ -167,8 +179,7 @@ int btrfs_add_ordered_extent_dio(struct btrfs_inode *inode, u64 file_offset,
u64 disk_num_bytes, int type);
int btrfs_add_ordered_extent_compress(struct btrfs_inode *inode, u64 file_offset,
u64 disk_bytenr, u64 num_bytes,
- u64 disk_num_bytes, int type,
- int compress_type);
+ u64 disk_num_bytes, int compress_type);
void btrfs_add_ordered_sum(struct btrfs_ordered_extent *entry,
struct btrfs_ordered_sum *sum);
struct btrfs_ordered_extent *btrfs_lookup_ordered_extent(struct btrfs_inode *inode,
diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
index ecd24c719de4..b9896fc06160 100644
--- a/include/trace/events/btrfs.h
+++ b/include/trace/events/btrfs.h
@@ -499,12 +499,13 @@ DEFINE_EVENT(
#define show_ordered_flags(flags) \
__print_flags(flags, "|", \
- { (1 << BTRFS_ORDERED_IO_DONE), "IO_DONE" }, \
- { (1 << BTRFS_ORDERED_COMPLETE), "COMPLETE" }, \
+ { (1 << BTRFS_ORDERED_REGULAR), "REGULAR" }, \
{ (1 << BTRFS_ORDERED_NOCOW), "NOCOW" }, \
- { (1 << BTRFS_ORDERED_COMPRESSED), "COMPRESSED" }, \
{ (1 << BTRFS_ORDERED_PREALLOC), "PREALLOC" }, \
+ { (1 << BTRFS_ORDERED_COMPRESSED), "COMPRESSED" }, \
{ (1 << BTRFS_ORDERED_DIRECT), "DIRECT" }, \
+ { (1 << BTRFS_ORDERED_IO_DONE), "IO_DONE" }, \
+ { (1 << BTRFS_ORDERED_COMPLETE), "COMPLETE" }, \
{ (1 << BTRFS_ORDERED_IOERR), "IOERR" }, \
{ (1 << BTRFS_ORDERED_TRUNCATED), "TRUNCATED" })
--
2.30.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] btrfs: rework the order of btrfs_ordered_extent::flags
2021-01-21 6:13 [PATCH] btrfs: rework the order of btrfs_ordered_extent::flags Qu Wenruo
@ 2021-01-21 7:38 ` Nikolay Borisov
2021-01-21 10:32 ` Filipe Manana
2021-01-21 16:47 ` David Sterba
2 siblings, 0 replies; 8+ messages in thread
From: Nikolay Borisov @ 2021-01-21 7:38 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs
On 21.01.21 г. 8:13 ч., Qu Wenruo wrote:
> [BUG]
> There is a long existing bug in the last parameter of
> btrfs_add_ordered_extent(), in commit 771ed689d2cd ("Btrfs: Optimize
> compressed writeback and reads") back to 2008.
>
> In that ancient commit btrfs_add_ordered_extent() expects the @type
> parameter to be one of the following:
> - BTRFS_ORDERED_REGULAR
> - BTRFS_ORDERED_NOCOW
> - BTRFS_ORDERED_PREALLOC
> - BTRFS_ORDERED_COMPRESSED
>
> But we pass 0 in cow_file_range(), which means BTRFS_ORDERED_IO_DONE.
>
> Ironically extra check in __btrfs_add_ordered_extent() won't set the bit
> if we're seeing (type == IO_DONE || type == IO_COMPLETE), and avoid any
> obvious bug.
>
> But this still leads to regular COW ordered extent having no bit to
> indicate its type in various trace events, rendering REGULAR bit
> useless.
>
> [FIX]
> This patch will change the following aspects to avoid such problem:
> - Reorder btrfs_ordered_extent::flags
> Now the type bits go first (REGULAR/NOCOW/PREALLCO/COMPRESSED), then
> DIRECT bit, finally extra status bits like IO_DONE/COMPLETE/IOERR.
>
> - Add extra ASSERT() for btrfs_add_ordered_extent_*()
>
> - Remove @type parameter for btrfs_add_ordered_extent_compress()
> As the only valid @type here is BTRFS_ORDERED_COMPRESSED.
>
> - Remove the unnecessary special check for IO_DONE/COMPLETE in
> __btrfs_add_ordered_extent()
> This is just to make the code work, with extra ASSERT(), there are
> limited values can be passed in.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] btrfs: rework the order of btrfs_ordered_extent::flags
2021-01-21 6:13 [PATCH] btrfs: rework the order of btrfs_ordered_extent::flags Qu Wenruo
2021-01-21 7:38 ` Nikolay Borisov
@ 2021-01-21 10:32 ` Filipe Manana
2021-01-21 11:07 ` Qu Wenruo
2021-01-21 16:47 ` David Sterba
2 siblings, 1 reply; 8+ messages in thread
From: Filipe Manana @ 2021-01-21 10:32 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
On Thu, Jan 21, 2021 at 6:27 AM Qu Wenruo <wqu@suse.com> wrote:
>
> [BUG]
> There is a long existing bug in the last parameter of
> btrfs_add_ordered_extent(), in commit 771ed689d2cd ("Btrfs: Optimize
> compressed writeback and reads") back to 2008.
>
> In that ancient commit btrfs_add_ordered_extent() expects the @type
> parameter to be one of the following:
> - BTRFS_ORDERED_REGULAR
> - BTRFS_ORDERED_NOCOW
> - BTRFS_ORDERED_PREALLOC
> - BTRFS_ORDERED_COMPRESSED
>
> But we pass 0 in cow_file_range(), which means BTRFS_ORDERED_IO_DONE.
>
> Ironically extra check in __btrfs_add_ordered_extent() won't set the bit
> if we're seeing (type == IO_DONE || type == IO_COMPLETE), and avoid any
> obvious bug.
>
> But this still leads to regular COW ordered extent having no bit to
> indicate its type in various trace events, rendering REGULAR bit
> useless.
>
> [FIX]
> This patch will change the following aspects to avoid such problem:
> - Reorder btrfs_ordered_extent::flags
> Now the type bits go first (REGULAR/NOCOW/PREALLCO/COMPRESSED), then
> DIRECT bit, finally extra status bits like IO_DONE/COMPLETE/IOERR.
>
> - Add extra ASSERT() for btrfs_add_ordered_extent_*()
>
> - Remove @type parameter for btrfs_add_ordered_extent_compress()
> As the only valid @type here is BTRFS_ORDERED_COMPRESSED.
>
> - Remove the unnecessary special check for IO_DONE/COMPLETE in
> __btrfs_add_ordered_extent()
> This is just to make the code work, with extra ASSERT(), there are
> limited values can be passed in.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> fs/btrfs/inode.c | 4 ++--
> fs/btrfs/ordered-data.c | 18 +++++++++++++-----
> fs/btrfs/ordered-data.h | 37 +++++++++++++++++++++++-------------
> include/trace/events/btrfs.h | 7 ++++---
> 4 files changed, 43 insertions(+), 23 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index ef6cb7b620d0..ea9056cc5559 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -917,7 +917,6 @@ static noinline void submit_compressed_extents(struct async_chunk *async_chunk)
> ins.objectid,
> async_extent->ram_size,
> ins.offset,
> - BTRFS_ORDERED_COMPRESSED,
> async_extent->compress_type);
> if (ret) {
> btrfs_drop_extent_cache(inode, async_extent->start,
> @@ -1127,7 +1126,8 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> free_extent_map(em);
>
> ret = btrfs_add_ordered_extent(inode, start, ins.objectid,
> - ram_size, cur_alloc_size, 0);
> + ram_size, cur_alloc_size,
> + BTRFS_ORDERED_REGULAR);
> if (ret)
> goto out_drop_extent_cache;
>
> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> index d5d326c674b1..bd7e187d9b16 100644
> --- a/fs/btrfs/ordered-data.c
> +++ b/fs/btrfs/ordered-data.c
> @@ -199,8 +199,11 @@ static int __btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset
> entry->compress_type = compress_type;
> entry->truncated_len = (u64)-1;
> entry->qgroup_rsv = ret;
> - if (type != BTRFS_ORDERED_IO_DONE && type != BTRFS_ORDERED_COMPLETE)
> - set_bit(type, &entry->flags);
> +
> + ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
> + type == BTRFS_ORDERED_PREALLOC ||
> + type == BTRFS_ORDERED_COMPRESSED);
> + set_bit(type, &entry->flags);
>
> if (dio) {
> percpu_counter_add_batch(&fs_info->dio_bytes, num_bytes,
> @@ -256,6 +259,8 @@ int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset,
> u64 disk_bytenr, u64 num_bytes, u64 disk_num_bytes,
> int type)
> {
> + ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
> + type == BTRFS_ORDERED_PREALLOC);
> return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
> num_bytes, disk_num_bytes, type, 0,
> BTRFS_COMPRESS_NONE);
> @@ -265,6 +270,8 @@ int btrfs_add_ordered_extent_dio(struct btrfs_inode *inode, u64 file_offset,
> u64 disk_bytenr, u64 num_bytes,
> u64 disk_num_bytes, int type)
> {
> + ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
> + type == BTRFS_ORDERED_PREALLOC);
> return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
> num_bytes, disk_num_bytes, type, 1,
> BTRFS_COMPRESS_NONE);
> @@ -272,11 +279,12 @@ int btrfs_add_ordered_extent_dio(struct btrfs_inode *inode, u64 file_offset,
>
> int btrfs_add_ordered_extent_compress(struct btrfs_inode *inode, u64 file_offset,
> u64 disk_bytenr, u64 num_bytes,
> - u64 disk_num_bytes, int type,
> - int compress_type)
> + u64 disk_num_bytes, int compress_type)
> {
> + ASSERT(compress_type != BTRFS_COMPRESS_NONE);
> return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
> - num_bytes, disk_num_bytes, type, 0,
> + num_bytes, disk_num_bytes,
> + BTRFS_ORDERED_COMPRESSED, 0,
> compress_type);
> }
>
> diff --git a/fs/btrfs/ordered-data.h b/fs/btrfs/ordered-data.h
> index 46194c2c05d4..151ec6bba405 100644
> --- a/fs/btrfs/ordered-data.h
> +++ b/fs/btrfs/ordered-data.h
> @@ -27,7 +27,7 @@ struct btrfs_ordered_sum {
> };
>
> /*
> - * bits for the flags field:
> + * Bits for btrfs_ordered_extent::flags.
> *
> * BTRFS_ORDERED_IO_DONE is set when all of the blocks are written.
> * It is used to make sure metadata is inserted into the tree only once
> @@ -38,24 +38,36 @@ struct btrfs_ordered_sum {
> * IO is done and any metadata is inserted into the tree.
> */
> enum {
> + /*
> + * Different types for direct io, one and only one of the 4 type can
Different types for both buffered and direct IO (except the compressed type).
Also "4 type" -> "4 types".
Other than that, it looks good, thanks.
> + * be set when creating ordered extent.
> + *
> + * REGULAR: For regular non-compressed COW write
> + * NOCOW: For NOCOW write into existing non-hole extent
> + * PREALLOC: For NOCOW write into preallocated extent
> + * COMPRESSED: For compressed COW write
> + */
> + BTRFS_ORDERED_REGULAR,
> + BTRFS_ORDERED_NOCOW,
> + BTRFS_ORDERED_PREALLOC,
> + BTRFS_ORDERED_COMPRESSED,
> +
> + /*
> + * Extra bit for DirectIO, can only be set for
> + * REGULAR/NOCOW/PREALLOC. No DIO for compressed extent.
> + */
> + BTRFS_ORDERED_DIRECT,
> +
> + /* Extra status bits for ordered extents */
> +
> /* set when all the pages are written */
> BTRFS_ORDERED_IO_DONE,
> /* set when removed from the tree */
> BTRFS_ORDERED_COMPLETE,
> - /* set when we want to write in place */
> - BTRFS_ORDERED_NOCOW,
> - /* writing a zlib compressed extent */
> - BTRFS_ORDERED_COMPRESSED,
> - /* set when writing to preallocated extent */
> - BTRFS_ORDERED_PREALLOC,
> - /* set when we're doing DIO with this extent */
> - BTRFS_ORDERED_DIRECT,
> /* We had an io error when writing this out */
> BTRFS_ORDERED_IOERR,
> /* Set when we have to truncate an extent */
> BTRFS_ORDERED_TRUNCATED,
> - /* Regular IO for COW */
> - BTRFS_ORDERED_REGULAR,
> /* Used during fsync to track already logged extents */
> BTRFS_ORDERED_LOGGED,
> /* We have already logged all the csums of the ordered extent */
> @@ -167,8 +179,7 @@ int btrfs_add_ordered_extent_dio(struct btrfs_inode *inode, u64 file_offset,
> u64 disk_num_bytes, int type);
> int btrfs_add_ordered_extent_compress(struct btrfs_inode *inode, u64 file_offset,
> u64 disk_bytenr, u64 num_bytes,
> - u64 disk_num_bytes, int type,
> - int compress_type);
> + u64 disk_num_bytes, int compress_type);
> void btrfs_add_ordered_sum(struct btrfs_ordered_extent *entry,
> struct btrfs_ordered_sum *sum);
> struct btrfs_ordered_extent *btrfs_lookup_ordered_extent(struct btrfs_inode *inode,
> diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
> index ecd24c719de4..b9896fc06160 100644
> --- a/include/trace/events/btrfs.h
> +++ b/include/trace/events/btrfs.h
> @@ -499,12 +499,13 @@ DEFINE_EVENT(
>
> #define show_ordered_flags(flags) \
> __print_flags(flags, "|", \
> - { (1 << BTRFS_ORDERED_IO_DONE), "IO_DONE" }, \
> - { (1 << BTRFS_ORDERED_COMPLETE), "COMPLETE" }, \
> + { (1 << BTRFS_ORDERED_REGULAR), "REGULAR" }, \
> { (1 << BTRFS_ORDERED_NOCOW), "NOCOW" }, \
> - { (1 << BTRFS_ORDERED_COMPRESSED), "COMPRESSED" }, \
> { (1 << BTRFS_ORDERED_PREALLOC), "PREALLOC" }, \
> + { (1 << BTRFS_ORDERED_COMPRESSED), "COMPRESSED" }, \
> { (1 << BTRFS_ORDERED_DIRECT), "DIRECT" }, \
> + { (1 << BTRFS_ORDERED_IO_DONE), "IO_DONE" }, \
> + { (1 << BTRFS_ORDERED_COMPLETE), "COMPLETE" }, \
> { (1 << BTRFS_ORDERED_IOERR), "IOERR" }, \
> { (1 << BTRFS_ORDERED_TRUNCATED), "TRUNCATED" })
>
> --
> 2.30.0
>
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] btrfs: rework the order of btrfs_ordered_extent::flags
2021-01-21 10:32 ` Filipe Manana
@ 2021-01-21 11:07 ` Qu Wenruo
0 siblings, 0 replies; 8+ messages in thread
From: Qu Wenruo @ 2021-01-21 11:07 UTC (permalink / raw)
To: fdmanana; +Cc: linux-btrfs
On 2021/1/21 下午6:32, Filipe Manana wrote:
> On Thu, Jan 21, 2021 at 6:27 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> [BUG]
>> There is a long existing bug in the last parameter of
>> btrfs_add_ordered_extent(), in commit 771ed689d2cd ("Btrfs: Optimize
>> compressed writeback and reads") back to 2008.
>>
>> In that ancient commit btrfs_add_ordered_extent() expects the @type
>> parameter to be one of the following:
>> - BTRFS_ORDERED_REGULAR
>> - BTRFS_ORDERED_NOCOW
>> - BTRFS_ORDERED_PREALLOC
>> - BTRFS_ORDERED_COMPRESSED
>>
>> But we pass 0 in cow_file_range(), which means BTRFS_ORDERED_IO_DONE.
>>
>> Ironically extra check in __btrfs_add_ordered_extent() won't set the bit
>> if we're seeing (type == IO_DONE || type == IO_COMPLETE), and avoid any
>> obvious bug.
>>
>> But this still leads to regular COW ordered extent having no bit to
>> indicate its type in various trace events, rendering REGULAR bit
>> useless.
>>
>> [FIX]
>> This patch will change the following aspects to avoid such problem:
>> - Reorder btrfs_ordered_extent::flags
>> Now the type bits go first (REGULAR/NOCOW/PREALLCO/COMPRESSED), then
>> DIRECT bit, finally extra status bits like IO_DONE/COMPLETE/IOERR.
>>
>> - Add extra ASSERT() for btrfs_add_ordered_extent_*()
>>
>> - Remove @type parameter for btrfs_add_ordered_extent_compress()
>> As the only valid @type here is BTRFS_ORDERED_COMPRESSED.
>>
>> - Remove the unnecessary special check for IO_DONE/COMPLETE in
>> __btrfs_add_ordered_extent()
>> This is just to make the code work, with extra ASSERT(), there are
>> limited values can be passed in.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> fs/btrfs/inode.c | 4 ++--
>> fs/btrfs/ordered-data.c | 18 +++++++++++++-----
>> fs/btrfs/ordered-data.h | 37 +++++++++++++++++++++++-------------
>> include/trace/events/btrfs.h | 7 ++++---
>> 4 files changed, 43 insertions(+), 23 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index ef6cb7b620d0..ea9056cc5559 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -917,7 +917,6 @@ static noinline void submit_compressed_extents(struct async_chunk *async_chunk)
>> ins.objectid,
>> async_extent->ram_size,
>> ins.offset,
>> - BTRFS_ORDERED_COMPRESSED,
>> async_extent->compress_type);
>> if (ret) {
>> btrfs_drop_extent_cache(inode, async_extent->start,
>> @@ -1127,7 +1126,8 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
>> free_extent_map(em);
>>
>> ret = btrfs_add_ordered_extent(inode, start, ins.objectid,
>> - ram_size, cur_alloc_size, 0);
>> + ram_size, cur_alloc_size,
>> + BTRFS_ORDERED_REGULAR);
>> if (ret)
>> goto out_drop_extent_cache;
>>
>> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
>> index d5d326c674b1..bd7e187d9b16 100644
>> --- a/fs/btrfs/ordered-data.c
>> +++ b/fs/btrfs/ordered-data.c
>> @@ -199,8 +199,11 @@ static int __btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset
>> entry->compress_type = compress_type;
>> entry->truncated_len = (u64)-1;
>> entry->qgroup_rsv = ret;
>> - if (type != BTRFS_ORDERED_IO_DONE && type != BTRFS_ORDERED_COMPLETE)
>> - set_bit(type, &entry->flags);
>> +
>> + ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
>> + type == BTRFS_ORDERED_PREALLOC ||
>> + type == BTRFS_ORDERED_COMPRESSED);
>> + set_bit(type, &entry->flags);
>>
>> if (dio) {
>> percpu_counter_add_batch(&fs_info->dio_bytes, num_bytes,
>> @@ -256,6 +259,8 @@ int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset,
>> u64 disk_bytenr, u64 num_bytes, u64 disk_num_bytes,
>> int type)
>> {
>> + ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
>> + type == BTRFS_ORDERED_PREALLOC);
>> return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
>> num_bytes, disk_num_bytes, type, 0,
>> BTRFS_COMPRESS_NONE);
>> @@ -265,6 +270,8 @@ int btrfs_add_ordered_extent_dio(struct btrfs_inode *inode, u64 file_offset,
>> u64 disk_bytenr, u64 num_bytes,
>> u64 disk_num_bytes, int type)
>> {
>> + ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
>> + type == BTRFS_ORDERED_PREALLOC);
>> return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
>> num_bytes, disk_num_bytes, type, 1,
>> BTRFS_COMPRESS_NONE);
>> @@ -272,11 +279,12 @@ int btrfs_add_ordered_extent_dio(struct btrfs_inode *inode, u64 file_offset,
>>
>> int btrfs_add_ordered_extent_compress(struct btrfs_inode *inode, u64 file_offset,
>> u64 disk_bytenr, u64 num_bytes,
>> - u64 disk_num_bytes, int type,
>> - int compress_type)
>> + u64 disk_num_bytes, int compress_type)
>> {
>> + ASSERT(compress_type != BTRFS_COMPRESS_NONE);
>> return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
>> - num_bytes, disk_num_bytes, type, 0,
>> + num_bytes, disk_num_bytes,
>> + BTRFS_ORDERED_COMPRESSED, 0,
>> compress_type);
>> }
>>
>> diff --git a/fs/btrfs/ordered-data.h b/fs/btrfs/ordered-data.h
>> index 46194c2c05d4..151ec6bba405 100644
>> --- a/fs/btrfs/ordered-data.h
>> +++ b/fs/btrfs/ordered-data.h
>> @@ -27,7 +27,7 @@ struct btrfs_ordered_sum {
>> };
>>
>> /*
>> - * bits for the flags field:
>> + * Bits for btrfs_ordered_extent::flags.
>> *
>> * BTRFS_ORDERED_IO_DONE is set when all of the blocks are written.
>> * It is used to make sure metadata is inserted into the tree only once
>> @@ -38,24 +38,36 @@ struct btrfs_ordered_sum {
>> * IO is done and any metadata is inserted into the tree.
>> */
>> enum {
>> + /*
>> + * Different types for direct io, one and only one of the 4 type can
>
> Different types for both buffered and direct IO (except the compressed type).
My bad, no direct io should be mentioned here, just 4 types of IO.
The direct IO has good enough explanation already for its bit.
Thanks for the review,
Qu
>
> Also "4 type" -> "4 types".
>
> Other than that, it looks good, thanks.
>
>> + * be set when creating ordered extent.
>> + *
>> + * REGULAR: For regular non-compressed COW write
>> + * NOCOW: For NOCOW write into existing non-hole extent
>> + * PREALLOC: For NOCOW write into preallocated extent
>> + * COMPRESSED: For compressed COW write
>> + */
>> + BTRFS_ORDERED_REGULAR,
>> + BTRFS_ORDERED_NOCOW,
>> + BTRFS_ORDERED_PREALLOC,
>> + BTRFS_ORDERED_COMPRESSED,
>> +
>> + /*
>> + * Extra bit for DirectIO, can only be set for
>> + * REGULAR/NOCOW/PREALLOC. No DIO for compressed extent.
>> + */
>> + BTRFS_ORDERED_DIRECT,
>> +
>> + /* Extra status bits for ordered extents */
>> +
>> /* set when all the pages are written */
>> BTRFS_ORDERED_IO_DONE,
>> /* set when removed from the tree */
>> BTRFS_ORDERED_COMPLETE,
>> - /* set when we want to write in place */
>> - BTRFS_ORDERED_NOCOW,
>> - /* writing a zlib compressed extent */
>> - BTRFS_ORDERED_COMPRESSED,
>> - /* set when writing to preallocated extent */
>> - BTRFS_ORDERED_PREALLOC,
>> - /* set when we're doing DIO with this extent */
>> - BTRFS_ORDERED_DIRECT,
>> /* We had an io error when writing this out */
>> BTRFS_ORDERED_IOERR,
>> /* Set when we have to truncate an extent */
>> BTRFS_ORDERED_TRUNCATED,
>> - /* Regular IO for COW */
>> - BTRFS_ORDERED_REGULAR,
>> /* Used during fsync to track already logged extents */
>> BTRFS_ORDERED_LOGGED,
>> /* We have already logged all the csums of the ordered extent */
>> @@ -167,8 +179,7 @@ int btrfs_add_ordered_extent_dio(struct btrfs_inode *inode, u64 file_offset,
>> u64 disk_num_bytes, int type);
>> int btrfs_add_ordered_extent_compress(struct btrfs_inode *inode, u64 file_offset,
>> u64 disk_bytenr, u64 num_bytes,
>> - u64 disk_num_bytes, int type,
>> - int compress_type);
>> + u64 disk_num_bytes, int compress_type);
>> void btrfs_add_ordered_sum(struct btrfs_ordered_extent *entry,
>> struct btrfs_ordered_sum *sum);
>> struct btrfs_ordered_extent *btrfs_lookup_ordered_extent(struct btrfs_inode *inode,
>> diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
>> index ecd24c719de4..b9896fc06160 100644
>> --- a/include/trace/events/btrfs.h
>> +++ b/include/trace/events/btrfs.h
>> @@ -499,12 +499,13 @@ DEFINE_EVENT(
>>
>> #define show_ordered_flags(flags) \
>> __print_flags(flags, "|", \
>> - { (1 << BTRFS_ORDERED_IO_DONE), "IO_DONE" }, \
>> - { (1 << BTRFS_ORDERED_COMPLETE), "COMPLETE" }, \
>> + { (1 << BTRFS_ORDERED_REGULAR), "REGULAR" }, \
>> { (1 << BTRFS_ORDERED_NOCOW), "NOCOW" }, \
>> - { (1 << BTRFS_ORDERED_COMPRESSED), "COMPRESSED" }, \
>> { (1 << BTRFS_ORDERED_PREALLOC), "PREALLOC" }, \
>> + { (1 << BTRFS_ORDERED_COMPRESSED), "COMPRESSED" }, \
>> { (1 << BTRFS_ORDERED_DIRECT), "DIRECT" }, \
>> + { (1 << BTRFS_ORDERED_IO_DONE), "IO_DONE" }, \
>> + { (1 << BTRFS_ORDERED_COMPLETE), "COMPLETE" }, \
>> { (1 << BTRFS_ORDERED_IOERR), "IOERR" }, \
>> { (1 << BTRFS_ORDERED_TRUNCATED), "TRUNCATED" })
>>
>> --
>> 2.30.0
>>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] btrfs: rework the order of btrfs_ordered_extent::flags
2021-01-21 6:13 [PATCH] btrfs: rework the order of btrfs_ordered_extent::flags Qu Wenruo
2021-01-21 7:38 ` Nikolay Borisov
2021-01-21 10:32 ` Filipe Manana
@ 2021-01-21 16:47 ` David Sterba
2021-01-25 12:15 ` Filipe Manana
2 siblings, 1 reply; 8+ messages in thread
From: David Sterba @ 2021-01-21 16:47 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
On Thu, Jan 21, 2021 at 02:13:54PM +0800, Qu Wenruo wrote:
> [BUG]
> There is a long existing bug in the last parameter of
> btrfs_add_ordered_extent(), in commit 771ed689d2cd ("Btrfs: Optimize
> compressed writeback and reads") back to 2008.
>
> In that ancient commit btrfs_add_ordered_extent() expects the @type
> parameter to be one of the following:
> - BTRFS_ORDERED_REGULAR
> - BTRFS_ORDERED_NOCOW
> - BTRFS_ORDERED_PREALLOC
> - BTRFS_ORDERED_COMPRESSED
>
> But we pass 0 in cow_file_range(), which means BTRFS_ORDERED_IO_DONE.
>
> Ironically extra check in __btrfs_add_ordered_extent() won't set the bit
> if we're seeing (type == IO_DONE || type == IO_COMPLETE), and avoid any
> obvious bug.
>
> But this still leads to regular COW ordered extent having no bit to
> indicate its type in various trace events, rendering REGULAR bit
> useless.
>
> [FIX]
> This patch will change the following aspects to avoid such problem:
> - Reorder btrfs_ordered_extent::flags
> Now the type bits go first (REGULAR/NOCOW/PREALLCO/COMPRESSED), then
> DIRECT bit, finally extra status bits like IO_DONE/COMPLETE/IOERR.
>
> - Add extra ASSERT() for btrfs_add_ordered_extent_*()
>
> - Remove @type parameter for btrfs_add_ordered_extent_compress()
> As the only valid @type here is BTRFS_ORDERED_COMPRESSED.
>
> - Remove the unnecessary special check for IO_DONE/COMPLETE in
> __btrfs_add_ordered_extent()
> This is just to make the code work, with extra ASSERT(), there are
> limited values can be passed in.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
Added to misc-next thanks.
> ---
> fs/btrfs/inode.c | 4 ++--
> fs/btrfs/ordered-data.c | 18 +++++++++++++-----
> fs/btrfs/ordered-data.h | 37 +++++++++++++++++++++++-------------
> include/trace/events/btrfs.h | 7 ++++---
> 4 files changed, 43 insertions(+), 23 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index ef6cb7b620d0..ea9056cc5559 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -917,7 +917,6 @@ static noinline void submit_compressed_extents(struct async_chunk *async_chunk)
> ins.objectid,
> async_extent->ram_size,
> ins.offset,
> - BTRFS_ORDERED_COMPRESSED,
> async_extent->compress_type);
> if (ret) {
> btrfs_drop_extent_cache(inode, async_extent->start,
> @@ -1127,7 +1126,8 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> free_extent_map(em);
>
> ret = btrfs_add_ordered_extent(inode, start, ins.objectid,
> - ram_size, cur_alloc_size, 0);
> + ram_size, cur_alloc_size,
> + BTRFS_ORDERED_REGULAR);
> if (ret)
> goto out_drop_extent_cache;
>
> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> index d5d326c674b1..bd7e187d9b16 100644
> --- a/fs/btrfs/ordered-data.c
> +++ b/fs/btrfs/ordered-data.c
> @@ -199,8 +199,11 @@ static int __btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset
> entry->compress_type = compress_type;
> entry->truncated_len = (u64)-1;
> entry->qgroup_rsv = ret;
> - if (type != BTRFS_ORDERED_IO_DONE && type != BTRFS_ORDERED_COMPLETE)
> - set_bit(type, &entry->flags);
> +
> + ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
> + type == BTRFS_ORDERED_PREALLOC ||
> + type == BTRFS_ORDERED_COMPRESSED);
I've reformatted that so that all the checks are on separate lines, it's
easier to read though it does not fill the whole line.
> + set_bit(type, &entry->flags);
>
> if (dio) {
> percpu_counter_add_batch(&fs_info->dio_bytes, num_bytes,
> @@ -256,6 +259,8 @@ int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset,
> u64 disk_bytenr, u64 num_bytes, u64 disk_num_bytes,
> int type)
> {
> + ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
> + type == BTRFS_ORDERED_PREALLOC);
> return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
> num_bytes, disk_num_bytes, type, 0,
> BTRFS_COMPRESS_NONE);
> @@ -265,6 +270,8 @@ int btrfs_add_ordered_extent_dio(struct btrfs_inode *inode, u64 file_offset,
> u64 disk_bytenr, u64 num_bytes,
> u64 disk_num_bytes, int type)
> {
> + ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
> + type == BTRFS_ORDERED_PREALLOC);
> return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
> num_bytes, disk_num_bytes, type, 1,
> BTRFS_COMPRESS_NONE);
> @@ -272,11 +279,12 @@ int btrfs_add_ordered_extent_dio(struct btrfs_inode *inode, u64 file_offset,
>
> int btrfs_add_ordered_extent_compress(struct btrfs_inode *inode, u64 file_offset,
> u64 disk_bytenr, u64 num_bytes,
> - u64 disk_num_bytes, int type,
> - int compress_type)
> + u64 disk_num_bytes, int compress_type)
> {
> + ASSERT(compress_type != BTRFS_COMPRESS_NONE);
> return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
> - num_bytes, disk_num_bytes, type, 0,
> + num_bytes, disk_num_bytes,
> + BTRFS_ORDERED_COMPRESSED, 0,
> compress_type);
> }
>
> diff --git a/fs/btrfs/ordered-data.h b/fs/btrfs/ordered-data.h
> index 46194c2c05d4..151ec6bba405 100644
> --- a/fs/btrfs/ordered-data.h
> +++ b/fs/btrfs/ordered-data.h
> @@ -27,7 +27,7 @@ struct btrfs_ordered_sum {
> };
>
> /*
> - * bits for the flags field:
> + * Bits for btrfs_ordered_extent::flags.
> *
> * BTRFS_ORDERED_IO_DONE is set when all of the blocks are written.
> * It is used to make sure metadata is inserted into the tree only once
> @@ -38,24 +38,36 @@ struct btrfs_ordered_sum {
> * IO is done and any metadata is inserted into the tree.
> */
> enum {
> + /*
> + * Different types for direct io, one and only one of the 4 type can
direct io
> + * be set when creating ordered extent.
> + *
> + * REGULAR: For regular non-compressed COW write
> + * NOCOW: For NOCOW write into existing non-hole extent
> + * PREALLOC: For NOCOW write into preallocated extent
> + * COMPRESSED: For compressed COW write
> + */
> + BTRFS_ORDERED_REGULAR,
> + BTRFS_ORDERED_NOCOW,
> + BTRFS_ORDERED_PREALLOC,
> + BTRFS_ORDERED_COMPRESSED,
> +
> + /*
> + * Extra bit for DirectIO, can only be set for
DirectIO
> + * REGULAR/NOCOW/PREALLOC. No DIO for compressed extent.
DIO
Three different ways to spell that but one is enough. Fixed.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] btrfs: rework the order of btrfs_ordered_extent::flags
2021-01-21 16:47 ` David Sterba
@ 2021-01-25 12:15 ` Filipe Manana
2021-01-25 13:35 ` Qu Wenruo
0 siblings, 1 reply; 8+ messages in thread
From: Filipe Manana @ 2021-01-25 12:15 UTC (permalink / raw)
To: dsterba, Qu Wenruo, linux-btrfs
On Thu, Jan 21, 2021 at 4:52 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Thu, Jan 21, 2021 at 02:13:54PM +0800, Qu Wenruo wrote:
> > [BUG]
> > There is a long existing bug in the last parameter of
> > btrfs_add_ordered_extent(), in commit 771ed689d2cd ("Btrfs: Optimize
> > compressed writeback and reads") back to 2008.
> >
> > In that ancient commit btrfs_add_ordered_extent() expects the @type
> > parameter to be one of the following:
> > - BTRFS_ORDERED_REGULAR
> > - BTRFS_ORDERED_NOCOW
> > - BTRFS_ORDERED_PREALLOC
> > - BTRFS_ORDERED_COMPRESSED
> >
> > But we pass 0 in cow_file_range(), which means BTRFS_ORDERED_IO_DONE.
> >
> > Ironically extra check in __btrfs_add_ordered_extent() won't set the bit
> > if we're seeing (type == IO_DONE || type == IO_COMPLETE), and avoid any
> > obvious bug.
> >
> > But this still leads to regular COW ordered extent having no bit to
> > indicate its type in various trace events, rendering REGULAR bit
> > useless.
> >
> > [FIX]
> > This patch will change the following aspects to avoid such problem:
> > - Reorder btrfs_ordered_extent::flags
> > Now the type bits go first (REGULAR/NOCOW/PREALLCO/COMPRESSED), then
> > DIRECT bit, finally extra status bits like IO_DONE/COMPLETE/IOERR.
> >
> > - Add extra ASSERT() for btrfs_add_ordered_extent_*()
> >
> > - Remove @type parameter for btrfs_add_ordered_extent_compress()
> > As the only valid @type here is BTRFS_ORDERED_COMPRESSED.
> >
> > - Remove the unnecessary special check for IO_DONE/COMPLETE in
> > __btrfs_add_ordered_extent()
> > This is just to make the code work, with extra ASSERT(), there are
> > limited values can be passed in.
> >
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> Added to misc-next thanks.
Btw, I see that you added a Reviewed-by tag from me to the patch.
However I did not give the tag, not because I forgot but because there
was a small detail in a comment that should be fixed, which was not
addressed in misc-next.
Thanks.
>
> > ---
> > fs/btrfs/inode.c | 4 ++--
> > fs/btrfs/ordered-data.c | 18 +++++++++++++-----
> > fs/btrfs/ordered-data.h | 37 +++++++++++++++++++++++-------------
> > include/trace/events/btrfs.h | 7 ++++---
> > 4 files changed, 43 insertions(+), 23 deletions(-)
> >
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index ef6cb7b620d0..ea9056cc5559 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -917,7 +917,6 @@ static noinline void submit_compressed_extents(struct async_chunk *async_chunk)
> > ins.objectid,
> > async_extent->ram_size,
> > ins.offset,
> > - BTRFS_ORDERED_COMPRESSED,
> > async_extent->compress_type);
> > if (ret) {
> > btrfs_drop_extent_cache(inode, async_extent->start,
> > @@ -1127,7 +1126,8 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> > free_extent_map(em);
> >
> > ret = btrfs_add_ordered_extent(inode, start, ins.objectid,
> > - ram_size, cur_alloc_size, 0);
> > + ram_size, cur_alloc_size,
> > + BTRFS_ORDERED_REGULAR);
> > if (ret)
> > goto out_drop_extent_cache;
> >
> > diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> > index d5d326c674b1..bd7e187d9b16 100644
> > --- a/fs/btrfs/ordered-data.c
> > +++ b/fs/btrfs/ordered-data.c
> > @@ -199,8 +199,11 @@ static int __btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset
> > entry->compress_type = compress_type;
> > entry->truncated_len = (u64)-1;
> > entry->qgroup_rsv = ret;
> > - if (type != BTRFS_ORDERED_IO_DONE && type != BTRFS_ORDERED_COMPLETE)
> > - set_bit(type, &entry->flags);
> > +
> > + ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
> > + type == BTRFS_ORDERED_PREALLOC ||
> > + type == BTRFS_ORDERED_COMPRESSED);
>
> I've reformatted that so that all the checks are on separate lines, it's
> easier to read though it does not fill the whole line.
>
> > + set_bit(type, &entry->flags);
> >
> > if (dio) {
> > percpu_counter_add_batch(&fs_info->dio_bytes, num_bytes,
> > @@ -256,6 +259,8 @@ int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset,
> > u64 disk_bytenr, u64 num_bytes, u64 disk_num_bytes,
> > int type)
> > {
> > + ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
> > + type == BTRFS_ORDERED_PREALLOC);
> > return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
> > num_bytes, disk_num_bytes, type, 0,
> > BTRFS_COMPRESS_NONE);
> > @@ -265,6 +270,8 @@ int btrfs_add_ordered_extent_dio(struct btrfs_inode *inode, u64 file_offset,
> > u64 disk_bytenr, u64 num_bytes,
> > u64 disk_num_bytes, int type)
> > {
> > + ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
> > + type == BTRFS_ORDERED_PREALLOC);
> > return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
> > num_bytes, disk_num_bytes, type, 1,
> > BTRFS_COMPRESS_NONE);
> > @@ -272,11 +279,12 @@ int btrfs_add_ordered_extent_dio(struct btrfs_inode *inode, u64 file_offset,
> >
> > int btrfs_add_ordered_extent_compress(struct btrfs_inode *inode, u64 file_offset,
> > u64 disk_bytenr, u64 num_bytes,
> > - u64 disk_num_bytes, int type,
> > - int compress_type)
> > + u64 disk_num_bytes, int compress_type)
> > {
> > + ASSERT(compress_type != BTRFS_COMPRESS_NONE);
> > return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
> > - num_bytes, disk_num_bytes, type, 0,
> > + num_bytes, disk_num_bytes,
> > + BTRFS_ORDERED_COMPRESSED, 0,
> > compress_type);
> > }
> >
> > diff --git a/fs/btrfs/ordered-data.h b/fs/btrfs/ordered-data.h
> > index 46194c2c05d4..151ec6bba405 100644
> > --- a/fs/btrfs/ordered-data.h
> > +++ b/fs/btrfs/ordered-data.h
> > @@ -27,7 +27,7 @@ struct btrfs_ordered_sum {
> > };
> >
> > /*
> > - * bits for the flags field:
> > + * Bits for btrfs_ordered_extent::flags.
> > *
> > * BTRFS_ORDERED_IO_DONE is set when all of the blocks are written.
> > * It is used to make sure metadata is inserted into the tree only once
> > @@ -38,24 +38,36 @@ struct btrfs_ordered_sum {
> > * IO is done and any metadata is inserted into the tree.
> > */
> > enum {
> > + /*
> > + * Different types for direct io, one and only one of the 4 type can
>
> direct io
>
> > + * be set when creating ordered extent.
> > + *
> > + * REGULAR: For regular non-compressed COW write
> > + * NOCOW: For NOCOW write into existing non-hole extent
> > + * PREALLOC: For NOCOW write into preallocated extent
> > + * COMPRESSED: For compressed COW write
> > + */
> > + BTRFS_ORDERED_REGULAR,
> > + BTRFS_ORDERED_NOCOW,
> > + BTRFS_ORDERED_PREALLOC,
> > + BTRFS_ORDERED_COMPRESSED,
> > +
> > + /*
> > + * Extra bit for DirectIO, can only be set for
>
> DirectIO
>
> > + * REGULAR/NOCOW/PREALLOC. No DIO for compressed extent.
>
> DIO
>
> Three different ways to spell that but one is enough. Fixed.
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] btrfs: rework the order of btrfs_ordered_extent::flags
2021-01-25 12:15 ` Filipe Manana
@ 2021-01-25 13:35 ` Qu Wenruo
2021-01-25 16:09 ` David Sterba
0 siblings, 1 reply; 8+ messages in thread
From: Qu Wenruo @ 2021-01-25 13:35 UTC (permalink / raw)
To: fdmanana, dsterba, linux-btrfs
On 2021/1/25 下午8:15, Filipe Manana wrote:
> On Thu, Jan 21, 2021 at 4:52 PM David Sterba <dsterba@suse.cz> wrote:
>>
>> On Thu, Jan 21, 2021 at 02:13:54PM +0800, Qu Wenruo wrote:
>>> [BUG]
>>> There is a long existing bug in the last parameter of
>>> btrfs_add_ordered_extent(), in commit 771ed689d2cd ("Btrfs: Optimize
>>> compressed writeback and reads") back to 2008.
>>>
>>> In that ancient commit btrfs_add_ordered_extent() expects the @type
>>> parameter to be one of the following:
>>> - BTRFS_ORDERED_REGULAR
>>> - BTRFS_ORDERED_NOCOW
>>> - BTRFS_ORDERED_PREALLOC
>>> - BTRFS_ORDERED_COMPRESSED
>>>
>>> But we pass 0 in cow_file_range(), which means BTRFS_ORDERED_IO_DONE.
>>>
>>> Ironically extra check in __btrfs_add_ordered_extent() won't set the bit
>>> if we're seeing (type == IO_DONE || type == IO_COMPLETE), and avoid any
>>> obvious bug.
>>>
>>> But this still leads to regular COW ordered extent having no bit to
>>> indicate its type in various trace events, rendering REGULAR bit
>>> useless.
>>>
>>> [FIX]
>>> This patch will change the following aspects to avoid such problem:
>>> - Reorder btrfs_ordered_extent::flags
>>> Now the type bits go first (REGULAR/NOCOW/PREALLCO/COMPRESSED), then
>>> DIRECT bit, finally extra status bits like IO_DONE/COMPLETE/IOERR.
>>>
>>> - Add extra ASSERT() for btrfs_add_ordered_extent_*()
>>>
>>> - Remove @type parameter for btrfs_add_ordered_extent_compress()
>>> As the only valid @type here is BTRFS_ORDERED_COMPRESSED.
>>>
>>> - Remove the unnecessary special check for IO_DONE/COMPLETE in
>>> __btrfs_add_ordered_extent()
>>> This is just to make the code work, with extra ASSERT(), there are
>>> limited values can be passed in.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>
>> Added to misc-next thanks.
>
> Btw, I see that you added a Reviewed-by tag from me to the patch.
> However I did not give the tag, not because I forgot but because there
> was a small detail in a comment that should be fixed, which was not
> addressed in misc-next.
And that comment update should not mention directIO completely.
Should I resend the patch with proper comment updated?
Thanks,
Qu
>
> Thanks.
>
>>
>>> ---
>>> fs/btrfs/inode.c | 4 ++--
>>> fs/btrfs/ordered-data.c | 18 +++++++++++++-----
>>> fs/btrfs/ordered-data.h | 37 +++++++++++++++++++++++-------------
>>> include/trace/events/btrfs.h | 7 ++++---
>>> 4 files changed, 43 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index ef6cb7b620d0..ea9056cc5559 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -917,7 +917,6 @@ static noinline void submit_compressed_extents(struct async_chunk *async_chunk)
>>> ins.objectid,
>>> async_extent->ram_size,
>>> ins.offset,
>>> - BTRFS_ORDERED_COMPRESSED,
>>> async_extent->compress_type);
>>> if (ret) {
>>> btrfs_drop_extent_cache(inode, async_extent->start,
>>> @@ -1127,7 +1126,8 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
>>> free_extent_map(em);
>>>
>>> ret = btrfs_add_ordered_extent(inode, start, ins.objectid,
>>> - ram_size, cur_alloc_size, 0);
>>> + ram_size, cur_alloc_size,
>>> + BTRFS_ORDERED_REGULAR);
>>> if (ret)
>>> goto out_drop_extent_cache;
>>>
>>> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
>>> index d5d326c674b1..bd7e187d9b16 100644
>>> --- a/fs/btrfs/ordered-data.c
>>> +++ b/fs/btrfs/ordered-data.c
>>> @@ -199,8 +199,11 @@ static int __btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset
>>> entry->compress_type = compress_type;
>>> entry->truncated_len = (u64)-1;
>>> entry->qgroup_rsv = ret;
>>> - if (type != BTRFS_ORDERED_IO_DONE && type != BTRFS_ORDERED_COMPLETE)
>>> - set_bit(type, &entry->flags);
>>> +
>>> + ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
>>> + type == BTRFS_ORDERED_PREALLOC ||
>>> + type == BTRFS_ORDERED_COMPRESSED);
>>
>> I've reformatted that so that all the checks are on separate lines, it's
>> easier to read though it does not fill the whole line.
>>
>>> + set_bit(type, &entry->flags);
>>>
>>> if (dio) {
>>> percpu_counter_add_batch(&fs_info->dio_bytes, num_bytes,
>>> @@ -256,6 +259,8 @@ int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset,
>>> u64 disk_bytenr, u64 num_bytes, u64 disk_num_bytes,
>>> int type)
>>> {
>>> + ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
>>> + type == BTRFS_ORDERED_PREALLOC);
>>> return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
>>> num_bytes, disk_num_bytes, type, 0,
>>> BTRFS_COMPRESS_NONE);
>>> @@ -265,6 +270,8 @@ int btrfs_add_ordered_extent_dio(struct btrfs_inode *inode, u64 file_offset,
>>> u64 disk_bytenr, u64 num_bytes,
>>> u64 disk_num_bytes, int type)
>>> {
>>> + ASSERT(type == BTRFS_ORDERED_REGULAR || type == BTRFS_ORDERED_NOCOW ||
>>> + type == BTRFS_ORDERED_PREALLOC);
>>> return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
>>> num_bytes, disk_num_bytes, type, 1,
>>> BTRFS_COMPRESS_NONE);
>>> @@ -272,11 +279,12 @@ int btrfs_add_ordered_extent_dio(struct btrfs_inode *inode, u64 file_offset,
>>>
>>> int btrfs_add_ordered_extent_compress(struct btrfs_inode *inode, u64 file_offset,
>>> u64 disk_bytenr, u64 num_bytes,
>>> - u64 disk_num_bytes, int type,
>>> - int compress_type)
>>> + u64 disk_num_bytes, int compress_type)
>>> {
>>> + ASSERT(compress_type != BTRFS_COMPRESS_NONE);
>>> return __btrfs_add_ordered_extent(inode, file_offset, disk_bytenr,
>>> - num_bytes, disk_num_bytes, type, 0,
>>> + num_bytes, disk_num_bytes,
>>> + BTRFS_ORDERED_COMPRESSED, 0,
>>> compress_type);
>>> }
>>>
>>> diff --git a/fs/btrfs/ordered-data.h b/fs/btrfs/ordered-data.h
>>> index 46194c2c05d4..151ec6bba405 100644
>>> --- a/fs/btrfs/ordered-data.h
>>> +++ b/fs/btrfs/ordered-data.h
>>> @@ -27,7 +27,7 @@ struct btrfs_ordered_sum {
>>> };
>>>
>>> /*
>>> - * bits for the flags field:
>>> + * Bits for btrfs_ordered_extent::flags.
>>> *
>>> * BTRFS_ORDERED_IO_DONE is set when all of the blocks are written.
>>> * It is used to make sure metadata is inserted into the tree only once
>>> @@ -38,24 +38,36 @@ struct btrfs_ordered_sum {
>>> * IO is done and any metadata is inserted into the tree.
>>> */
>>> enum {
>>> + /*
>>> + * Different types for direct io, one and only one of the 4 type can
>>
>> direct io
>>
>>> + * be set when creating ordered extent.
>>> + *
>>> + * REGULAR: For regular non-compressed COW write
>>> + * NOCOW: For NOCOW write into existing non-hole extent
>>> + * PREALLOC: For NOCOW write into preallocated extent
>>> + * COMPRESSED: For compressed COW write
>>> + */
>>> + BTRFS_ORDERED_REGULAR,
>>> + BTRFS_ORDERED_NOCOW,
>>> + BTRFS_ORDERED_PREALLOC,
>>> + BTRFS_ORDERED_COMPRESSED,
>>> +
>>> + /*
>>> + * Extra bit for DirectIO, can only be set for
>>
>> DirectIO
>>
>>> + * REGULAR/NOCOW/PREALLOC. No DIO for compressed extent.
>>
>> DIO
>>
>> Three different ways to spell that but one is enough. Fixed.
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] btrfs: rework the order of btrfs_ordered_extent::flags
2021-01-25 13:35 ` Qu Wenruo
@ 2021-01-25 16:09 ` David Sterba
0 siblings, 0 replies; 8+ messages in thread
From: David Sterba @ 2021-01-25 16:09 UTC (permalink / raw)
To: Qu Wenruo; +Cc: fdmanana, dsterba, linux-btrfs
On Mon, Jan 25, 2021 at 09:35:22PM +0800, Qu Wenruo wrote:
> >> Added to misc-next thanks.
> >
> > Btw, I see that you added a Reviewed-by tag from me to the patch.
> > However I did not give the tag, not because I forgot but because there
> > was a small detail in a comment that should be fixed, which was not
> > addressed in misc-next.
>
> And that comment update should not mention directIO completely.
>
> Should I resend the patch with proper comment updated?
Yes please.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-01-26 19:32 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-21 6:13 [PATCH] btrfs: rework the order of btrfs_ordered_extent::flags Qu Wenruo
2021-01-21 7:38 ` Nikolay Borisov
2021-01-21 10:32 ` Filipe Manana
2021-01-21 11:07 ` Qu Wenruo
2021-01-21 16:47 ` David Sterba
2021-01-25 12:15 ` Filipe Manana
2021-01-25 13:35 ` Qu Wenruo
2021-01-25 16:09 ` David Sterba
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.