On Thu, Apr 04, 2024 at 10:32:32AM +0300, Dan Carpenter wrote:
> Hello Darrick J. Wong,
>
> Commit 4bdfd7d15747 ("xfs: repair free space btrees") from Dec 15,
> 2023 (linux-next), leads to the following Smatch static checker
> warning:
>
> fs/xfs/scrub/alloc_repair.c:781 xrep_abt_build_new_trees()
> warn: missing unwind goto?
>
> fs/xfs/scrub/alloc_repair.c
> 702 STATIC int
> 703 xrep_abt_build_new_trees(
> 704 struct xrep_abt *ra)
> 705 {
> 706 struct xfs_scrub *sc = ra->sc;
> 707 struct xfs_btree_cur *bno_cur;
> 708 struct xfs_btree_cur *cnt_cur;
> 709 struct xfs_perag *pag = sc->sa.pag;
> 710 bool needs_resort = false;
> 711 int error;
> 712
> 713 /*
> 714 * Sort the free extents by length so that we can set up the free space
> 715 * btrees in as few extents as possible. This reduces the amount of
> 716 * deferred rmap / free work we have to do at the end.
> 717 */
> 718 error = xrep_cntbt_sort_records(ra, false);
> 719 if (error)
> 720 return error;
> 721
> 722 /*
> 723 * Prepare to construct the new btree by reserving disk space for the
> 724 * new btree and setting up all the accounting information we'll need
> 725 * to root the new btree while it's under construction and before we
> 726 * attach it to the AG header.
> 727 */
> 728 xrep_newbt_init_bare(&ra->new_bnobt, sc);
> 729 xrep_newbt_init_bare(&ra->new_cntbt, sc);
> 730
> 731 ra->new_bnobt.bload.get_records = xrep_abt_get_records;
> 732 ra->new_cntbt.bload.get_records = xrep_abt_get_records;
> 733
> 734 ra->new_bnobt.bload.claim_block = xrep_abt_claim_block;
> 735 ra->new_cntbt.bload.claim_block = xrep_abt_claim_block;
> 736
> 737 /* Allocate cursors for the staged btrees. */
> 738 bno_cur = xfs_bnobt_init_cursor(sc->mp, NULL, NULL, pag);
> 739 xfs_btree_stage_afakeroot(bno_cur, &ra->new_bnobt.afake);
> 740
> 741 cnt_cur = xfs_cntbt_init_cursor(sc->mp, NULL, NULL, pag);
> 742 xfs_btree_stage_afakeroot(cnt_cur, &ra->new_cntbt.afake);
> 743
> 744 /* Last chance to abort before we start committing fixes. */
> 745 if (xchk_should_terminate(sc, &error))
> 746 goto err_cur;
> 747
> 748 /* Reserve the space we'll need for the new btrees. */
> 749 error = xrep_abt_reserve_space(ra, bno_cur, cnt_cur, &needs_resort);
> 750 if (error)
> 751 goto err_cur;
> 752
> 753 /*
> 754 * If we need to re-sort the free extents by length, do so so that we
> 755 * can put the records into the cntbt in the correct order.
> 756 */
> 757 if (needs_resort) {
> 758 error = xrep_cntbt_sort_records(ra, needs_resort);
> 759 if (error)
> 760 goto err_cur;
> 761 }
> 762
> 763 /*
> 764 * Due to btree slack factors, it's possible for a new btree to be one
> 765 * level taller than the old btree. Update the alternate incore btree
> 766 * height so that we don't trip the verifiers when writing the new
> 767 * btree blocks to disk.
> 768 */
> 769 pag->pagf_repair_bno_level = ra->new_bnobt.bload.btree_height;
> 770 pag->pagf_repair_cnt_level = ra->new_cntbt.bload.btree_height;
> 771
> 772 /* Load the free space by length tree. */
> 773 ra->array_cur = XFARRAY_CURSOR_INIT;
> 774 ra->longest = 0;
> 775 error = xfs_btree_bload(cnt_cur, &ra->new_cntbt.bload, ra);
> 776 if (error)
> 777 goto err_levels;
> ^^^^^^^^^^^^^^^^
> 778
> 779 error = xrep_bnobt_sort_records(ra);
> 780 if (error)
> --> 781 return error;
> ^^^^^^^^^^^^^
> Should this be a goto err_levels?
Yep. Thanks for the report.
--D
> 782
> 783 /* Load the free space by block number tree. */
> 784 ra->array_cur = XFARRAY_CURSOR_INIT;
> 785 error = xfs_btree_bload(bno_cur, &ra->new_bnobt.bload, ra);
> 786 if (error)
> 787 goto err_levels;
> 788
> 789 /*
> 790 * Install the new btrees in the AG header. After this point the old
> 791 * btrees are no longer accessible and the new trees are live.
> 792 */
> 793 xfs_allocbt_commit_staged_btree(bno_cur, sc->tp, sc->sa.agf_bp);
> 794 xfs_btree_del_cursor(bno_cur, 0);
> 795 xfs_allocbt_commit_staged_btree(cnt_cur, sc->tp, sc->sa.agf_bp);
> 796 xfs_btree_del_cursor(cnt_cur, 0);
> 797
> 798 /* Reset the AGF counters now that we've changed the btree shape. */
> 799 error = xrep_abt_reset_counters(ra);
> 800 if (error)
> 801 goto err_newbt;
> 802
> 803 /* Dispose of any unused blocks and the accounting information. */
> 804 xrep_abt_dispose_reservations(ra, error);
> 805
> 806 return xrep_roll_ag_trans(sc);
> 807
> 808 err_levels:
> 809 pag->pagf_repair_bno_level = 0;
> 810 pag->pagf_repair_cnt_level = 0;
> 811 err_cur:
> 812 xfs_btree_del_cursor(cnt_cur, error);
> 813 xfs_btree_del_cursor(bno_cur, error);
> 814 err_newbt:
> 815 xrep_abt_dispose_reservations(ra, error);
> 816 return error;
> 817 }
>
> regards,
> dan carpenter
