Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue preventing reads after flush on a file handle #751

Merged
merged 10 commits into from
Feb 16, 2024

Conversation

passaro
Copy link
Contributor

@passaro passaro commented Feb 14, 2024

Description of change

Fix a regression introduced in 0030b0a where closing (flush) a file descriptor would prevent subsequent reads on the corresponding file handle. The issue was caused by flush setting the internal state of the file handle to Closed and decrementing the number of readers for the inode, which is used to disallow overwrites when there are open read file handles.

This PR includes 2 main changes:

  • it removes the Closed state and reverts flush to a no op for read file handles. The number of readers for an inode is decremented only on release.
  • it makes open eagerly discriminate between READ and WRITE, rather than on first read or write (or flush or release). This reverts the opposite change in Allow file overwrites #487 and allows to report error conditions on open when appropriate rather than on subsequent operations.

Relevant issues: #749

Does this change impact existing behavior?

Because release is async, there is a race condition when trying to overwrite a file immediately after closing a read file handle on it, which results in the overwrite to occasionally fail on open.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

@passaro passaro temporarily deployed to PR integration tests February 14, 2024 19:37 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests February 14, 2024 19:37 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests February 14, 2024 19:37 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests February 14, 2024 19:37 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests February 14, 2024 19:37 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests February 14, 2024 19:37 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests February 14, 2024 19:37 — with GitHub Actions Inactive
@passaro passaro requested a review from dannycjones February 14, 2024 19:39
dannycjones
dannycjones previously approved these changes Feb 15, 2024
Copy link
Contributor

@dannycjones dannycjones left a comment

Choose a reason for hiding this comment

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

LGTM.

Acknowledging trade-off that we mitigate the regression preventing reads after flush but introducing a race condition into file overwrites (available since v1.4.0 released this month) which could prevent opening a file for writing immediately after closing a file. (@passaro should we add an ignored test for this? I can add one in a follow-up PR.)

mountpoint-s3/tests/fuse_tests/read_test.rs Show resolved Hide resolved
@passaro passaro temporarily deployed to PR integration tests February 15, 2024 09:53 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests February 15, 2024 09:53 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests February 15, 2024 09:53 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests February 15, 2024 09:53 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests February 15, 2024 09:53 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests February 15, 2024 09:53 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests February 15, 2024 09:53 — with GitHub Actions Inactive
dannycjones
dannycjones previously approved these changes Feb 15, 2024
Copy link
Contributor

@dannycjones dannycjones left a comment

Choose a reason for hiding this comment

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

LGTM!

@dannycjones dannycjones requested a review from monthonk February 15, 2024 09:54
@passaro passaro marked this pull request as ready for review February 15, 2024 09:58
@passaro passaro added this pull request to the merge queue Feb 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 15, 2024
mountpoint-s3/src/fs.rs Show resolved Hide resolved
mountpoint-s3/src/fs.rs Show resolved Hide resolved
@dannycjones dannycjones temporarily deployed to PR integration tests February 16, 2024 14:29 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests February 16, 2024 14:29 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests February 16, 2024 14:29 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests February 16, 2024 14:29 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests February 16, 2024 14:29 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests February 16, 2024 14:29 — with GitHub Actions Inactive
@dannycjones dannycjones requested review from vladem and arsh February 16, 2024 14:31
@dannycjones dannycjones temporarily deployed to PR integration tests February 16, 2024 14:54 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests February 16, 2024 14:54 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests February 16, 2024 14:54 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests February 16, 2024 14:54 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests February 16, 2024 14:54 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests February 16, 2024 14:54 — with GitHub Actions Inactive
@dannycjones dannycjones temporarily deployed to PR integration tests February 16, 2024 14:54 — with GitHub Actions Inactive
Copy link
Contributor

@vladem vladem left a comment

Choose a reason for hiding this comment

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

LGTM:

  1. flush() is no-op for read;
  2. inode readers counter is decremented on release (accepted behaviour that open for write following a close may fail for short period of time);
  3. eager initialisation of file handles to avoid FH being readable when we already have a scheduled work to write to it

Copy link
Contributor

@arsh arsh left a comment

Choose a reason for hiding this comment

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

LGTM

@dannycjones dannycjones added this pull request to the merge queue Feb 16, 2024
Merged via the queue into awslabs:main with commit dd901f3 Feb 16, 2024
24 checks passed
@dannycjones dannycjones mentioned this pull request Feb 16, 2024
@passaro passaro deleted the bad-descriptor branch February 24, 2024 19:01
jamesbornholt added a commit to jamesbornholt/mountpoint-s3 that referenced this pull request Feb 27, 2024
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.

5 participants