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 GH-10548: copy() fails on cifs mounts because of incorrect length (cfr_max) specified in streams.c:1584 copy_file_range() #10551

Merged
merged 1 commit into from Feb 11, 2023

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Feb 9, 2023

On some filesystems, the copy operation fails if we specify a size
larger than the file size. We use a stat call to clamp the size to copy
to the actual filesize. This stat call shouldn't impact performance
notably because stat calls can be cached. In some cases (like for /proc
files), the returned size is 0, so we should avoid problems by not using
copy_file_range in those cases (copy_file_range wouldn't work anyway on
this particular example because the syscall is not supported for /proc).

I will do some cleanups and improvements on copy_file_range (like the loop I described in #10440 in a follow-up PR for the master branch).

/cc @knowsshit It would be great if you could test this patch out and let us know if this works for you :)
/cc @arnaud-lb since he reviewed the PR I referenced in here.

Edit: CI fail in x32 is unrelated

@knowsshit
Copy link

Thank you! It should work if strace shows that the length argument that is passed to copy_file_range() is not greater than the source file size. I have tested that copy_file_range() works fine in the environment when this is true.

The environment where copy() fails for me on the cifs mount is an Azure Web App, so not sure if I am able to test the patch there.

@nielsdos
Copy link
Member Author

Thank you! It should work if strace shows that the length argument that is passed to copy_file_range() is not greater than the source file size. I have tested that copy_file_range() works fine in the environment when this is true.

Thanks, I verified that is the case indeed so it should work. At least it worked in my testing.

The environment where copy() fails for me on the cifs mount is an Azure Web App, so not sure if I am able to test the patch there.

No problem :)

@nielsdos
Copy link
Member Author

Rebased on upstream 8.2 since the fix on which this depends got merged.

main/streams/streams.c Outdated Show resolved Hide resolved
…gth (cfr_max) specified in streams.c:1584 copy_file_range()

On some filesystems, the copy operation fails if we specify a size
larger than the file size in certain circumstances and configurations.
In those cases EIO will be returned as errno and we will therefore fall
back to other methods.
@nielsdos
Copy link
Member Author

@arnaud-lb Updated such that on EIO it will fall back to the mmap or classic copy method.

@arnaud-lb arnaud-lb merged commit e787d6c into php:PHP-8.2 Feb 11, 2023
@arnaud-lb
Copy link
Member

Thank you!

arnaud-lb added a commit that referenced this pull request Feb 11, 2023
* PHP-8.2:
  [ci skip] NEWS
  Fix GH-10548: copy() fails on cifs mounts because of incorrect length (cfr_max) specified in streams.c:1584 copy_file_range() (#10551)
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.

None yet

3 participants