Skip to content

btrfs: Do not disable quota on cleanup - #42273

Merged
cpuguy83 merged 1 commit into
moby:masterfrom
vadorovsky:btrfs-fix-quota
Apr 14, 2021
Merged

btrfs: Do not disable quota on cleanup#42273
cpuguy83 merged 1 commit into
moby:masterfrom
vadorovsky:btrfs-fix-quota

Conversation

@vadorovsky

@vadorovsky vadorovsky commented Apr 8, 2021 •
edited by thaJeztah
Loading

Copy link
Copy Markdown
Contributor

Before this change, cleanup of the btrfs driver (occuring on each daemon
shutdown) resulted in disabling quotas. It was done with an assumption
that quotas can be enabled or disabled on a subvolume level, which is
not true - enabling or disabling quota is always done on a filesystem
level.

That was leading to disabling quota on btrfs filesystems on each daemon
shutdown.

This change fixes that behavior and removes misleading subvol prefix
from functions and methods which set up quota (on a filesystem level).

Fixes: #34593
Fixes: docker/for-linux#78
Fixes: 401c8d1 ("Add disk quota support for btrfs" (#19651) )
Signed-off-by: Michal Rostecki mrostecki@opensuse.org

- What I did

Ensured that moby deamon doesn't disable quotas in the btrfs filesystem.

- How I did it

  • Removed the subvolDisableQuota method.
  • Limited the Cleanup method just to unmounting.
  • Removed the misleading subbol prefixes from methods which do operations on a filesystem level.

- How to verify it

  • Ensure that btrfs quota is enabled (btrfs quota enable /)
  • Start dockerd
  • Gracefully shutdown dockerd
  • Check if btrfs quota is still enabled by ensuring btrfs qgroup show / runs successfully.

I've done those steps inside the dev shell. They require btrfs-progs, which can be installed with apt update && apt install btrfs-progs.

- Description for the changelog

btrfs: Do not disable quota on cleanup

- A picture of a cute animal (not mandatory but encouraged)

doge

@thaJeztah thaJeztah added this to the 21.xx milestone Apr 8, 2021
@thaJeztah
thaJeztah requested a review from kolyshkin April 8, 2021 18:51
@cpuguy83

cpuguy83 commented Apr 8, 2021

Copy link
Copy Markdown
Member

It seems like the issue here may be that docker does not put itself into a subvolume.
And on systems such as opensuse, unless you actually give /var/lib/docker a subvolume, dockerd is on the subvolume for the rootfs.

I'm wondering if a better fix would be to give /var/lib/docker/btrfs its own subvolume.

@cpuguy83

cpuguy83 commented Apr 8, 2021

Copy link
Copy Markdown
Member

That said, I doubt we actually need to disable quota regardless.

kolyshkin
kolyshkin approved these changes Apr 8, 2021
View reviewed changes

@kolyshkin kolyshkin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM (disabling quota on unmount does not make sense).

tianon
tianon reviewed Apr 8, 2021
View reviewed changes
Comment thread daemon/graphdriver/btrfs/btrfs.go Outdated

@tianon tianon Apr 8, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(Somewhat orthogonally,) If the given path isn't a subvolume, do you think we should have this function error out so we don't accidentally set a quota on / thinking we're setting it on something more specific?

@vadorovsky vadorovsky Apr 8, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I like that idea. And I would also add the /var/lib/docker/btrfs subvolume by default if it's not there.

@vadorovsky

vadorovsky commented Apr 8, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@cpuguy83

I'm wondering if a better fix would be to give /var/lib/docker/btrfs its own subvolume.

That's a good idea too (to keep container subvolumes as children of /var/lib/docker/btrfs), but still, it doesn't change the fact that disabling quotas always happens on the whole fs. So I think we need both things fixes: 1) not disabling quotas; 2) ensuring we have /var/lib/docker/btrfs.

Thanks for review!

@vadorovsky vadorovsky mentioned this pull request Apr 12, 2021
Merged
@vadorovsky
btrfs: Do not disable quota on cleanup
1ec689c 
Before this change, cleanup of the btrfs driver (occuring on each daemon
shutdown) resulted in disabling quotas. It was done with an assumption
that quotas can be enabled or disabled on a subvolume level, which is
not true - enabling or disabling quota is always done on a filesystem
level.

That was leading to disabling quota on btrfs filesystems on each daemon
shutdown.

This change fixes that behavior and removes misleading `subvol` prefix
from functions and methods which set up quota (on a filesystem level).

Fixes: moby#34593
Fixes: 401c8d1 ("Add disk quota support for btrfs")
Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
@vadorovsky

vadorovsky commented Apr 13, 2021

Copy link
Copy Markdown
Contributor Author

@cpuguy83 @tianon After trying out your idea of enforcing /var/lib/docker/btrfs to be a subvolume, I got convinced that it's probably not a good one.

First of all, I don't think it's needed. Quotas are applied only on particular /var/lib/docker/btrfs/subvolumes/[id] subvolumes, here:

moby/daemon/graphdriver/btrfs/btrfs.go

Lines 556 to 558 in 62b5194

if err := d.setStorageSize(path.Join(subvolumes, id), driver); err != nil {
return err
}

moby/daemon/graphdriver/btrfs/btrfs.go

Lines 668 to 670 in 62b5194

if err := subvolLimitQgroup(dir, size); err != nil {
return nil, err
}

(these are the only places I found which call subvolLimitQgroup())

I don't see any part of the driver which would apply quota on any "higher" directory, thus I don't see any danger of accidentally applying quota on the rootfs subvol even if there is no /var/lib/docker/btrfs subvolume and subolumes/[id] are children of rootfs.

Secondly, to enforce /var/lib/docker/btrfs as a subvolume, we would have to provide some safe way of migrating old users. There is no possibility in btrfs to convert an existing directory to a subvolume. The only possible solution would be creating a new subvolume, reflinking the old contents (reflink is the only way to "move" files into another subvolume without copying and wasting the space), then removing the old directory.

That migration would look like:

mv /var/lib/docker/btrfs /var/lib/docker/btrfs.bak
btrfs subvolume create /var/lib/docker/btrfs
cp -a --reflink=always /var/lib/docker/btrfs.bak/* /var/lib/docker/btrfs
rm -rf /var/lib/docker/btrfs.bak

The biggest problem, which finally convinced me to not do that, is the cp --reflink command. It takes a long time to execute it on large amount of files and implementing it in Go would be hard. What that command does, is in fact calling the FICLONE ioctl on every file.

There is a Go library which does that, but only on single files:

https://github.com/KarpelesLab/reflink

If we really wanted to use it, we would have to combine it with filepath.Walk. But again, that would be a long operation, taking approximately several minutes. Definitely too long to do that just for initializing the graphdriver.

Please let me know if you agree with my thoughts or if I'm missing something.

For now, I pushed my old version of the PR branch which only removes the "disable quota" part of code, to fix #34593.

tianon
tianon approved these changes Apr 14, 2021
View reviewed changes

@tianon tianon left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍

(Thanks for testing /var/lib/docker/btrfs as a subvol and for the detailed write-up about why we probably shouldn't! 😅❤️)

cpuguy83
cpuguy83 approved these changes Apr 14, 2021
View reviewed changes

@cpuguy83 cpuguy83 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Thanks!

@cpuguy83
cpuguy83 merged commit 99d3c07 into moby:master Apr 14, 2021
@vadorovsky
vadorovsky deleted the btrfs-fix-quota branch April 15, 2021 09:38
@FroggyFlox FroggyFlox mentioned this pull request Jul 16, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tianon tianon tianon approved these changes
@cpuguy83 cpuguy83 cpuguy83 approved these changes
@kolyshkin kolyshkin kolyshkin approved these changes

Assignees

No one assigned

Labels

area/storage/btrfs impact/changelog kind/bugfix PR's that fix bugs status/2-code-review

Projects

None yet

Milestone

23.0.0

Development

Successfully merging this pull request may close these issues.

Docker's btrfs graph driver eliminates the existing quota controls Docker disables btrfs quotas creating an unrecoverable broken btrfs volume

5 participants

@vadorovsky @cpuguy83 @tianon @kolyshkin @thaJeztah