Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

9p: use fscache by default for RO mounts #787

Merged
merged 1 commit into from
Apr 11, 2022

Conversation

AkihiroSuda
Copy link
Member

Fix #786

@matejsp
Copy link

matejsp commented Apr 8, 2022

Shouldn't default be based on not having any writeable mounts?
As in: any writeable -> default mmap else fscache?

Fix issue 786

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
@jandubois
Copy link
Member

Shouldn't default be based on not having any writeable mounts?
As in: any writeable -> default mmap else fscache?

I don't think the default for one mount should depend on the writable property of a different mount.

This is not a global setting, so it would make more sense (to me) to say: defaults to fscache for readonly mounts and mmap for writable ones.

@AkihiroSuda AkihiroSuda changed the title 9p: change the default cache mode from mmap to fscache 9p: use fscache by default for RO mounts Apr 8, 2022
@AkihiroSuda
Copy link
Member Author

Updated PR to keep mmap as the default for RW mounts

@jandubois
Copy link
Member

This is not a global setting, so it would make more sense (to me) to say: defaults to fscache for readonly mounts and mmap for writable ones.

Already implemented before I can press the "Comment" button! 😂

@AkihiroSuda AkihiroSuda added this to the v0.10.0 milestone Apr 8, 2022
@afbjorklund
Copy link
Member

afbjorklund commented Apr 9, 2022

Sounds good to me, it is more complex than just keeping the upstream default (of "none") but performance could be worth it...

Never exposed the "cache" parameter on Podman Machine or for Minikube (in 2017), so there it uses cache=none always


I guess the same already applied to the msize parameter, where the upstream default (8K on Mac) caused runtime warnings.

qemu-system-aarch64: warning: 9p: degraded performance - fixed by increasing the default msize to 128K on all platforms

@AkihiroSuda AkihiroSuda requested a review from jandubois April 11, 2022 09:21
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

LGTM

@jandubois jandubois merged commit f362986 into lima-vm:master Apr 11, 2022
@LionsAd
Copy link

LionsAd commented Apr 13, 2022

@AkihiroSuda one thing to note:

fscache failed miserably for me when doing the dd test.

eG as it’s read only,

dd if=largefile of=/dev/null bs=1M count=1000

that said I tested writing and it failed, so maybe reading is fine but wanted to note it anyway.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9p: change the default cache mode from mmap to fscache (for read-only mounts, at least)
5 participants