FIX oom and blk alloc overflow which cause OOM #554
Conversation
yamingk
changed the title
shosseinimotlagh
force-pushed
the
fix-meta-oom
branch
from
f0a70f5 to
e7453e4
Compare
shosseinimotlagh
force-pushed
the
fix-meta-oom
branch
from
00a9874 to
aaefa88
Compare
src/engine/blkstore/blkstore.hpp
Outdated
| @@ -325,26 +325,31 @@ class BlkStore { | |||
| BlkAllocStatus alloc_contiguous_blk(const uint32_t size, blk_alloc_hints& hints, BlkId* const out_blkid) { | |||
| // Allocate a block from the device manager | |||
| assert(size % m_pagesz == 0); | |||
| const blk_count_t nblks{static_cast< blk_count_t >(size / m_pagesz)}; | |||
| const uint32_t nblks{static_cast< uint32_t >(size / m_pagesz)}; | |||
There was a problem hiding this comment.
This doesn't make sense to me. We change this from a 16bit value to a 32bit value (to avoid overflow?) and then later immediately cast it back to a 16bit parameter in the call to alloc_contig...; what's the point?
shosseinimotlagh
force-pushed
the
fix-meta-oom
branch
from
aaefa88 to
f057a76
Compare
src/engine/blkstore/blkstore.hpp
Outdated
| if (nblks <= BlkId::max_blks_in_op()) { | ||
| return (m_vdev.alloc_blk(nblks, hints, out_blkid)); | ||
| auto max_val = std::numeric_limits< blk_count_t >::max(); | ||
| HS_DBG_ASSERT_LE(nblks, max_val, "nblks must be less than {}", max_val); |
There was a problem hiding this comment.
nit: do we still this assert since you've added static assert at blk.h? I mean the static assert at blk.h can make sure this assert always true, right? Also can we just use one line: HS_DBG_ASSERT_LE(nblks, std::numeric_limits< blk_count_t >::max(), "nblks must be less than {}", max_val);, although compiler might do the same for optimization. I am fine either way.
There was a problem hiding this comment.
I remove it
| @@ -325,6 +325,9 @@ class BlkStore { | |||
| BlkAllocStatus alloc_contiguous_blk(const uint32_t size, blk_alloc_hints& hints, BlkId* const out_blkid) { | |||
| // Allocate a block from the device manager | |||
| assert(size % m_pagesz == 0); | |||
| static uint32_t max_suppoted_size = m_pagesz * std::numeric_limits< blk_count_t >::max(); | |||
There was a problem hiding this comment.
nit: ideally this max_supported_size should come from Chunk::BlkAlloc::max_supported_size(). We can do it from 4.x
shosseinimotlagh
force-pushed
the
fix-meta-oom
branch
from
f057a76 to
b44e697
Compare
No description provided.