[PATCH 0/2] bcachefs: fiemap delalloc support and cleanup

[PATCH 0/2] bcachefs: fiemap delalloc support and cleanup

[Brian Foster]

Hi,

Here's a couple patches to add basic fiemap support for delalloc extents
and make a somewhat unrelated cleanup. Patch 1 formats delalloc data
into fiemap info by faking up an extent key for pagecache resident
extents. When reading through some related code to grok how to do that
(I initially used a reservation type key), I happened across
bch2_extent_fallocate(), found the associated logic a bit wonky, and
thus tried to clean it up. Both patches are available in my CI test
branch.

Thoughts, reviews, flames appreciated.

Brian

P.S., I'm off soonish for PTO. If there are any issues to be addressed
with these patches, I'll pick it back up after the New Year.

Brian Foster (2):
  bcachefs: add fiemap delalloc extent detection
  bcachefs: clean up some dead fallocate code

 fs/bcachefs/fs.c      | 60 ++++++++++++++++++++++++++++++++++++++++---
 fs/bcachefs/io_misc.c | 35 +++++++++----------------
 2 files changed, 68 insertions(+), 27 deletions(-)

-- 
2.42.0

[PATCH 1/2] bcachefs: add fiemap delalloc extent detection

[Brian Foster]

bcachefs currently populates fiemap data from the extents btree.
This works correctly when the fiemap sync flag is provided, but if
not, it skips all delalloc extents that have not yet been flushed.
This is because delalloc extents from buffered writes are first
stored as reservation in the pagecache, and only become resident in
the extents btree after writeback completes.

Update the fiemap implementation to scan the pagecache for data for
file ranges that are not present in the extents btree. This uses the
preexisting seek data/hole mechanism to identify data ranges, and
then formats them as delayed allocation extents in the fiemap info.
This is done by faking up an extent key and passing that along to
the fiemap fill handler. We also tweak bch2_fiemap() to save fiemap
flags for the previous key in order to track that it is delalloc.

One caveat worth noting with respect to fiemap and COW is that
extent btree data is reported even when dirty pagecache exists over
the associated range of the file. This means the range is
reallocated on the next writeback and thus fiemap data is
technically out of date. This is not necessarily a serious issue
given fiemap is racy by definition, the final location of the
unflushed data is unknown, and the caller should probably use the
sync flag for most up to date information. FWIW, btrfs exhibits this
same behavior wrt to dirty pagecache over COW extents as well, so
this patch brings bcachefs to functional parity with btrfs.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/bcachefs/fs.c | 60 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 56 insertions(+), 4 deletions(-)

diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
index bc280a0a957d..0b3b35092818 100644
--- a/fs/bcachefs/fs.c
+++ b/fs/bcachefs/fs.c
@@ -868,6 +868,41 @@ static int bch2_fill_extent(struct bch_fs *c,
 	}
 }
 
+/*
+ * Scan a gap in the extent btree for delayed allocation in pagecache. If found,
+ * fake up an extent key so it looks like an extent to the rest of the fiemap
+ * processing code.
+ */
+static bool
+bch2_fiemap_scan_pagecache(struct inode		*vinode,
+			   u64			start,
+			   u64			end,
+			   struct bkey_buf	*cur)
+{
+	struct bch_fs		*c = vinode->i_sb->s_fs_info;
+	struct bch_inode_info	*ei = to_bch_ei(vinode);
+	struct bkey_i_extent	*delextent;
+	struct bch_extent_ptr	ptr = {};
+
+	start = bch2_seek_pagecache_data(vinode, start, end, 0, false);
+	if (start >= end)
+		return false;
+	end = bch2_seek_pagecache_hole(vinode, start, end, 0, false);
+
+	/*
+	 * Create a fake extent key in the buffer. We have to add a dummy extent
+	 * pointer for the fill code to add an extent entry. It's explicitly
+	 * zeroed to reflect delayed allocation (i.e. phys offset 0).
+	 */
+	bch2_bkey_buf_realloc(cur, c, sizeof(*delextent) / sizeof(u64));
+	delextent = bkey_extent_init(cur->k);
+	delextent->k.p = POS(ei->v.i_ino, start >> 9);
+	bch2_key_resize(&delextent->k, (end - start) >> 9);
+	bch2_bkey_append_ptr(&delextent->k_i, ptr);
+
+	return true;
+}
+
 static int bch2_fiemap(struct inode *vinode, struct fiemap_extent_info *info,
 		       u64 start, u64 len)
 {
@@ -879,6 +914,7 @@ static int bch2_fiemap(struct inode *vinode, struct fiemap_extent_info *info,
 	struct bkey_buf cur, prev;
 	struct bpos end = POS(ei->v.i_ino, (start + len) >> 9);
 	unsigned offset_into_extent, sectors;
+	unsigned cflags, pflags;
 	bool have_extent = false;
 	u32 snapshot;
 	int ret = 0;
@@ -916,6 +952,19 @@ static int bch2_fiemap(struct inode *vinode, struct fiemap_extent_info *info,
 			continue;
 		}
 
+		/*
+		 * Outstanding buffered writes aren't tracked in the extent
+		 * btree until dirty folios are written back. Check holes in the
+		 * extent tree for data in pagecache and report it as delalloc.
+		 */
+		if (iter.pos.offset > start &&
+		    bch2_fiemap_scan_pagecache(vinode, start << 9,
+					       iter.pos.offset << 9, &cur)) {
+			cflags = FIEMAP_EXTENT_DELALLOC;
+			start = bkey_start_offset(&cur.k->k) + cur.k->k.size;
+			goto fill;
+		}
+
 		offset_into_extent	= iter.pos.offset -
 			bkey_start_offset(k.k);
 		sectors			= k.k->size - offset_into_extent;
@@ -940,19 +989,22 @@ static int bch2_fiemap(struct inode *vinode, struct fiemap_extent_info *info,
 		cur.k->k.p = iter.pos;
 		cur.k->k.p.offset += cur.k->k.size;
 
+		cflags = 0;
+		start = iter.pos.offset + sectors;
+fill:
 		if (have_extent) {
 			bch2_trans_unlock(trans);
 			ret = bch2_fill_extent(c, info,
-					bkey_i_to_s_c(prev.k), 0);
+					bkey_i_to_s_c(prev.k), pflags);
 			if (ret)
 				break;
 		}
 
 		bkey_copy(prev.k, cur.k);
+		pflags = cflags;
 		have_extent = true;
 
-		bch2_btree_iter_set_pos(&iter,
-			POS(iter.pos.inode, iter.pos.offset + sectors));
+		bch2_btree_iter_set_pos(&iter, POS(iter.pos.inode, start));
 	}
 	start = iter.pos.offset;
 	bch2_trans_iter_exit(trans, &iter);
@@ -963,7 +1015,7 @@ static int bch2_fiemap(struct inode *vinode, struct fiemap_extent_info *info,
 	if (!ret && have_extent) {
 		bch2_trans_unlock(trans);
 		ret = bch2_fill_extent(c, info, bkey_i_to_s_c(prev.k),
-				       FIEMAP_EXTENT_LAST);
+				       pflags|FIEMAP_EXTENT_LAST);
 	}
 
 	bch2_trans_put(trans);
-- 
2.42.0

[PATCH 2/2] bcachefs: clean up some dead fallocate code

[Brian Foster]

The have_reservation local variable in bch2_extent_fallocate() is
initialized to false and set to true further down in the function.
Between this two points, one branch of code checks for negative
value and one for positive, and nothing ever checks the variable
after it is set to true. Clean up some of the unnecessary logic and
code.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/bcachefs/io_misc.c | 35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

diff --git a/fs/bcachefs/io_misc.c b/fs/bcachefs/io_misc.c
index eab0c8c57785..521c32554dd0 100644
--- a/fs/bcachefs/io_misc.c
+++ b/fs/bcachefs/io_misc.c
@@ -34,8 +34,7 @@ int bch2_extent_fallocate(struct btree_trans *trans,
 	struct open_buckets open_buckets = { 0 };
 	struct bkey_s_c k;
 	struct bkey_buf old, new;
-	unsigned sectors_allocated = 0;
-	bool have_reservation = false;
+	unsigned sectors_allocated = 0, new_replicas;
 	bool unwritten = opts.nocow &&
 	    c->sb.version >= bcachefs_metadata_version_unwritten_extents;
 	int ret;
@@ -50,28 +49,20 @@ int bch2_extent_fallocate(struct btree_trans *trans,
 		return ret;
 
 	sectors = min_t(u64, sectors, k.k->p.offset - iter->pos.offset);
+	new_replicas = max(0, (int) opts.data_replicas -
+			   (int) bch2_bkey_nr_ptrs_fully_allocated(k));
 
-	if (!have_reservation) {
-		unsigned new_replicas =
-			max(0, (int) opts.data_replicas -
-			    (int) bch2_bkey_nr_ptrs_fully_allocated(k));
-		/*
-		 * Get a disk reservation before (in the nocow case) calling
-		 * into the allocator:
-		 */
-		ret = bch2_disk_reservation_get(c, &disk_res, sectors, new_replicas, 0);
-		if (unlikely(ret))
-			goto err;
-
-		bch2_bkey_buf_reassemble(&old, c, k);
-	}
+	/*
+	 * Get a disk reservation before (in the nocow case) calling
+	 * into the allocator:
+	 */
+	ret = bch2_disk_reservation_get(c, &disk_res, sectors, new_replicas, 0);
+	if (unlikely(ret))
+		goto err;
 
-	if (have_reservation) {
-		if (!bch2_extents_match(k, bkey_i_to_s_c(old.k)))
-			goto err;
+	bch2_bkey_buf_reassemble(&old, c, k);
 
-		bch2_key_resize(&new.k->k, sectors);
-	} else if (!unwritten) {
+	if (!unwritten) {
 		struct bkey_i_reservation *reservation;
 
 		bch2_bkey_buf_realloc(&new, c, sizeof(*reservation) / sizeof(u64));
@@ -118,8 +109,6 @@ int bch2_extent_fallocate(struct btree_trans *trans,
 			ptr->unwritten = true;
 	}
 
-	have_reservation = true;
-
 	ret = bch2_extent_update(trans, inum, iter, new.k, &disk_res,
 				 0, i_sectors_delta, true);
 err:
-- 
2.42.0

Re: [PATCH 1/2] bcachefs: add fiemap delalloc extent detection

[Kent Overstreet]

On Tue, Dec 19, 2023 at 09:02:14AM -0500, Brian Foster wrote:
> bcachefs currently populates fiemap data from the extents btree.
> This works correctly when the fiemap sync flag is provided, but if
> not, it skips all delalloc extents that have not yet been flushed.
> This is because delalloc extents from buffered writes are first
> stored as reservation in the pagecache, and only become resident in
> the extents btree after writeback completes.
> 
> Update the fiemap implementation to scan the pagecache for data for
> file ranges that are not present in the extents btree. This uses the
> preexisting seek data/hole mechanism to identify data ranges, and
> then formats them as delayed allocation extents in the fiemap info.
> This is done by faking up an extent key and passing that along to
> the fiemap fill handler. We also tweak bch2_fiemap() to save fiemap
> flags for the previous key in order to track that it is delalloc.
> 
> One caveat worth noting with respect to fiemap and COW is that
> extent btree data is reported even when dirty pagecache exists over
> the associated range of the file. This means the range is
> reallocated on the next writeback and thus fiemap data is
> technically out of date. This is not necessarily a serious issue
> given fiemap is racy by definition, the final location of the
> unflushed data is unknown, and the caller should probably use the
> sync flag for most up to date information. FWIW, btrfs exhibits this
> same behavior wrt to dirty pagecache over COW extents as well, so
> this patch brings bcachefs to functional parity with btrfs.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/bcachefs/fs.c | 60 ++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
> index bc280a0a957d..0b3b35092818 100644
> --- a/fs/bcachefs/fs.c
> +++ b/fs/bcachefs/fs.c
> @@ -868,6 +868,41 @@ static int bch2_fill_extent(struct bch_fs *c,
>  	}
>  }
>  
> +/*
> + * Scan a gap in the extent btree for delayed allocation in pagecache. If found,
> + * fake up an extent key so it looks like an extent to the rest of the fiemap
> + * processing code.
> + */
> +static bool
> +bch2_fiemap_scan_pagecache(struct inode		*vinode,
> +			   u64			start,
> +			   u64			end,
> +			   struct bkey_buf	*cur)
> +{
> +	struct bch_fs		*c = vinode->i_sb->s_fs_info;
> +	struct bch_inode_info	*ei = to_bch_ei(vinode);
> +	struct bkey_i_extent	*delextent;
> +	struct bch_extent_ptr	ptr = {};

funny, for some reason I always thought I had to initialize at least one
member to 0 to do this - I'll remember that

> +
> +	start = bch2_seek_pagecache_data(vinode, start, end, 0, false);
> +	if (start >= end)
> +		return false;
> +	end = bch2_seek_pagecache_hole(vinode, start, end, 0, false);
> +
> +	/*
> +	 * Create a fake extent key in the buffer. We have to add a dummy extent
> +	 * pointer for the fill code to add an extent entry. It's explicitly
> +	 * zeroed to reflect delayed allocation (i.e. phys offset 0).
> +	 */
> +	bch2_bkey_buf_realloc(cur, c, sizeof(*delextent) / sizeof(u64));
> +	delextent = bkey_extent_init(cur->k);
> +	delextent->k.p = POS(ei->v.i_ino, start >> 9);
> +	bch2_key_resize(&delextent->k, (end - start) >> 9);
> +	bch2_bkey_append_ptr(&delextent->k_i, ptr);
> +
> +	return true;
> +}

I don't like that this returns a delalloc extent when data is merely
present in the pagecache - that's wrong.

bch2_seek_pagecache_dirty_data()?

> +
>  static int bch2_fiemap(struct inode *vinode, struct fiemap_extent_info *info,
>  		       u64 start, u64 len)
>  {
> @@ -879,6 +914,7 @@ static int bch2_fiemap(struct inode *vinode, struct fiemap_extent_info *info,
>  	struct bkey_buf cur, prev;
>  	struct bpos end = POS(ei->v.i_ino, (start + len) >> 9);
>  	unsigned offset_into_extent, sectors;
> +	unsigned cflags, pflags;

Now that we're not compiling in gnu89 mode anymore, I'm moving code away
from this "declare all variables at the top" style - I want variables
declared where they're initialized.

The reason is that bugs of the "I accidently used a var before I
initialized it" are really common, and the compiler does a crap job of
catching them.

Anywhere where we can write our code in a more functional style (in the
SSA sense, variables are declared where they are initialized and never
mutated) without it affecting the quality of the output is a big win.

(across function calls, we can't generally do this because C doesn't
have return value optimization).


>  	bool have_extent = false;
>  	u32 snapshot;
>  	int ret = 0;
> @@ -916,6 +952,19 @@ static int bch2_fiemap(struct inode *vinode, struct fiemap_extent_info *info,
>  			continue;
>  		}
>  
> +		/*
> +		 * Outstanding buffered writes aren't tracked in the extent
> +		 * btree until dirty folios are written back. Check holes in the
> +		 * extent tree for data in pagecache and report it as delalloc.
> +		 */
> +		if (iter.pos.offset > start &&
> +		    bch2_fiemap_scan_pagecache(vinode, start << 9,
> +					       iter.pos.offset << 9, &cur)) {
> +			cflags = FIEMAP_EXTENT_DELALLOC;

but cflags/plags are unnecessary, no? can't we just detected this in
bch2_fill_extent when the ptr offset is 0?

> +			start = bkey_start_offset(&cur.k->k) + cur.k->k.size;
> +			goto fill;
> +		}

The goto fill is also a code smell.

The right way to do it would be to search, from the same position, both
the btree and the pagecache: then compare those two extents, using
delalloc extent if it comes first, and trimming the btree extent if it
comes first but the delalloc extent overlaps.

Then there's no goto fill; after we've decided which extent to use the
rest of the loop is unchanged.

(The btree iterator code works roughly this way, where it has to overlay
keys from the journal and pending updates).

I would also think about changing this code to use two iterators, since
bch2_btree_iter_peek() will advance the iterator to the key it returns,
and we do not want to advance it yet if we found a delalloc extent at a
prior position - we'd clone the iterator, and peek that.

Right now the code is effectively using start as the first iterator,
which makes sense given that it probably is cleaner to just reinit the
iterator on transaction restart (since snapshot ID may have changed). So
maybe that shouldn't change, just something to consider. Since we're
using set_pos(.., start) at the end of the loop instead of advance -
it's probably fine.

> +
>  		offset_into_extent	= iter.pos.offset -
>  			bkey_start_offset(k.k);
>  		sectors			= k.k->size - offset_into_extent;
> @@ -940,19 +989,22 @@ static int bch2_fiemap(struct inode *vinode, struct fiemap_extent_info *info,
>  		cur.k->k.p = iter.pos;
>  		cur.k->k.p.offset += cur.k->k.size;
>  
> +		cflags = 0;
> +		start = iter.pos.offset + sectors;
> +fill:
>  		if (have_extent) {
>  			bch2_trans_unlock(trans);
>  			ret = bch2_fill_extent(c, info,
> -					bkey_i_to_s_c(prev.k), 0);
> +					bkey_i_to_s_c(prev.k), pflags);
>  			if (ret)
>  				break;
>  		}
>  
>  		bkey_copy(prev.k, cur.k);
> +		pflags = cflags;
>  		have_extent = true;
>  
> -		bch2_btree_iter_set_pos(&iter,
> -			POS(iter.pos.inode, iter.pos.offset + sectors));
> +		bch2_btree_iter_set_pos(&iter, POS(iter.pos.inode, start));
>  	}
>  	start = iter.pos.offset;
>  	bch2_trans_iter_exit(trans, &iter);
> @@ -963,7 +1015,7 @@ static int bch2_fiemap(struct inode *vinode, struct fiemap_extent_info *info,
>  	if (!ret && have_extent) {
>  		bch2_trans_unlock(trans);
>  		ret = bch2_fill_extent(c, info, bkey_i_to_s_c(prev.k),
> -				       FIEMAP_EXTENT_LAST);
> +				       pflags|FIEMAP_EXTENT_LAST);
>  	}
>  
>  	bch2_trans_put(trans);
> -- 
> 2.42.0
> 

Re: [PATCH 2/2] bcachefs: clean up some dead fallocate code

[Kent Overstreet]

On Tue, Dec 19, 2023 at 09:02:15AM -0500, Brian Foster wrote:
> The have_reservation local variable in bch2_extent_fallocate() is
> initialized to false and set to true further down in the function.
> Between this two points, one branch of code checks for negative
> value and one for positive, and nothing ever checks the variable
> after it is set to true. Clean up some of the unnecessary logic and
> code.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

This appears to have just been a leftover from when this code was
extracted from the loop in fs-io.c - applied.

Re: [PATCH 1/2] bcachefs: add fiemap delalloc extent detection

[Brian Foster]

On Tue, Dec 19, 2023 at 06:57:42PM -0500, Kent Overstreet wrote:
> On Tue, Dec 19, 2023 at 09:02:14AM -0500, Brian Foster wrote:
> > bcachefs currently populates fiemap data from the extents btree.
> > This works correctly when the fiemap sync flag is provided, but if
> > not, it skips all delalloc extents that have not yet been flushed.
> > This is because delalloc extents from buffered writes are first
> > stored as reservation in the pagecache, and only become resident in
> > the extents btree after writeback completes.
> > 
> > Update the fiemap implementation to scan the pagecache for data for
> > file ranges that are not present in the extents btree. This uses the
> > preexisting seek data/hole mechanism to identify data ranges, and
> > then formats them as delayed allocation extents in the fiemap info.
> > This is done by faking up an extent key and passing that along to
> > the fiemap fill handler. We also tweak bch2_fiemap() to save fiemap
> > flags for the previous key in order to track that it is delalloc.
> > 
> > One caveat worth noting with respect to fiemap and COW is that
> > extent btree data is reported even when dirty pagecache exists over
> > the associated range of the file. This means the range is
> > reallocated on the next writeback and thus fiemap data is
> > technically out of date. This is not necessarily a serious issue
> > given fiemap is racy by definition, the final location of the
> > unflushed data is unknown, and the caller should probably use the
> > sync flag for most up to date information. FWIW, btrfs exhibits this
> > same behavior wrt to dirty pagecache over COW extents as well, so
> > this patch brings bcachefs to functional parity with btrfs.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/bcachefs/fs.c | 60 ++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 56 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
> > index bc280a0a957d..0b3b35092818 100644
> > --- a/fs/bcachefs/fs.c
> > +++ b/fs/bcachefs/fs.c
> > @@ -868,6 +868,41 @@ static int bch2_fill_extent(struct bch_fs *c,
> >  	}
> >  }
> >  
> > +/*
> > + * Scan a gap in the extent btree for delayed allocation in pagecache. If found,
> > + * fake up an extent key so it looks like an extent to the rest of the fiemap
> > + * processing code.
> > + */
> > +static bool
> > +bch2_fiemap_scan_pagecache(struct inode		*vinode,
> > +			   u64			start,
> > +			   u64			end,
> > +			   struct bkey_buf	*cur)
> > +{
> > +	struct bch_fs		*c = vinode->i_sb->s_fs_info;
> > +	struct bch_inode_info	*ei = to_bch_ei(vinode);
> > +	struct bkey_i_extent	*delextent;
> > +	struct bch_extent_ptr	ptr = {};
> 
> funny, for some reason I always thought I had to initialize at least one
> member to 0 to do this - I'll remember that
> 

Same, then I think someone told me to do this at one point when I did
the "init one variable thing," and then inevitably somebody else will
declare this as wrong and to do the other thing...

> > +
> > +	start = bch2_seek_pagecache_data(vinode, start, end, 0, false);
> > +	if (start >= end)
> > +		return false;
> > +	end = bch2_seek_pagecache_hole(vinode, start, end, 0, false);
> > +
> > +	/*
> > +	 * Create a fake extent key in the buffer. We have to add a dummy extent
> > +	 * pointer for the fill code to add an extent entry. It's explicitly
> > +	 * zeroed to reflect delayed allocation (i.e. phys offset 0).
> > +	 */
> > +	bch2_bkey_buf_realloc(cur, c, sizeof(*delextent) / sizeof(u64));
> > +	delextent = bkey_extent_init(cur->k);
> > +	delextent->k.p = POS(ei->v.i_ino, start >> 9);
> > +	bch2_key_resize(&delextent->k, (end - start) >> 9);
> > +	bch2_bkey_append_ptr(&delextent->k_i, ptr);
> > +
> > +	return true;
> > +}
> 
> I don't like that this returns a delalloc extent when data is merely
> present in the pagecache - that's wrong.
> 
> bch2_seek_pagecache_dirty_data()?
> 

Technically it just returns a file offset range represented in an extent
key. I could tweak it to just return a range or something and make the
delalloc part a separate helper.

More to the broader point, I thought that technically this was all
somewhat racy wrt to folio state and I was a little concerned about
possibly showing weird/inconsistent state if this runs concurrently with
writeback. I'll have to take a closer look at the bch2_seek_* helpers
though to better quantify that concern...

> > +
> >  static int bch2_fiemap(struct inode *vinode, struct fiemap_extent_info *info,
> >  		       u64 start, u64 len)
> >  {
> > @@ -879,6 +914,7 @@ static int bch2_fiemap(struct inode *vinode, struct fiemap_extent_info *info,
> >  	struct bkey_buf cur, prev;
> >  	struct bpos end = POS(ei->v.i_ino, (start + len) >> 9);
> >  	unsigned offset_into_extent, sectors;
> > +	unsigned cflags, pflags;
> 
> Now that we're not compiling in gnu89 mode anymore, I'm moving code away
> from this "declare all variables at the top" style - I want variables
> declared where they're initialized.
> 
> The reason is that bugs of the "I accidently used a var before I
> initialized it" are really common, and the compiler does a crap job of
> catching them.
> 
> Anywhere where we can write our code in a more functional style (in the
> SSA sense, variables are declared where they are initialized and never
> mutated) without it affecting the quality of the output is a big win.
> 
> (across function calls, we can't generally do this because C doesn't
> have return value optimization).
> 

Hmm.. not really sure I want to get into this. My initial reaction is
that this strikes me as odd formatting that makes the code incrementally
more annoying to read for marginal benefit. Has there been any broader
discussion on this sort of thing anywhere that I can catch up on?

> 
> >  	bool have_extent = false;
> >  	u32 snapshot;
> >  	int ret = 0;
> > @@ -916,6 +952,19 @@ static int bch2_fiemap(struct inode *vinode, struct fiemap_extent_info *info,
> >  			continue;
> >  		}
> >  
> > +		/*
> > +		 * Outstanding buffered writes aren't tracked in the extent
> > +		 * btree until dirty folios are written back. Check holes in the
> > +		 * extent tree for data in pagecache and report it as delalloc.
> > +		 */
> > +		if (iter.pos.offset > start &&
> > +		    bch2_fiemap_scan_pagecache(vinode, start << 9,
> > +					       iter.pos.offset << 9, &cur)) {
> > +			cflags = FIEMAP_EXTENT_DELALLOC;
> 
> but cflags/plags are unnecessary, no? can't we just detected this in
> bch2_fill_extent when the ptr offset is 0?
> 

I thought (briefly) about if/how this state could be implied by the
helper (moreso out of laziness ;), but I feel that sort of logic is more
fragile than just being explicit about state.

> > +			start = bkey_start_offset(&cur.k->k) + cur.k->k.size;
> > +			goto fill;
> > +		}
> 
> The goto fill is also a code smell.
> 

It's just basically a goto next pattern. I think the logical wart is
more how this post-processes the gaps/hoes in the extent offset ranges
and set_pos' the same start value repeatedly as we process delalloc
extents. Not really sure if that's what you mean here, but that's the
part that I didn't really love when hacking on this.

> The right way to do it would be to search, from the same position, both
> the btree and the pagecache: then compare those two extents, using
> delalloc extent if it comes first, and trimming the btree extent if it
> comes first but the delalloc extent overlaps.
> 
> Then there's no goto fill; after we've decided which extent to use the
> rest of the loop is unchanged.
> 
> (The btree iterator code works roughly this way, where it has to overlay
> keys from the journal and pending updates).
> 

Yeah, I thought that something like this would be necessary for changing
behavior of the "dirty data over existing COW blocks" case, if we ever
wanted to do that, but figured it wasn't worth the complexity unless we
go that direction. It might be improvement enough for the iteration
either way though. I'll try to take a fresh look at that. Thanks for the
review.

Brian

> I would also think about changing this code to use two iterators, since
> bch2_btree_iter_peek() will advance the iterator to the key it returns,
> and we do not want to advance it yet if we found a delalloc extent at a
> prior position - we'd clone the iterator, and peek that.
> 
> Right now the code is effectively using start as the first iterator,
> which makes sense given that it probably is cleaner to just reinit the
> iterator on transaction restart (since snapshot ID may have changed). So
> maybe that shouldn't change, just something to consider. Since we're
> using set_pos(.., start) at the end of the loop instead of advance -
> it's probably fine.
> 
> > +
> >  		offset_into_extent	= iter.pos.offset -
> >  			bkey_start_offset(k.k);
> >  		sectors			= k.k->size - offset_into_extent;
> > @@ -940,19 +989,22 @@ static int bch2_fiemap(struct inode *vinode, struct fiemap_extent_info *info,
> >  		cur.k->k.p = iter.pos;
> >  		cur.k->k.p.offset += cur.k->k.size;
> >  
> > +		cflags = 0;
> > +		start = iter.pos.offset + sectors;
> > +fill:
> >  		if (have_extent) {
> >  			bch2_trans_unlock(trans);
> >  			ret = bch2_fill_extent(c, info,
> > -					bkey_i_to_s_c(prev.k), 0);
> > +					bkey_i_to_s_c(prev.k), pflags);
> >  			if (ret)
> >  				break;
> >  		}
> >  
> >  		bkey_copy(prev.k, cur.k);
> > +		pflags = cflags;
> >  		have_extent = true;
> >  
> > -		bch2_btree_iter_set_pos(&iter,
> > -			POS(iter.pos.inode, iter.pos.offset + sectors));
> > +		bch2_btree_iter_set_pos(&iter, POS(iter.pos.inode, start));
> >  	}
> >  	start = iter.pos.offset;
> >  	bch2_trans_iter_exit(trans, &iter);
> > @@ -963,7 +1015,7 @@ static int bch2_fiemap(struct inode *vinode, struct fiemap_extent_info *info,
> >  	if (!ret && have_extent) {
> >  		bch2_trans_unlock(trans);
> >  		ret = bch2_fill_extent(c, info, bkey_i_to_s_c(prev.k),
> > -				       FIEMAP_EXTENT_LAST);
> > +				       pflags|FIEMAP_EXTENT_LAST);
> >  	}
> >  
> >  	bch2_trans_put(trans);
> > -- 
> > 2.42.0
> > 
> 

Re: [PATCH 1/2] bcachefs: add fiemap delalloc extent detection

[Kent Overstreet]

+cc btrfs folks

On Thu, Jan 04, 2024 at 07:12:47AM -0500, Brian Foster wrote:
> On Tue, Dec 19, 2023 at 06:57:42PM -0500, Kent Overstreet wrote:
> > On Tue, Dec 19, 2023 at 09:02:14AM -0500, Brian Foster wrote:
> > > +	start = bch2_seek_pagecache_data(vinode, start, end, 0, false);
> > > +	if (start >= end)
> > > +		return false;
> > > +	end = bch2_seek_pagecache_hole(vinode, start, end, 0, false);
> > > +
> > > +	/*
> > > +	 * Create a fake extent key in the buffer. We have to add a dummy extent
> > > +	 * pointer for the fill code to add an extent entry. It's explicitly
> > > +	 * zeroed to reflect delayed allocation (i.e. phys offset 0).
> > > +	 */
> > > +	bch2_bkey_buf_realloc(cur, c, sizeof(*delextent) / sizeof(u64));
> > > +	delextent = bkey_extent_init(cur->k);
> > > +	delextent->k.p = POS(ei->v.i_ino, start >> 9);
> > > +	bch2_key_resize(&delextent->k, (end - start) >> 9);
> > > +	bch2_bkey_append_ptr(&delextent->k_i, ptr);
> > > +
> > > +	return true;
> > > +}
> > 
> > I don't like that this returns a delalloc extent when data is merely
> > present in the pagecache - that's wrong.
> > 
> > bch2_seek_pagecache_dirty_data()?
> > 
> 
> Technically it just returns a file offset range represented in an extent
> key. I could tweak it to just return a range or something and make the
> delalloc part a separate helper.
> 
> More to the broader point, I thought that technically this was all
> somewhat racy wrt to folio state and I was a little concerned about
> possibly showing weird/inconsistent state if this runs concurrently with
> writeback. I'll have to take a closer look at the bch2_seek_* helpers
> though to better quantify that concern...

Yeah, it does get tricky there; writeback will flip the folio state from
dirty to allocated before it shows up in the btree.

To fix that we'd need to add a new state to BCH_FOLIO_SECTOR_STATE, and
it seems like that's something we should strongly consider.

There may also be some lingering bugginess in the i_sectors accounting
code, if we're touching this stuff we definitely want to be looking for
ways to make that more rigoroous if we can - I thought I was done with
that, but I have an assert patch in my garbage branch that was firing -
hadn't had time to track it down:
https://evilpiepirate.org/git/bcachefs.git/commit/?h=bcachefs-garbage&id=50010457597c6c30aec1195b0568868d2d30bb76

This is likely related to the warning that's been popping in the CI from
time to time about i_sectors_delta being the wrong sign in writeback
completion.

Not the main focus here, just wanted to bring it up in case you start to
go down that rabbit hole


> > Now that we're not compiling in gnu89 mode anymore, I'm moving code away
> > from this "declare all variables at the top" style - I want variables
> > declared where they're initialized.
> > 
> > The reason is that bugs of the "I accidently used a var before I
> > initialized it" are really common, and the compiler does a crap job of
> > catching them.
> > 
> > Anywhere where we can write our code in a more functional style (in the
> > SSA sense, variables are declared where they are initialized and never
> > mutated) without it affecting the quality of the output is a big win.
> > 
> > (across function calls, we can't generally do this because C doesn't
> > have return value optimization).
> > 
> 
> Hmm.. not really sure I want to get into this. My initial reaction is
> that this strikes me as odd formatting that makes the code incrementally
> more annoying to read for marginal benefit. Has there been any broader
> discussion on this sort of thing anywhere that I can catch up on?

Starting that discussion now :)

Actually, this did come up not too long ago; there was a bug in some usb
code where they were using the loop iterator after breaking out of the
loop - which should have been ok, but for some horribly subtly reasons
was not, and Linus brought up that it's probably time to switch to
declaring the loop iterator in the for () statement to make those sorts
of bugs impossible.

More broadly, I regularly come across subtle bugs that should not have
happened purely because of variable reuse; consider the way we declare
int ret; at the top of a lot of functions, and then reuse it throughout.

That's error prone for a whole bunch of reasons, and the bugs it causes
tends to be ones that get introduced in later refactoring - some code
skips initializing ret where it's declared because _at the time that
code was written_ it was immediately assigned to, or code will use it
for something that wasn't an error code and then forget to set it back
to 0.

It's really much better to only declare variables when they're first
assigned to, and only use them for one thing.

You'll notice I've got a whole bunch of patches queued up for 6.8 that
change almost all of our for loop macros to declaring the loop iter in
the macro, and I will be going through and changing other variable uses
to this style as it comes up.

> > >  	bool have_extent = false;
> > >  	u32 snapshot;
> > >  	int ret = 0;
> > > @@ -916,6 +952,19 @@ static int bch2_fiemap(struct inode *vinode, struct fiemap_extent_info *info,
> > >  			continue;
> > >  		}
> > >  
> > > +		/*
> > > +		 * Outstanding buffered writes aren't tracked in the extent
> > > +		 * btree until dirty folios are written back. Check holes in the
> > > +		 * extent tree for data in pagecache and report it as delalloc.
> > > +		 */
> > > +		if (iter.pos.offset > start &&
> > > +		    bch2_fiemap_scan_pagecache(vinode, start << 9,
> > > +					       iter.pos.offset << 9, &cur)) {
> > > +			cflags = FIEMAP_EXTENT_DELALLOC;
> > 
> > but cflags/plags are unnecessary, no? can't we just detected this in
> > bch2_fill_extent when the ptr offset is 0?
> > 
> 
> I thought (briefly) about if/how this state could be implied by the
> helper (moreso out of laziness ;), but I feel that sort of logic is more
> fragile than just being explicit about state.

The logic can be confined to one place - a helper with a good name -
while the way you're doing it you're now spreading the state that's
passed around across multiple variables.

The clean way to do what you're doing would be to have a wrapper type so
we could store the bkey_buf with the fiemap flags in one variable.

> 
> > > +			start = bkey_start_offset(&cur.k->k) + cur.k->k.size;
> > > +			goto fill;
> > > +		}
> > 
> > The goto fill is also a code smell.
> > 
> 
> It's just basically a goto next pattern. I think the logical wart is
> more how this post-processes the gaps/hoes in the extent offset ranges
> and set_pos' the same start value repeatedly as we process delalloc
> extents. Not really sure if that's what you mean here, but that's the
> part that I didn't really love when hacking on this.
> 
> > The right way to do it would be to search, from the same position, both
> > the btree and the pagecache: then compare those two extents, using
> > delalloc extent if it comes first, and trimming the btree extent if it
> > comes first but the delalloc extent overlaps.
> > 
> > Then there's no goto fill; after we've decided which extent to use the
> > rest of the loop is unchanged.
> > 
> > (The btree iterator code works roughly this way, where it has to overlay
> > keys from the journal and pending updates).
> > 
> 
> Yeah, I thought that something like this would be necessary for changing
> behavior of the "dirty data over existing COW blocks" case, if we ever
> wanted to do that, but figured it wasn't worth the complexity unless we
> go that direction. It might be improvement enough for the iteration
> either way though. I'll try to take a fresh look at that. Thanks for the
> review.

btrfs folks have probably looked at this as well - any notes from you
guys on fiemap behaviour - has anything come up where a particular
behaviour is important?

Also, someone (Darrick?) mentioned in the Tuesday meeting that maybe we
actually can extend fiemap for multiple device filesystems, wonder if
anyone's interested in that...

Re: [PATCH 1/2] bcachefs: add fiemap delalloc extent detection

[Brian Foster]

On Thu, Jan 04, 2024 at 06:41:26PM -0500, Kent Overstreet wrote:
> +cc btrfs folks
> 

-cc btrfs folks ;) (for unrelated discussion mostly around the variable
declaration thing...)

> On Thu, Jan 04, 2024 at 07:12:47AM -0500, Brian Foster wrote:
> > On Tue, Dec 19, 2023 at 06:57:42PM -0500, Kent Overstreet wrote:
> > > On Tue, Dec 19, 2023 at 09:02:14AM -0500, Brian Foster wrote:
> > > > +	start = bch2_seek_pagecache_data(vinode, start, end, 0, false);
> > > > +	if (start >= end)
> > > > +		return false;
> > > > +	end = bch2_seek_pagecache_hole(vinode, start, end, 0, false);
> > > > +
> > > > +	/*
> > > > +	 * Create a fake extent key in the buffer. We have to add a dummy extent
> > > > +	 * pointer for the fill code to add an extent entry. It's explicitly
> > > > +	 * zeroed to reflect delayed allocation (i.e. phys offset 0).
> > > > +	 */
> > > > +	bch2_bkey_buf_realloc(cur, c, sizeof(*delextent) / sizeof(u64));
> > > > +	delextent = bkey_extent_init(cur->k);
> > > > +	delextent->k.p = POS(ei->v.i_ino, start >> 9);
> > > > +	bch2_key_resize(&delextent->k, (end - start) >> 9);
> > > > +	bch2_bkey_append_ptr(&delextent->k_i, ptr);
> > > > +
> > > > +	return true;
> > > > +}
> > > 
> > > I don't like that this returns a delalloc extent when data is merely
> > > present in the pagecache - that's wrong.
> > > 
> > > bch2_seek_pagecache_dirty_data()?
> > > 
> > 
> > Technically it just returns a file offset range represented in an extent
> > key. I could tweak it to just return a range or something and make the
> > delalloc part a separate helper.
> > 
> > More to the broader point, I thought that technically this was all
> > somewhat racy wrt to folio state and I was a little concerned about
> > possibly showing weird/inconsistent state if this runs concurrently with
> > writeback. I'll have to take a closer look at the bch2_seek_* helpers
> > though to better quantify that concern...
> 
> Yeah, it does get tricky there; writeback will flip the folio state from
> dirty to allocated before it shows up in the btree.
> 
> To fix that we'd need to add a new state to BCH_FOLIO_SECTOR_STATE, and
> it seems like that's something we should strongly consider.
> 
> There may also be some lingering bugginess in the i_sectors accounting
> code, if we're touching this stuff we definitely want to be looking for
> ways to make that more rigoroous if we can - I thought I was done with
> that, but I have an assert patch in my garbage branch that was firing -
> hadn't had time to track it down:
> https://evilpiepirate.org/git/bcachefs.git/commit/?h=bcachefs-garbage&id=50010457597c6c30aec1195b0568868d2d30bb76
> 
> This is likely related to the warning that's been popping in the CI from
> time to time about i_sectors_delta being the wrong sign in writeback
> completion.
> 
> Not the main focus here, just wanted to bring it up in case you start to
> go down that rabbit hole
> 

Ok, thanks. I'll give it a look.

> 
> > > Now that we're not compiling in gnu89 mode anymore, I'm moving code away
> > > from this "declare all variables at the top" style - I want variables
> > > declared where they're initialized.
> > > 
> > > The reason is that bugs of the "I accidently used a var before I
> > > initialized it" are really common, and the compiler does a crap job of
> > > catching them.
> > > 
> > > Anywhere where we can write our code in a more functional style (in the
> > > SSA sense, variables are declared where they are initialized and never
> > > mutated) without it affecting the quality of the output is a big win.
> > > 
> > > (across function calls, we can't generally do this because C doesn't
> > > have return value optimization).
> > > 
> > 
> > Hmm.. not really sure I want to get into this. My initial reaction is
> > that this strikes me as odd formatting that makes the code incrementally
> > more annoying to read for marginal benefit. Has there been any broader
> > discussion on this sort of thing anywhere that I can catch up on?
> 
> Starting that discussion now :)
> 

Heh, Ok.

> Actually, this did come up not too long ago; there was a bug in some usb
> code where they were using the loop iterator after breaking out of the
> loop - which should have been ok, but for some horribly subtly reasons
> was not, and Linus brought up that it's probably time to switch to
> declaring the loop iterator in the for () statement to make those sorts
> of bugs impossible.
> 

That seems like a perfectly reasonable adjustment/policy to me.

It also seems like a decent opportunity to pose the idea of a broader
change to revisit how variables are declared, and solicit some initial
reactions..?

> More broadly, I regularly come across subtle bugs that should not have
> happened purely because of variable reuse; consider the way we declare
> int ret; at the top of a lot of functions, and then reuse it throughout.
> 
> That's error prone for a whole bunch of reasons, and the bugs it causes
> tends to be ones that get introduced in later refactoring - some code
> skips initializing ret where it's declared because _at the time that
> code was written_ it was immediately assigned to, or code will use it
> for something that wasn't an error code and then forget to set it back
> to 0.
> 

Yep.

> It's really much better to only declare variables when they're first
> assigned to, and only use them for one thing.
> 

That seems somewhat reasonable in principle, particularly the bit about
limiting purpose of variables, but I'm more skeptical about this in
practice. A few high level points come to mind..

1. I'm skeptical the invariant of "declaration at first use" will be
reliably maintained as time passes and development contributions scale.
Unless there is some tool or compiler option to address this
automatically (which I thought existed?), ISTM the existence of these
bugs (or not) still ultimately comes down to catching them during
development or review.

2. I wonder if a tradeoff for reducing these sorts of errors comes at an
indirect cost of making readability and review incrementally harder, and
thus less effective. For example, I'd probably get pretty annoyed fairly
quickly having to hunt around for variable declarations in a complex
function to grok type usage or whatever, or having to move them around
when hacking, etc.

3. Logistically, this is likely to be a bit awkward for pretty much
everybody who has a history of working with C code. In the context of
kernel development, something also tells me this has potential to be a
tinderbox for a flamewar, but who knows.. ;P

> You'll notice I've got a whole bunch of patches queued up for 6.8 that
> change almost all of our for loop macros to declaring the loop iter in
> the macro, and I will be going through and changing other variable uses
> to this style as it comes up.
> 

I think the loop thing makes sense, particularly because it's a small,
isolated change.

WRT moving all other vars around, IMO it would be preferable to perhaps
have more discussion and see if others have similar opinions on that
sort of change before going so far as to swizzle code around and/or
insist at code review time.

That's just my .02 of course; it's your project. ;)

> > > >  	bool have_extent = false;
> > > >  	u32 snapshot;
> > > >  	int ret = 0;
> > > > @@ -916,6 +952,19 @@ static int bch2_fiemap(struct inode *vinode, struct fiemap_extent_info *info,
> > > >  			continue;
> > > >  		}
> > > >  
> > > > +		/*
> > > > +		 * Outstanding buffered writes aren't tracked in the extent
> > > > +		 * btree until dirty folios are written back. Check holes in the
> > > > +		 * extent tree for data in pagecache and report it as delalloc.
> > > > +		 */
> > > > +		if (iter.pos.offset > start &&
> > > > +		    bch2_fiemap_scan_pagecache(vinode, start << 9,
> > > > +					       iter.pos.offset << 9, &cur)) {
> > > > +			cflags = FIEMAP_EXTENT_DELALLOC;
> > > 
> > > but cflags/plags are unnecessary, no? can't we just detected this in
> > > bch2_fill_extent when the ptr offset is 0?
> > > 
> > 
> > I thought (briefly) about if/how this state could be implied by the
> > helper (moreso out of laziness ;), but I feel that sort of logic is more
> > fragile than just being explicit about state.
> 
> The logic can be confined to one place - a helper with a good name -
> while the way you're doing it you're now spreading the state that's
> passed around across multiple variables.
> 
> The clean way to do what you're doing would be to have a wrapper type so
> we could store the bkey_buf with the fiemap flags in one variable.
> 

That's a good point. I'll clean that up if this logic ends up sticking
around. Thanks.

Brian

> > 
> > > > +			start = bkey_start_offset(&cur.k->k) + cur.k->k.size;
> > > > +			goto fill;
> > > > +		}
> > > 
> > > The goto fill is also a code smell.
> > > 
> > 
> > It's just basically a goto next pattern. I think the logical wart is
> > more how this post-processes the gaps/hoes in the extent offset ranges
> > and set_pos' the same start value repeatedly as we process delalloc
> > extents. Not really sure if that's what you mean here, but that's the
> > part that I didn't really love when hacking on this.
> > 
> > > The right way to do it would be to search, from the same position, both
> > > the btree and the pagecache: then compare those two extents, using
> > > delalloc extent if it comes first, and trimming the btree extent if it
> > > comes first but the delalloc extent overlaps.
> > > 
> > > Then there's no goto fill; after we've decided which extent to use the
> > > rest of the loop is unchanged.
> > > 
> > > (The btree iterator code works roughly this way, where it has to overlay
> > > keys from the journal and pending updates).
> > > 
> > 
> > Yeah, I thought that something like this would be necessary for changing
> > behavior of the "dirty data over existing COW blocks" case, if we ever
> > wanted to do that, but figured it wasn't worth the complexity unless we
> > go that direction. It might be improvement enough for the iteration
> > either way though. I'll try to take a fresh look at that. Thanks for the
> > review.
> 
> btrfs folks have probably looked at this as well - any notes from you
> guys on fiemap behaviour - has anything come up where a particular
> behaviour is important?
> 
> Also, someone (Darrick?) mentioned in the Tuesday meeting that maybe we
> actually can extend fiemap for multiple device filesystems, wonder if
> anyone's interested in that...
> 

Re: [PATCH 1/2] bcachefs: add fiemap delalloc extent detection

[Carl E. Thompson]

I'm not a kernel developer so feel free to ignore this.

> On 2024-01-08 7:33 AM PST Brian Foster <bfoster@redhat.com> wrote:

> ...

> 3. Logistically, this is likely to be a bit awkward for pretty much
> everybody who has a history of working with C code.

I disagree with this point. The ability to declare variables anywhere has been around officially since c99 and unofficially for much longer. For half or more of the 50-year history of C it has been allowed so most people with longtime C experience aren't going to be confused.

In fact C **never** required that **all** variables be declared at the beginning of a function. C has always allowed variables to be declared at the beginning of **any** block including unattached / unconditional blocks which I have personally used to great effect to limit the scope of temporary variables and to declare them close to where they were used going as far back as the 80s.

> In the context of kernel development, something also tells me this has
> potential to be a tinderbox for a flamewar, but who knows.. ;P

Well that may be the **real** issue! From the outside the kernel developers seem reluctant to revisit old decisions... It's 2024 and the mailing lists still don't accept modern email formats (HTML) which seem like they would make discussion easier. I used to be one of those people who resisted the change and preferred text-based email only but it's now been decades. Time to let that one go!

> ...

Re: [PATCH 1/2] bcachefs: add fiemap delalloc extent detection

[Brian Foster]

On Mon, Jan 08, 2024 at 02:34:43PM -0800, Carl E. Thompson wrote:
> I'm not a kernel developer so feel free to ignore this.
> 
> > On 2024-01-08 7:33 AM PST Brian Foster <bfoster@redhat.com> wrote:
> 
> > ...
> 
> > 3. Logistically, this is likely to be a bit awkward for pretty much
> > everybody who has a history of working with C code.
> 
> I disagree with this point. The ability to declare variables anywhere has been around officially since c99 and unofficially for much longer. For half or more of the 50-year history of C it has been allowed so most people with longtime C experience aren't going to be confused.
> 
> In fact C **never** required that **all** variables be declared at the beginning of a function. C has always allowed variables to be declared at the beginning of **any** block including unattached / unconditional blocks which I have personally used to great effect to limit the scope of temporary variables and to declare them close to where they were used going as far back as the 80s.
> 

Ok, but I'm not referring to the pure act of declaring variables
differently (i.e. such as limiting scope, the other examples given,
etc.). I'm referring specifically to a policy where all function scope
variables are uniformly declared at first use rather than at
top-of-function.

> > In the context of kernel development, something also tells me this has
> > potential to be a tinderbox for a flamewar, but who knows.. ;P
> 
> Well that may be the **real** issue! From the outside the kernel developers seem reluctant to revisit old decisions... It's 2024 and the mailing lists still don't accept modern email formats (HTML) which seem like they would make discussion easier. I used to be one of those people who resisted the change and preferred text-based email only but it's now been decades. Time to let that one go!
> 
> > ...
> 

Hah, well as a mutt user I have to disagree with the HTML thing. ;) But
to be fair, the fact that something might incite a flamewar in a
community that is historically pretty bad at level-headed discussion is
not a great argument. The real issue there is indeed something
different.

My concern with this was more that perhaps there might be indirect cost
to such a policy that might not be fully factored into the value
judgement of adopting it purely to minimize likelihood of a particular
class of bug.

Brian

Re: [PATCH 1/2] bcachefs: add fiemap delalloc extent detection

[Carl E. Thompson]

> On 2024-01-09 8:42 AM PST Brian Foster <bfoster@redhat.com> wrote:

> ...

> Ok, but I'm not referring to the pure act of declaring variables
> differently (i.e. such as limiting scope, the other examples given,
> etc.). I'm referring specifically to a policy where all function scope
> variables are uniformly declared at first use rather than at
> top-of-function.

Ahh, I see. I apologize for misunderstanding. In my experience there are definitely cases where declaring some variables at the top of functions makes sense for clarity.

> ...

Carl