Run btrfs rescan only if userDiskQuota is enabled#29835
Conversation
|
👍 I did the same patch locally earlier today and works for me. |
|
Thanks for your fix. Good to me. |
There was a problem hiding this comment.
I was looking at the code, and a bit in doubt. The quotaEnabled boolean
is set either if --storage-opt size=btrfs.min_space is set globally (on the daemon), or if a size is passed when creating a container (see set storage driver options per container)
I'm not too familiar with this part of the code; is the Create() method also called when starting an existing container, or only the first time its filesystem is created?
Otherwise, this may not work, because the quotaEnabled variable is only set here; https://github.com/docker/docker/blob/v1.12.5/daemon/graphdriver/btrfs/btrfs.go#L461-L466 (in situations where btrfs.min_space is not set on the daemon)
So, maybe this is the right change, but I'm just writing down my thoughts, in case it's useful.
ping @cpuguy83 you probably know
There was a problem hiding this comment.
Create is not called on container startup. Cleanup has a similar check.
There was a problem hiding this comment.
Yes, this will be a problem on restore, for sure.
We need a better way to determine if quota is enabled.
This fix tries to address the issue raised in 29810 where btrfs subvolume removal failed when docker is in an unprivileged lxc container. The failure was caused by `Failed to rescan btrfs quota` with `operation not permitted`. However, if disk quota is not enabled, there is no need to run a btrfs rescan at the first place. This fix checks for `quotaEnabled` and only run btrfs rescan if `quotaEnabled` is true. This fix fixes 29810. Signed-off-by: Yong Tang <[email protected]>
5c0b0d8 to
b36e613
Compare
|
To check if quota is enabled, I think one way is to look up the qgroup and see if it returns something. In case quota is not enabled then error will be returned. I updated the PR so that now if |
| quotaEnabled = false | ||
| userDiskQuota = false | ||
| ) | ||
|
|
There was a problem hiding this comment.
Think the two global variables could be avoided. the quotaEnabled could be wrapped into driver *, and userDiskQuota is only a temporary variable.
|
|
||
| // Cleanup unmounts the home directory. | ||
| func (d *Driver) Cleanup() error { | ||
| if quotaEnabled { |
There was a problem hiding this comment.
Move quotaEnabled inside the d *Driver to hide the logic of quotaEnabled checking.
| // In case quotaEnabled is not set, check qgroup and update quotaEnabled as needed | ||
| if _, err := subvolLookupQgroup(d.home); err == nil { | ||
| d.quotaEnabled = true | ||
| return nil |
There was a problem hiding this comment.
Perform a qgroup lookup and sync quotaEnabled as needed.
|
ping @cpuguy83 @tonistiigi |
|
LGTM |
9 participants
This fix tries to address the issue raised in #29810 where btrfs subvolume removal failed when docker
is in an unprivileged lxc container. The failure was caused by
Failed to rescan btrfs quotawithoperation not permitted.However, if disk quota is not enabled, there is no need to run a btrfs rescan at the first place.
This fix checks for
quotaEnabledand only run btrfs rescan ifquotaEnabledis true.This fix fixes #29810.
Signed-off-by: Yong Tang [email protected]
/cc @zhuguihua