ipn/ipnlocal: reduce allocations in TailFS share notifications

This eliminates unnecessary map.Clone() calls and also eliminates
repetitive notifications about the same set of shares.

Updates tailscale/corp#16827

Signed-off-by: Percy Wegmann <percy@tailscale.com>
This commit is contained in:
Percy Wegmann 2024-02-29 13:49:45 -06:00 committed by Percy Wegmann
parent 6f66f5a75a
commit fd942b5384
2 changed files with 25 additions and 28 deletions

View File

@ -308,6 +308,10 @@ type LocalBackend struct {
// Last ClientVersion received in MapResponse, guarded by mu. // Last ClientVersion received in MapResponse, guarded by mu.
lastClientVersion *tailcfg.ClientVersion lastClientVersion *tailcfg.ClientVersion
// notifyTailFSSharesOnce is used to only send one initial notification
// with the latest set of TailFS shares.
notifyTailFSSharesOnce sync.Once
} }
type updateStatus struct { type updateStatus struct {
@ -431,9 +435,7 @@ func NewLocalBackend(logf logger.Logf, logID logid.PublicID, sys *tsd.System, lo
// initialize TailFS shares from saved state // initialize TailFS shares from saved state
fs, ok := b.sys.TailFSForRemote.GetOK() fs, ok := b.sys.TailFSForRemote.GetOK()
if ok { if ok {
b.mu.Lock() shares, err := b.TailFSGetShares()
shares, err := b.tailFSGetSharesLocked()
b.mu.Unlock()
if err == nil && len(shares) > 0 { if err == nil && len(shares) > 0 {
fs.SetShares(shares) fs.SetShares(shares)
} }
@ -2283,7 +2285,7 @@ func (b *LocalBackend) WatchNotifications(ctx context.Context, mask ipn.NotifyWa
ini.NetMap = b.netMap ini.NetMap = b.netMap
} }
if mask&ipn.NotifyInitialTailFSShares != 0 && b.tailFSSharingEnabledLocked() { if mask&ipn.NotifyInitialTailFSShares != 0 && b.tailFSSharingEnabledLocked() {
shares, err := b.tailFSGetSharesLocked() shares, err := b.TailFSGetShares()
if err != nil { if err != nil {
b.logf("unable to notify initial tailfs shares: %v", err) b.logf("unable to notify initial tailfs shares: %v", err)
} else { } else {
@ -4669,7 +4671,7 @@ func (b *LocalBackend) setNetMapLocked(nm *netmap.NetworkMap) {
if b.tailFSSharingEnabledLocked() { if b.tailFSSharingEnabledLocked() {
b.updateTailFSPeersLocked(nm) b.updateTailFSPeersLocked(nm)
b.tailFSNotifyCurrentSharesLocked() b.tailFSNotifyCurrentSharesOnce()
} }
} }

View File

@ -7,7 +7,6 @@
"encoding/json" "encoding/json"
"errors" "errors"
"fmt" "fmt"
"maps"
"os" "os"
"regexp" "regexp"
"strings" "strings"
@ -115,7 +114,7 @@ func (b *LocalBackend) tailfsAddShareLocked(share *tailfs.Share) (map[string]*ta
return nil, errors.New("tailfs not enabled") return nil, errors.New("tailfs not enabled")
} }
shares, err := b.tailFSGetSharesLocked() shares, err := b.TailFSGetShares()
if err != nil { if err != nil {
return nil, err return nil, err
} }
@ -130,7 +129,7 @@ func (b *LocalBackend) tailfsAddShareLocked(share *tailfs.Share) (map[string]*ta
} }
fs.SetShares(shares) fs.SetShares(shares)
return maps.Clone(shares), nil return shares, nil
} }
// TailFSRemoveShare removes the named share. Share names are forced to // TailFSRemoveShare removes the named share. Share names are forced to
@ -161,7 +160,7 @@ func (b *LocalBackend) tailfsRemoveShareLocked(name string) (map[string]*tailfs.
return nil, errors.New("tailfs not enabled") return nil, errors.New("tailfs not enabled")
} }
shares, err := b.tailFSGetSharesLocked() shares, err := b.TailFSGetShares()
if err != nil { if err != nil {
return nil, err return nil, err
} }
@ -180,7 +179,7 @@ func (b *LocalBackend) tailfsRemoveShareLocked(name string) (map[string]*tailfs.
} }
fs.SetShares(shares) fs.SetShares(shares)
return maps.Clone(shares), nil return shares, nil
} }
// tailfsNotifyShares notifies IPN bus listeners (e.g. Mac Application process) // tailfsNotifyShares notifies IPN bus listeners (e.g. Mac Application process)
@ -189,28 +188,24 @@ func (b *LocalBackend) tailfsNotifyShares(shares map[string]*tailfs.Share) {
b.send(ipn.Notify{TailFSShares: shares}) b.send(ipn.Notify{TailFSShares: shares})
} }
// tailFSNotifyCurrentSharesLocked sends an ipn.Notify with the current set of // tailFSNotifyCurrentSharesOnce sends a one-time ipn.Notify with the current
// TailFS shares. // set of TailFS shares.
func (b *LocalBackend) tailFSNotifyCurrentSharesLocked() { func (b *LocalBackend) tailFSNotifyCurrentSharesOnce() {
shares, err := b.tailFSGetSharesLocked() b.notifyTailFSSharesOnce.Do(func() {
if err != nil { shares, err := b.TailFSGetShares()
b.logf("error notifying current tailfs shares: %v", err) if err != nil {
return b.logf("error notifying current tailfs shares: %v", err)
} return
// Do the below on a goroutine to avoid deadlocking on b.mu in b.send(). }
go b.tailfsNotifyShares(maps.Clone(shares)) // Do the below on a goroutine to avoid deadlocking on b.mu in b.send().
go b.tailfsNotifyShares(shares)
})
} }
// TailFSGetShares returns the current set of shares from the state store, // TailFSGetShares returns the current set of shares from the state store,
// stored under ipn.StateKey("_tailfs-shares"). // stored under ipn.StateKey("_tailfs-shares"). The caller owns this map and
// is free to mutate it.
func (b *LocalBackend) TailFSGetShares() (map[string]*tailfs.Share, error) { func (b *LocalBackend) TailFSGetShares() (map[string]*tailfs.Share, error) {
b.mu.Lock()
defer b.mu.Unlock()
return b.tailFSGetSharesLocked()
}
func (b *LocalBackend) tailFSGetSharesLocked() (map[string]*tailfs.Share, error) {
data, err := b.store.ReadState(tailfsSharesStateKey) data, err := b.store.ReadState(tailfsSharesStateKey)
if err != nil { if err != nil {
if errors.Is(err, ipn.ErrStateNotExist) { if errors.Is(err, ipn.ErrStateNotExist) {