From 8f446c7463c9183d59a30343682e31ad9f85b307 Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Fri, 12 Aug 2011 19:02:48 -0400 Subject: [PATCH] Windows: Fix cm_serverRef ref counts Use Interlocked operations consistently Simplify cm_ServerInsertList(). It no longer increments the refCount on the serverRef object. Instead it leaves the refCount as is. Its the caller's responsibility to add a reference if required. Add reference counts and hold locks in places where the volume server list was used unprotected. Change-Id: Ie65cdca4461e84c675e8a29e22cef3e15679fda7 Reviewed-on: http://gerrit.openafs.org/5248 Tested-by: BuildBot Reviewed-by: Derrick Brashear --- src/WINNT/afsd/cm_cell.c | 4 ---- src/WINNT/afsd/cm_server.c | 47 ++++++++++++++++++++++++++++++-------- src/WINNT/afsd/cm_server.h | 4 ++++ src/WINNT/afsd/cm_volume.c | 18 +++++---------- 4 files changed, 48 insertions(+), 25 deletions(-) diff --git a/src/WINNT/afsd/cm_cell.c b/src/WINNT/afsd/cm_cell.c index 855d0f97ca..b6aa482b81 100644 --- a/src/WINNT/afsd/cm_cell.c +++ b/src/WINNT/afsd/cm_cell.c @@ -65,10 +65,6 @@ long cm_AddCellProc(void *rockp, struct sockaddr_in *addrp, char *hostnamep, uns /* Insert the vlserver into a sorted list, sorted by server rank */ tsrp = cm_NewServerRef(tsp, 0); cm_InsertServerList(&cellp->vlServersp, tsrp); - /* drop the allocation reference */ - lock_ObtainWrite(&cm_serverLock); - tsrp->refCount--; - lock_ReleaseWrite(&cm_serverLock); return 0; } diff --git a/src/WINNT/afsd/cm_server.c b/src/WINNT/afsd/cm_server.c index 1a3a02f010..963c05d4ae 100644 --- a/src/WINNT/afsd/cm_server.c +++ b/src/WINNT/afsd/cm_server.c @@ -950,6 +950,10 @@ cm_server_vols_t *cm_NewServerVols(void) { return tsvp; } +/* + * cm_NewServerRef() returns with the allocated cm_serverRef_t + * with a refCount of 1. + */ cm_serverRef_t *cm_NewServerRef(cm_server_t *serverp, afs_uint32 volID) { cm_serverRef_t *tsrp; @@ -1010,6 +1014,34 @@ cm_serverRef_t *cm_NewServerRef(cm_server_t *serverp, afs_uint32 volID) return tsrp; } +void cm_GetServerRef(cm_serverRef_t *tsrp, int locked) +{ + afs_int32 refCount; + + if (!locked) + lock_ObtainRead(&cm_serverLock); + refCount = InterlockedIncrement(&tsrp->refCount); + if (!locked) + lock_ReleaseRead(&cm_serverLock); +} + +afs_int32 cm_PutServerRef(cm_serverRef_t *tsrp, int locked) +{ + afs_int32 refCount; + + if (!locked) + lock_ObtainRead(&cm_serverLock); + refCount = InterlockedDecrement(&tsrp->refCount); + osi_assertx(refCount >= 0, "cm_serverRef_t refCount underflow"); + + if (!locked) + lock_ReleaseRead(&cm_serverLock); + + return refCount; +} + + + LONG_PTR cm_ChecksumServerList(cm_serverRef_t *serversp) { LONG_PTR sum = 0; @@ -1035,7 +1067,7 @@ LONG_PTR cm_ChecksumServerList(cm_serverRef_t *serversp) ** Insert a server into the server list keeping the list sorted in ** ascending order of ipRank. ** -** The refCount of the cm_serverRef_t is increased +** The refCount of the cm_serverRef_t is not altered. */ void cm_InsertServerList(cm_serverRef_t** list, cm_serverRef_t* element) { @@ -1046,8 +1078,6 @@ void cm_InsertServerList(cm_serverRef_t** list, cm_serverRef_t* element) current=*list; ipRank = element->server->ipRank; - element->refCount++; /* increase refCount */ - /* insertion into empty list or at the beginning of the list */ if ( !current || (current->server->ipRank > ipRank) ) { @@ -1065,6 +1095,7 @@ void cm_InsertServerList(cm_serverRef_t** list, cm_serverRef_t* element) } element->next = current->next; current->next = element; + lock_ReleaseWrite(&cm_serverLock); } /* @@ -1093,7 +1124,7 @@ long cm_ChangeRankServer(cm_serverRef_t** list, cm_server_t* server) if ( (*current)->server == server) { element = (*current); - *current = (*current)->next; /* delete it */ + *current = element->next; /* delete it */ break; } current = & ( (*current)->next); @@ -1107,10 +1138,6 @@ long cm_ChangeRankServer(cm_serverRef_t** list, cm_server_t* server) /* re-insert deleted element into the list with modified rank*/ cm_InsertServerList(list, element); - /* reduce refCount which was increased by cm_InsertServerList */ - lock_ObtainWrite(&cm_serverLock); - element->refCount--; - lock_ReleaseWrite(&cm_serverLock); return 0; } /* @@ -1255,6 +1282,7 @@ void cm_FreeServerList(cm_serverRef_t** list, afs_uint32 flags) cm_serverRef_t **current; cm_serverRef_t **nextp; cm_serverRef_t * next; + afs_int32 refCount; lock_ObtainWrite(&cm_serverLock); current = list; @@ -1267,7 +1295,8 @@ void cm_FreeServerList(cm_serverRef_t** list, afs_uint32 flags) while (*current) { nextp = &(*current)->next; - if (--((*current)->refCount) == 0) { + refCount = cm_PutServerRef(*current, TRUE); + if (refCount == 0) { next = *nextp; if ((*current)->volID) diff --git a/src/WINNT/afsd/cm_server.h b/src/WINNT/afsd/cm_server.h index 74cf91daaf..2aa03d80f0 100644 --- a/src/WINNT/afsd/cm_server.h +++ b/src/WINNT/afsd/cm_server.h @@ -85,6 +85,10 @@ extern cm_server_t *cm_NewServer(struct sockaddr_in *addrp, int type, extern cm_serverRef_t *cm_NewServerRef(struct cm_server *serverp, afs_uint32 volID); +extern void cm_GetServerRef(cm_serverRef_t *tsrp, int locked); + +extern afs_int32 cm_PutServerRef(cm_serverRef_t *tsrp, int locked); + extern LONG_PTR cm_ChecksumServerList(cm_serverRef_t *serversp); extern void cm_GetServer(cm_server_t *); diff --git a/src/WINNT/afsd/cm_volume.c b/src/WINNT/afsd/cm_volume.c index c34d5df7ec..1c1e32a370 100644 --- a/src/WINNT/afsd/cm_volume.c +++ b/src/WINNT/afsd/cm_volume.c @@ -524,6 +524,7 @@ long cm_UpdateVolumeLocation(struct cm_cell *cellp, cm_user_t *userp, cm_req_t * tempAddr = htonl(serverNumber[i]); tsockAddr.sin_addr.s_addr = tempAddr; tsp = cm_FindServer(&tsockAddr, CM_SERVER_FILE); +#ifdef MULTIHOMED if (tsp && (method == 2) && (tsp->flags & CM_SERVERFLAG_UUID)) { /* * Check to see if the uuid of the server we know at this address @@ -544,6 +545,7 @@ long cm_UpdateVolumeLocation(struct cm_cell *cellp, cm_user_t *userp, cm_req_t * osi_LogSaveString(afsd_logp, hoststr)); } } +#endif if (!tsp) { /* * cm_NewServer will probe the file server which in turn will @@ -580,20 +582,12 @@ long cm_UpdateVolumeLocation(struct cm_cell *cellp, cm_user_t *userp, cm_req_t * if ((tflags & VLSF_RWVOL) && (flags & VLF_RWEXISTS)) { tsrp = cm_NewServerRef(tsp, rwID); cm_InsertServerList(&volp->vol[RWVOL].serversp, tsrp); - - lock_ObtainWrite(&cm_serverLock); - tsrp->refCount--; /* drop allocation reference */ - lock_ReleaseWrite(&cm_serverLock); - if (!(tsp->flags & CM_SERVERFLAG_DOWN)) rwServers_alldown = 0; } if ((tflags & VLSF_ROVOL) && (flags & VLF_ROEXISTS)) { tsrp = cm_NewServerRef(tsp, roID); cm_InsertServerList(&volp->vol[ROVOL].serversp, tsrp); - lock_ObtainWrite(&cm_serverLock); - tsrp->refCount--; /* drop allocation reference */ - lock_ReleaseWrite(&cm_serverLock); ROcount++; if (!(tsp->flags & CM_SERVERFLAG_DOWN)) @@ -607,9 +601,6 @@ long cm_UpdateVolumeLocation(struct cm_cell *cellp, cm_user_t *userp, cm_req_t * if ((tflags & VLSF_RWVOL) && (flags & VLF_BACKEXISTS)) { tsrp = cm_NewServerRef(tsp, bkID); cm_InsertServerList(&volp->vol[BACKVOL].serversp, tsrp); - lock_ObtainWrite(&cm_serverLock); - tsrp->refCount--; /* drop allocation reference */ - lock_ReleaseWrite(&cm_serverLock); if (!(tsp->flags & CM_SERVERFLAG_DOWN)) bkServers_alldown = 0; @@ -1131,7 +1122,7 @@ cm_serverRef_t **cm_GetVolServers(cm_volume_t *volp, afs_uint32 volume, cm_user_ * They will be freed by cm_FreeServerList when they get to zero */ for (current = *serverspp; current; current = current->next) - current->refCount++; + cm_GetServerRef(current, TRUE); lock_ReleaseWrite(&cm_serverLock); @@ -1229,6 +1220,7 @@ cm_CheckOfflineVolumeState(cm_volume_t *volp, cm_vol_state_t *statep, afs_uint32 *volumeUpdatedp = 1; } + lock_ObtainRead(&cm_serverLock); if (statep->serversp) { alldown = 1; alldeleted = 1; @@ -1243,6 +1235,7 @@ cm_CheckOfflineVolumeState(cm_volume_t *volp, cm_vol_state_t *statep, afs_uint32 if (serversp->status == srv_busy || serversp->status == srv_offline) serversp->status = srv_not_busy; } + lock_ReleaseRead(&cm_serverLock); if (alldeleted && !(*volumeUpdatedp)) { cm_InitReq(&req); @@ -1283,6 +1276,7 @@ cm_CheckOfflineVolumeState(cm_volume_t *volp, cm_vol_state_t *statep, afs_uint32 statep->state = vl_alldown; } } else if (statep->state != vl_alldown) { + lock_ReleaseRead(&cm_serverLock); cm_VolumeStatusNotification(volp, statep->ID, statep->state, vl_alldown); statep->state = vl_alldown; }