On Tue, Feb 20, 2024 at 04:26:38PM -0500, Kent Overstreet wrote:
> On Tue, Feb 20, 2024 at 02:39:35PM +0300, Dan Carpenter wrote:
> > So right now I only have two bcache warnings. The missing error code
> > warning isn't published unfortunately... I don't think I'm going to
> > have a lot of time to dedicate to this stuff in the up coming months
> > honestly.
>
> Why not just put this stuff up in a dashboard? Make it easy for people
> to see the full results for their code, without nagging them with a
> separate email for every warning?
>
Honestly, it would require quite a bit of work to do that and someone
would need to step up to fund Linaro for a project like that. If
someone else feels like doing that, I'm happy to help them but right now
I have a bunch of other things on my plate.
> > fs/bcachefs/journal.c:575 __journal_res_get() error: uninitialized symbol 'can_discard'.
>
> your error message seems wrong to me. If it was an uninitialized
> _symbol_, i.e. that symbol didn't exist, then the code wouldn't build.
> Instead you seem to be warning about a possibly uninitialized variable.
>
> Yet a cursory inspection reveals the variable is initialized in every
> path before it's used, and gcc gets it right.
>
Hm. You're right. I'll look into this.
> > fs/bcachefs/alloc_background.c:1751 bch2_discard_one_bucket() warn: missing error code here? 'discard_in_flight_add()' failed. 'ret' = '0'
>
> No idea what smatch is complaining about here, we're returning either
> -EEXIST the error code of darray_push().
I don't understand. I don't see an -EEXIST or a darray_push()... :/
fs/bcachefs/alloc_background.c
1670 static int bch2_discard_one_bucket(struct btree_trans *trans,
1671 struct btree_iter *need_discard_iter,
1672 struct bpos *discard_pos_done,
1673 struct discard_buckets_state *s)
1674 {
1675 struct bch_fs *c = trans->c;
1676 struct bpos pos = need_discard_iter->pos;
1677 struct btree_iter iter = { NULL };
1678 struct bkey_s_c k;
1679 struct bch_dev *ca;
1680 struct bkey_i_alloc_v4 *a;
1681 struct printbuf buf = PRINTBUF;
1682 bool discard_locked = false;
1683 int ret = 0;
1684
1685 ca = bch_dev_bkey_exists(c, pos.inode);
1686
1687 if (!percpu_ref_tryget(&ca->io_ref)) {
1688 bch2_btree_iter_set_pos(need_discard_iter, POS(pos.inode + 1, 0));
1689 return 0;
1690 }
1691
1692 discard_buckets_next_dev(c, s, ca);
1693
1694 if (bch2_bucket_is_open_safe(c, pos.inode, pos.offset)) {
1695 s->open++;
1696 goto out;
1697 }
1698
1699 if (bch2_bucket_needs_journal_commit(&c->buckets_waiting_for_journal,
1700 c->journal.flushed_seq_ondisk,
1701 pos.inode, pos.offset)) {
1702 s->need_journal_commit++;
1703 s->need_journal_commit_this_dev++;
1704 goto out;
1705 }
1706
1707 k = bch2_bkey_get_iter(trans, &iter, BTREE_ID_alloc,
1708 need_discard_iter->pos,
1709 BTREE_ITER_CACHED);
1710 ret = bkey_err(k);
1711 if (ret)
1712 goto out;
1713
1714 a = bch2_alloc_to_v4_mut(trans, k);
1715 ret = PTR_ERR_OR_ZERO(a);
1716 if (ret)
1717 goto out;
ret is zero
1718
1719 if (BCH_ALLOC_V4_NEED_INC_GEN(&a->v)) {
1720 a->v.gen++;
1721 SET_BCH_ALLOC_V4_NEED_INC_GEN(&a->v, false);
1722 goto write;
1723 }
1724
1725 if (a->v.journal_seq > c->journal.flushed_seq_ondisk) {
1726 if (c->curr_recovery_pass > BCH_RECOVERY_PASS_check_alloc_info) {
1727 bch2_trans_inconsistent(trans,
1728 "clearing need_discard but journal_seq %llu > flushed_seq %llu\n"
1729 "%s",
1730 a->v.journal_seq,
1731 c->journal.flushed_seq_ondisk,
1732 (bch2_bkey_val_to_text(&buf, c, k), buf.buf));
1733 ret = -EIO;
1734 }
1735 goto out;
1736 }
1737
1738 if (a->v.data_type != BCH_DATA_need_discard) {
1739 if (c->curr_recovery_pass > BCH_RECOVERY_PASS_check_alloc_info) {
1740 bch2_trans_inconsistent(trans,
1741 "bucket incorrectly set in need_discard btree\n"
1742 "%s",
1743 (bch2_bkey_val_to_text(&buf, c, k), buf.buf));
1744 ret = -EIO;
1745 }
1746
1747 goto out;
1748 }
1749
1750 if (discard_in_flight_add(c, SPOS(iter.pos.inode, iter.pos.offset, true)))
1751 goto out;
We this goto out.
1752
1753 discard_locked = true;
1754
1755 if (!bkey_eq(*discard_pos_done, iter.pos) &&
1756 ca->mi.discard && !c->opts.nochanges) {
1757 /*
1758 * This works without any other locks because this is the only
1759 * thread that removes items from the need_discard tree
1760 */
1761 bch2_trans_unlock_long(trans);
1762 blkdev_issue_discard(ca->disk_sb.bdev,
1763 k.k->p.offset * ca->mi.bucket_size,
1764 ca->mi.bucket_size,
1765 GFP_KERNEL);
1766 *discard_pos_done = iter.pos;
1767
1768 ret = bch2_trans_relock_notrace(trans);
1769 if (ret)
1770 goto out;
1771 }
1772
1773 SET_BCH_ALLOC_V4_NEED_DISCARD(&a->v, false);
1774 a->v.data_type = alloc_data_type(a->v, a->v.data_type);
1775 write:
1776 ret = bch2_trans_update(trans, &iter, &a->k_i, 0) ?:
1777 bch2_trans_commit(trans, NULL, NULL,
1778 BCH_WATERMARK_btree|
1779 BCH_TRANS_COMMIT_no_enospc);
1780 if (ret)
1781 goto out;
1782
1783 count_event(c, bucket_discard);
1784 s->discarded++;
1785 out:
1786 if (discard_locked)
1787 discard_in_flight_remove(c, iter.pos);
1788 s->seen++;
1789 bch2_trans_iter_exit(trans, &iter);
1790 percpu_ref_put(&ca->io_ref);
1791 printbuf_exit(&buf);
1792 return ret;
It returns ret.
1793 }
regards,
dan carpenter
