On Thu, Jan 21, 2021 at 4:52 PM David Sterba <[email protected]> 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 <[email protected]>
>
> 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.”
