Re: [PATCH] btrfs: replace writeback inhibition xarray with a fixed inline buffer

On Thu, Jun 25, 2026 at 10:25:17AM +0800, Sun YangKai wrote:
> > -	/* Handle replacement of different eb at same index. */
> > -	if (old && old != eb) {
> > -		struct extent_buffer *old_eb = old;
> > +	if (trans->nr_inhibited_ebs < BTRFS_INHIBITED_EBS_SLOTS) {
> > +		slot = trans->nr_inhibited_ebs++;
> > +	} else {
> > +		/*
> > +		 * Full: advance the CLOCK hand, clearing reference bits until a
> > +		 * slot without one is found and evicted. Clearing a bit per step
> > +		 * bounds this to BTRFS_INHIBITED_EBS_SLOTS iterations.
> > +		 */
> > +		while (trans->inhibited_ebs_referenced &
> > +		       (1U << trans->inhibited_ebs_hand)) {
> > +			trans->inhibited_ebs_referenced &=
> > +				~(1U << trans->inhibited_ebs_hand);
> > +			trans->inhibited_ebs_hand =
> > +				(trans->inhibited_ebs_hand + 1) %
> > +				BTRFS_INHIBITED_EBS_SLOTS;
> 
> I think we can using overflowing add with ` & (SLOTS - 1)` as the index 
> as long as SLOTS is power of 2, instead of `% SLOTS`.

For immediate constants the compiler does various transformations that
utilize the power of two abilities to reduce the operations (I think
it's called 'strength reduce', e.g. mod -> bitwise and). You can check
the assembly if there's a division or and.

> And the higher 
> bits can parsed as number of "rounds" if we keep them, and I'm not sure 
> if the extra info is useful for any purpose.
> 
> > +		}
> > +		slot = trans->inhibited_ebs_hand;
> > +		trans->inhibited_ebs_hand =
> > +			(trans->inhibited_ebs_hand + 1) % BTRFS_INHIBITED_EBS_SLOTS;
> 
> the same math here.
> 
> >   
> > -		atomic_dec(&old_eb->writeback_inhibitors);
> > -		free_extent_buffer(old_eb);
> > +		atomic_dec(&trans->inhibited_ebs[slot]->writeback_inhibitors);
> > +		free_extent_buffer(trans->inhibited_ebs[slot]);
> >   	}
> 
> And personally I'd love to wrap this part of logic into a helper 
> function like
> 
> static inline u8 btrfs_inhibit_prepare_slot(struct btrfs_trans_handle 
> *trans) {...}
> 
> Or something with a better name.
> 
> >   
> > +	/*
> > +	 * Pin the eb while the array holds a raw pointer to it; the counter is
> > +	 * what lock_extent_buffer_for_io() checks.
> > +	 */
> > +	refcount_inc(&eb->refs);
> >   	atomic_inc(&eb->writeback_inhibitors);
> > +	trans->inhibited_ebs[slot] = eb;
> > +	trans->inhibited_ebs_referenced |= 1U << slot;
> >   }
> >   
> >   /*

> >   struct btrfs_trans_handle {
> >   	u64 transid;
> >   	u64 bytes_reserved;
> > @@ -163,8 +169,11 @@ struct btrfs_trans_handle {
> >   	struct btrfs_fs_info *fs_info;
> >   	struct list_head new_bgs;
> >   	struct btrfs_block_rsv delayed_rsv;
> > -	/* Extent buffers with writeback inhibited by this handle. */
> > -	struct xarray writeback_inhibited_ebs;
> > +	/* Extent buffers this handle has inhibited writeback on. */
> > +	struct extent_buffer *inhibited_ebs[BTRFS_INHIBITED_EBS_SLOTS];
> 
> This takes 64 bytes, which is necessary, fine.
> 
> > +	u32 inhibited_ebs_referenced;	/* CLOCK reference bit per slot */
> > +	u32 nr_inhibited_ebs;
> 
> Given the 8-slot limit for inhibited_ebs (4 bits (8) for capacity, 8 
> bits for bitmap), using u8 for these two fields is a clear and 
> straightforward choice IMO. Even if we later extend the limit to 16, 
> only the bitmap field (inhibited_ebs_referenced) would need to be 
> widened to u16.

The u32 is a bit more convenient on the instruction level, we can
consider u8 in case the transaction handle size grows more than what
fits to common slabs.

The xarray version has 184 bytes and quick estimate of this patch is
about 240 so we're in the 256 bucket anyway.