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

aufs: apply dirperm1 by default if supported #11799

Merged
merged 3 commits into from Mar 30, 2015

Conversation

Projects
None yet
7 participants
@dqminh
Contributor

dqminh commented Mar 26, 2015

Automatically detect support for aufs dirperm1 option and apply it.
dirperm1 tells aufs to check the permission bits of the directory on the
topmost branch and ignore the permission bits on all lower branches.
It can be used to fix aufs' permission bug (i.e., upper layer having
broader mask than the lower layer).

dirperm1 man page is at: http://aufs.sourceforge.net/aufs3/man.html

Fixes #783

@dqminh

This comment has been minimized.

Show comment
Hide comment
@dqminh

dqminh Mar 26, 2015

Contributor

cc @tiborvass

Also it seems like enabling dirperm1 makes remounting layer significantly slower (1+ms in my VM) ( happens when the user has a lot of layers that total length of mount options exceeds PAGE_SIZE ). TestMountMoreThan42Layers is much more slower with this patch.

Contributor

dqminh commented Mar 26, 2015

cc @tiborvass

Also it seems like enabling dirperm1 makes remounting layer significantly slower (1+ms in my VM) ( happens when the user has a lot of layers that total length of mount options exceeds PAGE_SIZE ). TestMountMoreThan42Layers is much more slower with this patch.

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Mar 26, 2015

Collaborator

+1 for this, I like it.

Ping @vbatts @unclejack feel free to step in to say if you don't like the way this is implemented.

Collaborator

tiborvass commented Mar 26, 2015

+1 for this, I like it.

Ping @vbatts @unclejack feel free to step in to say if you don't like the way this is implemented.

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Mar 26, 2015

Collaborator

@jfrazelle this would be a great fix that a lot of people would welcome. Also it would close an issue with a 3-digit number :P

@dqminh would you mind removing the part that mentions the issue in some KNOWN issues section somewhere?

Collaborator

tiborvass commented Mar 26, 2015

@jfrazelle this would be a great fix that a lot of people would welcome. Also it would close an issue with a 3-digit number :P

@dqminh would you mind removing the part that mentions the issue in some KNOWN issues section somewhere?

aufs: apply dirperm1 by default if supported
Automatically detect support for aufs `dirperm1` option and apply it.
`dirperm1` tells aufs to check the permission bits of the directory on the
topmost branch and ignore the permission bits on all lower branches.
It can be used to fix aufs' permission bug (i.e., upper layer having
broader mask than the lower layer).

More information about the bug can be found at #783
`dirperm1` man page is at: http://aufs.sourceforge.net/aufs3/man.html

Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com>
@dqminh

This comment has been minimized.

Show comment
Hide comment
@dqminh

dqminh Mar 26, 2015

Contributor

would you mind removing the part that mentions the issue in some KNOWN issues section somewhere?

@tiborvass this won't fix it for system that doesnt have dirperm1 patch though, so I think we still need to keep the note. I can add some note about dirperm1 being set as the default option if it's available.

Contributor

dqminh commented Mar 26, 2015

would you mind removing the part that mentions the issue in some KNOWN issues section somewhere?

@tiborvass this won't fix it for system that doesnt have dirperm1 patch though, so I think we still need to keep the note. I can add some note about dirperm1 being set as the default option if it's available.

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Mar 26, 2015

Collaborator

@dqminh fair enough! Thanks :)

Collaborator

tiborvass commented Mar 26, 2015

@dqminh fair enough! Thanks :)

document dirperm1 fix for #783 in known issues
Since `dirperm1` requires a more recent aufs patch than many current OS release,
we cant remove #783 completely. This documents that docker will apply `dirperm1`
automatically for systems that support it

Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com>
@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Mar 26, 2015

Contributor

LGTM

Contributor

crosbymichael commented Mar 26, 2015

LGTM

print dirperm1 supported status in docker info
It's easier for users to check if their systems support dirperm1 just by using
docker info

Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com>
@dqminh

This comment has been minimized.

Show comment
Hide comment
@dqminh

dqminh Mar 26, 2015

Contributor

@tiborvass i added dirperm1 support status to docker info output too so there's an easy way to tell user whether their system is still affected by the permission bug.

Contributor

dqminh commented Mar 26, 2015

@tiborvass i added dirperm1 support status to docker info output too so there's an easy way to tell user whether their system is still affected by the permission bug.

@dqminh

This comment has been minimized.

Show comment
Hide comment
@dqminh

dqminh Mar 30, 2015

Contributor

@tiborvass is this merge-able for 1.6, or is it too late now ?

Contributor

dqminh commented Mar 30, 2015

@tiborvass is this merge-able for 1.6, or is it too late now ?

@jessfraz jessfraz added this to the 1.6.0 milestone Mar 30, 2015

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Mar 30, 2015

Contributor

I added the milestone @crosbymichael is that ok with you

Contributor

jessfraz commented Mar 30, 2015

I added the milestone @crosbymichael is that ok with you

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Mar 30, 2015

Contributor

LGTM

Contributor

vbatts commented Mar 30, 2015

LGTM

crosbymichael added a commit that referenced this pull request Mar 30, 2015

Merge pull request #11799 from dqminh/aufs-dirperm1
aufs: apply dirperm1 by default if supported

@crosbymichael crosbymichael merged commit 14fed35 into moby:master Mar 30, 2015

2 checks passed

janky Jenkins build Docker-PRs 4356 has succeeded
Details
windows Jenkins build Windows-PRs 1348 has succeeded
Details
defer os.RemoveAll(union)
opts := fmt.Sprintf("br:%s,dirperm1,xino=/dev/shm/aufs.xino", base)
if err := mount("none", union, "aufs", 0, opts); err != nil {

This comment has been minimized.

@Blaisorblade

Blaisorblade Jul 20, 2015

Contributor

While investigating some other (transient) docker problem, I saw this error (once) in my kernel log — is it a harmless error produced here while testing the presence of dirperm1? Google wasn't very helpful.

[   16.825747] aufs au_opts_parse:1155:docker[1679]: unknown option dirperm1
@Blaisorblade

Blaisorblade Jul 20, 2015

Contributor

While investigating some other (transient) docker problem, I saw this error (once) in my kernel log — is it a harmless error produced here while testing the presence of dirperm1? Google wasn't very helpful.

[   16.825747] aufs au_opts_parse:1155:docker[1679]: unknown option dirperm1

This comment has been minimized.

@Blaisorblade

Blaisorblade Jul 20, 2015

Contributor

OK, I'm guessing yes:

# docker info | grep Dirperm1
 Dirperm1 Supported: false
@Blaisorblade

Blaisorblade Jul 20, 2015

Contributor

OK, I'm guessing yes:

# docker info | grep Dirperm1
 Dirperm1 Supported: false
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment