> On Jun 20, 2025, at 04:35, Kent Overstreet <[email protected]> wrote:
>
> On Fri, Jun 20, 2025 at 03:58:26AM +0800, Alan Huang wrote:
>> Signed-off-by: Alan Huang <[email protected]>
>> ---
>> fs/bcachefs/btree_iter.c | 65 ++++++++++++++++++++--------------------
>> fs/bcachefs/btree_iter.h | 14 +++++++++
>> 2 files changed, 46 insertions(+), 33 deletions(-)
>>
>> diff --git a/fs/bcachefs/btree_iter.c b/fs/bcachefs/btree_iter.c
>> index 352f9cd2634f..0168c4023c74 100644
>> --- a/fs/bcachefs/btree_iter.c
>> +++ b/fs/bcachefs/btree_iter.c
>> @@ -3238,32 +3238,30 @@ void *__bch2_trans_kmalloc(struct btree_trans
>> *trans, size_t size, unsigned long
>> }
>>
>> EBUG_ON(trans->mem);
>> + EBUG_ON(trans->mem_bytes);
>> + EBUG_ON(trans->mem_top);
>
> ???
It’s the first time we allocate memory.
>
>> + EBUG_ON(new_bytes > BTREE_TRANS_MEM_MAX);
>
> This can't be a BUG_ON(), it's not something we can prove can't happen
> with static analysis. It needs to be a warning (with debug info! like
> the previous warning provided!)
>
If new_bytes > BTREE_TRANS_MEM_MAX, we only return -ENOMEM
>> +
>> + bool lock_dropped = false;
>> + new_mem = allocate_dropping_locks_norelock(trans, lock_dropped,
>> kmalloc(new_bytes, _gfp));
>> + if (!new_mem) {
>> + new_mem = mempool_alloc(&c->btree_trans_mem_pool, GFP_KERNEL);
>> + new_bytes = BTREE_TRANS_MEM_MAX;
>> + trans->used_mempool = true;
>> + }
>>
>> - new_mem = kmalloc(new_bytes, GFP_NOWAIT|__GFP_NOWARN);
>> - if (unlikely(!new_mem)) {
>> - bch2_trans_unlock(trans);
>> -
>> - new_mem = kmalloc(new_bytes, GFP_KERNEL);
>> - if (!new_mem && new_bytes <= BTREE_TRANS_MEM_MAX) {
>> - new_mem = mempool_alloc(&c->btree_trans_mem_pool, GFP_KERNEL);
>> - new_bytes = BTREE_TRANS_MEM_MAX;
>> - trans->used_mempool = true;
>> - }
>> -
>> - EBUG_ON(!new_mem);
>> + EBUG_ON(!new_mem);
>>
>> - trans->mem = new_mem;
>> - trans->mem_bytes = new_bytes;
>> + trans->mem = new_mem;
>> + trans->mem_bytes = new_bytes;
>>
>> + if (unlikely(lock_dropped)) {
>> ret = bch2_trans_relock(trans);
>> if (ret)
>> return ERR_PTR(ret);
>> }
>>
>> - trans->mem = new_mem;
>> - trans->mem_bytes = new_bytes;
>> -
>> - p = trans->mem + trans->mem_top;
>> + p = trans->mem;
>> trans->mem_top += size;
>> memset(p, 0, size);
>> return p;
>> @@ -3324,22 +3322,23 @@ u32 bch2_trans_begin(struct btree_trans *trans)
>> trans->mem_top = 0;
>>
>> if (trans->restarted == BCH_ERR_transaction_restart_mem_realloced) {
>> - EBUG_ON(!trans->mem || !trans->mem_bytes);
>> unsigned new_bytes = trans->realloc_bytes_required;
>> - void *new_mem = krealloc(trans->mem, new_bytes, GFP_NOWAIT|__GFP_NOWARN);
>> - if (unlikely(!new_mem)) {
>> - bch2_trans_unlock(trans);
>> - new_mem = krealloc(trans->mem, new_bytes, GFP_KERNEL);
>> -
>> - EBUG_ON(new_bytes > BTREE_TRANS_MEM_MAX);
>> -
>> - if (!new_mem) {
>> - new_mem = mempool_alloc(&trans->c->btree_trans_mem_pool, GFP_KERNEL);
>> - new_bytes = BTREE_TRANS_MEM_MAX;
>> - trans->used_mempool = true;
>> - kfree(trans->mem);
>> - }
>> - }
>> + EBUG_ON(new_bytes > BTREE_TRANS_MEM_MAX);
>> + EBUG_ON(!trans->mem);
>> + EBUG_ON(!trans->mem_bytes);
>
> Not sure what the purpose of these assertions even is
>
Make sure it’s a reallocation
>> +
>> + bool lock_dropped = false;
>> + void *new_mem = allocate_dropping_locks_norelock(trans, lock_dropped,
>> + krealloc(trans->mem, new_bytes, _gfp));
>> + if (!new_mem) {
>> + new_mem = mempool_alloc(&trans->c->btree_trans_mem_pool, GFP_KERNEL);
>> + new_bytes = BTREE_TRANS_MEM_MAX;
>> + trans->used_mempool = true;
>> + kfree(trans->mem);
>> + }
>> +
>> + EBUG_ON(!new_mem);
>> +
>> trans->mem = new_mem;
>> trans->mem_bytes = new_bytes;
>> }
>> diff --git a/fs/bcachefs/btree_iter.h b/fs/bcachefs/btree_iter.h
>> index 09dd3e52622e..cc2c6bb6b6a8 100644
>> --- a/fs/bcachefs/btree_iter.h
>> +++ b/fs/bcachefs/btree_iter.h
>> @@ -963,6 +963,20 @@ struct bkey_s_c
>> bch2_btree_iter_peek_and_restart_outlined(struct btree_trans *,
>> _p; \
>> })
>>
>> +#define allocate_dropping_locks_norelock(_trans, _lock_dropped, _do) \
>> +({ \
>> + gfp_t _gfp = GFP_NOWAIT|__GFP_NOWARN; \
>> + typeof(_do) _p = _do; \
>> + _lock_dropped = false; \
>> + if (unlikely(!_p)) { \
>> + bch2_trans_unlock(_trans); \
>> + _lock_dropped = true; \
>> + _gfp = GFP_KERNEL; \
>> + _p = _do; \
>> + } \
>> + _p; \
>> +})
>
> This helper is fine
>
>> +
>> struct btree_trans *__bch2_trans_get(struct bch_fs *, unsigned);
>> void bch2_trans_put(struct btree_trans *);
>>
>> --
>> 2.48.1
