Simplify fs-info getting in make_file_impl() #2297
Conversation
|
CI fails due to #2296 |
src/core/file.cc
Outdated
| @@ -1078,8 +1078,8 @@ make_file_impl(int fd, file_open_options options, int flags, struct stat st) noe | |||
| auto st_dev = st.st_dev; | |||
| static thread_local std::unordered_map<decltype(st_dev), internal::fs_info> s_fstype; | |||
|
|
|||
| future<> get_fs_info = s_fstype.count(st_dev) ? make_ready_future<>() : | |||
| engine().fstatfs(fd).then([fd, st_dev] (struct statfs sfs) { | |||
| if (s_fstype.count(st_dev) == 0) { | |||
There was a problem hiding this comment.
could also use
if (!s_fstype.contains(st_dev)) {since we dropped the support of C++17.
There was a problem hiding this comment.
Yeah, this patch keeps finding method as is, next patch fixes it
xemul
force-pushed
the
br-make-file-impl-simplify
branch
from
78cffd7
to
65b7aac
Compare
|
upd:
|
The most common execution path for this helper is to return a single ready future with file impl. However, to facilitate the fs_info getting it creates a chain of two ready futures. Make things simpler. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Every time make_file_impl() creates a file it first counts the number of entries for the device, then tries to lookup the entry. Instead of relying on compiler to optimize it, it's nicer to lookup the entry once. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
xemul
force-pushed
the
br-make-file-impl-simplify
branch
from
65b7aac
to
cc21298
Compare
|
upd:
|
|
@scylladb/seastar-maint , please review |
| s_fstype[st_dev] = std::move(fsi); | ||
| return make_file_impl(fd, std::move(options), flags, std::move(st)); |
There was a problem hiding this comment.
I tried, but failed, to understand this change, maybe you can help me understand it...
You have a cache s_fstype of fstatfs results. Before this patch, the complicated code below, using the result of the fstatfs to decide how to open the file, was used regardless of whether the fsstatfs results were previously cached or not. After this patch, unless I'm misreading it something (it's really hard to understand the indentation in github), it seems that when the information isn't cached, you cache it, but then don't use what you cached and unconditionlay use the non-append-challenged variant. Why?
There was a problem hiding this comment.
Before this patch, the complicated code below, using the result of the fstatfs to decide how to open the file, was used regardless of whether the fsstatfs results were previously cached or not.
No, if the cached result is there it doesn't call fstat but gets the make_ready_future<>() instantly
future<> get_fs_info = s_fstype.count(st_dev) ? make_ready_future<>() :
engine().fstatfs(fd).then([fd, st_dev] (struct statfs sfs) {
...
s_fstype[st_dev] = std::move(fsi);
});After this patch, unless I'm misreading it something (it's really hard to understand the indentation in github), it seems that when the information isn't cached, you cache it, but then don't use what you cached and unconditionlay use the non-append-challenged variant.
I do exactly the same, but without make_ready_future<>. If the result is not there I do
if (s_fstype.count(st_dev) == 0) {
return engine().fstatfs(fd).then([fd, options = std::move(options), flags, st = std::move(st)] (struct statfs sfs) {
...
return ... // "recursive" call to the current function
});and if the result is not there it just returns the future with file impl without .then()-ing it to the make_ready_future<>()
return make_ready_future<shared_ptr<file_impl>>(...);There was a problem hiding this comment.
Oh, I missed the recursive call to the current function. I misread it as a call to another function.
There was a problem hiding this comment.
Nitpick (not important): I don't think moving "struct stat" actually does anything.
| s_fstype[st_dev] = std::move(fsi); | ||
| return make_file_impl(fd, std::move(options), flags, std::move(st)); |
There was a problem hiding this comment.
Nitpick (not important): I don't think moving "struct stat" actually does anything.
The helper looks up in the map of fs-info-s twice and returns the chain of ready futures on the common paths. This PR makes things simpler.