From 707000c026c6355c9bb0aae69dba7fd656825e24 Mon Sep 17 00:00:00 2001 From: Albert Louis Rossi Date: Tue, 5 Sep 2023 12:20:34 -0500 Subject: [PATCH] dcache-webdav: improve efficiency of directory listing 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 --- .../webdav/DcacheDirectoryResource.java | 34 ++++++++++--- .../dcache/webdav/DcacheResourceFactory.java | 51 +++++++++++++++---- .../resources/org/dcache/webdav/webdav.xml | 1 + .../org/dcache/space/ReservationCaches.java | 10 ++-- skel/share/defaults/webdav.properties | 35 +++++++++++-- 5 files changed, 106 insertions(+), 25 deletions(-) diff --git a/modules/dcache-webdav/src/main/java/org/dcache/webdav/DcacheDirectoryResource.java b/modules/dcache-webdav/src/main/java/org/dcache/webdav/DcacheDirectoryResource.java index 8ff2c591e94..be9468e8f86 100644 --- a/modules/dcache-webdav/src/main/java/org/dcache/webdav/DcacheDirectoryResource.java +++ b/modules/dcache-webdav/src/main/java/org/dcache/webdav/DcacheDirectoryResource.java @@ -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; @@ -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; @@ -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 @@ -254,6 +260,12 @@ public LockToken createAndLock(String name, LockTimeout timeout, LockInfo lockIn return createNullLock(); } + private Optional getWriteToken() { + return _attributes.isDefined(STORAGEINFO) + ? ReservationCaches.writeToken(_attributes) + : _factory.lookupWriteToken(_path); + } + @Override public Object getProperty(QName name) { Object value = super.getProperty(name); @@ -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) { @@ -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; @@ -299,7 +316,12 @@ public PropertyMetaData getPropertyMetaData(QName name) { public List getAllPropertyNames() { List genericNames = super.getAllPropertyNames(); - if (!_factory.isSpaceManaged(_path)) { + if (!_allAttributes) { + return genericNames; + } + + var maybeToken = getWriteToken(); + if (!_factory.isSpaceManaged(maybeToken)) { return genericNames; } diff --git a/modules/dcache-webdav/src/main/java/org/dcache/webdav/DcacheResourceFactory.java b/modules/dcache-webdav/src/main/java/org/dcache/webdav/DcacheResourceFactory.java index 72488e32f27..59918d4d7a3 100644 --- a/modules/dcache-webdav/src/main/java/org/dcache/webdav/DcacheResourceFactory.java +++ b/modules/dcache-webdav/src/main/java/org/dcache/webdav/DcacheResourceFactory.java @@ -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; @@ -168,6 +169,10 @@ public class DcacheResourceFactory public static final String TRANSACTION_ATTRIBUTE = "org.dcache.transaction"; + private static final Set MINIMALLY_REQUIRED_ATTRIBUTES = + EnumSet.of(TYPE, PNFSID, CREATION_TIME, MODIFICATION_TIME, SIZE, + MODE, OWNER, OWNER_GROUP); + private static final Set REQUIRED_ATTRIBUTES = EnumSet.of(TYPE, PNFSID, CREATION_TIME, MODIFICATION_TIME, SIZE, MODE, OWNER, OWNER_GROUP, XATTR); @@ -178,7 +183,7 @@ public class DcacheResourceFactory // Additional attributes needed for PROPFIND requests; e.g., to supply // values for properties. private static final Set 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"; @@ -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. *

@@ -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; @@ -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) { @@ -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); } @@ -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; @@ -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) @@ -1413,13 +1431,16 @@ private void initializeTransfer(HttpTransfer transfer, Subject subject) } private Set buildRequestedAttributes() { - Set attributes = EnumSet.copyOf(REQUIRED_ATTRIBUTES); + boolean all = isFetchAllAttributes(); + + Set 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 @@ -1430,6 +1451,14 @@ private Set 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 @@ -1487,7 +1516,7 @@ private Optional lookupSpaceById(String id) { } } - private Optional lookupWriteToken(FsPath path) { + public Optional lookupWriteToken(FsPath path) { try { return _writeTokenCache.get(path); } catch (ExecutionException e) { @@ -1498,14 +1527,14 @@ private Optional lookupWriteToken(FsPath path) { } } - public Space spaceForPath(FsPath path) throws SpaceException { - return lookupWriteToken(path) + public Space spaceForToken(Optional 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 maybeToken) { + return maybeToken .flatMap(this::lookupSpaceById) .isPresent(); } diff --git a/modules/dcache-webdav/src/main/resources/org/dcache/webdav/webdav.xml b/modules/dcache-webdav/src/main/resources/org/dcache/webdav/webdav.xml index aeb5eb18117..7b7e8efab74 100644 --- a/modules/dcache-webdav/src/main/resources/org/dcache/webdav/webdav.xml +++ b/modules/dcache-webdav/src/main/resources/org/dcache/webdav/webdav.xml @@ -230,6 +230,7 @@ + diff --git a/modules/dcache/src/main/java/org/dcache/space/ReservationCaches.java b/modules/dcache/src/main/java/org/dcache/space/ReservationCaches.java index cfbba718566..01f5debdf6b 100644 --- a/modules/dcache/src/main/java/org/dcache/space/ReservationCaches.java +++ b/modules/dcache/src/main/java/org/dcache/space/ReservationCaches.java @@ -251,6 +251,11 @@ public void failure(int rc, Object error) { }); } + public static java.util.Optional 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. */ @@ -262,11 +267,6 @@ public static LoadingCache> buildWriteTokenLo .refreshAfterWrite(5, MINUTES) .recordStats() .build(new CacheLoader>() { - private java.util.Optional writeToken(FileAttributes attr) { - StorageInfo info = attr.getStorageInfo(); - return java.util.Optional.ofNullable(info.getMap().get("writeToken")); - } - @Override public java.util.Optional load(FsPath path) throws CacheException, NoRouteToCellException, InterruptedException { diff --git a/skel/share/defaults/webdav.properties b/skel/share/defaults/webdav.properties index d775d040983..4dfc5d6f35d 100644 --- a/skel/share/defaults/webdav.properties +++ b/skel/share/defaults/webdav.properties @@ -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} @@ -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 \ No newline at end of file +(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 \ No newline at end of file