Skip to content

[Linux] Implement update methods on Linux for placeholder files/dirs#1128

Merged
chrisd8088 merged 9 commits intomicrosoft:features/linuxprototypefrom
github:linux-projfs-update-view
May 17, 2019
Merged

[Linux] Implement update methods on Linux for placeholder files/dirs#1128
chrisd8088 merged 9 commits intomicrosoft:features/linuxprototypefrom
github:linux-projfs-update-view

Conversation

@chrisd8088
Copy link
Contributor

@chrisd8088 chrisd8088 commented May 9, 2019

For the Linux implementation of the equivalent of the PrjFSLib.Mac.DeleteFile() method (and PrjFS_DeleteFile() C++ function), we rely on the projfs_get_attrs() function in our libprojfs library, which allows us to read the user.projection.empty xattr. If this xattr exists, the file or directory is not fully local, and may be deleted; otherwise, we return an error.

Our implementation of these functions follows the logic in the Mac PrjFSLib.cpp code, including the small races in the DeleteFile() method between performing a stat(2) to check the nature of the file/dir, then a getxattr(2) to look for our projection flag xattr, and finally an unlink(2) or rmdir(2). (Note that unlike the Mac C++ implementation, here these low-level syscalls are actually performed in either libprojfs or in the C# standard library; they are just wrapped inside our calls to ProjFS.GetProjState(), File.Delete(), etc.)

Although we could try for a "more atomic" implementation by moving the code into libprojfs and acquiring a flock(2), after consultation with @wilbaker this seems excessive, given that Git itself performs stat-followed-by-delete when trying to avoid deleting locally modified files, which is all this code is doing too.

Separately, we ensure Linux platform CI builds succeed by ignoring the Profiling(Release) builds (as done for Mac as well) and searching the usual system locations for our libprojfs headers and library.

Along with #1104, #1119, and #1123 (ALL DONE), this PR is a prerequisite for the GVFS.Platform.Linux implementation in #1125.

Will resolve github#13.

@chrisd8088
Copy link
Contributor Author

/azp run PR - Linux - Build and Unit Test

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@chrisd8088 chrisd8088 force-pushed the features/linuxprototype branch from 47fb515 to 972f05a Compare May 11, 2019 05:14
@chrisd8088 chrisd8088 force-pushed the linux-projfs-update-view branch 3 times, most recently from 9612110 to 2b77aab Compare May 12, 2019 23:42
Copy link
Contributor

@kivikakk kivikakk left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you 👍

The libprojfs API sets attribute sizes to the appropriate length
of the actual value read, and to -1 if the attribute is not found.
@chrisd8088 chrisd8088 force-pushed the linux-projfs-update-view branch from 2b77aab to 1f2cd71 Compare May 13, 2019 07:40
Copy link
Contributor

@derrickstolee derrickstolee left a comment

Choose a reason for hiding this comment

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

This looks fine to me. I ran a Linux build as a sanity check.

For the Linux implementation of the equivalent of the ProjFS.Mac
DeleteFile() method, we rely on the projfs_get_attrs() function
in the libprojfs library, which allows us to read the
"user.projection.empty" xattr.  If this xattr exists, the file is
not fully local, and may be deleted; otherwise, we return an error.
By using native rmdir(2) syscalls when deleting placeholder
directories, we can specifically detect the non-empty directory
error condition ENOTEMPTY, which is otherwise folded into a
generic IOException by Directory.Delete().

We need to return EDirectoryNotEmpty in this case because the
calling code in GitIndexProjection handles this result as an
expected and non-exceptional condition.

To simplify our code we then also use unlink(2) to delete files
in a common NativeMethods subclass.

We further detect the case where an inode is neither a file
nor a directory (libprojfs's API will return EPERM for such inodes)
by introducing an Unknown projection state and reporting this as
a "full file" condition to our caller, in the same manner as the
macOS PrjFSLib C++ implementation.

Finally, we handle requests to delete the repository's root
directory by returning EDirectoryNotEmpty, instead of ignoring
them and returning Success.  This is necessary to avoid short-
circuiting the logic of the calling UpdatePlaceholders() method
in GitIndexProjection, which otherwise would fail to correctly
populate a new view of the repository if we return Success when
asked to delete the repository's root.  Since the repository's
root directory is expected to always contain, at a minimum, the
.git subdirectory, EDirectoryNotEmpty is appropriate.
@chrisd8088 chrisd8088 force-pushed the linux-projfs-update-view branch from 5d9c233 to 18a1486 Compare May 15, 2019 21:33
@chrisd8088
Copy link
Contributor Author

chrisd8088 commented May 15, 2019

Hi @kivikakk and @derrickstolee -- would you mind taking a second look at this PR, now that I've ended up making some moderately significant changes (in 18a1486)?

In summary: from studying the calling context for #1160, I realized we really should be returning EDirectoryNotEmpty when a directory removal fails for that reason, but AFAIK there's no way to pick out that specific error condition from the generic IOException returned by Directory.Delete(). Hence the switch to using the libc syscalls instead.

For the same reason, we also return EDirectoryNotEmpty when asked to remove the repo's root (which we can reasonably assume to never be empty, since it should have .git in it); this is a belt-and-suspenders check so that even if #1160 is merged in some form, any other callers won't trigger the same issue.

My apologies for the extra go-round of another review cycle.

@kivikakk
Copy link
Contributor

but AFAIK there's no way to pick out that specific error condition from the generic IOException returned by Directory.Delete().

The IOException's Message property will read "Directory not empty", but I don't know how stable that text can be expected to be. The HResult corresponds to the errno, and indeed, on Linux trying to perform Directory.Delete() on a non-empty directory yields an IOException with a HResult of 39 (ENOTEMPTY).

All looks good otherwise, though I'd slightly prefer using the framework methods over libc where possible.

@chrisd8088
Copy link
Contributor Author

chrisd8088 commented May 16, 2019

Me too, @kivikakk -- that is, I had originally written this using Directory.Delete() and File.Delete() and was happy to avoid any DllImports. I could go back if there's a stable way to convert IOException to a Linux errno -- thanks for the tip about the HResult property. Some sources suggest bitwise masking that; there's also this dotnet/corefx#34221 thread about making IOException more fine-grained.

I did find one example in the existing GVFS codebase of non-Windows code testing an exception's HResult.

I'm honestly not sure if it would make this code more or less readable/maintainable to go back to Directory.Delete() and File.Delete() -- I'm zonked tonight, but let me try it that way tomorrow and if it looks better, I'll add a second commit to this PR.

We can return to using managed methods to delete files and
directories when updating projection views on Linux, so long as
we check the IOException's HResult property to detect the
EDirectoryNotEmpty (ENOTEMPTY) condition.

Also set the (unused?) failure cause of ReadOnly when an
access exception occurs.

Thanks to @kivikakk for the hint on checking HResult!
@chrisd8088
Copy link
Contributor Author

Hi @kivikakk and @derrickstolee -- I think third time might be a charm on this PR. Would you mind doing a once-over and re-approving if it looks OK? Thanks so much and apologies for all the churn.

Copy link
Contributor

@kivikakk kivikakk left a comment

Choose a reason for hiding this comment

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

LGTM! One suggestion.

Co-authored-by: Ashe Connor <ashe@kivikakk.ee>
@chrisd8088
Copy link
Contributor Author

For reference, @derrickstolee, here's what's changed:
https://github.com/github/VFSForGit/compare/ac974c7..a50f57a

Copy link
Contributor

@derrickstolee derrickstolee left a comment

Choose a reason for hiding this comment

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

LGTM. Sorry for the review delay.

@chrisd8088
Copy link
Contributor Author

There's no delay to apologize for, @derrickstolee -- I only finished this up last night! Thanks for the fast review!

@chrisd8088 chrisd8088 merged commit 3d639ff into microsoft:features/linuxprototype May 17, 2019
@chrisd8088 chrisd8088 deleted the linux-projfs-update-view branch May 17, 2019 16:39
chrisd8088 added a commit that referenced this pull request May 21, 2019
[Linux] Implement update methods on Linux for placeholder files/dirs

For the Linux implementation of the equivalent of the PrjFSLib.Mac
DeleteFile(), UpdatePlaceholderIfNeeded(), and
ReplacePlaceholderFileWithSymLink() methods, we rely on the
projfs_get_attrs() function in our libprojfs library, which allows us to
read the "user.projection.empty" xattr. If this xattr exists, the file or
directory is not fully local, and may be deleted.

Our implementation of these functions follows the logic in the Mac
C++ code, including the small races in the DeleteFile() method
between performing a stat(2) to check the nature of the file/dir, then
a getxattr(2) to look for our projection flag xattr, and finally a File.Delete()
or Directory.Delete().

We detect the case where an inode is neither a file nor a directory
by introducing an Unknown projection state and reporting this as
a "full file" condition to our caller, again parallel to the Mac C++ code.

We handle requests to delete the repository's root directory by returning
EDirectoryNotEmpty, instead of ignoring them and returning Success.
This is necessary to avoid short-circuiting the logic of the calling
UpdatePlaceholders() method in GitIndexProjection.

Separately, we ensure Linux platform CI builds succeed by ignoring
the Profiling(Release) builds (as done for Mac as well) and searching
the usual system locations for our libprojfs headers and library.
chrisd8088 added a commit that referenced this pull request May 21, 2019
[Linux] Implement update methods on Linux for placeholder files/dirs

For the Linux implementation of the equivalent of the PrjFSLib.Mac
DeleteFile(), UpdatePlaceholderIfNeeded(), and
ReplacePlaceholderFileWithSymLink() methods, we rely on the
projfs_get_attrs() function in our libprojfs library, which allows us to
read the "user.projection.empty" xattr. If this xattr exists, the file or
directory is not fully local, and may be deleted.

Our implementation of these functions follows the logic in the Mac
C++ code, including the small races in the DeleteFile() method
between performing a stat(2) to check the nature of the file/dir, then
a getxattr(2) to look for our projection flag xattr, and finally a File.Delete()
or Directory.Delete().

We detect the case where an inode is neither a file nor a directory
by introducing an Unknown projection state and reporting this as
a "full file" condition to our caller, again parallel to the Mac C++ code.

We handle requests to delete the repository's root directory by returning
EDirectoryNotEmpty, instead of ignoring them and returning Success.
This is necessary to avoid short-circuiting the logic of the calling
UpdatePlaceholders() method in GitIndexProjection.

Separately, we ensure Linux platform CI builds succeed by ignoring
the Profiling(Release) builds (as done for Mac as well) and searching
the usual system locations for our libprojfs headers and library.
kivikakk pushed a commit to github/VFSForGit that referenced this pull request May 24, 2019
[Linux] Implement update methods on Linux for placeholder files/dirs

For the Linux implementation of the equivalent of the PrjFSLib.Mac
DeleteFile(), UpdatePlaceholderIfNeeded(), and
ReplacePlaceholderFileWithSymLink() methods, we rely on the
projfs_get_attrs() function in our libprojfs library, which allows us to
read the "user.projection.empty" xattr. If this xattr exists, the file or
directory is not fully local, and may be deleted.

Our implementation of these functions follows the logic in the Mac
C++ code, including the small races in the DeleteFile() method
between performing a stat(2) to check the nature of the file/dir, then
a getxattr(2) to look for our projection flag xattr, and finally a File.Delete()
or Directory.Delete().

We detect the case where an inode is neither a file nor a directory
by introducing an Unknown projection state and reporting this as
a "full file" condition to our caller, again parallel to the Mac C++ code.

We handle requests to delete the repository's root directory by returning
EDirectoryNotEmpty, instead of ignoring them and returning Success.
This is necessary to avoid short-circuiting the logic of the calling
UpdatePlaceholders() method in GitIndexProjection.

Separately, we ensure Linux platform CI builds succeed by ignoring
the Profiling(Release) builds (as done for Mac as well) and searching
the usual system locations for our libprojfs headers and library.
@jrbriggs jrbriggs modified the milestone: M153 May 24, 2019
chrisd8088 added a commit that referenced this pull request May 24, 2019
[Linux] Implement update methods on Linux for placeholder files/dirs

For the Linux implementation of the equivalent of the PrjFSLib.Mac
DeleteFile(), UpdatePlaceholderIfNeeded(), and
ReplacePlaceholderFileWithSymLink() methods, we rely on the
projfs_get_attrs() function in our libprojfs library, which allows us to
read the "user.projection.empty" xattr. If this xattr exists, the file or
directory is not fully local, and may be deleted.

Our implementation of these functions follows the logic in the Mac
C++ code, including the small races in the DeleteFile() method
between performing a stat(2) to check the nature of the file/dir, then
a getxattr(2) to look for our projection flag xattr, and finally a File.Delete()
or Directory.Delete().

We detect the case where an inode is neither a file nor a directory
by introducing an Unknown projection state and reporting this as
a "full file" condition to our caller, again parallel to the Mac C++ code.

We handle requests to delete the repository's root directory by returning
EDirectoryNotEmpty, instead of ignoring them and returning Success.
This is necessary to avoid short-circuiting the logic of the calling
UpdatePlaceholders() method in GitIndexProjection.

Separately, we ensure Linux platform CI builds succeed by ignoring
the Profiling(Release) builds (as done for Mac as well) and searching
the usual system locations for our libprojfs headers and library.
chrisd8088 added a commit that referenced this pull request Jul 20, 2019
[Linux] Implement update methods on Linux for placeholder files/dirs

For the Linux implementation of the equivalent of the PrjFSLib.Mac
DeleteFile(), UpdatePlaceholderIfNeeded(), and
ReplacePlaceholderFileWithSymLink() methods, we rely on the
projfs_get_attrs() function in our libprojfs library, which allows us to
read the "user.projection.empty" xattr. If this xattr exists, the file or
directory is not fully local, and may be deleted.

Our implementation of these functions follows the logic in the Mac
C++ code, including the small races in the DeleteFile() method
between performing a stat(2) to check the nature of the file/dir, then
a getxattr(2) to look for our projection flag xattr, and finally a File.Delete()
or Directory.Delete().

We detect the case where an inode is neither a file nor a directory
by introducing an Unknown projection state and reporting this as
a "full file" condition to our caller, again parallel to the Mac C++ code.

We handle requests to delete the repository's root directory by returning
EDirectoryNotEmpty, instead of ignoring them and returning Success.
This is necessary to avoid short-circuiting the logic of the calling
UpdatePlaceholders() method in GitIndexProjection.

Separately, we ensure Linux platform CI builds succeed by ignoring
the Profiling(Release) builds (as done for Mac as well) and searching
the usual system locations for our libprojfs headers and library.
chrisd8088 added a commit that referenced this pull request Jul 26, 2019
[Linux] Implement update methods on Linux for placeholder files/dirs

For the Linux implementation of the equivalent of the PrjFSLib.Mac
DeleteFile(), UpdatePlaceholderIfNeeded(), and
ReplacePlaceholderFileWithSymLink() methods, we rely on the
projfs_get_attrs() function in our libprojfs library, which allows us to
read the "user.projection.empty" xattr. If this xattr exists, the file or
directory is not fully local, and may be deleted.

Our implementation of these functions follows the logic in the Mac
C++ code, including the small races in the DeleteFile() method
between performing a stat(2) to check the nature of the file/dir, then
a getxattr(2) to look for our projection flag xattr, and finally a File.Delete()
or Directory.Delete().

We detect the case where an inode is neither a file nor a directory
by introducing an Unknown projection state and reporting this as
a "full file" condition to our caller, again parallel to the Mac C++ code.

We handle requests to delete the repository's root directory by returning
EDirectoryNotEmpty, instead of ignoring them and returning Success.
This is necessary to avoid short-circuiting the logic of the calling
UpdatePlaceholders() method in GitIndexProjection.

Separately, we ensure Linux platform CI builds succeed by ignoring
the Profiling(Release) builds (as done for Mac as well) and searching
the usual system locations for our libprojfs headers and library.
chrisd8088 added a commit that referenced this pull request Jul 28, 2019
[Linux] Implement update methods on Linux for placeholder files/dirs

For the Linux implementation of the equivalent of the PrjFSLib.Mac
DeleteFile(), UpdatePlaceholderIfNeeded(), and
ReplacePlaceholderFileWithSymLink() methods, we rely on the
projfs_get_attrs() function in our libprojfs library, which allows us to
read the "user.projection.empty" xattr. If this xattr exists, the file or
directory is not fully local, and may be deleted.

Our implementation of these functions follows the logic in the Mac
C++ code, including the small races in the DeleteFile() method
between performing a stat(2) to check the nature of the file/dir, then
a getxattr(2) to look for our projection flag xattr, and finally a File.Delete()
or Directory.Delete().

We detect the case where an inode is neither a file nor a directory
by introducing an Unknown projection state and reporting this as
a "full file" condition to our caller, again parallel to the Mac C++ code.

We handle requests to delete the repository's root directory by returning
EDirectoryNotEmpty, instead of ignoring them and returning Success.
This is necessary to avoid short-circuiting the logic of the calling
UpdatePlaceholders() method in GitIndexProjection.

Separately, we ensure Linux platform CI builds succeed by ignoring
the Profiling(Release) builds (as done for Mac as well) and searching
the usual system locations for our libprojfs headers and library.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants