[PATCH 0/2] btrfs: some trivial updates for delayed ref space reservation

[PATCH 0/2] btrfs: some trivial updates for delayed ref space reservation

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

One cleanup and one small fix regarding reserving space for delayed refs
when starting a transaction.

Filipe Manana (2):
  btrfs: make btrfs_block_rsv_full() check more boolean when starting transaction
  btrfs: correctly calculate delayed ref bytes when starting transaction

 fs/btrfs/transaction.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/2] btrfs: make btrfs_block_rsv_full() check more boolean when starting transaction

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When starting a transaction we are comparing the result of a call to
btrfs_block_rsv_full() with 0, but the function returns a boolean. While
in practice it is not incorrect, as 0 is equivalent to false, it makes it
a bit odd and less readable. So update the check to not compare against 0
and instead use the logical not (!) operator.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/transaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index c57d4408e7f1..a54a5c5a5db3 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -607,7 +607,7 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
 		 */
 		num_bytes = btrfs_calc_insert_metadata_size(fs_info, num_items);
 		if (flush == BTRFS_RESERVE_FLUSH_ALL &&
-		    btrfs_block_rsv_full(delayed_refs_rsv) == 0) {
+		    !btrfs_block_rsv_full(delayed_refs_rsv)) {
 			delayed_refs_bytes = num_bytes;
 			num_bytes <<= 1;
 		}
-- 
2.34.1

[PATCH 2/2] btrfs: correctly calculate delayed ref bytes when starting transaction

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When starting a transaction, we are assuming the number of bytes used for
each delayed ref update matches the number of bytes used for each item
update, that is the return value of:

   btrfs_calc_insert_metadata_size(fs_info, num_items)

However that is not correct when we are using the free space tree, as we
need to multiply that value by 2, since delayed ref updates need to modify
the free space tree besides the extent tree.

So fix this by using btrfs_calc_delayed_ref_bytes() to get the correct
number of bytes used for delayed ref updates.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/transaction.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index a54a5c5a5db3..9e7ba07a35e8 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -601,15 +601,16 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
 		/*
 		 * We want to reserve all the bytes we may need all at once, so
 		 * we only do 1 enospc flushing cycle per transaction start.  We
-		 * accomplish this by simply assuming we'll do 2 x num_items
-		 * worth of delayed refs updates in this trans handle, and
-		 * refill that amount for whatever is missing in the reserve.
+		 * accomplish this by simply assuming we'll do num_items worth
+		 * of delayed refs updates in this trans handle, and refill that
+		 * amount for whatever is missing in the reserve.
 		 */
 		num_bytes = btrfs_calc_insert_metadata_size(fs_info, num_items);
 		if (flush == BTRFS_RESERVE_FLUSH_ALL &&
 		    !btrfs_block_rsv_full(delayed_refs_rsv)) {
-			delayed_refs_bytes = num_bytes;
-			num_bytes <<= 1;
+			delayed_refs_bytes = btrfs_calc_delayed_ref_bytes(fs_info,
+									  num_items);
+			num_bytes += delayed_refs_bytes;
 		}
 
 		/*
-- 
2.34.1

Re: [PATCH 2/2] btrfs: correctly calculate delayed ref bytes when starting transaction

[kernel test robot]

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kdave/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/fdmanana-kernel-org/btrfs-make-btrfs_block_rsv_full-check-more-boolean-when-starting-transaction/20230330-224056
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
patch link:    https://lore.kernel.org/r/93c382a002210831e1051456cdc5c44dbcef4562.1680185833.git.fdmanana%40suse.com
patch subject: [PATCH 2/2] btrfs: correctly calculate delayed ref bytes when starting transaction
config: riscv-allmodconfig (https://download.01.org/0day-ci/archive/20230331/202303310221.Zjv7RAZT-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/7f30c221a11d9dc6fad3f763b3df7ecd0b6d966c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review fdmanana-kernel-org/btrfs-make-btrfs_block_rsv_full-check-more-boolean-when-starting-transaction/20230330-224056
        git checkout 7f30c221a11d9dc6fad3f763b3df7ecd0b6d966c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=riscv olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash fs/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303310221.Zjv7RAZT-lkp@intel.com/

All errors (new ones prefixed by >>):

   fs/btrfs/transaction.c: In function 'start_transaction':
>> fs/btrfs/transaction.c:611:46: error: implicit declaration of function 'btrfs_calc_delayed_ref_bytes'; did you mean 'btrfs_run_delayed_refs'? [-Werror=implicit-function-declaration]
     611 |                         delayed_refs_bytes = btrfs_calc_delayed_ref_bytes(fs_info,
         |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
         |                                              btrfs_run_delayed_refs
   cc1: some warnings being treated as errors


vim +611 fs/btrfs/transaction.c

   558	
   559	static struct btrfs_trans_handle *
   560	start_transaction(struct btrfs_root *root, unsigned int num_items,
   561			  unsigned int type, enum btrfs_reserve_flush_enum flush,
   562			  bool enforce_qgroups)
   563	{
   564		struct btrfs_fs_info *fs_info = root->fs_info;
   565		struct btrfs_block_rsv *delayed_refs_rsv = &fs_info->delayed_refs_rsv;
   566		struct btrfs_trans_handle *h;
   567		struct btrfs_transaction *cur_trans;
   568		u64 num_bytes = 0;
   569		u64 qgroup_reserved = 0;
   570		bool reloc_reserved = false;
   571		bool do_chunk_alloc = false;
   572		int ret;
   573	
   574		if (BTRFS_FS_ERROR(fs_info))
   575			return ERR_PTR(-EROFS);
   576	
   577		if (current->journal_info) {
   578			WARN_ON(type & TRANS_EXTWRITERS);
   579			h = current->journal_info;
   580			refcount_inc(&h->use_count);
   581			WARN_ON(refcount_read(&h->use_count) > 2);
   582			h->orig_rsv = h->block_rsv;
   583			h->block_rsv = NULL;
   584			goto got_it;
   585		}
   586	
   587		/*
   588		 * Do the reservation before we join the transaction so we can do all
   589		 * the appropriate flushing if need be.
   590		 */
   591		if (num_items && root != fs_info->chunk_root) {
   592			struct btrfs_block_rsv *rsv = &fs_info->trans_block_rsv;
   593			u64 delayed_refs_bytes = 0;
   594	
   595			qgroup_reserved = num_items * fs_info->nodesize;
   596			ret = btrfs_qgroup_reserve_meta_pertrans(root, qgroup_reserved,
   597					enforce_qgroups);
   598			if (ret)
   599				return ERR_PTR(ret);
   600	
   601			/*
   602			 * We want to reserve all the bytes we may need all at once, so
   603			 * we only do 1 enospc flushing cycle per transaction start.  We
   604			 * accomplish this by simply assuming we'll do num_items worth
   605			 * of delayed refs updates in this trans handle, and refill that
   606			 * amount for whatever is missing in the reserve.
   607			 */
   608			num_bytes = btrfs_calc_insert_metadata_size(fs_info, num_items);
   609			if (flush == BTRFS_RESERVE_FLUSH_ALL &&
   610			    !btrfs_block_rsv_full(delayed_refs_rsv)) {
 > 611				delayed_refs_bytes = btrfs_calc_delayed_ref_bytes(fs_info,
   612										  num_items);
   613				num_bytes += delayed_refs_bytes;
   614			}
   615	
   616			/*
   617			 * Do the reservation for the relocation root creation
   618			 */
   619			if (need_reserve_reloc_root(root)) {
   620				num_bytes += fs_info->nodesize;
   621				reloc_reserved = true;
   622			}
   623	
   624			ret = btrfs_block_rsv_add(fs_info, rsv, num_bytes, flush);
   625			if (ret)
   626				goto reserve_fail;
   627			if (delayed_refs_bytes) {
   628				btrfs_migrate_to_delayed_refs_rsv(fs_info, rsv,
   629								  delayed_refs_bytes);
   630				num_bytes -= delayed_refs_bytes;
   631			}
   632	
   633			if (rsv->space_info->force_alloc)
   634				do_chunk_alloc = true;
   635		} else if (num_items == 0 && flush == BTRFS_RESERVE_FLUSH_ALL &&
   636			   !btrfs_block_rsv_full(delayed_refs_rsv)) {
   637			/*
   638			 * Some people call with btrfs_start_transaction(root, 0)
   639			 * because they can be throttled, but have some other mechanism
   640			 * for reserving space.  We still want these guys to refill the
   641			 * delayed block_rsv so just add 1 items worth of reservation
   642			 * here.
   643			 */
   644			ret = btrfs_delayed_refs_rsv_refill(fs_info, flush);
   645			if (ret)
   646				goto reserve_fail;
   647		}
   648	again:
   649		h = kmem_cache_zalloc(btrfs_trans_handle_cachep, GFP_NOFS);
   650		if (!h) {
   651			ret = -ENOMEM;
   652			goto alloc_fail;
   653		}
   654	
   655		/*
   656		 * If we are JOIN_NOLOCK we're already committing a transaction and
   657		 * waiting on this guy, so we don't need to do the sb_start_intwrite
   658		 * because we're already holding a ref.  We need this because we could
   659		 * have raced in and did an fsync() on a file which can kick a commit
   660		 * and then we deadlock with somebody doing a freeze.
   661		 *
   662		 * If we are ATTACH, it means we just want to catch the current
   663		 * transaction and commit it, so we needn't do sb_start_intwrite(). 
   664		 */
   665		if (type & __TRANS_FREEZABLE)
   666			sb_start_intwrite(fs_info->sb);
   667	
   668		if (may_wait_transaction(fs_info, type))
   669			wait_current_trans(fs_info);
   670	
   671		do {
   672			ret = join_transaction(fs_info, type);
   673			if (ret == -EBUSY) {
   674				wait_current_trans(fs_info);
   675				if (unlikely(type == TRANS_ATTACH ||
   676					     type == TRANS_JOIN_NOSTART))
   677					ret = -ENOENT;
   678			}
   679		} while (ret == -EBUSY);
   680	
   681		if (ret < 0)
   682			goto join_fail;
   683	
   684		cur_trans = fs_info->running_transaction;
   685	
   686		h->transid = cur_trans->transid;
   687		h->transaction = cur_trans;
   688		refcount_set(&h->use_count, 1);
   689		h->fs_info = root->fs_info;
   690	
   691		h->type = type;
   692		INIT_LIST_HEAD(&h->new_bgs);
   693	
   694		smp_mb();
   695		if (cur_trans->state >= TRANS_STATE_COMMIT_START &&
   696		    may_wait_transaction(fs_info, type)) {
   697			current->journal_info = h;
   698			btrfs_commit_transaction(h);
   699			goto again;
   700		}
   701	
   702		if (num_bytes) {
   703			trace_btrfs_space_reservation(fs_info, "transaction",
   704						      h->transid, num_bytes, 1);
   705			h->block_rsv = &fs_info->trans_block_rsv;
   706			h->bytes_reserved = num_bytes;
   707			h->reloc_reserved = reloc_reserved;
   708		}
   709	
   710	got_it:
   711		if (!current->journal_info)
   712			current->journal_info = h;
   713	
   714		/*
   715		 * If the space_info is marked ALLOC_FORCE then we'll get upgraded to
   716		 * ALLOC_FORCE the first run through, and then we won't allocate for
   717		 * anybody else who races in later.  We don't care about the return
   718		 * value here.
   719		 */
   720		if (do_chunk_alloc && num_bytes) {
   721			u64 flags = h->block_rsv->space_info->flags;
   722	
   723			btrfs_chunk_alloc(h, btrfs_get_alloc_profile(fs_info, flags),
   724					  CHUNK_ALLOC_NO_FORCE);
   725		}
   726	
   727		/*
   728		 * btrfs_record_root_in_trans() needs to alloc new extents, and may
   729		 * call btrfs_join_transaction() while we're also starting a
   730		 * transaction.
   731		 *
   732		 * Thus it need to be called after current->journal_info initialized,
   733		 * or we can deadlock.
   734		 */
   735		ret = btrfs_record_root_in_trans(h, root);
   736		if (ret) {
   737			/*
   738			 * The transaction handle is fully initialized and linked with
   739			 * other structures so it needs to be ended in case of errors,
   740			 * not just freed.
   741			 */
   742			btrfs_end_transaction(h);
   743			return ERR_PTR(ret);
   744		}
   745	
   746		return h;
   747	
   748	join_fail:
   749		if (type & __TRANS_FREEZABLE)
   750			sb_end_intwrite(fs_info->sb);
   751		kmem_cache_free(btrfs_trans_handle_cachep, h);
   752	alloc_fail:
   753		if (num_bytes)
   754			btrfs_block_rsv_release(fs_info, &fs_info->trans_block_rsv,
   755						num_bytes, NULL);
   756	reserve_fail:
   757		btrfs_qgroup_free_meta_pertrans(root, qgroup_reserved);
   758		return ERR_PTR(ret);
   759	}
   760	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 0/2] btrfs: some trivial updates for delayed ref space reservation

[David Sterba]

On Thu, Mar 30, 2023 at 03:39:01PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> One cleanup and one small fix regarding reserving space for delayed refs
> when starting a transaction.
> 
> Filipe Manana (2):
>   btrfs: make btrfs_block_rsv_full() check more boolean when starting transaction
>   btrfs: correctly calculate delayed ref bytes when starting transaction

Added to misc-next, thanks.