[bug report] bcachefs: Kill bch2_fs_usage_scratch_get()

* [bug report] bcachefs: Kill bch2_fs_usage_scratch_get()
@ 2023-09-14 13:38 Dan Carpenter
  2023-09-20  2:16 ` Kent Overstreet
  0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2023-09-14 13:38 UTC (permalink / raw)
  To: kent.overstreet; +Cc: linux-bcachefs

Hello Kent Overstreet,

The patch 502bed819cc3: "bcachefs: Kill bch2_fs_usage_scratch_get()"
from Apr 3, 2021 (linux-next), leads to the following Smatch static
checker warning:

	fs/bcachefs/buckets.c:1352 bch2_trans_fs_usage_apply()
	error: we previously assumed 'trans->disk_res' could be null (see line 1302)

fs/bcachefs/buckets.c
    1296 int bch2_trans_fs_usage_apply(struct btree_trans *trans,
    1297                               struct replicas_delta_list *deltas)
    1298 {
    1299         struct bch_fs *c = trans->c;
    1300         static int warned_disk_usage = 0;
    1301         bool warn = false;
    1302         unsigned disk_res_sectors = trans->disk_res ? trans->disk_res->sectors : 0;
                                             ^^^^^^^^^^^^^^^
This code is quite subtle, but the static checker says that we check
for NULL here.

    1303         struct replicas_delta *d = deltas->d, *d2;
    1304         struct replicas_delta *top = (void *) deltas->d + deltas->used;
    1305         struct bch_fs_usage *dst;
    1306         s64 added = 0, should_not_have_added;
    1307         unsigned i;
    1308 
    1309         percpu_down_read(&c->mark_lock);
    1310         preempt_disable();
    1311         dst = fs_usage_ptr(c, trans->journal_res.seq, false);
    1312 
    1313         for (d = deltas->d; d != top; d = replicas_delta_next(d)) {
    1314                 switch (d->r.data_type) {
    1315                 case BCH_DATA_btree:
    1316                 case BCH_DATA_user:
    1317                 case BCH_DATA_parity:
    1318                         added += d->delta;

And we set added to non-zero here so that doesn't seem related.  Or the
relationship is not obvious.

    1319                 }
    1320 
    1321                 if (__update_replicas(c, dst, &d->r, d->delta))
    1322                         goto need_mark;
    1323         }
    1324 
    1325         dst->nr_inodes += deltas->nr_inodes;
    1326 
    1327         for (i = 0; i < BCH_REPLICAS_MAX; i++) {
    1328                 added                                += deltas->persistent_reserved[i];
    1329                 dst->reserved                        += deltas->persistent_reserved[i];
    1330                 dst->persistent_reserved[i]        += deltas->persistent_reserved[i];
    1331         }
    1332 
    1333         /*
    1334          * Not allowed to reduce sectors_available except by getting a
    1335          * reservation:
    1336          */
    1337         should_not_have_added = added - (s64) disk_res_sectors;
    1338         if (unlikely(should_not_have_added > 0)) {
    1339                 u64 old, new, v = atomic64_read(&c->sectors_available);
    1340 
    1341                 do {
    1342                         old = v;
    1343                         new = max_t(s64, 0, old - should_not_have_added);
    1344                 } while ((v = atomic64_cmpxchg(&c->sectors_available,
    1345                                                old, new)) != old);
    1346 
    1347                 added -= should_not_have_added;
    1348                 warn = true;
    1349         }
    1350 
    1351         if (added > 0) {
--> 1352                 trans->disk_res->sectors -= added;
                         ^^^^^^^^^^^^^^^^^
Unchecked dereference.

    1353                 this_cpu_sub(*c->online_reserved, added);
    1354         }
    1355 
    1356         preempt_enable();
    1357         percpu_up_read(&c->mark_lock);
    1358 
    1359         if (unlikely(warn) && !xchg(&warned_disk_usage, 1))
    1360                 bch2_trans_inconsistent(trans,
    1361                                         "disk usage increased %lli more than %u sectors reserved)",
    1362                                         should_not_have_added, disk_res_sectors);
    1363         return 0;
    1364 need_mark:
    1365         /* revert changes: */
    1366         for (d2 = deltas->d; d2 != d; d2 = replicas_delta_next(d2))
    1367                 BUG_ON(__update_replicas(c, dst, &d2->r, -d2->delta));
    1368 
    1369         preempt_enable();
    1370         percpu_up_read(&c->mark_lock);
    1371         return -1;
    1372 }

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 2+ messages in thread