Project

General

Profile

Bug #38682

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

Added by Kefu Chai 12 months ago. Updated 7 months ago.

Status:
Resolved
Priority:
High
Assignee:
-
Category:
Correctness/Safety
Target version:
-
% Done:

0%

Source:
Tags:
low-hanging-fruit
Backport:
luminous,mimic
Regression:
No
Severity:
3 - minor
Reviewed:
Affected Versions:
ceph-qa-suite:
Component(RADOS):
Pull request ID:
Crash signature:

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

Copied to RADOS - Backport #38750: luminous: should report EINVAL in ErasureCode::parse() if m<=0 Resolved
Copied to RADOS - Backport #38751: mimic: should report EINVAL in ErasureCode::parse() if m<=0 Resolved

History

#1 Updated by Kefu Chai 12 months 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.

#2 Updated by Sage Weil 12 months 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.

#3 Updated by Sage Weil 12 months ago

  • Status changed from 12 to Fix Under Review
  • Backport set to luminous,mimic

#4 Updated by Kefu Chai 12 months ago

  • Pull request ID set to 26894

#5 Updated by Sage Weil 11 months ago

  • Status changed from Fix Under Review to Pending Backport

#6 Updated by Nathan Cutler 11 months ago

  • Copied to Backport #38750: luminous: should report EINVAL in ErasureCode::parse() if m<=0 added

#7 Updated by Nathan Cutler 11 months ago

  • Copied to Backport #38751: mimic: should report EINVAL in ErasureCode::parse() if m<=0 added

#8 Updated by Nathan Cutler 7 months ago

  • Status changed from Pending Backport to Resolved

Also available in: Atom PDF