From 7f1491bfddded2567b7fe0dc34922196d11eecb8 Mon Sep 17 00:00:00 2001 From: Thomas Parrott Date: Thu, 24 Nov 2022 13:26:39 +0000 Subject: [PATCH] lxd/fsmonitor/drivers/driver/fsnotify: Switch away from github.com/fsnotify/fsnotify package There is an unfortunate behaviour in the fsnotify package in that it watches for all events on a path, and doesn't give the user the option of only watching for specific events. This means that when watching, for example on /dev/net, any writes to /dev/net/tun will cause a WRITE event to be sent to LXD. As you can image, for high traffic interfaces this can cause many events and causes LXD to use a lot of CPU. This is equally the case for other devices that can be written to, such as block devices. The issue is identified in the upstream package (https://github.com/fsnotify/fsnotify/issues/7) but there doesn't seem to be any consensus on how to move forward so the issue has languished. LXD however doesn't need cross-platform support that fsnotify package offers and so can use a lightweight wrapper around the inotify feature (which is what we use with fsnotify anyway). There used to be an inotify wrapper at golang.org/x/exp/inotify which has subsequently moved to gopkg.in/fsnotify.v0. Unfortunately that doesn't support go mod and so isn't usable by LXD. Its also not been updated since 2015. There is however a fork of golang.org/x/exp/inotify at https://github.com/kubernetes/utils/tree/master/inotify which has seen some minor maintenance this year. This package also has go mod support. This commit also makes the following changes: - Avoids the need to make a stat syscall to detect if event is for a directory by using the InIsdir flag in the event Mask. - Avoids calling every registered handler for every new directory and instead performs a path prefix match. - Improves comments and readability. - Cleans the path coming from the event to ensure comparison with registered handlers works. - Reduces repetition in event action logic. Fixes #11151 Signed-off-by: Thomas Parrott --- lxd/fsmonitor/drivers/driver_fsnotify.go | 72 ++++++++++++++---------- 1 file changed, 41 insertions(+), 31 deletions(-) diff --git a/lxd/fsmonitor/drivers/driver_fsnotify.go b/lxd/fsmonitor/drivers/driver_fsnotify.go index 33df03f1015b..54ce1c470859 100644 --- a/lxd/fsmonitor/drivers/driver_fsnotify.go +++ b/lxd/fsmonitor/drivers/driver_fsnotify.go @@ -6,8 +6,9 @@ import ( "fmt" "os" "path/filepath" + "strings" - fsn "github.com/fsnotify/fsnotify" + "k8s.io/utils/inotify" "github.com/lxc/lxd/shared" "github.com/lxc/lxd/shared/logger" @@ -18,7 +19,7 @@ var fsnotifyLoaded bool type fsnotify struct { common - watcher *fsn.Watcher + watcher *inotify.Watcher } func (d *fsnotify) load(ctx context.Context) error { @@ -28,7 +29,7 @@ func (d *fsnotify) load(ctx context.Context) error { var err error - d.watcher, err = fsn.NewWatcher() + d.watcher, err = inotify.NewWatcher() if err != nil { return fmt.Errorf("Failed to initialize fsnotify: %w", err) } @@ -50,35 +51,45 @@ func (d *fsnotify) load(ctx context.Context) error { func (d *fsnotify) getEvents(ctx context.Context) { for { select { - // Clean up if context is done + // Clean up if context is done. case <-ctx.Done(): _ = d.watcher.Close() fsnotifyLoaded = false return - case event := <-d.watcher.Events: - // Only consider create and remove events - if event.Op&fsn.Create == 0 && event.Op&fsn.Remove == 0 { + case event := <-d.watcher.Event: + event.Name = filepath.Clean(event.Name) + isCreate := event.Mask&inotify.InCreate != 0 + isDelete := event.Mask&inotify.InDelete != 0 + + // Only consider create and delete events. + if !isCreate && !isDelete { continue } - // If there's a new event for a directory, watch it if it's a create event. - // Always call the handlers in case a watched file is inside the newly created or - // now deleted directory, otherwise we'll miss the event. - stat, err := os.Lstat(event.Name) - if err == nil && stat.IsDir() { - if event.Op&fsn.Create != 0 { + // New event for a directory. + if event.Mask&inotify.InIsdir != 0 { + // If it's a create event, then setup watches on any sub-directories. + if isCreate { _ = d.watchFSTree(event.Name) } - // Check whether there's a watch on a specific file or directory. + // Check whether there's a watch on the directory. d.mu.Lock() - for path := range d.watches { - var action Event + var action Event - if event.Op&fsn.Create != 0 { - action = Add - } else { - action = Remove + if isCreate { + action = Add + } else { + action = Remove + } + + for path := range d.watches { + // Always call the handlers that have a prefix of the event path, + // in case a watched file is inside the newly created or now deleted + // directory, otherwise we'll miss the event. The handlers themselves are + // expected to check the state of the specific path they are interested in. + if !strings.HasPrefix(path, event.Name) { + continue } for identifier, f := range d.watches[path] { @@ -98,19 +109,18 @@ func (d *fsnotify) getEvents(ctx context.Context) { // Check whether there's a watch on a specific file or directory. d.mu.Lock() + var action Event + if isCreate { + action = Add + } else { + action = Remove + } + for path := range d.watches { if event.Name != path { continue } - var action Event - - if event.Op&fsn.Create != 0 { - action = Add - } else { - action = Remove - } - for identifier, f := range d.watches[path] { ret := f(path, action.String()) if !ret { @@ -126,7 +136,7 @@ func (d *fsnotify) getEvents(ctx context.Context) { } d.mu.Unlock() - case err := <-d.watcher.Errors: + case err := <-d.watcher.Error: d.logger.Error("Received event error", logger.Ctx{"err": err}) } } @@ -153,8 +163,8 @@ func (d *fsnotify) watchFSTree(path string) error { return nil } - // Only watch on real paths. - err = d.watcher.Add(path) + // Only watch on real paths for CREATE and DELETE events. + err = d.watcher.AddWatch(path, inotify.InCreate|inotify.InDelete) if err != nil { d.logger.Warn("Failed to watch path", logger.Ctx{"path": path, "err": err}) return nil