Skip to content

Reduce syscalls for posixpath.ismount #117394

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

Closed
Tracked by #117361
nineteendo opened this issue Mar 30, 2024 · 7 comments
Closed
Tracked by #117361

Reduce syscalls for posixpath.ismount #117394

nineteendo opened this issue Mar 30, 2024 · 7 comments
Labels
3.13 bugs and security fixes performance Performance or resource usage type-feature A feature request or enhancement

Comments

@nineteendo
Copy link
Contributor

nineteendo commented Mar 30, 2024

Feature or enhancement

Proposal:

Currently posixpath.ismount uses realpath(join(path, '..')) to get the parent of the mount. This is absolutely overkill and should only be used when join(path, '..') isn't sufficient.

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

Linked PRs

@nineteendo nineteendo added the type-feature A feature request or enhancement label Mar 30, 2024
@AlexWaygood AlexWaygood added performance Performance or resource usage 3.13 bugs and security fixes labels Mar 30, 2024
@nineteendo nineteendo mentioned this issue Mar 30, 2024
16 tasks
@barneygale
Copy link
Contributor

Related to #96328

@serhiy-storchaka
Copy link
Member

abspath is broken by design.

Let /mnt/storage/link be a link to /mnt, /mnt/storage is a mount point, and the original path is /mnt/storage/link/../mnt/storage. abspath() will return /mnt/storage/mnt/storage. dirname() will then return /mnt/storage/mnt which is on the same device as the original path, therefore ismount() will return False. But the real path is /mnt/storage, and it is a mount point.

@nineteendo
Copy link
Contributor Author

Are you sure? ntpath.realpath() doesn't handle symlinks that way:

>>> import ntpath
>>> ntpath.realpath(r"C:\Users\wanne\path-picker\link")
'C:\\Users\\wanne'
>>> ntpath.realpath(r"C:\Users\wanne\path-picker\link\..")
'C:\\Users\\wanne\\path-picker'

Does that work the same on Linux?

@barneygale
Copy link
Contributor

It works differently on Linux - .. entries point to the real parent of a directory.

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Apr 2, 2024
It is now 2-3 times faster if the user has permissions.
@serhiy-storchaka
Copy link
Member

#117447 is an alternate PR which achieves the same or better speed up.

@nineteendo
Copy link
Contributor Author

Let /mnt/storage/link be a link to /mnt, /mnt/storage is a mount point, and the original path is /mnt/storage/link/../mnt/storage.

Actually, you can work around this: ismount(realpath('/mnt/storage/link/../mnt/storage')).
You couldn't remove realpath from the old implemenation, because that would raise a PermissionError.
If documented, I feel like this is a cleaner solution, what do you think?

@serhiy-storchaka
Copy link
Member

Currently this workaround is built in ismount() (see bpo-2466). Your solution replaces realpath() with abspath(), but the latter is not suitable for this. realpath() is relatively expensive, so my solution only calls it if it is needed.

serhiy-storchaka added a commit that referenced this issue Apr 17, 2024
It is now 2-3 times faster if the user has permissions.
diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
It is now 2-3 times faster if the user has permissions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes performance Performance or resource usage type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants