Skip to content

Do not append an empty buffer to S3 during file handle RELEASE or FLUSH #1591

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Do not append an empty buffer to S3 during file handle RELEASE or FLUSH #1591

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7da606a
Do not append an empty buffer to am empty S3 object during RELEASE
Aug 29, 2025
cbc5eb8
Add a previously_flushed flag and update commit conditions in Release…
Sep 1, 2025
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
22 changes: 19 additions & 3 deletions mountpoint-s3-fs/src/fs/handles.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ where
request,
initial_etag,
written_bytes: 0,
previously_flushed: false,
})
} else {
let request = fs
Expand Down Expand Up @@ -119,6 +120,7 @@ pub enum UploadState<Client: ObjectClient + Send + Sync> {
request: AppendUploadRequest<Client>,
initial_etag: Option<ETag>,
written_bytes: usize,
previously_flushed: bool,
},
MPUInProgress {
request: UploadRequest<Client>,
Expand Down Expand Up @@ -196,6 +198,7 @@ where
request,
initial_etag,
written_bytes,
..
} => {
let current_offset = request.current_offset();
match Self::commit_append(request, &handle.location).await {
Expand All @@ -212,6 +215,7 @@ where
request,
initial_etag,
written_bytes,
previously_flushed: true, // Mark that the "append" has seen a Flush at least once
};
Ok(())
}
Expand Down Expand Up @@ -240,8 +244,12 @@ where
open_pid: u32,
) -> Result<(), Error> {
match self {
UploadState::AppendInProgress { written_bytes, .. } => {
if *written_bytes == 0 || !are_from_same_process(open_pid, pid) {
UploadState::AppendInProgress {
written_bytes,
previously_flushed,
..
} => {
if (*written_bytes == 0 && !*previously_flushed) || !are_from_same_process(open_pid, pid) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I need to fix this condition to move if (*written_bytes == 0 && *previously_flushed && are_from_same_process(open_pid, pid)) return Ok(()) condition inside so we don't go to the code on line 280

Copy link
Contributor

Choose a reason for hiding this comment

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

looks that addition of !*previously_flushed results in a change in behaviour, I am not sure if we want this:

  • now on flush we keep the file handle open only if no data was written *written_bytes == 0 and append have not been "commited" yet !*previously_flushed
  • append is "commited" on \1 flush (close) and \2 fsync
  • previously if close or fsync happened before current close we still may have kept the file handle open

what's the reasoning, why is this desired? also may it break behaviour of tools like touch or dd if they fsync before the first close?

// Commit current changes, but do not close the write handle.
return self.commit(fs, handle).await;
}
Expand Down Expand Up @@ -296,8 +304,16 @@ where
) -> Result<bool, Error> {
match self {
UploadState::AppendInProgress {
request, initial_etag, ..
request,
initial_etag,
written_bytes,
previously_flushed,
} => {
// No data upload should be pending completion as part of the release - if we have not written anything locally AND we already committed on a preceding Flush
Copy link
Contributor

Choose a reason for hiding this comment

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

I would clarify here, that with this if we're skipping an S3 request, since we know that it happened before (and an empty object was already created)

if written_bytes == 0 && previously_flushed {
Self::finish(fs, ino, initial_etag).await;
return Ok(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the reasoning for returning false here?

}
Self::complete_append(fs, ino, key, request, initial_etag).await?;
Ok(true)
}
Expand Down
Loading