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

Fixed about ParallelMixMultipartUpload #1313

Merged
merged 1 commit into from Jun 24, 2020
Merged

Conversation

ggtakec
Copy link
Member

@ggtakec ggtakec commented Jun 21, 2020

Relevant Issue (if applicable)

n/a(or some issues)

Details

The following bugs were found in the processing of mixed multipart upload.

Details of the bug

For example, there is a 500MB file and it doesn't exist in the s3fs cache file yet.
In this case, if user perform the following operations on the target file, the result will be different for each.

  • Write a few bytes from the 400th MB and close the file.
    This is working correctly.
  • Write from the 0th byte to within 5MB.
    It fails with a 400 HTTP error(EIO).
  • Write data from 495MB to the end of the file.
    It fails with a 400 HTTP error(EIO).
    It also unnecessarily downloads before uploading, which reduces performance.

These fatal errors are EntityTooSmall.

Cause

In the processing of mixed multipart upload, there was a mistake in the calculation of the range of Copy and Upload.
It may be possible to complete normally, but it will fail depending on the write position without the cache file.

Fixes

Made following fixes.

Download and upload range processing

In case of mixed multi-part upload, the insufficient area is downloaded in advance by s3fs because of the minimum upload range.
For this reason, it is necessary to calculate the areas of Copy and Uoload.
But there were some bugs in this logic.
For fixing, the following functions that performed these processes were changed/deleted.

  • PageList::GetPageListForMultipartUpload
    It was rewritten significantly.
    And the function name(PageList::GetPageListsForMultipartUpload) and arguments have also been changed.
  • PageList::RawGetUnloadPageList
    It has been absorbed in PageList::GetPageListsForMultipartUpload, is no longer needed, and has been removed.
  • PageList::GetMultipartSizeList
    It has been absorbed in PageList::GetPageListsForMultipartUpload, is no longer needed, and has been removed.
  • PageList::Compress
    It has been modified to prepare local functions to operate on fdcache_list_t and only call it.
  • S3fsCurl::ParallelMixMultipartUploadRequest
    This has changed with the changes to GetPageListsForMultipartUpload.
  • FdEntity::RowFlush
    Changes such as calling ParallelMixMultipartUploadRequest

Others

  • Changed PageList::Dump to const.
  • Cleaned up #include due to file dependency.

Impact

In mixed multipart upload, EIO(HTTP response code=400) problem may be the same cause as this PR.
Also, there are times when performance is poor, which may also be affecting it.(Performance gets worse because it downloads a range that does not need to be downloaded due to a bug.)

@ggtakec ggtakec added the bug label Jun 21, 2020
@ggtakec ggtakec requested a review from gaul June 21, 2020 18:46
Copy link
Member

@gaul gaul left a comment

Choose a reason for hiding this comment

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

Can we add any tests which failed before but succeed with this PR?

src/fdcache.cpp Outdated Show resolved Hide resolved
@ggtakec
Copy link
Member Author

ggtakec commented Jun 22, 2020

This bug led to two consequences:

  • EIO(400 HTTP status) error
  • Downloading unnecessary range in object(file)

Regarding first case, I think that I can identify the test case that caused the error and incorporate it into the test. So I try to prepare it.
The second case seems to be difficult to detect from outside s3fs process.
Clarifying only the part that uploads the data can be a daunting task, and even a small change in the test case could change that area.
(If I can find a way around this, prepare a test case.)

@ggtakec
Copy link
Member Author

ggtakec commented Jun 23, 2020

I added test code for this case.

If this added test code is tested in my local environment connected to AWS S3, old code before this PR failed.
When the old code is executed with this test code, there is a case that one entity is smaller than the minimum entity size(5MB), and it results in EntityTooSmall error.
However, when old code with s3proxy as the backend, this test was successful.
As a result of the investigation, it was found that s3proxy did not output EntityTooSmall and the upload was successful.

I'm not sure if the EntityTooSmall error is an error that also occurs on S3 other than AWS.
But, there is no problem with the test code itself, so I will include it in this PR.

@gaul
Copy link
Member

gaul commented Jun 24, 2020

I understand the S3Proxy EntityTooSmall issue and will work on a fix.

@ggtakec ggtakec deleted the bug_fix branch June 24, 2020 08:02
@ggtakec
Copy link
Member Author

ggtakec commented Jun 24, 2020

@gaul Thanks, and wait for new s3proxy.:-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants