Skip to content

Commit

Permalink
srm: Use shared read locks for read-only property access
Browse files Browse the repository at this point in the history 
I verified all places in which we obtain a read-lock. As far as
I could see, we now never upgrade a read lock to a write lock.
Thus it is safe to use a shared read lock for read access.

I had to fix a number of getters, though. In particular turl
computation was done as a side-effect in a getter and would
thus cause a read-lock to write-lock upgrade attempt during
save. I now moved the turl computation into the main processing
of put and get. Note that this causes the turl to be computed
as the request moved to RQUEUD rather than READY. If this is
unacceptable, we could move the computation into the
stateChanged notification, but I find that slightly more
messy.

Target: trunk
Require-notes: no
Require-book: no
Acked-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>
Acked-by: Dmitry Litvintsev <litvinse@fnal.gov>
Patch: http://rb.dcache.org/r/6157/
  • Loading branch information
@gbehrmann
gbehrmann committed Oct 31, 2013
1 parent 54df75e commit 895efe4
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 134 deletions.
109 changes: 48 additions & 61 deletions modules/srm-server/src/main/java/org/dcache/srm/request/GetFileRequest.java
View file
Expand Up @@ -234,45 +234,13 @@ public final String getSurlString() {
}

public String getTurlString() {
wlock();
rlock();
try {
State state = getState();
if(getTurl() == null && (state == State.READY ||
state == State.TRANSFERRING)) {
try {
setTurl(getTURL());
}
catch(SRMAuthorizationException srmae) {
String error =srmae.getMessage();
logger.error(error);
try {
setStateAndStatusCode(
State.FAILED,
error,
TStatusCode.SRM_AUTHORIZATION_FAILURE);
}
catch(IllegalStateTransition ist) {
logger.warn("Illegal State Transition : " +ist.getMessage());
}

}
catch(SRMException e) {
String error = "cannot obtain turl for file:" + e.getMessage();
logger.error(error);
try {
setState(State.FAILED,error);
}
catch(IllegalStateTransition ist) {
logger.warn("Illegal State Transition : " +ist.getMessage());
}
}
}

if(getTurl()!= null) {
return getTurl().toASCIIString();
if (turl != null) {
return turl.toASCIIString();
}
} finally {
wunlock();
runlock();
}
return null;
}
Expand Down Expand Up @@ -375,31 +343,6 @@ public TSURLReturnStatus getTSURLReturnStatus() throws SRMInvalidRequestExcepti
}
}

private URI getTURL() throws SRMException {
String firstDcapTurl = null;
GetRequest request = getContainerRequest();
if (request != null) {
firstDcapTurl = request.getFirstDcapTurl();
if (firstDcapTurl == null) {
URI turl =
getStorage().getGetTurl(getUser(),
getSurl(),
request.protocols);
if (turl == null) {
throw new SRMException("turl is null");
}
if (turl.getScheme().equals("dcap")) {
request.setFirstDcapTurl(turl.toString());
}
return turl;
}
}

return getStorage().getGetTurl(getUser(),
getSurl(),
URI.create(firstDcapTurl));
}

@Override
public void toString(StringBuilder sb, boolean longformat) {
sb.append(" GetFileRequest ");
Expand Down Expand Up @@ -451,6 +394,29 @@ public synchronized void run() throws NonFatalJobFailure, FatalJobFailure {
return;
}
}

try {
computeTurl();
} catch (SRMAuthorizationException e) {
String error =e.getMessage();
logger.error(error);
try {
setStateAndStatusCode(
State.FAILED,
error,
TStatusCode.SRM_AUTHORIZATION_FAILURE);
} catch(IllegalStateTransition ist) {
logger.error("Illegal State Transition : " + ist.getMessage());
}
} catch (SRMException e) {
String error = "cannot obtain turl for file:" + e.getMessage();
logger.error(error);
try {
setState(State.FAILED,error);
} catch(IllegalStateTransition ist) {
logger.error("Illegal State Transition : " + ist.getMessage());
}
}
} catch (IllegalStateTransition | DataAccessException | SRMException e) {
// FIXME some SRMException failures are temporary and others are
// permanent. Code currently doesn't distinguish between them and
Expand Down Expand Up @@ -509,6 +475,27 @@ protected void stateChanged(State oldState) {
super.stateChanged(oldState);
}

private void computeTurl() throws SRMException
{
URI turl;
GetRequest request = getContainerRequest();
String firstDcapTurl = request.getFirstDcapTurl();
if (firstDcapTurl != null) {
turl = getStorage().getGetTurl(getUser(),
getSurl(),
URI.create(firstDcapTurl));
} else {
turl = getStorage().getGetTurl(getUser(),
getSurl(),
request.protocols);
if (turl.getScheme().equals("dcap")) {
request.setFirstDcapTurl(turl.toString());
}
}

setTurl(turl);
}

@Override
public boolean isTouchingSurl(URI surl)
{
Expand Down
25 changes: 8 additions & 17 deletions modules/srm-server/src/main/java/org/dcache/srm/request/Job.java
View file
Expand Up @@ -92,7 +92,6 @@ LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
import java.util.Set;
import java.util.TimerTask;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock.WriteLock;

import org.dcache.srm.SRMAbortedException;
import org.dcache.srm.SRMException;
Expand Down Expand Up @@ -151,9 +150,8 @@ public abstract class Job {

protected transient JDC jdc;

private final ReentrantReadWriteLock reentrantReadWriteLock =
private final ReentrantReadWriteLock lock =
new ReentrantReadWriteLock();
private final WriteLock writeLock = reentrantReadWriteLock.writeLock();

// this constructor is used for restoring the job from permanent storage
// should be called through the Job.getJob only, otherwise the expireRestoredJobOrCreateExperationTimer
Expand Down Expand Up @@ -567,11 +565,11 @@ public JobHistory getLastJobChange()
*
*/
public final int getNumberOfRetries() {
wlock();
rlock();
try {
return numberOfRetries;
} finally {
wunlock();
runlock();
}
}

Expand Down Expand Up @@ -1112,27 +1110,20 @@ private void notifySchedulerOfStateChange(State oldState, State newState) {
}

public final void wlock() {
writeLock.lock();
lock.writeLock().lock();
}

public final void wunlock() {
writeLock.unlock();
lock.writeLock().unlock();
}

/* Note that a read lock cannot be upgraded to a write lock. */
public final void rlock() {
// Terracotta currently does not support upgrading read lock to
// write lock. So we use write logs everywhere.
// See bug at
// https://jira.terracotta.org/jira/browse/CDV-787
writeLock.lock();
lock.readLock().lock();
}

public final void runlock() {
// Terracotta currently does not support upgrading read lock to
// write lock. So we use write logs everywhere.
// See bug at
// https://jira.terracotta.org/jira/browse/CDV-787
writeLock.unlock();
lock.readLock().unlock();
}

@Override
Expand Down
106 changes: 50 additions & 56 deletions modules/srm-server/src/main/java/org/dcache/srm/request/PutFileRequest.java
View file
Expand Up @@ -261,46 +261,15 @@ public final String getSurlString() {
return getSurl().toASCIIString();
}

public String getTurlString() {
wlock();
public String getTurlString()
{
rlock();
try {
State state = getState();
if(getTurl() == null && (state == State.READY ||
state == State.TRANSFERRING)) {
try {
setTurl(getTURL());

}
catch(SRMAuthorizationException srme) {
String error =srme.getMessage();
logger.error(error);
try {
setStateAndStatusCode(State.FAILED,
error,
TStatusCode.SRM_AUTHORIZATION_FAILURE);
}
catch(IllegalStateTransition ist) {
logger.warn("Illegal State Transition : " +ist.getMessage());
}
}
catch(SRMException e) {
String error = "cannot obtain turl for file:" + e.getMessage();
logger.error(error);
try {
setState(State.FAILED,error);
}
catch(IllegalStateTransition ist)
{
logger.warn("Illegal State Transition : " +ist.getMessage());
}
}
}

if(getTurl()!= null) {
return getTurl().toASCIIString();
if (turl != null) {
return turl.toASCIIString();
}
} finally {
wunlock();
runlock();
}
return null;
}
Expand Down Expand Up @@ -374,25 +343,6 @@ public TPutRequestFileStatus getTPutRequestFileStatus()
return fileStatus;
}

private URI getTURL() throws SRMException {
PutRequest request = getContainerRequest();
// do not synchronize on request, since it might cause deadlock
String firstDcapTurl = request.getFirstDcapTurl();
if(firstDcapTurl == null) {
URI turl =
getStorage().getPutTurl(getUser(), getSurl(),
request.getProtocols());
if(turl.getScheme().equals("dcap")) {
request.setFirstDcapTurl(turl.toString());
}
return turl;
}

return getStorage().getPutTurl(request.getUser(),
getSurl(),
URI.create(firstDcapTurl));
}

@Override
public void toString(StringBuilder sb, boolean longformat) {
sb.append(" PutFileRequest ");
Expand Down Expand Up @@ -540,6 +490,29 @@ public void run() throws NonFatalJobFailure, FatalJobFailure
callbacks );
return;
}

try {
computeTurl();
} catch (SRMAuthorizationException e) {
String error = e.getMessage();
logger.error(error);
try {
setStateAndStatusCode(State.FAILED,
error,
TStatusCode.SRM_AUTHORIZATION_FAILURE);
} catch (IllegalStateTransition ist) {
logger.error("Illegal State Transition : " + ist.getMessage());
}
} catch (SRMException e) {
String error = "cannot obtain turl for file:" + e.getMessage();
logger.error(error);
try {
setState(State.FAILED, error);
} catch (IllegalStateTransition ist) {
logger.error("Illegal State Transition : " + ist.getMessage());
}
}

logger.debug("run() returns, scheduler should bring file request into the ready state eventually");
}
catch(SRMException | DataAccessException | IllegalStateTransition e) {
Expand Down Expand Up @@ -591,6 +564,27 @@ protected void stateChanged(State oldState) {
super.stateChanged(oldState);
}

private void computeTurl() throws SRMException
{
PutRequest request = getContainerRequest();
// do not synchronize on request, since it might cause deadlock
String firstDcapTurl = request.getFirstDcapTurl();
URI turl;
if (firstDcapTurl != null) {
turl = getStorage().getPutTurl(request.getUser(),
getSurl(),
URI.create(firstDcapTurl));
} else {
turl = getStorage().getPutTurl(getUser(), getSurl(),
request.getProtocols());
if(turl.getScheme().equals("dcap")) {
request.setFirstDcapTurl(turl.toString());
}
}

setTurl(turl);
}

@Override
public void abort() throws IllegalStateTransition
{
Expand Down

0 comments on commit 895efe4

Please sign in to comment.