rgw: cls_bucket_list_unordered() might return one redundent entry every time is_truncated is true #42151
Conversation
RGWRados::cls_bucket_list_unordered() will produce one redundent entry every time is_truncated is true.The issue could be easily reproduced when a bucket is filled with amounts of incomplete multipart upload. To be more specific, the number of incomplete multipart upload objects should be greater than 1100. Signed-off-by: Peng Zhang <zhangpeng@vclusters.com>
|
@ivancich Hello, please review when you have a moment, many thanks! |
src/rgw/rgw_rados.cc
Outdated
| string index_hash_source = obj_key.name; | ||
| if (obj_key.ns == RGW_OBJ_NS_MULTIPART) { | ||
| // Use obj_key.ns == RGW_OBJ_NS_MULTIPART instead of | ||
| // the implementation relying on MultipartMetaFilter | ||
| // because MultipartMetaFilter only checks .meta suffix, which may | ||
| // exclude data multiparts but include some regular objects with .meta suffix | ||
| // by mistake. | ||
| string index_hash_source; | ||
| r = parse_index_hash_source(obj_key.name, &index_hash_source); | ||
| if (r < 0) { | ||
| return r; | ||
| } | ||
| current_shard = svc.bi_rados->bucket_shard_index(index_hash_source, num_shards); | ||
| } else { | ||
| current_shard = svc.bi_rados->bucket_shard_index(obj_key.name, num_shards); | ||
| } | ||
| } | ||
| current_shard = svc.bi_rados->bucket_shard_index(index_hash_source, num_shards); |
There was a problem hiding this comment.
Do these changes actually modify the logic? My reading is that the logic is the same, except that the updated code can have an unnecessary string copy when iterating over multipart entries.
There was a problem hiding this comment.
no，the logic is basically the same. There will be an unnecessary string copy.
| @@ -8780,6 +8778,7 @@ int RGWRados::cls_bucket_list_unordered(const DoutPrefixProvider *dpp, | |||
| ent_list.emplace_back(std::move(dirent)); | |||
| ++count; | |||
| } else { | |||
| last_added_entry = dirent.key; | |||
There was a problem hiding this comment.
I believe this alone is the solution, right?
chapche
force-pushed
the
wip-tracker-51466
branch
from
deccd8e
to
7511f9f
Compare
Sorry for the late reply. After some struggling, it is obvious that this unnecessary string copy is inevitable for the purpose of reducing if-else blocks. So the commit introducing string copy is abandoned according to your request, and now there is only one commit which contains the solution left. Thank you very much for your professional suggestion! |
|
@chapche Would you check to see if this needs backports? If so, we need to update the tracker appropriately. Thanks! |
Yes, backports are needed. |
|
This commit needs to be reverted as it seems to have caused this issue: https://tracker.ceph.com/issues/59400 . And in taking a closer look at this commit, it clearly seems wrong. @chapche , would you provide a minimal producer of the error you describe? Thank you. |
cls_bucket_list_unordered() might return one redundent entry, which is returned last loop when number of incomplete multipart upload in index shards exceeds 1100. This is because when count == num_entries, last_added_entries is not updated. This is really easy to reproduce. And it could be observed in rgw's log that start_after entry for next loop is not the last entry returned last loop. Although this is not fatal but it still turns out that the code is not working as expected.
Fixes: https://tracker.ceph.com/issues/51466
Signed-off-by: Peng Zhang zhangpeng@vclusters.com
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume tox