Skip to content

Commit

Permalink
dcache-webdav: improve efficiency of directory listing
Browse files Browse the repository at this point in the history 
Motivation:

Listing through webdav is very slow on largish
directories (see under testing).  This creates
backlog and DOS-like conditions on FNAL
production.

There were two issues:

(a) for each directory, the write-token-cache
that is maintained in webdav was being checked;
the cache loader uses the PnfsHandler to supply
that token by requesting the STORAGEINFO attribute.
Hence, an extra attribute fetch for each
directory.

This is rectified by accessing an optional
write token from the storage info if it
has already been fetched.   Merged from
patch https://rb.dcache.org/r/14082/.

(b) even without fetching STORAGEINFO again
for the directories, the original fetch is
slow because it includes things like locality
and joins for non-POSIX attributes.

This is fixed by checking to see if this
is a propfind request.  If it is, only
the minimal set of (POSIX-) attributes
are requested, unless the
dCache property for propfind default
is set to CLIENT_COMPATIBLE.

Result:

Considerable speed-up (see testing).

Target:  master
Request: 9.1
Request: 9.0
Request: 8.2
Patch: https://rb.dcache.org/r/14085/
Requires-notes: yes
Acked-by: Tigran
  • Loading branch information
@alrossi
alrossi committed Sep 5, 2023
1 parent 7e691a4 commit 707000c
Show file tree
Hide file tree
Showing 5 changed files with 106 additions and 25 deletions.
34 changes: 28 additions & 6 deletions modules/dcache-webdav/src/main/java/org/dcache/webdav/DcacheDirectoryResource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static io.milton.property.PropertySource.PropertyAccessibility.READ_ONLY;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.dcache.namespace.FileAttribute.STORAGEINFO;

import com.google.common.collect.ImmutableSet;
import diskCacheV111.services.space.Space;
Expand Down Expand Up @@ -43,7 +44,9 @@
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import javax.xml.namespace.QName;
import org.dcache.space.ReservationCaches;
import org.dcache.vehicles.FileAttributes;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -70,9 +73,12 @@ public class DcacheDirectoryResource
private static final PropertyMetaData READONLY_LONG = new PropertyMetaData(READ_ONLY,
Long.class);

private final boolean _allAttributes;

public DcacheDirectoryResource(DcacheResourceFactory factory,
FsPath path, FileAttributes attributes) {
FsPath path, FileAttributes attributes, boolean allAttributes) {
super(factory, path, attributes);
_allAttributes = allAttributes;
}

@Override
Expand Down Expand Up @@ -254,6 +260,12 @@ public LockToken createAndLock(String name, LockTimeout timeout, LockInfo lockIn
return createNullLock();
}

private Optional<String> getWriteToken() {
return _attributes.isDefined(STORAGEINFO)
? ReservationCaches.writeToken(_attributes)
: _factory.lookupWriteToken(_path);
}

@Override
public Object getProperty(QName name) {
Object value = super.getProperty(name);
Expand All @@ -264,11 +276,13 @@ public Object getProperty(QName name) {

try {
if (name.equals(QUOTA_AVAILABLE)) {
return _factory.spaceForPath(_path).getAvailableSpaceInBytes();
var maybeToken = getWriteToken();
return _factory.spaceForToken(maybeToken).getAvailableSpaceInBytes();
}

if (name.equals(QUOTA_USED)) {
Space space = _factory.spaceForPath(_path);
var maybeToken = getWriteToken();
Space space = _factory.spaceForToken(maybeToken);
return space.getUsedSizeInBytes() + space.getAllocatedSpaceInBytes();
}
} catch (SpaceException e) {
Expand All @@ -282,14 +296,17 @@ public Object getProperty(QName name) {
public PropertyMetaData getPropertyMetaData(QName name) {
PropertyMetaData metadata = super.getPropertyMetaData(name);

if (!_factory.isSpaceManaged(_path)) {
if (!_allAttributes) {
return metadata;
}

// Milton accepts null and PropertyMetaData.UNKNOWN to mean the
// property is unknown.
if ((metadata == null || metadata.isUnknown()) && QUOTA_PROPERTIES.contains(name)) {
metadata = READONLY_LONG;
var maybeToken = getWriteToken();
if (_factory.isSpaceManaged(maybeToken)) {
return READONLY_LONG;
}
}

return metadata;
Expand All @@ -299,7 +316,12 @@ public PropertyMetaData getPropertyMetaData(QName name) {
public List<QName> getAllPropertyNames() {
List<QName> genericNames = super.getAllPropertyNames();

if (!_factory.isSpaceManaged(_path)) {
if (!_allAttributes) {
return genericNames;
}

var maybeToken = getWriteToken();
if (!_factory.isSpaceManaged(maybeToken)) {
return genericNames;
}

Expand Down
51 changes: 40 additions & 11 deletions modules/dcache-webdav/src/main/java/org/dcache/webdav/DcacheResourceFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import static org.dcache.namespace.FileAttribute.PNFSID;
import static org.dcache.namespace.FileAttribute.RETENTION_POLICY;
import static org.dcache.namespace.FileAttribute.SIZE;
import static org.dcache.namespace.FileAttribute.STORAGEINFO;
import static org.dcache.namespace.FileAttribute.TYPE;
import static org.dcache.namespace.FileAttribute.XATTR;
import static org.dcache.namespace.FileType.DIR;
Expand Down Expand Up @@ -168,6 +169,10 @@ public class DcacheResourceFactory

public static final String TRANSACTION_ATTRIBUTE = "org.dcache.transaction";

private static final Set<FileAttribute> MINIMALLY_REQUIRED_ATTRIBUTES =
EnumSet.of(TYPE, PNFSID, CREATION_TIME, MODIFICATION_TIME, SIZE,
MODE, OWNER, OWNER_GROUP);

private static final Set<FileAttribute> REQUIRED_ATTRIBUTES =
EnumSet.of(TYPE, PNFSID, CREATION_TIME, MODIFICATION_TIME, SIZE,
MODE, OWNER, OWNER_GROUP, XATTR);
Expand All @@ -178,7 +183,7 @@ public class DcacheResourceFactory
// Additional attributes needed for PROPFIND requests; e.g., to supply
// values for properties.
private static final Set<FileAttribute> PROPFIND_ATTRIBUTES = Sets.union(
EnumSet.of(CHECKSUM, ACCESS_LATENCY, RETENTION_POLICY),
EnumSet.of(CHECKSUM, ACCESS_LATENCY, RETENTION_POLICY, STORAGEINFO),
PoolMonitorV5.getRequiredAttributesForFileLocality());

private static final String PROTOCOL_INFO_NAME = "Http";
Expand All @@ -193,6 +198,11 @@ public class DcacheResourceFactory
private static final Splitter PATH_SPLITTER =
Splitter.on('/').omitEmptyStrings();

enum PropfindProperties {
PERFORMANCE,
CLIENT_COMPATIBLE
};

/**
* In progress transfers. The key of the map is the session id of the transfer.
* <p>
Expand Down Expand Up @@ -235,6 +245,7 @@ public class DcacheResourceFactory
private boolean _impatientClientProxied = true;
private boolean _isOverwriteAllowed;
private boolean _isAnonymousListingAllowed;
private boolean _includeAllAttributesForPropfind;

private String _staticContentPath;
private ReloadableTemplate _template;
Expand Down Expand Up @@ -606,9 +617,9 @@ public Resource getResource(String host, String requestPath) {
public DcacheResource getResource(FsPath path) {
checkPathAllowed(path);

String requestPath = getRequestPath();
boolean haveRetried = false;
Subject subject = getSubject();
String requestPath = getRequestPath();

try {
while (true) {
Expand Down Expand Up @@ -650,7 +661,8 @@ public DcacheResource getResource(FsPath path) {
*/
private DcacheResource getResource(FsPath path, FileAttributes attributes) {
if (attributes.getFileType() == DIR) {
return new DcacheDirectoryResource(this, path, attributes);
return new DcacheDirectoryResource(this, path, attributes,
_includeAllAttributesForPropfind);
} else {
return new DcacheFileResource(this, path, attributes);
}
Expand Down Expand Up @@ -919,6 +931,11 @@ public void print(FsPath dir, FileAttributes dirAttr, DirectoryEntry entry) {
return result;
}

public void setDefaultPropfindProperties(PropfindProperties defaultPropfindProperties) {
_includeAllAttributesForPropfind =
defaultPropfindProperties == PropfindProperties.CLIENT_COMPATIBLE;
}

private class FileLocalityWrapper {

private final FileLocality _inner;
Expand Down Expand Up @@ -1122,7 +1139,8 @@ public void deleteDirectory(PnfsId pnfsid, FsPath path)
PnfsCreateEntryMessage reply =
pnfs.createPnfsDirectory(path.toString(), REQUIRED_ATTRIBUTES);

return new DcacheDirectoryResource(this, path, reply.getFileAttributes());
return new DcacheDirectoryResource(this, path, reply.getFileAttributes(),
_includeAllAttributesForPropfind);
}

public void move(FsPath sourcePath, PnfsId pnfsId, FsPath newPath)
Expand Down Expand Up @@ -1413,13 +1431,16 @@ private void initializeTransfer(HttpTransfer transfer, Subject subject)
}

private Set<FileAttribute> buildRequestedAttributes() {
Set<FileAttribute> attributes = EnumSet.copyOf(REQUIRED_ATTRIBUTES);
boolean all = isFetchAllAttributes();

Set<FileAttribute> attributes = all ? EnumSet.copyOf(REQUIRED_ATTRIBUTES) :
EnumSet.copyOf(MINIMALLY_REQUIRED_ATTRIBUTES);

if (isDigestRequested()) {
attributes.add(CHECKSUM);
}

if (isPropfindRequest()) {
if (isPropfindRequest() && all) {
// FIXME: Unfortunately, Milton parses the request body after
// requesting the Resource, so we cannot know which additional
// attributes are being requested; therefore, we must request all
Expand All @@ -1430,6 +1451,14 @@ private Set<FileAttribute> buildRequestedAttributes() {
return attributes;
}

private boolean isFetchAllAttributes() {
if (!isPropfindRequest()) {
return true;
}

return _includeAllAttributesForPropfind;
}

/**
* Return the RFC 3230 Want-Digest header value, if present. If multiple headers are present
* then return a single value obtained by taking the values and creating the equivalent
Expand Down Expand Up @@ -1487,7 +1516,7 @@ private Optional<Space> lookupSpaceById(String id) {
}
}

private Optional<String> lookupWriteToken(FsPath path) {
public Optional<String> lookupWriteToken(FsPath path) {
try {
return _writeTokenCache.get(path);
} catch (ExecutionException e) {
Expand All @@ -1498,14 +1527,14 @@ private Optional<String> lookupWriteToken(FsPath path) {
}
}

public Space spaceForPath(FsPath path) throws SpaceException {
return lookupWriteToken(path)
public Space spaceForToken(Optional<String> maybeToken) throws SpaceException {
return maybeToken
.flatMap(this::lookupSpaceById)
.orElseThrow(() -> new SpaceException("Path not under space management"));
}

public boolean isSpaceManaged(FsPath path) {
return lookupWriteToken(path)
public boolean isSpaceManaged(Optional<String> maybeToken) {
return maybeToken
.flatMap(this::lookupSpaceById)
.isPresent();
}
Expand Down
1 change: 1 addition & 0 deletions modules/dcache-webdav/src/main/resources/org/dcache/webdav/webdav.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@
<property name="overwriteAllowed" value="${webdav.enable.overwrite}"/>
<property name="redirectToHttps" value="${webdav.redirect.allow-https}"/>
<property name="poolMonitor" ref="pool-monitor"/>
<property name="defaultPropfindProperties" value="${webdav.default-propfind-properties}"/>
</bean>

<bean id="pool-manager-handler" class="org.dcache.poolmanager.PoolManagerHandlerSubscriber">
Expand Down
10 changes: 5 additions & 5 deletions modules/dcache/src/main/java/org/dcache/space/ReservationCaches.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,11 @@ public void failure(int rc, Object error) {
});
}

public static java.util.Optional<String> writeToken(FileAttributes attr) {
StorageInfo info = attr.getStorageInfo();
return java.util.Optional.ofNullable(info.getMap().get("writeToken"));
}

/**
* Cache queries to discover if a directory has the "WriteToken" tag set.
*/
Expand All @@ -262,11 +267,6 @@ public static LoadingCache<FsPath, java.util.Optional<String>> buildWriteTokenLo
.refreshAfterWrite(5, MINUTES)
.recordStats()
.build(new CacheLoader<FsPath, java.util.Optional<String>>() {
private java.util.Optional<String> writeToken(FileAttributes attr) {
StorageInfo info = attr.getStorageInfo();
return java.util.Optional.ofNullable(info.getMap().get("writeToken"));
}

@Override
public java.util.Optional<String> load(FsPath path)
throws CacheException, NoRouteToCellException, InterruptedException {
Expand Down
35 changes: 32 additions & 3 deletions skel/share/defaults/webdav.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -763,8 +763,6 @@ webdav.allowed.client.origins =
#
(one-of?true|false)webdav.redirect.allow-https=true



# ---- Kafka service enabled
#
(one-of?true|false|${dcache.enable.kafka})webdav.enable.kafka = ${dcache.enable.kafka}
Expand All @@ -777,4 +775,35 @@ webdav.kafka.topic = ${dcache.kafka.topic}
webdav.kafka.producer.bootstrap.servers = ${dcache.kafka.bootstrap-servers}


(prefix)webdav.kafka.producer.configs = Configuration for Kafka Producer
(prefix)webdav.kafka.producer.configs = Configuration for Kafka Producer

# --- Default PROPFIND properties
#
# The PROPFIND request allows a client to discover information
# (properties) of dCache content. When making a PROPFIND request,
# the client normally indicates which properties are of interest. If
# not specified then the WebDAV server (dCache) is free to return a
# default set of information.
#
# Certain clients make PROPFIND requests without specifying
# the desired set of properties, triggering a default set of
# properties. There are two problems with this: first, the
# clients may react badly if information is missing from this default;
# second, some of the required properties may have an adverse
# performance impact for dCache.
#
# This property controls whether to support client-side requests for
# properties beyond a minimal set or not.
#
# PERFORMANCE -- always return only a minimal set of information that does
# not incur any additional overhead. (These are basically
# the same as what a POSIX list request would proviode).
#
# CLIENT_COMPATIBLE -- return the set of information required by
# the clients. If the client does not specify the properties,
# it will get a maximal default set.
#
# Because the performance impact of the latter under the most common usage scenarios
# could be considerable, the default is set to PERFORMANCE.
#
(one-of?PERFORMANCE|CLIENT_COMPATIBLE)webdav.default-propfind-properties = PERFORMANCE

0 comments on commit 707000c

Please sign in to comment.