Skip to content
This repository has been archived by the owner on Jan 3, 2024. It is now read-only.

Make MP mark_complete a write transaction #225

Merged
merged 2 commits into from
Oct 13, 2023
+19 −54
Merged

Make MP mark_complete a write transaction #225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b187b5
rgw/sfs: Remove unused SQLiteMultipart::mark_complete
Oct 6, 2023
605668c
rgw/sfs: MP mark_complete: Check duplicate after update
Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 19 additions & 45 deletions src/rgw/driver/sfs/sqlite/sqlite_multipart.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,59 +344,33 @@ bool SQLiteMultipart::abort(const std::string& upload_id) const {
return committed;
}

static int _mark_complete(
rgw::sal::sfs::sqlite::Storage& storage, const std::string& upload_id
) {
storage.update_all(
set(c(&DBMultipart::state) = MultipartState::COMPLETE,
c(&DBMultipart::state_change_time) = ceph::real_time::clock::now()),
where(
is_equal(&DBMultipart::upload_id, upload_id) and
greater_or_equal(&DBMultipart::state, MultipartState::INIT) and
lesser_or_equal(&DBMultipart::state, MultipartState::INPROGRESS)
)
);
return storage.changes();
}

bool SQLiteMultipart::mark_complete(const std::string& upload_id) const {
auto storage = conn->get_storage();
auto committed = storage.transaction([&]() mutable {
auto num_complete = _mark_complete(storage, upload_id);
if (num_complete == 0) {
return false;
}
ceph_assert(num_complete == 1);
return true;
});

return committed;
}

bool SQLiteMultipart::mark_complete(
const std::string& upload_id, bool* duplicate
) const {
ceph_assert(duplicate != nullptr);
auto storage = conn->get_storage();
auto committed = storage.transaction([&]() mutable {
auto entries = storage.get_all<DBMultipart>(
where(is_equal(&DBMultipart::upload_id, upload_id))
storage.update_all(
set(c(&DBMultipart::state) = MultipartState::COMPLETE,
c(&DBMultipart::state_change_time) = ceph::real_time::clock::now()),
where(
is_equal(&DBMultipart::upload_id, upload_id) and
greater_or_equal(&DBMultipart::state, MultipartState::INIT) and
lesser_or_equal(&DBMultipart::state, MultipartState::INPROGRESS)
)
);
ceph_assert(entries.size() <= 1);
if (entries.size() == 0) {
return false;
}
auto entry = entries.front();
if (entry.state == MultipartState::DONE) {
*duplicate = true;
return true;
}

auto num_complete = _mark_complete(storage, upload_id);
if (num_complete == 0) {
return false;
if (storage.changes() == 0) {
const auto state = storage.select(
columns(&DBMultipart::state),
where(is_equal(&DBMultipart::upload_id, upload_id))
);
if (state.size() == 0) {
return false;
}
*duplicate = (std::get<0>(state[0]) >= MultipartState::COMPLETE);
} else {
*duplicate = false;
Comment on lines +362 to +372
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, this only makes the operation potentially write-only, right? If the conditions are met, this will still be a read-write transaction. Or am I misunderstanding the point?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a write transaction, because it starts with an update. Write transactions may also do reads. Read-then-write transactions require an upgrade to a write transaction. Starting the write transaction directly is subject to the busy timeout handler. The read-then-write in many of our cases not. They may trigger the deadlock prevention logic if multiple concurrent connections already have the read lock.

This gives us two options to get transaction retries: the busy transaction retry utility or starting with writes to use the busy timeout handler.

Since the update is the important bit here and not dependent on a previous select, making it a write transaction makes most sense I think.

Related docs:
https://www.sqlite.org/lang_transaction.html#:~:text=2.1.%20Read%20transactions%20versus%20write%20transactions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, makes sense. Thanks! :)

}
ceph_assert(num_complete == 1);
return true;
});
return committed;
Expand Down
9 changes: 0 additions & 9 deletions src/rgw/driver/sfs/sqlite/sqlite_multipart.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,15 +176,6 @@ class SQLiteMultipart {
*/
bool abort(const std::string& upload_id) const;

/**
* @brief Mark an on-going Multipart Upload as being complete.
*
* @param upload_id The Multipart Upload's ID.
* @return true if a multipart upload was marked complete.
* @return false if no multipart upload was found.
*/
bool mark_complete(const std::string& upload_id) const;

/**
* @brief Mark an on-going Multipart Upload as being complete.
*
Expand Down
Loading