Skip to content

Optimize for non-local posix filesystems#25

Open
nalinigans wants to merge 11 commits intodevelopfrom
nalini_nfs
Open

Optimize for non-local posix filesystems#25
nalinigans wants to merge 11 commits intodevelopfrom
nalini_nfs

Conversation

@nalinigans
Copy link
Copy Markdown
Contributor

No description provided.

@nalinigans nalinigans requested review from hillsd and mlathara March 8, 2019 21:34
@codecov-io
Copy link
Copy Markdown

codecov-io commented Mar 8, 2019

Codecov Report

Merging #25 into develop will increase coverage by 0.09%.
The diff coverage is 71.42%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #25      +/-   ##
===========================================
+ Coverage    55.53%   55.62%   +0.09%     
===========================================
  Files           42       42              
  Lines        13001    13071      +70     
===========================================
+ Hits          7220     7271      +51     
- Misses        5781     5800      +19
Impacted Files Coverage Δ
core/include/storage_manager/storage_posixfs.h 100% <ø> (ø) ⬆️
core/src/misc/hilbert_curve.cc 1.49% <0%> (ø) ⬆️
core/src/array/array.cc 51.01% <0%> (-0.15%) ⬇️
core/src/storage_manager/storage_posixfs.cc 85.2% <73.86%> (-2.86%) ⬇️
core/src/storage_manager/storage_fs.cc 65.21% <0%> (-8.7%) ⬇️
core/src/array/array_read_state.cc 72.47% <0%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5dd7618...bfbbce7. Read the comment docs.

int close_file(const std::string& filename);

protected:
std::mutex write_map_mtx_;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So, naive question here. What is this mutex protecting? We don't really support multiple writers to the same file anyway, right?

If we're adding the mutex just in case to ensure we don't end up with multiple file descriptors to the same file, I think we should have the mutex around get_fd and open the file/add to the map in there as well. The way we have it right now, a thread could do a get_fd and see there the file isn't in the map, then another thread may beat it to the opening and adding the map.

Copy link
Copy Markdown
Contributor Author

@nalinigans nalinigans Mar 9, 2019

Choose a reason for hiding this comment

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

The mutex is only protecting write_map_(map of filenames to open fds). There is parallelism supported in async io, not used by GenomicsDB currently, and so, even if multiple writers may not write to the same file, write_map_ could get corrupted.

I will probably add another check by performing get_fd after acquiring the lock, so we also ensure only one writer is opening/closing the file at any point of time.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am trying to not acquire a lock in get_fd, for performance sake, btw. Hopefully, std::map find is thread safe, otherwise, we will need a read lock too. Let me research a little more there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants