[PATCH 0/2] btrfs: enhance BTRFS_ORDERED_* flags sanity checks

[PATCH 0/2] btrfs: enhance BTRFS_ORDERED_* flags sanity checks

[Qu Wenruo]

During my development of introduce a new DELAYED type, I incorrectly
called something like:

	oe = alloc_ordered_extent(.., BTRFS_ORDERED_DELAYED, ..);

That doesn't trigger any warning at runtime, but will cause unexpected
bugs due to the fact that, BTRFS_ORDERED_* can not be directly used as a
flag. They are only bit numbers, thus should be utilized with bit
operations like "test_bit(BTRFS_ORDERED_DELAYED, ..)".

My stupid bug inspired me to enhance the @flags sanity checks in
alloc_ordered_extent().

The first one is to make sure that my stupid bug can always be caught
early.

The second one is to enhance the error message when a duplicated OE is
found during insert_ordered_extent().

Qu Wenruo (2):
  btrfs: check type flags in alloc_ordered_extent()
  btrfs: output more info when duplicated ordered extent is found

 fs/btrfs/ordered-data.c | 24 +++++++++++++++---
 fs/btrfs/ordered-data.h | 55 +++++++++++++++++++++++------------------
 2 files changed, 52 insertions(+), 27 deletions(-)

-- 
2.53.0

[PATCH 1/2] btrfs: check type flags in alloc_ordered_extent()

[Qu Wenruo]

Unlike other flags used in btrfs, BTRFS_ORDERED_* macros are different as
they can not be directly used as flags.

They are defined as bit numbers, thus they should be utilized with
bit operations, not directly with logical operations.

Unfortunately sometimes I forgot this and can pass the incorrect flags
to alloc_ordered_extent() and hit weird bugs.

Enhance the type checks in alloc_ordered_extent() by:

- Make sure there is one and only one bit set for exclusive type flags
  There are four exclusive type flags, REGULAR, NOCOW, PREALLOC and
  COMPRESSED.
  So introduce a new macro, BTRFS_ORDERED_EXCLUSIVE_FLAGS, to cover
  above flags.

  And add an ASSERT() to check one and only one of those exclusive flags
  can be set for alloc_ordered_extent().

- Re-order the type bit numbers to the end of the enum
  This is make it much harder to get a valid false negative.

  E.g, with the old BTRFS_ORDERED_REGULAR starts at zero, we can have
  the following flags passing the weight check:

  * BTRFS_ORDERED_NOCOW
    Be treated as BTRFS_ORDERED_REGULAR (1 == 1UL << 0).

  * BTRFS_ORDERED_PREALLOC
    Be treated as BTRFS_ORDERED_NOCOW (2 == 1UL << 1).

  * BTRFS_ORDERED_DIRECT
    Be treated as BTRFS_ORDERED_PREALLOC (4 == 1UL << 2).

  Now all those types starts at 8, passing any of those bit numbers as
  flags directly will not pass the ASSERT().

- Add a static assert to avoid overflow
  To make sure all BTRFS_ORDERED_* flags can fit into an unsigned long.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ordered-data.c | 13 ++++++++++
 fs/btrfs/ordered-data.h | 55 +++++++++++++++++++++++------------------
 2 files changed, 44 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
index 8405d07b49cd..b34a0df282f3 100644
--- a/fs/btrfs/ordered-data.c
+++ b/fs/btrfs/ordered-data.c
@@ -156,6 +156,19 @@ static struct btrfs_ordered_extent *alloc_ordered_extent(
 	const bool is_nocow = (flags &
 	       ((1U << BTRFS_ORDERED_NOCOW) | (1U << BTRFS_ORDERED_PREALLOC)));
 
+	/* Only one type flag can be set. */
+	ASSERT(hweight_long(flags & BTRFS_ORDERED_EXCLUSIVE_FLAGS) == 1);
+
+	/* DIRECT can not be set with COMPRESSED nor ENCODED. */
+	if (test_bit(BTRFS_ORDERED_DIRECT, &flags)) {
+		ASSERT(!test_bit(BTRFS_ORDERED_COMPRESSED, &flags));
+		ASSERT(!test_bit(BTRFS_ORDERED_ENCODED, &flags));
+	}
+
+	/* ENCODED must be set with COMPRESSED. */
+	if (test_bit(BTRFS_ORDERED_ENCODED, &flags))
+		ASSERT(test_bit(BTRFS_ORDERED_COMPRESSED, &flags));
+
 	/*
 	 * For a NOCOW write we can free the qgroup reserve right now. For a COW
 	 * one we transfer the reserved space from the inode's iotree into the
diff --git a/fs/btrfs/ordered-data.h b/fs/btrfs/ordered-data.h
index cd74c5ecfd67..2664ea455229 100644
--- a/fs/btrfs/ordered-data.h
+++ b/fs/btrfs/ordered-data.h
@@ -47,8 +47,25 @@ struct btrfs_ordered_sum {
  * IO is done and any metadata is inserted into the tree.
  */
 enum {
+	/* 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,
+	/* We had an io error when writing this out */
+	BTRFS_ORDERED_IOERR,
+	/* Set when we have to truncate an extent */
+	BTRFS_ORDERED_TRUNCATED,
+	/* Used during fsync to track already logged extents */
+	BTRFS_ORDERED_LOGGED,
+	/* We have already logged all the csums of the ordered extent */
+	BTRFS_ORDERED_LOGGED_CSUM,
+	/* We wait for this extent to complete in the current transaction */
+	BTRFS_ORDERED_PENDING,
+
 	/*
-	 * Different types for ordered extents, one and only one of the 4 types
+	 * Different types for ordered extents, one and only one of those types
 	 * need to be set when creating ordered extent.
 	 *
 	 * REGULAR:	For regular non-compressed COW write
@@ -61,37 +78,27 @@ enum {
 	BTRFS_ORDERED_PREALLOC,
 	BTRFS_ORDERED_COMPRESSED,
 
+	/* Extra bit for encoded write, must be set with COMPRESSED. */
+	BTRFS_ORDERED_ENCODED,
+
 	/*
 	 * Extra bit for direct io, can only be set for
-	 * REGULAR/NOCOW/PREALLOC. No direct io for compressed extent.
+	 * REGULAR/NOCOW/PREALLOC. Can not be set for COMPRESSED nor ENCODED.
 	 */
 	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,
-	/* We had an io error when writing this out */
-	BTRFS_ORDERED_IOERR,
-	/* Set when we have to truncate an extent */
-	BTRFS_ORDERED_TRUNCATED,
-	/* Used during fsync to track already logged extents */
-	BTRFS_ORDERED_LOGGED,
-	/* We have already logged all the csums of the ordered extent */
-	BTRFS_ORDERED_LOGGED_CSUM,
-	/* We wait for this extent to complete in the current transaction */
-	BTRFS_ORDERED_PENDING,
-	/* BTRFS_IOC_ENCODED_WRITE */
-	BTRFS_ORDERED_ENCODED,
+	BTRFS_ORDERED_NR_FLAGS,
 };
+static_assert(BTRFS_ORDERED_NR_FLAGS <= BITS_PER_LONG);
+
+/* One and only one flag can be set. */
+#define BTRFS_ORDERED_EXCLUSIVE_FLAGS ((1UL << BTRFS_ORDERED_REGULAR) |	\
+				       (1UL << BTRFS_ORDERED_NOCOW) |	\
+				       (1UL << BTRFS_ORDERED_PREALLOC) |\
+				       (1UL << BTRFS_ORDERED_COMPRESSED))
 
 /* BTRFS_ORDERED_* flags that specify the type of the extent. */
-#define BTRFS_ORDERED_TYPE_FLAGS ((1UL << BTRFS_ORDERED_REGULAR) |	\
-				  (1UL << BTRFS_ORDERED_NOCOW) |	\
-				  (1UL << BTRFS_ORDERED_PREALLOC) |	\
-				  (1UL << BTRFS_ORDERED_COMPRESSED) |	\
+#define BTRFS_ORDERED_TYPE_FLAGS (BTRFS_ORDERED_EXCLUSIVE_FLAGS |	\
 				  (1UL << BTRFS_ORDERED_DIRECT) |	\
 				  (1UL << BTRFS_ORDERED_ENCODED))
 
-- 
2.53.0

Re: [PATCH 1/2] btrfs: check type flags in alloc_ordered_extent()

[David Sterba]

On Sat, Mar 07, 2026 at 07:43:36PM +1030, Qu Wenruo wrote:
> Unlike other flags used in btrfs, BTRFS_ORDERED_* macros are different as
> they can not be directly used as flags.
> 
> They are defined as bit numbers, thus they should be utilized with
> bit operations, not directly with logical operations.
> 
> Unfortunately sometimes I forgot this and can pass the incorrect flags
> to alloc_ordered_extent() and hit weird bugs.
> 
> Enhance the type checks in alloc_ordered_extent() by:
> 
> - Make sure there is one and only one bit set for exclusive type flags
>   There are four exclusive type flags, REGULAR, NOCOW, PREALLOC and
>   COMPRESSED.
>   So introduce a new macro, BTRFS_ORDERED_EXCLUSIVE_FLAGS, to cover
>   above flags.
> 
>   And add an ASSERT() to check one and only one of those exclusive flags
>   can be set for alloc_ordered_extent().
> 
> - Re-order the type bit numbers to the end of the enum
>   This is make it much harder to get a valid false negative.
> 
>   E.g, with the old BTRFS_ORDERED_REGULAR starts at zero, we can have
>   the following flags passing the weight check:
> 
>   * BTRFS_ORDERED_NOCOW
>     Be treated as BTRFS_ORDERED_REGULAR (1 == 1UL << 0).
> 
>   * BTRFS_ORDERED_PREALLOC
>     Be treated as BTRFS_ORDERED_NOCOW (2 == 1UL << 1).
> 
>   * BTRFS_ORDERED_DIRECT
>     Be treated as BTRFS_ORDERED_PREALLOC (4 == 1UL << 2).
> 
>   Now all those types starts at 8, passing any of those bit numbers as
>   flags directly will not pass the ASSERT().

Reordering the extra bits to the beginning looks a bit strange but the
functional reasons make more sense.

> 
> - Add a static assert to avoid overflow
>   To make sure all BTRFS_ORDERED_* flags can fit into an unsigned long.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/ordered-data.c | 13 ++++++++++
>  fs/btrfs/ordered-data.h | 55 +++++++++++++++++++++++------------------
>  2 files changed, 44 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> index 8405d07b49cd..b34a0df282f3 100644
> --- a/fs/btrfs/ordered-data.c
> +++ b/fs/btrfs/ordered-data.c
> @@ -156,6 +156,19 @@ static struct btrfs_ordered_extent *alloc_ordered_extent(
>  	const bool is_nocow = (flags &
>  	       ((1U << BTRFS_ORDERED_NOCOW) | (1U << BTRFS_ORDERED_PREALLOC)));
>  
> +	/* Only one type flag can be set. */
> +	ASSERT(hweight_long(flags & BTRFS_ORDERED_EXCLUSIVE_FLAGS) == 1);

We have has_single_bit_set() for that

> +
> +	/* DIRECT can not be set with COMPRESSED nor ENCODED. */

                  cannot

> +	if (test_bit(BTRFS_ORDERED_DIRECT, &flags)) {
> +		ASSERT(!test_bit(BTRFS_ORDERED_COMPRESSED, &flags));
> +		ASSERT(!test_bit(BTRFS_ORDERED_ENCODED, &flags));
> +	}
> +
> +	/* ENCODED must be set with COMPRESSED. */
> +	if (test_bit(BTRFS_ORDERED_ENCODED, &flags))
> +		ASSERT(test_bit(BTRFS_ORDERED_COMPRESSED, &flags));
> +
>  	/*
>  	 * For a NOCOW write we can free the qgroup reserve right now. For a COW
>  	 * one we transfer the reserved space from the inode's iotree into the
> diff --git a/fs/btrfs/ordered-data.h b/fs/btrfs/ordered-data.h
> index cd74c5ecfd67..2664ea455229 100644
> --- a/fs/btrfs/ordered-data.h
> +++ b/fs/btrfs/ordered-data.h
> @@ -47,8 +47,25 @@ struct btrfs_ordered_sum {
>   * IO is done and any metadata is inserted into the tree.
>   */
>  enum {
> +	/* 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,
> +	/* We had an io error when writing this out */
> +	BTRFS_ORDERED_IOERR,
> +	/* Set when we have to truncate an extent */
> +	BTRFS_ORDERED_TRUNCATED,
> +	/* Used during fsync to track already logged extents */
> +	BTRFS_ORDERED_LOGGED,
> +	/* We have already logged all the csums of the ordered extent */
> +	BTRFS_ORDERED_LOGGED_CSUM,
> +	/* We wait for this extent to complete in the current transaction */
> +	BTRFS_ORDERED_PENDING,
> +
>  	/*
> -	 * Different types for ordered extents, one and only one of the 4 types
> +	 * Different types for ordered extents, one and only one of those types
>  	 * need to be set when creating ordered extent.
>  	 *
>  	 * REGULAR:	For regular non-compressed COW write
> @@ -61,37 +78,27 @@ enum {
>  	BTRFS_ORDERED_PREALLOC,
>  	BTRFS_ORDERED_COMPRESSED,
>  
> +	/* Extra bit for encoded write, must be set with COMPRESSED. */
> +	BTRFS_ORDERED_ENCODED,
> +
>  	/*
>  	 * Extra bit for direct io, can only be set for
> -	 * REGULAR/NOCOW/PREALLOC. No direct io for compressed extent.
> +	 * REGULAR/NOCOW/PREALLOC. Can not be set for COMPRESSED nor ENCODED.

                                   Cannot

or maybe "Must not ..."

>  	 */
>  	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,
> -	/* We had an io error when writing this out */
> -	BTRFS_ORDERED_IOERR,
> -	/* Set when we have to truncate an extent */
> -	BTRFS_ORDERED_TRUNCATED,
> -	/* Used during fsync to track already logged extents */
> -	BTRFS_ORDERED_LOGGED,
> -	/* We have already logged all the csums of the ordered extent */
> -	BTRFS_ORDERED_LOGGED_CSUM,
> -	/* We wait for this extent to complete in the current transaction */
> -	BTRFS_ORDERED_PENDING,
> -	/* BTRFS_IOC_ENCODED_WRITE */
> -	BTRFS_ORDERED_ENCODED,
> +	BTRFS_ORDERED_NR_FLAGS,
>  };
> +static_assert(BTRFS_ORDERED_NR_FLAGS <= BITS_PER_LONG);
> +
> +/* One and only one flag can be set. */
> +#define BTRFS_ORDERED_EXCLUSIVE_FLAGS ((1UL << BTRFS_ORDERED_REGULAR) |	\
> +				       (1UL << BTRFS_ORDERED_NOCOW) |	\
> +				       (1UL << BTRFS_ORDERED_PREALLOC) |\
> +				       (1UL << BTRFS_ORDERED_COMPRESSED))
>  
>  /* BTRFS_ORDERED_* flags that specify the type of the extent. */
> -#define BTRFS_ORDERED_TYPE_FLAGS ((1UL << BTRFS_ORDERED_REGULAR) |	\
> -				  (1UL << BTRFS_ORDERED_NOCOW) |	\
> -				  (1UL << BTRFS_ORDERED_PREALLOC) |	\
> -				  (1UL << BTRFS_ORDERED_COMPRESSED) |	\
> +#define BTRFS_ORDERED_TYPE_FLAGS (BTRFS_ORDERED_EXCLUSIVE_FLAGS |	\
>  				  (1UL << BTRFS_ORDERED_DIRECT) |	\
>  				  (1UL << BTRFS_ORDERED_ENCODED))
>  
> -- 
> 2.53.0
> 

[PATCH 2/2] btrfs: output more info when duplicated ordered extent is found

[Qu Wenruo]

During development of a new feature, I triggered that btrfs_panic()
inside insert_ordered_extent() and spent quite some unnecessary before
noticing I'm passing incorrect flags when creating a new ordered extent.

Unfortunately the existing error message is not providing much help.

Enhance the output to provide file offset, num bytes and flags of both
existing and new ordered extents.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ordered-data.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
index b34a0df282f3..bc88b904d024 100644
--- a/fs/btrfs/ordered-data.c
+++ b/fs/btrfs/ordered-data.c
@@ -253,10 +253,15 @@ static void insert_ordered_extent(struct btrfs_ordered_extent *entry)
 	spin_lock(&inode->ordered_tree_lock);
 	node = tree_insert(&inode->ordered_tree, entry->file_offset,
 			   &entry->rb_node);
-	if (unlikely(node))
+	if (unlikely(node)) {
+		struct btrfs_ordered_extent *exist =
+			rb_entry(node, struct btrfs_ordered_extent, rb_node);
+
 		btrfs_panic(fs_info, -EEXIST,
-				"inconsistency in ordered tree at offset %llu",
-				entry->file_offset);
+"existing oe file_offset=%llu num_bytes=%llu flags=0x%lx new oe file_offset=%llu num_bytes=%llu flags=0x%lx",
+			    exist->file_offset, exist->num_bytes, exist->flags,
+			    entry->file_offset, entry->num_bytes, entry->flags);
+	}
 	spin_unlock(&inode->ordered_tree_lock);
 
 	spin_lock(&root->ordered_extent_lock);
-- 
2.53.0

Re: [PATCH 2/2] btrfs: output more info when duplicated ordered extent is found

[David Sterba]

On Sat, Mar 07, 2026 at 07:43:37PM +1030, Qu Wenruo wrote:
> During development of a new feature, I triggered that btrfs_panic()
> inside insert_ordered_extent() and spent quite some unnecessary before
> noticing I'm passing incorrect flags when creating a new ordered extent.
> 
> Unfortunately the existing error message is not providing much help.
> 
> Enhance the output to provide file offset, num bytes and flags of both
> existing and new ordered extents.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/ordered-data.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> index b34a0df282f3..bc88b904d024 100644
> --- a/fs/btrfs/ordered-data.c
> +++ b/fs/btrfs/ordered-data.c
> @@ -253,10 +253,15 @@ static void insert_ordered_extent(struct btrfs_ordered_extent *entry)
>  	spin_lock(&inode->ordered_tree_lock);
>  	node = tree_insert(&inode->ordered_tree, entry->file_offset,
>  			   &entry->rb_node);
> -	if (unlikely(node))
> +	if (unlikely(node)) {
> +		struct btrfs_ordered_extent *exist =
> +			rb_entry(node, struct btrfs_ordered_extent, rb_node);
> +
>  		btrfs_panic(fs_info, -EEXIST,
> -				"inconsistency in ordered tree at offset %llu",
> -				entry->file_offset);
> +"existing oe file_offset=%llu num_bytes=%llu flags=0x%lx new oe file_offset=%llu num_bytes=%llu flags=0x%lx",

This uses the "key=value" format while this is an exception in the
messages. The tracepoints use "=" as it's main purpose is to print just
a list of values but messages also contain other text. We should keep it
consistent.

> +			    exist->file_offset, exist->num_bytes, exist->flags,
> +			    entry->file_offset, entry->num_bytes, entry->flags);
> +	}
>  	spin_unlock(&inode->ordered_tree_lock);
>  
>  	spin_lock(&root->ordered_extent_lock);
> -- 
> 2.53.0
> 

Re: [PATCH 2/2] btrfs: output more info when duplicated ordered extent is found

[Qu Wenruo]

在 2026/3/14 06:08, David Sterba 写道:
> On Sat, Mar 07, 2026 at 07:43:37PM +1030, Qu Wenruo wrote:
>> During development of a new feature, I triggered that btrfs_panic()
>> inside insert_ordered_extent() and spent quite some unnecessary before
>> noticing I'm passing incorrect flags when creating a new ordered extent.
>>
>> Unfortunately the existing error message is not providing much help.
>>
>> Enhance the output to provide file offset, num bytes and flags of both
>> existing and new ordered extents.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/ordered-data.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
>> index b34a0df282f3..bc88b904d024 100644
>> --- a/fs/btrfs/ordered-data.c
>> +++ b/fs/btrfs/ordered-data.c
>> @@ -253,10 +253,15 @@ static void insert_ordered_extent(struct btrfs_ordered_extent *entry)
>>   	spin_lock(&inode->ordered_tree_lock);
>>   	node = tree_insert(&inode->ordered_tree, entry->file_offset,
>>   			   &entry->rb_node);
>> -	if (unlikely(node))
>> +	if (unlikely(node)) {
>> +		struct btrfs_ordered_extent *exist =
>> +			rb_entry(node, struct btrfs_ordered_extent, rb_node);
>> +
>>   		btrfs_panic(fs_info, -EEXIST,
>> -				"inconsistency in ordered tree at offset %llu",
>> -				entry->file_offset);
>> +"existing oe file_offset=%llu num_bytes=%llu flags=0x%lx new oe file_offset=%llu num_bytes=%llu flags=0x%lx",
> 
> This uses the "key=value" format while this is an exception in the
> messages. The tracepoints use "=" as it's main purpose is to print just
> a list of values but messages also contain other text. We should keep it
> consistent.

Got it, will go using the existing "key value" style.

Thanks,
Qu

> 
>> +			    exist->file_offset, exist->num_bytes, exist->flags,
>> +			    entry->file_offset, entry->num_bytes, entry->flags);
>> +	}
>>   	spin_unlock(&inode->ordered_tree_lock);
>>   
>>   	spin_lock(&root->ordered_extent_lock);
>> -- 
>> 2.53.0
>>