Open butonic opened 1 year ago
First attempt: delegating the path lookup to the os (instead of walking child by child) does improve the listing of a folder with a relative path of depth 10 by ~40-50ms::
Under the hood the linux kernel still has to make multiple directory lookups to walk the path internally, just as the decomposedfs did.
diff --git a/pkg/storage/utils/decomposedfs/lookup/lookup.go b/pkg/storage/utils/decomposedfs/lookup/lookup.go
index 395559eb3..42e9d4438 100644
--- a/pkg/storage/utils/decomposedfs/lookup/lookup.go
+++ b/pkg/storage/utils/decomposedfs/lookup/lookup.go
@@ -119,10 +119,14 @@ func (lu *Lookup) NodeFromResource(ctx context.Context, ref *provider.Reference)
if ref.Path != "" {
p := filepath.Clean(ref.Path)
if p != "." && p != "/" {
- // walk the relative path
- n, err = lu.WalkPath(ctx, n, p, false, func(ctx context.Context, n *node.Node) error { return nil })
+ // try to read the target directly
+ n, err = n.Child(ctx, p)
if err != nil {
- return nil, err
+ // the shortcut didn't work, potentially due to references. walk the path step by step instead
+ n, err = lu.WalkPath(ctx, n, p, false, func(ctx context.Context, n *node.Node) error { return nil })
+ if err != nil {
+ return nil, err
+ }
}
n.SpaceID = ref.ResourceId.SpaceId
}
diff --git a/pkg/storage/utils/decomposedfs/upload/processing.go b/pkg/storage/utils/decomposedfs/upload/processing.go
index 3ba1a0445..b0714645d 100644
--- a/pkg/storage/utils/decomposedfs/upload/processing.go
+++ b/pkg/storage/utils/decomposedfs/upload/processing.go
@@ -453,9 +453,14 @@ func lookupNode(ctx context.Context, spaceRoot *node.Node, path string, lu *look
p = chunkInfo.Path
}
- n, err := lu.WalkPath(ctx, spaceRoot, p, true, func(ctx context.Context, n *node.Node) error { return nil })
+ // try to read the target directly
+ n, err := spaceRoot.Child(ctx, p)
if err != nil {
- return nil, errors.Wrap(err, "Decomposedfs: error walking path")
+ // the shortcut didn't work, potentially due to references. walk the path step by step instead
+ n, err = lu.WalkPath(ctx, spaceRoot, p, true, func(ctx context.Context, n *node.Node) error { return nil })
+ if err != nil {
+ return nil, errors.Wrap(err, "Decomposedfs: error walking path")
+ }
}
if isChunked {
The patch is a quickshot and seems to break when renaming folders, I don't think the result justifies looking further into this approach as it also kills persisted cs3 references...
@kulmann @michaelstingl Regardless of any server side changes we might come up with, all clients should try to use the fileid with the /dav/space/{fileid}
endpoint when making requests. It roughly reduces the execution time by 17ms per path segment that can be omitted.
So instead of making a PROPFIND to /dav/spaces/storage$spaceid/relative/path/to/resource
they can directly PROPFIND /dav/spaces/{fileid}
where fileid = {storageid}${spaceid}!{nodeid}
which should have been returned in a PROPFIND to the parent dir:
<oc:fileid>
1284d238-aa92-42ce-bdc4-0b0000009157$ddc2004c-0977-11eb-9d3f-a793888cd0f8!643d2ecf-f64b-40e6-8053-2f175c26baed
</oc:fileid>
This obviously also affects DELETE, GET and PUT requests. For MOVE and COPY the source can be a node based reference, the target can reference the parent node and the name. Same for MKCOL which can use a node based reference of the directory in which a new child should be created and a single path segment for the new dir.
@butonic I'm currently implementing these id-based requests in the web client. An id-based DELETE
(e.g. https://localhost:9200/remote.php/dav/spaces/e42289c1-0d29-4651-b505-d827d17df7c7$8dcb291f-96c3-4e0b-999a-22544fd21727!faa52162-7210-44b6-abe9-b16bd59532f0
) however doesn't work, it gives me 405 Method Not Allowed
. Am I missing something or is this a bug?
As a legacy from oc10 all clients currently make path based requests, relative to a space root. This forces the decomposedfs to walk the path, which takes time. For a dir https://cloud.ocis.test/files/spaces/personal/admin/f1/f2/f3/f4/f5/f6/f7/f8/f9/f10 on an NFS it might look like this:![image](https://github.com/owncloud/ocis/assets/956847/8e42fb0f-de3f-47a5-bd06-97e3aed13097)
This is negligible on local filesystems, but on NFS the Child lookup becomes painfully visible.
For now we only implemented a stat cache in decomposedfs. A direntry cache is certainly possible, but requires invalidation and coordination effort when running multiple storage providers.
As a client, you typically already have the file id when navigating the tree as every PROPFIND response for a directory listing also returns the file id of every child. The easiest way to take load off the server is to not generate it in the first place.
Making a PROPFIND with an id only reference can immediately look up the correct node:![image](https://github.com/owncloud/ocis/assets/956847/5289f599-2900-4538-b6d7-b9c226629277)
hm, retrying the propfind at lvl 10 sometimes gives really bad performance:![image](https://github.com/owncloud/ocis/assets/956847/bd7b139c-362a-4a33-9b8c-1d94b94cc043)
hmmmm, seems to resolve itself after a while ... ![Uploading image.png…]()