Skip to content

Commit

Permalink
dcache-webdav: revert improve efficiency of directory listing (14085/…
Browse files Browse the repository at this point in the history 
…14088)

Motivation:

See `RT 10505: Problem with webdav PROPFIND request for dirs with symlinks`
https://rt.dcache.org/Ticket/Display.html?id=10505

The problem may be more generalized than the discussion
suggests.   Since we cut off user requests when at the
`PERFORMANCE` setting, if the user wants locality, for
instance, it fails.

Modification:

We propose to commit #14082, #14086, and #14087 on top
of the reverted base.  We will also add a modification
to ignore quota requests when the default setting is
`PERFORMANCE`.

Result:

reverts commit 3567dde
https://rb.dcache.org/r/14088/
`dcache-webdav:  fix incorrect parameter value given to DcacheDirectoryResource constructor`

reverts commit 707000c.
https://rb.dcache.org/r/14085/
`dcache-webdav: improve efficiency of directory listing – revised`
(metalink addition conflicts resolved)

Target: master
Request: 9.1
Request: 9.0
Request: 8.2
Requires-notes:  absolutely
Patch:  https://rb.dcache.org/r/14101/
Acked-by: Dmitry
  • Loading branch information
@alrossi
alrossi committed Sep 15, 2023
1 parent 4f45bcf commit c2c509a
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 106 deletions.
34 changes: 6 additions & 28 deletions modules/dcache-webdav/src/main/java/org/dcache/webdav/DcacheDirectoryResource.java
View file
Expand Up @@ -2,7 +2,6 @@

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 @@ -44,9 +43,7 @@
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 @@ -73,12 +70,9 @@ 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, boolean allAttributes) {
FsPath path, FileAttributes attributes) {
super(factory, path, attributes);
_allAttributes = allAttributes;
}

@Override
Expand Down Expand Up @@ -260,12 +254,6 @@ 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 @@ -276,13 +264,11 @@ public Object getProperty(QName name) {

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

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

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

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

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

if (!_allAttributes) {
return genericNames;
}

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

Expand Down
51 changes: 11 additions & 40 deletions modules/dcache-webdav/src/main/java/org/dcache/webdav/DcacheResourceFactory.java
View file
Expand Up @@ -18,7 +18,6 @@
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 @@ -167,10 +166,6 @@ 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 @@ -181,7 +176,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, STORAGEINFO),
EnumSet.of(CHECKSUM, ACCESS_LATENCY, RETENTION_POLICY),
PoolMonitorV5.getRequiredAttributesForFileLocality());

private static final String PROTOCOL_INFO_NAME = "Http";
Expand All @@ -196,11 +191,6 @@ 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 @@ -243,7 +233,6 @@ enum PropfindProperties {
private boolean _impatientClientProxied = true;
private boolean _isOverwriteAllowed;
private boolean _isAnonymousListingAllowed;
private boolean _includeAllAttributesForPropfind;

private String _staticContentPath;
private ReloadableTemplate _template;
Expand Down Expand Up @@ -615,9 +604,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 @@ -659,8 +648,7 @@ public DcacheResource getResource(FsPath path) {
*/
private DcacheResource getResource(FsPath path, FileAttributes attributes) {
if (attributes.getFileType() == DIR) {
return new DcacheDirectoryResource(this, path, attributes,
isFetchAllAttributes());
return new DcacheDirectoryResource(this, path, attributes);
} else {
return new DcacheFileResource(this, path, attributes);
}
Expand Down Expand Up @@ -929,11 +917,6 @@ 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 @@ -1137,8 +1120,7 @@ public void deleteDirectory(PnfsId pnfsid, FsPath path)
PnfsCreateEntryMessage reply =
pnfs.createPnfsDirectory(path.toString(), REQUIRED_ATTRIBUTES);

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

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

private Set<FileAttribute> buildRequestedAttributes() {
boolean all = isFetchAllAttributes();

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

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

if (isPropfindRequest() && all) {
if (isPropfindRequest()) {
// 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 @@ -1449,14 +1428,6 @@ 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 @@ -1514,7 +1485,7 @@ private Optional<Space> lookupSpaceById(String id) {
}
}

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

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

public boolean isSpaceManaged(Optional<String> maybeToken) {
return maybeToken
public boolean isSpaceManaged(FsPath path) {
return lookupWriteToken(path)
.flatMap(this::lookupSpaceById)
.isPresent();
}
Expand Down
1 change: 0 additions & 1 deletion modules/dcache-webdav/src/main/resources/org/dcache/webdav/webdav.xml
View file
Expand Up @@ -230,7 +230,6 @@
<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
Expand Up @@ -251,11 +251,6 @@ 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 @@ -267,6 +262,11 @@ 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: 3 additions & 32 deletions skel/share/defaults/webdav.properties
View file
Expand Up @@ -763,6 +763,8 @@ 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 @@ -775,35 +777,4 @@ webdav.kafka.topic = ${dcache.kafka.topic}
webdav.kafka.producer.bootstrap.servers = ${dcache.kafka.bootstrap-servers}


(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
(prefix)webdav.kafka.producer.configs = Configuration for Kafka Producer

0 comments on commit c2c509a

Please sign in to comment.