Bug #38682: should report EINVAL in ErasureCode::parse() if m<=0 - RADOS - Ceph

Actions

Copy link

Bug #38682

closed

should report EINVAL in ErasureCode::parse() if m<=0

Added by Kefu Chai about 5 years ago. Updated almost 5 years ago.

Status:

Resolved

Priority:

High

Assignee:

Category:

Correctness/Safety

Target version:

% Done:

Source:

Tags:

low-hanging-fruit

Backport:

luminous,mimic

Regression:

Severity:

3 - minor

Reviewed:

Affected Versions:

ceph-qa-suite:

Component(RADOS):

Pull request ID:

26894

Crash signature (v1):

Crash signature (v2):

Description

MDS=0 MGR=1 OSD=4 MON=1 ../src/vstart.sh -n --without-dashboard --memstore -X
ceph osd erasure-code-profile set isaprofile1 k=4 m=-1
ceph osd pool create ecpool1 4 4 erasure isaprofile1
rados -p ecpool1 put foo-obj Makefile

 ceph version Development (no_version) nautilus (rc)
 1: (()+0x2b70010) [0x563a4d1cc010]
 2: (()+0x12730) [0x7fabf2945730]
 3: (gsignal()+0x10b) [0x7fabf23d58bb]
 4: (abort()+0x121) [0x7fabf23c0535]
 5: (ceph::__ceph_assert_fail(char const*, char const*, int, char const*)+0x36c) [0x563a4d235874]
 6: (()+0x2bd9927) [0x563a4d235927]
 7: (ECUtil::HashInfo::append(unsigned long, std::map<int, ceph::buffer::v14_2_0::list, std::less<int>, std::allocator<std::pair<int const, ceph::buffer::v14_2_0::list> > >&)+0xb1) [0x563a4cd32fe5]
 8: (encode_and_write(pg_t, hobject_t const&, ECUtil::stripe_info_t const&, std::shared_ptr<ceph::ErasureCodeInterface>&, std::set<int, std::less<int>, std::allocator<int> > const&, unsigned long, ceph::buffer::v14_2_0::list, unsigned int, std::shared_ptr<ECUtil::HashInfo>, interval_map<unsigned long, ceph::buffer::v14_2_0::list, bl_split_merge>&, std::map<shard_id_t, ObjectStore::Transaction, std::less<shard_id_t>, std::allocator<std::pair<shard_id_t const, ObjectStore::Transaction> > >*, DoutPrefixProvider*)+0x416) [0x563a4ce8923e]
 9: (()+0x2833279) [0x563a4ce8f279]
 10: (()+0x28347de) [0x563a4ce907de]
 11: (ECTransaction::generate_transactions(ECTransaction::WritePlan&, std::shared_ptr<ceph::ErasureCodeInterface>&, pg_t, ECUtil::stripe_info_t const&, std::map<hobject_t, interval_map<unsigned long, ceph::buffer::v14_2_0::list, bl_split_merge>, std::less<hobject_t>, std::allocator<std::pair<hobject_t const, interval_map<unsigned long, ceph::buffer::v14_2_0::list, bl_split_merge> > > > const&, std::vector<pg_log_entry_t, std::allocator<pg_log_entry_t> >&, std::map<hobject_t, interval_map<unsigned long, ceph::buffer::v14_2_0::list, bl_split_merge>, std::less<hobject_t>, std::allocator<std::pair<hobject_t const, interval_map<unsigned long, ceph::buffer::v14_2_0::list, bl_split_merge> > > >*, std::map<shard_id_t, ObjectStore::Transaction, std::less<shard_id_t>, std::allocator<std::pair<shard_id_t const, ObjectStore::Transaction> > >*, std::set<hobject_t, std::less<hobject_t>, std::allocator<hobject_t> >*, std::set<hobject_t, std::less<hobject_t>, std::allocator<hobject_t> >*, DoutPrefixProvider*)+0x2e0) [0x563a4ce90352]
 12: (ECBackend::try_reads_to_commit()+0x654) [0x563a4ce3bcf4]
 13: (ECBackend::check_ops()+0x28) [0x563a4ce3d914]
...

Related issues 2 (0 open — 2 closed)

Actions

Copy link

Updated by Kefu Chai about 5 years ago

i think we can even go further -- to prevent user from creating a profile with m=0. technically, it's correct. but practically, it defeats the purpose of erasure by disabling the erasure codec from creating code chunks.

also, we should not use

err |= to_int("k", profile, &k, DEFAULT_K, ss);

for updating the errcode, as it assumes that to_int() either returns 0 or a certain number which is identical to err. in that case, would be better to just return a boolean.

Actions

Copy link

Updated by Sage Weil about 5 years ago

Subject changed from should report EINVAL in ErasureCode::parse() if m<0 to should report EINVAL in ErasureCode::parse() if m<=0
Status changed from New to 12
Priority changed from Normal to High

I agree that m=0 is useless--it's no better than num_rep=1... just a (much) more complicated code path and more corner cases to worry about.

Actions

Copy link