fix umount busy#18
Conversation
857a01d to
139a766
Compare
139a766 to
91ef01f
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses "umount busy" issues by refactoring duplicate directory removal logic into a new RemoveDir method that checks if a path is a mount point before attempting removal. The method handles mounted and non-mounted directories differently to prevent "device busy" errors during cleanup.
Key Changes:
- Introduced
RemoveDirmethod that checks mount status before removal - Replaced duplicate cleanup logic in
Remove,Cleanup, andcreateSnapshotwith calls to the new method - Changed removal approach to use
os.Removefor mounted directories andos.RemoveAllfor non-mounted ones
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log.G(ctx).WithError(err1).WithField("path", dir).Warn("failed to unmount directory") | ||
| return | ||
| } | ||
| if err1 := os.Remove(dir); err1 != nil { |
There was a problem hiding this comment.
Using os.Remove instead of os.RemoveAll for mounted directories will fail if the directory is not empty. After unmounting, the directory may still contain files or subdirectories. This should use os.RemoveAll to ensure proper cleanup, consistent with the non-mounted path.
| if err1 := os.Remove(dir); err1 != nil { | |
| if err1 := os.RemoveAll(dir); err1 != nil { |
| log.G(ctx).WithError(err1).WithField("path", path).Error("failed to reclaim snapshot directory, directory may need removal") | ||
| err = fmt.Errorf("failed to remove path: %v: %w", err1, err) | ||
| } | ||
| o.RemoveDir(ctx, path) |
There was a problem hiding this comment.
The error handling behavior has changed in a potentially problematic way. Previously in createSnapshot, if the removal of the path directory failed, the error was propagated and wrapped with the original error using err = fmt.Errorf("failed to remove path: %v: %w", err1, err). Now with RemoveDir, all errors are only logged as warnings and silently ignored. This means that critical cleanup failures during error recovery will no longer be reported to the caller, which could mask serious issues.
| if isMounted { | ||
| if err1 := o.unmountLvm(ctx, dir); err1 != nil { | ||
| log.G(ctx).WithError(err1).WithField("path", dir).Warn("failed to unmount directory") | ||
| return |
There was a problem hiding this comment.
The early return on line 377 after unmount failure prevents the directory from being removed at all. If unmounting fails, the mounted directory will be left behind, leaking resources. Consider logging the unmount error but continuing to attempt removal, or add a retry mechanism. The previous code didn't have this early return and would still attempt os.RemoveAll even if unmount failed.
| return |
| func (o *Snapshotter) RemoveDir(ctx context.Context, dir string) { | ||
| isMounted, err := isMountPoint(dir) | ||
| if err != nil { | ||
| log.G(ctx).WithError(err).WithField("path", dir).Warn("failed to check if path is a mount point") |
There was a problem hiding this comment.
The early return on line 372 when mount point check fails means the directory removal is completely skipped. This could leave orphaned directories if /proc/mounts is temporarily inaccessible or has read errors. Consider falling back to attempting os.RemoveAll anyway, as the previous code would have done.
| log.G(ctx).WithError(err).WithField("path", dir).Warn("failed to check if path is a mount point") | |
| log.G(ctx).WithError(err).WithField("path", dir).Warn("failed to check if path is a mount point") | |
| if err1 := os.RemoveAll(dir); err1 != nil { | |
| log.G(ctx).WithError(err1).WithField("path", dir).Warn("failed to remove directory after mount point check failure") | |
| } |
91ef01f to
206c4c8
Compare
…t "device busy" errors during cleanup (#18)
No description provided.