From 9fc4b5d675cd6dc0b2503abe95b1c761d8d05abe Mon Sep 17 00:00:00 2001 From: Kent Overstreet Date: Sat, 4 Mar 2023 07:35:29 -0500 Subject: [PATCH] rust: Fix ptr casting in Fs::open() Signed-off-by: Kent Overstreet --- .bcachefs_revision | 2 +- libbcachefs/alloc_foreground.c | 8 +- libbcachefs/alloc_types.h | 3 +- libbcachefs/btree_gc.c | 28 +++--- libbcachefs/buckets.h | 3 + libbcachefs/ec.c | 156 +++++++++++++++++++++----------- libbcachefs/ec.h | 5 +- libbcachefs/errcode.h | 1 + rust-src/bch_bindgen/src/fs.rs | 11 +-- rust-src/bch_bindgen/src/lib.rs | 2 +- 10 files changed, 136 insertions(+), 83 deletions(-) diff --git a/.bcachefs_revision b/.bcachefs_revision index afaf4c1..033b6af 100644 --- a/.bcachefs_revision +++ b/.bcachefs_revision @@ -1 +1 @@ -2272c5f5b76a5dc0c925064b3682110218a3e53b +c28937622fbd373f152df01f29efa2d79af99633 diff --git a/libbcachefs/alloc_foreground.c b/libbcachefs/alloc_foreground.c index af1c1e2..b2755c1 100644 --- a/libbcachefs/alloc_foreground.c +++ b/libbcachefs/alloc_foreground.c @@ -754,6 +754,7 @@ static int bucket_alloc_from_stripe(struct btree_trans *trans, unsigned nr_replicas, unsigned *nr_effective, bool *have_cache, + enum alloc_reserve reserve, unsigned flags, struct closure *cl) { @@ -771,9 +772,7 @@ static int bucket_alloc_from_stripe(struct btree_trans *trans, if (ec_open_bucket(c, ptrs)) return 0; - h = bch2_ec_stripe_head_get(trans, target, 0, nr_replicas - 1, - wp == &c->copygc_write_point, - cl); + h = bch2_ec_stripe_head_get(trans, target, 0, nr_replicas - 1, reserve, cl); if (IS_ERR(h)) return PTR_ERR(h); if (!h) @@ -958,7 +957,8 @@ static int __open_bucket_add_buckets(struct btree_trans *trans, ret = bucket_alloc_from_stripe(trans, ptrs, wp, &devs, target, nr_replicas, nr_effective, - have_cache, flags, _cl); + have_cache, + reserve, flags, _cl); } else { retry_blocking: /* diff --git a/libbcachefs/alloc_types.h b/libbcachefs/alloc_types.h index 8bfd4e6..cd0c50a 100644 --- a/libbcachefs/alloc_types.h +++ b/libbcachefs/alloc_types.h @@ -22,7 +22,8 @@ struct ec_bucket_buf; x(btree_movinggc) \ x(btree) \ x(movinggc) \ - x(none) + x(none) \ + x(stripe) enum alloc_reserve { #define x(name) RESERVE_##name, diff --git a/libbcachefs/btree_gc.c b/libbcachefs/btree_gc.c index 5712e5c..65fda70 100644 --- a/libbcachefs/btree_gc.c +++ b/libbcachefs/btree_gc.c @@ -1690,6 +1690,7 @@ static int bch2_gc_write_stripes_key(struct btree_trans *trans, struct printbuf buf = PRINTBUF; const struct bch_stripe *s; struct gc_stripe *m; + bool bad = false; unsigned i; int ret = 0; @@ -1699,18 +1700,21 @@ static int bch2_gc_write_stripes_key(struct btree_trans *trans, s = bkey_s_c_to_stripe(k).v; m = genradix_ptr(&c->gc_stripes, k.k->p.offset); - for (i = 0; i < s->nr_blocks; i++) - if (stripe_blockcount_get(s, i) != (m ? m->block_sectors[i] : 0)) - goto inconsistent; - return 0; -inconsistent: - if (fsck_err_on(true, c, - "stripe has wrong block sector count %u:\n" - " %s\n" - " should be %u", i, - (printbuf_reset(&buf), - bch2_bkey_val_to_text(&buf, c, k), buf.buf), - m ? m->block_sectors[i] : 0)) { + for (i = 0; i < s->nr_blocks; i++) { + u32 old = stripe_blockcount_get(s, i); + u32 new = (m ? m->block_sectors[i] : 0); + + if (old != new) { + prt_printf(&buf, "stripe block %u has wrong sector count: got %u, should be %u\n", + i, old, new); + bad = true; + } + } + + if (bad) + bch2_bkey_val_to_text(&buf, c, k); + + if (fsck_err_on(bad, c, "%s", buf.buf)) { struct bkey_i_stripe *new; new = bch2_trans_kmalloc(trans, bkey_bytes(k.k)); diff --git a/libbcachefs/buckets.h b/libbcachefs/buckets.h index 22721bf..d677b02 100644 --- a/libbcachefs/buckets.h +++ b/libbcachefs/buckets.h @@ -157,6 +157,9 @@ static inline u64 bch2_dev_buckets_reserved(struct bch_dev *ca, enum alloc_reser switch (reserve) { case RESERVE_NR: unreachable(); + case RESERVE_stripe: + reserved += ca->mi.nbuckets >> 6; + fallthrough; case RESERVE_none: reserved += ca->mi.nbuckets >> 6; fallthrough; diff --git a/libbcachefs/ec.c b/libbcachefs/ec.c index a09b39c..f759888 100644 --- a/libbcachefs/ec.c +++ b/libbcachefs/ec.c @@ -866,8 +866,18 @@ static int ec_stripe_key_update(struct btree_trans *trans, goto err; } - for (i = 0; i < new->v.nr_blocks; i++) - stripe_blockcount_set(&new->v, i, stripe_blockcount_get(old, i)); + for (i = 0; i < new->v.nr_blocks; i++) { + unsigned v = stripe_blockcount_get(old, i); + + if (!v) + continue; + + BUG_ON(old->ptrs[i].dev != new->v.ptrs[i].dev || + old->ptrs[i].gen != new->v.ptrs[i].gen || + old->ptrs[i].offset != new->v.ptrs[i].offset); + + stripe_blockcount_set(&new->v, i, v); + } } ret = bch2_trans_update(trans, &iter, &new->k_i, 0); @@ -1336,7 +1346,7 @@ static int ec_new_stripe_alloc(struct bch_fs *c, struct ec_stripe_head *h) static struct ec_stripe_head * ec_new_stripe_head_alloc(struct bch_fs *c, unsigned target, unsigned algo, unsigned redundancy, - bool copygc) + enum alloc_reserve reserve) { struct ec_stripe_head *h; struct bch_dev *ca; @@ -1352,7 +1362,7 @@ ec_new_stripe_head_alloc(struct bch_fs *c, unsigned target, h->target = target; h->algo = algo; h->redundancy = redundancy; - h->copygc = copygc; + h->reserve = reserve; rcu_read_lock(); h->devs = target_rw_devs(c, BCH_DATA_user, target); @@ -1387,7 +1397,7 @@ struct ec_stripe_head *__bch2_ec_stripe_head_get(struct btree_trans *trans, unsigned target, unsigned algo, unsigned redundancy, - bool copygc) + enum alloc_reserve reserve) { struct bch_fs *c = trans->c; struct ec_stripe_head *h; @@ -1404,21 +1414,21 @@ struct ec_stripe_head *__bch2_ec_stripe_head_get(struct btree_trans *trans, if (h->target == target && h->algo == algo && h->redundancy == redundancy && - h->copygc == copygc) { + h->reserve == reserve) { ret = bch2_trans_mutex_lock(trans, &h->lock); if (ret) h = ERR_PTR(ret); goto found; } - h = ec_new_stripe_head_alloc(c, target, algo, redundancy, copygc); + h = ec_new_stripe_head_alloc(c, target, algo, redundancy, reserve); found: mutex_unlock(&c->ec_stripe_head_lock); return h; } static int new_stripe_alloc_buckets(struct btree_trans *trans, struct ec_stripe_head *h, - struct closure *cl) + enum alloc_reserve reserve, struct closure *cl) { struct bch_fs *c = trans->c; struct bch_devs_mask devs = h->devs; @@ -1428,14 +1438,12 @@ static int new_stripe_alloc_buckets(struct btree_trans *trans, struct ec_stripe_ bool have_cache = true; int ret = 0; - for (i = 0; i < h->s->new_stripe.key.v.nr_blocks; i++) { - if (test_bit(i, h->s->blocks_gotten)) { - __clear_bit(h->s->new_stripe.key.v.ptrs[i].dev, devs.d); - if (i < h->s->nr_data) - nr_have_data++; - else - nr_have_parity++; - } + for_each_set_bit(i, h->s->blocks_gotten, h->s->new_stripe.key.v.nr_blocks) { + __clear_bit(h->s->new_stripe.key.v.ptrs[i].dev, devs.d); + if (i < h->s->nr_data) + nr_have_data++; + else + nr_have_parity++; } BUG_ON(nr_have_data > h->s->nr_data); @@ -1450,9 +1458,7 @@ static int new_stripe_alloc_buckets(struct btree_trans *trans, struct ec_stripe_ &nr_have_parity, &have_cache, BCH_DATA_parity, - h->copygc - ? RESERVE_movinggc - : RESERVE_none, + reserve, cl); open_bucket_for_each(c, &buckets, ob, i) { @@ -1479,9 +1485,7 @@ static int new_stripe_alloc_buckets(struct btree_trans *trans, struct ec_stripe_ &nr_have_data, &have_cache, BCH_DATA_user, - h->copygc - ? RESERVE_movinggc - : RESERVE_none, + reserve, cl); open_bucket_for_each(c, &buckets, ob, i) { @@ -1548,10 +1552,11 @@ static int __bch2_ec_stripe_head_reuse(struct btree_trans *trans, struct ec_stri if (idx < 0) return -BCH_ERR_ENOSPC_stripe_reuse; - h->s->have_existing_stripe = true; ret = get_stripe_key_trans(trans, idx, &h->s->existing_stripe); if (ret) { - bch2_fs_fatal_error(c, "error reading stripe key: %i", ret); + bch2_stripe_close(c, h->s); + if (!bch2_err_matches(ret, BCH_ERR_transaction_restart)) + bch2_fs_fatal_error(c, "error reading stripe key: %s", bch2_err_str(ret)); return ret; } @@ -1566,6 +1571,17 @@ static int __bch2_ec_stripe_head_reuse(struct btree_trans *trans, struct ec_stri BUG_ON(h->s->existing_stripe.size != h->blocksize); BUG_ON(h->s->existing_stripe.size != h->s->existing_stripe.key.v.sectors); + /* + * Free buckets we initially allocated - they might conflict with + * blocks from the stripe we're reusing: + */ + for_each_set_bit(i, h->s->blocks_gotten, h->s->new_stripe.key.v.nr_blocks) { + bch2_open_bucket_put(c, c->open_buckets + h->s->blocks[i]); + h->s->blocks[i] = 0; + } + memset(h->s->blocks_gotten, 0, sizeof(h->s->blocks_gotten)); + memset(h->s->blocks_allocated, 0, sizeof(h->s->blocks_allocated)); + for (i = 0; i < h->s->existing_stripe.key.v.nr_blocks; i++) { if (stripe_blockcount_get(&h->s->existing_stripe.key.v, i)) { __set_bit(i, h->s->blocks_gotten); @@ -1575,8 +1591,10 @@ static int __bch2_ec_stripe_head_reuse(struct btree_trans *trans, struct ec_stri ec_block_io(c, &h->s->existing_stripe, READ, i, &h->s->iodone); } - bkey_copy(&h->s->new_stripe.key.k_i, - &h->s->existing_stripe.key.k_i); + bkey_copy(&h->s->new_stripe.key.k_i, &h->s->existing_stripe.key.k_i); + h->s->have_existing_stripe = true; + + pr_info("reused %llu", h->s->idx); return 0; } @@ -1590,13 +1608,14 @@ static int __bch2_ec_stripe_head_reserve(struct btree_trans *trans, struct ec_st struct bpos start_pos = bpos_max(min_pos, POS(0, c->ec_stripe_hint)); int ret; - BUG_ON(h->s->res.sectors); - - ret = bch2_disk_reservation_get(c, &h->s->res, + if (!h->s->res.sectors) { + ret = bch2_disk_reservation_get(c, &h->s->res, h->blocksize, - h->s->nr_parity, 0); - if (ret) - return ret; + h->s->nr_parity, + BCH_DISK_RESERVATION_NOFAIL); + if (ret) + return ret; + } for_each_btree_key_norestart(trans, iter, BTREE_ID_stripes, start_pos, BTREE_ITER_SLOTS|BTREE_ITER_INTENT, k, ret) { @@ -1640,22 +1659,21 @@ struct ec_stripe_head *bch2_ec_stripe_head_get(struct btree_trans *trans, unsigned target, unsigned algo, unsigned redundancy, - bool copygc, + enum alloc_reserve reserve, struct closure *cl) { struct bch_fs *c = trans->c; struct ec_stripe_head *h; + bool waiting = false; int ret; - bool needs_stripe_new; - h = __bch2_ec_stripe_head_get(trans, target, algo, redundancy, copygc); + h = __bch2_ec_stripe_head_get(trans, target, algo, redundancy, reserve); if (!h) bch_err(c, "no stripe head"); if (IS_ERR_OR_NULL(h)) return h; - needs_stripe_new = !h->s; - if (needs_stripe_new) { + if (!h->s) { if (ec_new_stripe_alloc(c, h)) { ret = -ENOMEM; bch_err(c, "failed to allocate new stripe"); @@ -1666,32 +1684,60 @@ struct ec_stripe_head *bch2_ec_stripe_head_get(struct btree_trans *trans, BUG(); } - /* - * Try reserve a new stripe before reusing an - * existing stripe. This will prevent unnecessary - * read amplification during write oriented workloads. - */ - ret = 0; - if (!h->s->allocated && !h->s->res.sectors && !h->s->have_existing_stripe) - ret = __bch2_ec_stripe_head_reserve(trans, h); - if (bch2_err_matches(ret, BCH_ERR_transaction_restart)) + if (h->s->allocated) + goto allocated; + + if (h->s->idx) + goto alloc_existing; +#if 0 + /* First, try to allocate a full stripe: */ + ret = new_stripe_alloc_buckets(trans, h, RESERVE_stripe, NULL) ?: + __bch2_ec_stripe_head_reserve(trans, h); + if (!ret) + goto allocated; + if (bch2_err_matches(ret, BCH_ERR_transaction_restart) || + bch2_err_matches(ret, ENOMEM)) goto err; - if (ret && needs_stripe_new) - ret = __bch2_ec_stripe_head_reuse(trans, h); - if (ret) { - bch_err_ratelimited(c, "failed to get stripe: %s", bch2_err_str(ret)); - goto err; + if (ret == -BCH_ERR_open_buckets_empty) { + /* don't want to reuse in this case */ } - - if (!h->s->allocated) { - ret = new_stripe_alloc_buckets(trans, h, cl); +#endif + /* + * Not enough buckets available for a full stripe: we must reuse an + * existing stripe: + */ + while (1) { + ret = __bch2_ec_stripe_head_reuse(trans, h); if (ret) + ret = __bch2_ec_stripe_head_reserve(trans, h); + if (!ret) + break; + pr_info("err %s", bch2_err_str(ret)); + if (ret == -BCH_ERR_ENOSPC_stripe_reuse && cl) + ret = -BCH_ERR_stripe_alloc_blocked; + if (waiting || !cl) goto err; - h->s->allocated = true; + /* XXX freelist_wait? */ + closure_wait(&c->freelist_wait, cl); + waiting = true; } + if (waiting) + closure_wake_up(&c->freelist_wait); +alloc_existing: + /* + * Retry allocating buckets, with the reserve watermark for this + * particular write: + */ + ret = new_stripe_alloc_buckets(trans, h, reserve, cl); + if (ret) + goto err; +allocated: + h->s->allocated = true; + BUG_ON(!h->s->idx); + BUG_ON(trans->restarted); return h; err: diff --git a/libbcachefs/ec.h b/libbcachefs/ec.h index c9e4fb2..56d1b5e 100644 --- a/libbcachefs/ec.h +++ b/libbcachefs/ec.h @@ -181,7 +181,7 @@ struct ec_stripe_head { unsigned target; unsigned algo; unsigned redundancy; - bool copygc; + enum alloc_reserve reserve; struct bch_devs_mask devs; unsigned nr_active_devs; @@ -205,7 +205,8 @@ int bch2_ec_stripe_new_alloc(struct bch_fs *, struct ec_stripe_head *); void bch2_ec_stripe_head_put(struct bch_fs *, struct ec_stripe_head *); struct ec_stripe_head *bch2_ec_stripe_head_get(struct btree_trans *, - unsigned, unsigned, unsigned, bool, struct closure *); + unsigned, unsigned, unsigned, + enum alloc_reserve, struct closure *); void bch2_stripes_heap_update(struct bch_fs *, struct stripe *, size_t); void bch2_stripes_heap_del(struct bch_fs *, struct stripe *, size_t); diff --git a/libbcachefs/errcode.h b/libbcachefs/errcode.h index 6129af6..283303d 100644 --- a/libbcachefs/errcode.h +++ b/libbcachefs/errcode.h @@ -93,6 +93,7 @@ x(BCH_ERR_operation_blocked, journal_res_get_blocked) \ x(BCH_ERR_operation_blocked, journal_preres_get_blocked) \ x(BCH_ERR_operation_blocked, bucket_alloc_blocked) \ + x(BCH_ERR_operation_blocked, stripe_alloc_blocked) \ x(BCH_ERR_invalid, invalid_sb) \ x(BCH_ERR_invalid_sb, invalid_sb_magic) \ x(BCH_ERR_invalid_sb, invalid_sb_version) \ diff --git a/rust-src/bch_bindgen/src/fs.rs b/rust-src/bch_bindgen/src/fs.rs index 1176846..b26c51b 100644 --- a/rust-src/bch_bindgen/src/fs.rs +++ b/rust-src/bch_bindgen/src/fs.rs @@ -9,15 +9,12 @@ pub struct Fs { } impl Fs { - pub fn open(devices: &Vec, opts: c::bch_opts) -> Result { - let devices: Vec<_> = devices.iter() - .map(|i| CString::new(i.as_os_str().as_bytes()).unwrap()).collect(); - let dev_c_strs: Vec<_> = devices.iter() - .map(|i| { let p: *const i8 = i.as_ptr(); p }) + pub fn open(devs: &Vec, opts: c::bch_opts) -> Result { + let devs: Vec<_> = devs.iter() + .map(|i| CString::new(i.as_os_str().as_bytes()).unwrap().into_raw()) .collect(); - let dev_c_strarray: *const *mut i8 = dev_c_strs[..].as_ptr() as *const *mut i8; - let ret = unsafe { c::bch2_fs_open(dev_c_strarray, dev_c_strs.len() as u32, opts) }; + let ret = unsafe { c::bch2_fs_open(devs[..].as_ptr(), devs.len() as u32, opts) }; errptr_to_result(ret).map(|fs| Fs { raw: fs}) } diff --git a/rust-src/bch_bindgen/src/lib.rs b/rust-src/bch_bindgen/src/lib.rs index 3bc21a1..d2b5851 100644 --- a/rust-src/bch_bindgen/src/lib.rs +++ b/rust-src/bch_bindgen/src/lib.rs @@ -91,7 +91,7 @@ impl FromStr for c::btree_id { fn from_str(s: &str) -> Result { let s = CString::new(s).unwrap(); - let p: *const i8 = s.as_ptr(); + let p = s.as_ptr(); let v = unsafe {c::match_string(c::bch2_btree_ids[..].as_ptr(), (-(1 as isize)) as usize, p)}; if v >= 0 { -- 2.39.2