Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cephfs: safeguard localClusterState struct from race conditions #4163

Merged
merged 2 commits into from
Oct 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions internal/cephfs/core/metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,32 @@ func (s *subVolumeClient) isUnsupportedSubVolMetadata(err error) bool {
return true
}

// isSubVolumeGroupCreated returns true if subvolume group is created.
func (s *subVolumeClient) isSubVolumeGroupCreated() bool {
newLocalClusterState(s.clusterID)
clusterAdditionalInfo[s.clusterID].subVolumeGroupsRWMutex.RLock()
defer clusterAdditionalInfo[s.clusterID].subVolumeGroupsRWMutex.RUnlock()

if clusterAdditionalInfo[s.clusterID].subVolumeGroupsCreated == nil {
return false
}

return clusterAdditionalInfo[s.clusterID].subVolumeGroupsCreated[s.SubvolumeGroup]
}

// updateSubVolumeGroupCreated updates subvolume group created map.
// If the map is nil, it creates a new map and updates it.
func (s *subVolumeClient) updateSubVolumeGroupCreated(state bool) {
clusterAdditionalInfo[s.clusterID].subVolumeGroupsRWMutex.Lock()
defer clusterAdditionalInfo[s.clusterID].subVolumeGroupsRWMutex.Unlock()

if clusterAdditionalInfo[s.clusterID].subVolumeGroupsCreated == nil {
clusterAdditionalInfo[s.clusterID].subVolumeGroupsCreated = make(map[string]bool)
}

clusterAdditionalInfo[s.clusterID].subVolumeGroupsCreated[s.SubvolumeGroup] = state
}

// setMetadata sets custom metadata on the subvolume in a volume as a
// key-value pair.
func (s *subVolumeClient) setMetadata(key, value string) error {
Expand Down
30 changes: 20 additions & 10 deletions internal/cephfs/core/volume.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"fmt"
"path"
"strings"
"sync"

cerrors "github.com/ceph/ceph-csi/internal/cephfs/errors"
fsutil "github.com/ceph/ceph-csi/internal/cephfs/util"
Expand All @@ -32,12 +33,17 @@ import (
"github.com/ceph/go-ceph/rados"
)

// clusterAdditionalInfo contains information regarding if resize is
// supported in the particular cluster and subvolumegroup is
// created or not.
// Subvolumegroup creation and volume resize decisions are
// taken through this additional cluster information.
var clusterAdditionalInfo = make(map[string]*localClusterState)
var (
// clusterAdditionalInfo contains information regarding if resize is
// supported in the particular cluster and subvolumegroup is
// created or not.
// Subvolumegroup creation and volume resize decisions are
// taken through this additional cluster information.
clusterAdditionalInfo = make(map[string]*localClusterState)
// clusterAdditionalInfoMutex is used to protect against
// concurrent writes.
clusterAdditionalInfoMutex = sync.Mutex{}
Copy link
Member

Choose a reason for hiding this comment

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

Would a sync.RWMutex not be better?

If you only mean to protect against concurrent writes, please state it clearly.

)

// Subvolume holds subvolume information. This includes only the needed members
// from fsAdmin.SubVolumeInfo.
Expand Down Expand Up @@ -209,14 +215,18 @@ type localClusterState struct {
// set true once a subvolumegroup is created
// for corresponding filesystem in a cluster.
subVolumeGroupsCreated map[string]bool
// subVolumeGroupsRWMutex is used to protect subVolumeGroupsCreated map
// against concurrent writes while allowing multiple readers.
subVolumeGroupsRWMutex sync.RWMutex
Copy link
Member

Choose a reason for hiding this comment

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

describe the usage of this mutex, what does it protect, when do callers need to lock it?

}

func newLocalClusterState(clusterID string) {
// verify if corresponding clusterID key is present in the map,
// and if not, initialize with default values(false).
clusterAdditionalInfoMutex.Lock()
defer clusterAdditionalInfoMutex.Unlock()
if _, keyPresent := clusterAdditionalInfo[clusterID]; !keyPresent {
clusterAdditionalInfo[clusterID] = &localClusterState{}
clusterAdditionalInfo[clusterID].subVolumeGroupsCreated = make(map[string]bool)
}
}

Expand All @@ -232,7 +242,7 @@ func (s *subVolumeClient) CreateVolume(ctx context.Context) error {
}

// create subvolumegroup if not already created for the cluster.
if !clusterAdditionalInfo[s.clusterID].subVolumeGroupsCreated[s.FsName] {
if !s.isSubVolumeGroupCreated() {
opts := fsAdmin.SubVolumeGroupOptions{}
err = ca.CreateSubVolumeGroup(s.FsName, s.SubvolumeGroup, &opts)
if err != nil {
Expand All @@ -246,7 +256,7 @@ func (s *subVolumeClient) CreateVolume(ctx context.Context) error {
return err
}
log.DebugLog(ctx, "cephfs: created subvolume group %s", s.SubvolumeGroup)
clusterAdditionalInfo[s.clusterID].subVolumeGroupsCreated[s.FsName] = true
s.updateSubVolumeGroupCreated(true)
}

opts := fsAdmin.SubVolumeOptions{
Expand All @@ -264,7 +274,7 @@ func (s *subVolumeClient) CreateVolume(ctx context.Context) error {
if errors.Is(err, rados.ErrNotFound) {
// Reset the subVolumeGroupsCreated so that we can try again to create the
// subvolumegroup in next request if the error is Not Found.
clusterAdditionalInfo[s.clusterID].subVolumeGroupsCreated[s.FsName] = false
s.updateSubVolumeGroupCreated(false)
}

return err
Expand Down