bpo-38319: Fix shutil._fastcopy_sendfile(): set sendfile() max block size by giampaolo · Pull Request #16491 · python/cpython

giampaolo · 2019-09-30T14:04:15Z

An attempt to fix:
https://bugs.python.org/issue38319

https://bugs.python.org/issue38319

serhiy-storchaka · 2019-09-30T14:18:59Z

Lib/shutil.py

    # changes while being copied.
    try:
        blocksize = max(os.fstat(infd).st_size, 2 ** 23)  # min 8MB
+        blocksize = min(blocksize, 2 ** 31 - 2)  # max 2GB


Why not 2 ** 31 - 1?

Would not be better to use a power of two, 2**30?

Agreed, used 2**30.

That sounds inefficient on 64-bit system: files larger than 2 GB will need multiple sendfile() syscalls, rather currently a single one is enough, no? Maybe use sys.maxsize instead?

On my 64-bit Ubuntu 18.04 box sendfile() always copies up to 2GB per call, never more than that.

Well ok, but the kernel can be updated to support larger copies. Why should Python put an arbitrary limit on a syscall? Well, you're the expert, it's up to you. It was just a comment ;-)

OK, I think it makes sense. I added:

+ if sys.maxsize < 2 ** 32: # 32-bit architecture + blocksize = min(blocksize, 2 ** 30) # max 1GiB

...I did that for shutil only. In socket.py I decided to always limit to max 1GiB: since we're dealing with a socket I want to exercise the loop more often (and hit the selector(), which implements the timeout logic).

serhiy-storchaka · 2019-09-30T14:21:19Z

Lib/shutil.py

    try:
        blocksize = max(os.fstat(infd).st_size, 2 ** 23)  # min 8MB
+        blocksize = min(blocksize, 2 ** 31 - 2)  # max 2GB
    except Exception:


What exceptions besides OSError can be raised? I do not feel comfortable with silencing a wide range of exceptions.

serhiy-storchaka · 2019-09-30T14:22:22Z

Lib/socket.py

@@ -357,6 +357,7 @@ def _sendfile_use_sendfile(self, file, offset=0, count=None):
            if not fsize:
                return 0  # empty file
            blocksize = fsize if not count else count


Suggested change

blocksize = fsize if not count else count

blocksize = count or fsize

serhiy-storchaka

Nitpick: I would use binary prefixes for exact powers of two (MiB, GiB).

Misc/NEWS.d/next/Library/2019-09-30-22-06-33.bpo-38319.5QjiDa.rst

vstinner · 2019-09-30T20:47:39Z

Lib/shutil.py

-    except Exception:
+        blocksize = max(os.fstat(infd).st_size, 2 ** 23)  # min 8MiB
+        if sys.maxsize < 2 ** 32:  # 32-bit architecture
+            blocksize = min(blocksize, 2 ** 30)  # max 1GiB


Can you please add a comment explaining this 1 GiB limit?

vstinner · 2019-09-30T20:51:23Z

Lib/socket.py

            if not fsize:
                return 0  # empty file
-            blocksize = fsize if not count else count
+            blocksize = min(count or fsize, 2 ** 30)  # max 1GiB


Do we have to truncate the size on 64-bit system? Please add a comment. I suggest to write this instruction in two limits, to make the purpose of each line more explicit. Lilke:

blocksize = fsize if not count else count # bpo-38319: truncate to 1 GiB to prevent OverflowError blocksize = min(blocksize, 2 ** 30)

vstinner · 2019-09-30T20:59:14Z

...I did that for shutil only. In socket.py I decided to always limit to max 1GiB: since we're dealing with a socket I want to exercise the loop more often (and hit the selector(), which implements the timeout logic).

1 GiB sounds an arbitrary limit. If the developer wants a fine control on the timing, they can pass a smaller "count", no?

The current implementation of _sendfile_use_sendfile() doesn't seem to care about timeout. Can't it be implemented using a non-blocking socket and poll until the socket is ready to send? It should prevent to block in sendfile(), no? asyncio uses sendfile() on non-blocking socket, so it should be possible to do something similar in socket to respect the timeout :-)

To fix https://bugs.python.org/issue38319 and only fix this issue: size = min(size, sys.maxsize) should be enough and work on any platform, no?

serhiy-storchaka · 2019-09-30T21:08:41Z

I think it is better to use a block size which is a power of two. It guaranties that it is divisible by the page size and by the logical or physical disk block size. 1 GiB is the largest power of two which not larger than sys.maxsize on 32-bit platform.

I think it is worth to convert a count to size_t instead of Py_ssize_t in future. This will allow to raise the limit to 2 GiB. But this is a separate issue.

vstinner · 2019-09-30T21:12:16Z

I think it is better to use a block size which is a power of two. It guaranties that it is divisible by the page size and by the logical or physical disk block size. 1 GiB is the largest power of two which not larger than sys.maxsize on 32-bit platform.

Oh, I see. Sorry, I was lost in all these comments on the PR. That's why I asked for a quick comment added directly in the code to explain that.

miss-islington · 2019-10-01T04:15:54Z

Thanks @giampaolo for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington · 2019-10-01T04:15:54Z

I'm having trouble backporting to 3.8. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.8 label.

miss-islington · 2019-10-01T04:16:38Z

Thanks @giampaolo for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

bedevere-bot · 2019-10-01T04:16:48Z

GH-16506 is a backport of this pull request to the 3.8 branch.

giampaolo · 2019-10-01T05:12:09Z

Thanks for the code review.

…size (GH-16491) (#16506) (cherry picked from commit 94e1650) Co-authored-by: Giampaolo Rodola <g.rodola@gmail.com>

vstinner · 2019-10-01T08:15:54Z

Thanks @giampaolo for the fix, but also for your overall work: it's really cool that Python can use transparently sendfile() internally! I like this syscall() :-)

…size (pythonGH-16491)

set blocksize max 2G

d5fc0e9

the-knights-who-say-ni added the CLA signed label Sep 30, 2019

bedevere-bot added the awaiting core review label Sep 30, 2019

add news entry

5ea0ee1

serhiy-storchaka reviewed Sep 30, 2019

View reviewed changes

use power of 2; catch OSError

549e508

vstinner changed the title ~~BPO-38319 set sendfile() max block size~~ bpo-38319: Fix shutil._fastcopy_sendfile(): set sendfile() max block size Sep 30, 2019

serhiy-storchaka approved these changes Sep 30, 2019

View reviewed changes

Misc/NEWS.d/next/Library/2019-09-30-22-06-33.bpo-38319.5QjiDa.rst Outdated Show resolved Hide resolved

bedevere-bot added awaiting merge and removed awaiting core review labels Sep 30, 2019

giampaolo added 2 commits September 30, 2019 22:37

use binary prefixes

49e6684

only limit blocksize to 1GiB for 32 archs

222a267

vstinner reviewed Sep 30, 2019

View reviewed changes

serhiy-storchaka approved these changes Sep 30, 2019

View reviewed changes

add comments

6614996

giampaolo merged commit 94e1650 into python:master Oct 1, 2019

giampaolo deleted the sendfile-32bit-2 branch October 1, 2019 03:40

bedevere-bot removed the awaiting merge label Oct 1, 2019

giampaolo added the needs backport to 3.8 label Oct 1, 2019

giampaolo added needs backport to 3.8 and removed needs backport to 3.8 labels Oct 1, 2019

bedevere-bot removed the needs backport to 3.8 label Oct 1, 2019

ambv pushed a commit that referenced this pull request Oct 1, 2019

bpo-38319: Fix shutil._fastcopy_sendfile(): set sendfile() max block …

938c00c

…size (GH-16491) (#16506) (cherry picked from commit 94e1650) Co-authored-by: Giampaolo Rodola <g.rodola@gmail.com>

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019

bpo-38319: Fix shutil._fastcopy_sendfile(): set sendfile() max block …

5a92ef4

…size (pythonGH-16491)

vstinner mentioned this pull request Apr 10, 2022

shutil.copyfile(): os.sendfile() fails with OverflowError on 32-bit system #82500

Closed

serhiy-storchaka mentioned this pull request Jan 15, 2024

socket.sendfile(): os.sendfile() can fail with OverflowError on 32-bit system #114077

Closed

	blocksize = fsize if not count else count
	blocksize = count or fsize

Uh oh!

Conversation

giampaolo commented Sep 30, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Sep 30, 2019

Uh oh!

serhiy-storchaka commented Sep 30, 2019

Uh oh!

vstinner commented Sep 30, 2019

Uh oh!

miss-islington commented Oct 1, 2019

Uh oh!

miss-islington commented Oct 1, 2019

Uh oh!

miss-islington commented Oct 1, 2019

Uh oh!

bedevere-bot commented Oct 1, 2019

Uh oh!

giampaolo commented Oct 1, 2019

Uh oh!

vstinner commented Oct 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

giampaolo commented Sep 30, 2019 •

edited by bedevere-bot

Loading