Skip to content
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

os: Remove can result in unreclaimed space due to interaction between syscall.Unlink and filesystem #16452

Closed
szaydel opened this issue Jul 20, 2016 · 7 comments

Comments

@szaydel
Copy link

@szaydel szaydel commented Jul 20, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)? 1.6.3
  2. What operating system and processor architecture are you using (go env)? Solaris Variant (Illumos-based BrickstorOS)
  3. What did you do?

This is issue has been reproduced through a number of tests, all done over a tmpfs filesystem. At least as far we know, tmpfs as implemented on Solaris and variants anyway, does not result in space associated with children of a "directory" being reclaimed through a unlink syscall, without calling rmdir. What happens, based on the code in os.Remove is we first unlink and check for errors. If syscall.Unlink does not return an error, we assume we are done and return. Otherwise we try next case, which is syscall.Rmdir. In our case it appears that because Unlink does not result in an error, we never get to the Rmdir call, and so directory is apparently removed, but the capacity that was associated with its children is not reclaimed until after filesystem is unmounted.

If possible, provide a recipe for reproducing the error.

Recipe is quite straight-forward. This is reproducible each time with the os.RemoveAll function, which uses os.Remove, where the problem seems to be.

  1. Create a directory and fill with some data, one or more files will do.
  2. Use os.RemoveAll to remove that directory and its contents.
  1. What did you expect to see?

Expected for directory to be removed, and space associated with all files under that directory to be freed.

  1. What did you see instead?

Space is not freed as a result, only directory appears to be removed, as well as its contents.

@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Jul 20, 2016

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jul 20, 2016

Yeah, sounds like a Solaris tmpfs kernel bug.

Please provide a sample program to demonstrate it.

@ianlancetaylor ianlancetaylor changed the title os.Remove can result in unreclaimed space due to interaction between syscall.Unlink and filesystem os: Remove can result in unreclaimed space due to interaction between syscall.Unlink and filesystem Jul 20, 2016
@ianlancetaylor ianlancetaylor added this to the Go1.8Maybe milestone Jul 20, 2016
@szaydel

This comment has been minimized.

Copy link
Author

@szaydel szaydel commented Jul 21, 2016

This is a tmpfs bug. But it seems like checking whether something is a dir and rmdir(ing) it instead of unlink is a good practice. There are a lot of filesystems out there, and POSIX spec clearly says that it is not necessarily supported everywhere. There is already work my team is doing to address this in Illumos, but it will still likely be a problem in Solaris, and in earlier versions which are still in active use, but are not really maintained, like OpenSolaris, and OpenIndiana.

As for sample program, I am assuming you would like that to be a playground link?

Thanks for considering this!

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jul 21, 2016

This is a tmpfs bug. But it seems like checking whether something is a dir and rmdir(ing) it instead of unlink is a good practice.

Glad we helped find a bug! :) From memory, this seems like about the 9th kernel bug Go has accidentally brought to light in various operating systems.

As for sample program, I am assuming you would like that to be a playground link?

Or a gist. Whatever you'd like. Something I can click and read online, even if it doesn't run.

@szaydel

This comment has been minimized.

Copy link
Author

@szaydel szaydel commented Jul 21, 2016

Bugs exposed in indirect ways like these are always awesome, and certainly appreciated. :)

Here's that link: https://play.golang.org/p/u8fVnADfsN Program is naive, but should communicate intent. Thanks again!

@binarycrusader

This comment has been minimized.

Copy link
Contributor

@binarycrusader binarycrusader commented Sep 9, 2016

Solaris does not support using unlink() on directories on ZFS or tmpfs, Solaris 10 and newer (at least) will fail with EPERM if you attempt this either using a ZFS or tmpfs system, so the tmpfs bug appears to be specific to Illumos.

Using the sample program, we can see this if we truss it on any version of Solaris 10+:

/1: unlink("/tmp/tdir") Err#1 EPERM
/1: lwp_unpark(4) = 0
/4: lwp_park(0x00000000, 0) = 0
/1: rmdir("/tmp/tdir") Err#17 EEXIST
...
/1: unlink("/tmp/tdir/testfile.txt") = 0
...
/1: unlink("/tmp/tdir") Err#1 EPERM
/1: lwp_unpark(4) = 0
/4: lwp_park(0x00000000, 0) = 0
/1: rmdir("/tmp/tdir") = 0
/1: _exit(0)

On Solaris, go should always use rmdir() instead to not only avoid the unnecessary overhead of trying and failing repeatedly, but the other undesirable semantics if it is allowed:

   If the path  argument  is  a  directory  and  the  filesystem  supports
   unlink()  and unlinkat() on directories, the directory is unlinked from
   its parent with **no cleanup being performed**.  In UFS,  the  disconnected
   directory  will  be  found the next time the filesystem is checked with
   fsck(1M). The unlink() and unlinkat() functions will  not  fail  simply
   because  a directory is not empty. The user with appropriate privileges
   can orphan a non-empty directory without generating an error message.

Note the "no cleanup" bit above.

In short, Go should not generally be using unlink() to remove directories, at least on Solaris, since it's unlikely to ever work as expected.

OpenSolaris-based derivatives may behave differently of course.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Sep 9, 2016

The source explains why Go does what it does:

func Remove(name string) error {
        // System call interface forces us to know                                                                                        
        // whether name is a file or directory.                                                                                           
        // Try both: it is cheaper on average than                                                                                        
        // doing a Stat plus the right one.                                                                                               
        e := syscall.Unlink(name)
        if e == nil {
                return nil
        }
        e1 := syscall.Rmdir(name)
        if e1 == nil {
                return nil
        }

        // Both failed: figure out which error to return.                                                                                 
        // OS X and Linux differ on whether unlink(dir)                                                                                   
        // returns EISDIR, so can't use that. However,                                                                                    
        // both agree that rmdir(file) returns ENOTDIR,                                                                                   
        // so we can use that to decide which error is real.                                                                              
        // Rmdir might also return ENOTDIR if given a bad                                                                                 
        // file path, like /etc/passwd/foo, but in that case,                                                                             
        // both errors will be ENOTDIR, so it's okay to                                                                                   
        // use the error from unlink.                                                                                                     
        if e1 != syscall.ENOTDIR {
                e = e1
        }
        return &PathError{"remove", name, e}

As long as Solaris returns an error on Unlink of a directory and doesn't like catch on fire or something, I think we're good here.

@bradfitz bradfitz closed this Sep 9, 2016
@golang golang locked and limited conversation to collaborators Sep 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.