Skip to content

Commit

Permalink
lxd/fsmonitor/drivers/driver/fsnotify: Switch away from github.com/fs…
Browse files Browse the repository at this point in the history
…notify/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 (fsnotify/fsnotify#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 <thomas.parrott@canonical.com>
  • Loading branch information
tomponline authored and stgraber committed Jan 10, 2023
1 parent bea5489 commit 7f1491b
Showing 1 changed file with 41 additions and 31 deletions.
72 changes: 41 additions & 31 deletions lxd/fsmonitor/drivers/driver_fsnotify.go
Expand Up @@ -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"
Expand All @@ -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 {
Expand All @@ -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)
}
Expand All @@ -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] {
Expand All @@ -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 {
Expand All @@ -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})
}
}
Expand All @@ -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
Expand Down

0 comments on commit 7f1491b

Please sign in to comment.