diff mbox series

btrfs: rework the order of btrfs_ordered_extent::flags

Message ID 20210121061354.61271-1-wqu@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs: rework the order of btrfs_ordered_extent::flags | expand

Commit Message

Qu Wenruo Jan. 21, 2021, 6:13 a.m. UTC
[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(-)

Comments

Nikolay Borisov Jan. 21, 2021, 7:38 a.m. UTC | #1
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>
Filipe Manana Jan. 21, 2021, 10:32 a.m. UTC | #2
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
>
Qu Wenruo Jan. 21, 2021, 11:07 a.m. UTC | #3
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
>>
> 
>
David Sterba Jan. 21, 2021, 4:47 p.m. UTC | #4
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.
Filipe Manana Jan. 25, 2021, 12:15 p.m. UTC | #5
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.
Qu Wenruo Jan. 25, 2021, 1:35 p.m. UTC | #6
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.
> 
> 
>
David Sterba Jan. 25, 2021, 4:09 p.m. UTC | #7
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.
diff mbox series

Patch

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"	})