On Wed, Feb 21, 2024 at 09:01:26AM +0300, Dan Carpenter wrote: > 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. naw, that's just making excuses :) you're the engineer - it's our job to figure out the most efficient way of solving the problem and making it happen :) A dashboard doesn't have to be anything fancy whatsoever; just dumping text files to a webserver would already be useful. If we wanted something easier to read, the only thing I would add would be (and I'd have to find someone who knows javascript for this) - with a bit of js and trivial postprocessing of your smatch output you could have an html file where each line is the file and line number of an error, and clicking on it expands to see the actual error. That would be an absolutely trivial project to a web programmer - I've got something similar for my CI, and the javascript is ~20 lines: https://evilpiepirate.org/~testdashboard/ci?branch=bcachefs-testing&commit=ac1591b48eaf5f3fd173b4144ac21b4c36de8428 > > > 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()... :/ Sorry, I was looking inside discard_in_flight_add() > 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. Ok, this is intentional; this is "success; the bucket is already being discarded by another thread so we just bail out and move on to the next thing". It's not as clear as I'd like, but - this is C, I'm not sure we can change it to please smatch without annoying other static analyzers (and, like I was saying, way too much of the linux-bcachefs traffic is already absolutely trivial static analyzer nitpicky bullshit). > > 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
