Add btrfs COW option. - Githubissues

tteck / Proxmox

Proxmox VE Helper-Scripts

https://Helper-Scripts.com

MIT License

13.23k stars 2.05k forks source link

Add btrfs COW option. #3163

Closed prudy closed 3 months ago

prudy commented 3 months ago

I wanted to make you aware that I am meticulous when it comes to merging code into the main branch, so please don't take it personally if I reject your request.

Description

Prevents combining VM disk cache mode none on btrfs with COW. For other cache option adds a selection choice (default keeps original behavior).

Type of change

Please delete options that are not relevant.

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] I have performed a self-review of my code, adhering to established codebase patterns and conventions.

tteck commented 3 months ago

Can you give a little more explanation for this? I don't use BTRFS.

Currently, the default setting is DISK_CACHE="cache=writethrough,"

prudy commented 3 months ago

Sure, the btrfs on host by default uses COW (as copy on write plus checksuming). When the VM disk cache is 'none' it issues the O_DIRECT to the host to access image content. This would produce errors reported. It is advised not to use cache none when image is stored on btrfs: https://pve.proxmox.com/wiki/Storage:_BTRFS However, this can be still allowed when COW is disabled for that particular image. For other cache modes it's by preference, so to have a choice is helpful as it does not have to be done manually. But I've just noticed that I did not skip asking yes/no if the default installation is selected - I'll update it tomorrow.

tteck commented 3 months ago

So, this only affects BTRFS if the user selects DISK_CACHE="cache=none," in Advanced settings?

prudy commented 3 months ago

It goes like this:	Installation + cache \ Storage FS	non-btrfs
Standard	No changes	msg_ok "✓ Btrfs COW: default"
Advanced + writethrough(default)	No changes	Dialog choice to disable COW + msg_ok "✓ Btrfs COW: default" or "✓ Btrfs COW: disabled"
Advanced + none	No changes	No dialog, COW is disabled + msg_ok "✓ Btrfs COW: disabled"

prudy commented 3 months ago

Please hold on with this - I realized the cache=None in the menu is actually not 'none' but 'default', as given to qm as empty value. As I started wit this patch already I'll include other cache options too.

tteck commented 3 months ago

please create a new PR when ready, thanks.