From e303dc1cf543fa467d6e0a16c122f77570865e21 Mon Sep 17 00:00:00 2001 From: Mykola Golub Date: Tue, 21 Sep 2021 16:17:17 +0100 Subject: [PATCH] osd: handle case when stat returns with error in get_hash_info Previously in the case of the error we stored in the cache and returned HashInfo(ec_impl->get_chunk_count()), which e.g. could propagate to non-primary shards, introducing inconsistency. The function's `checks` flag is replaced with `create` flag, which seems to have more clear meaning here. In be_deep_scrub the get_hash_info is still called with the second argument false (i.e. with `create=false`, while previously it was `checks=false`), which is done intentionally. Fixes: https://tracker.ceph.com/issues/48959 Signed-off-by: Mykola Golub --- src/osd/ECBackend.cc | 11 +++++++---- src/osd/ECBackend.h | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/osd/ECBackend.cc b/src/osd/ECBackend.cc index 15bb059033a..b08be6e6f70 100644 --- a/src/osd/ECBackend.cc +++ b/src/osd/ECBackend.cc @@ -1827,7 +1827,7 @@ void ECBackend::do_read_op(ReadOp &op) } ECUtil::HashInfoRef ECBackend::get_hash_info( - const hobject_t &hoid, bool checks, const map> *attrs) + const hobject_t &hoid, bool create, const map> *attrs) { dout(10) << __func__ << ": Getting attr on " << hoid << dendl; ECUtil::HashInfoRef ref = unstable_hashinfo_registry.lookup(hoid); @@ -1839,7 +1839,6 @@ ECUtil::HashInfoRef ECBackend::get_hash_info( ghobject_t(hoid, ghobject_t::NO_GEN, get_parent()->whoami_shard().shard), &st); ECUtil::HashInfo hinfo(ec_impl->get_chunk_count()); - // XXX: What does it mean if there is no object on disk? if (r >= 0) { dout(10) << __func__ << ": found on disk, size " << st.st_size << dendl; bufferlist bl; @@ -1869,7 +1868,7 @@ ECUtil::HashInfoRef ECBackend::get_hash_info( dout(0) << __func__ << ": Can't decode hinfo for " << hoid << dendl; return ECUtil::HashInfoRef(); } - if (checks && hinfo.get_total_chunk_size() != (uint64_t)st.st_size) { + if (hinfo.get_total_chunk_size() != (uint64_t)st.st_size) { dout(0) << __func__ << ": Mismatch of total_chunk_size " << hinfo.get_total_chunk_size() << dendl; return ECUtil::HashInfoRef(); @@ -1877,6 +1876,10 @@ ECUtil::HashInfoRef ECBackend::get_hash_info( } else if (st.st_size > 0) { // If empty object and no hinfo, create it return ECUtil::HashInfoRef(); } + } else if (r != -ENOENT || !create) { + derr << __func__ << ": stat " << hoid << " failed: " << cpp_strerror(r) + << dendl; + return ECUtil::HashInfoRef(); } ref = unstable_hashinfo_registry.lookup_or_create(hoid, hinfo); } @@ -1891,7 +1894,7 @@ void ECBackend::start_rmw(Op *op, PGTransactionUPtr &&t) sinfo, std::move(t), [&](const hobject_t &i) { - ECUtil::HashInfoRef ref = get_hash_info(i, false); + ECUtil::HashInfoRef ref = get_hash_info(i, true); if (!ref) { derr << __func__ << ": get_hash_info(" << i << ")" << " returned a null pointer and there is no " diff --git a/src/osd/ECBackend.h b/src/osd/ECBackend.h index e5e4acee870..296c1202afa 100644 --- a/src/osd/ECBackend.h +++ b/src/osd/ECBackend.h @@ -629,7 +629,7 @@ public: const ECUtil::stripe_info_t sinfo; /// If modified, ensure that the ref is held until the update is applied SharedPtrRegistry unstable_hashinfo_registry; - ECUtil::HashInfoRef get_hash_info(const hobject_t &hoid, bool checks = true, + ECUtil::HashInfoRef get_hash_info(const hobject_t &hoid, bool create = false, const std::map> *attr = NULL); public: -- 2.39.5