keep a read lock while scanning a file or folder #16729

icewind1991 · 2015-06-03T14:46:08Z

Note that this does not prevent concurrent scanners, only stops the file or folder being changed while scanning

cc @DeepDiver1975 @PVince81

PVince81 · 2015-06-03T14:47:41Z

If this can fix the data loss issue #13391 (comment) then it already rocks

icewind1991 · 2015-06-03T14:55:04Z

If the issue is not fixed with just this PR we could add a lock for the cache next to the current lock to prevent partial updated cache data being returned from PROPFIND and friends while the cache is being updated

PVince81 · 2015-06-03T15:02:51Z

BUG: upload in root folder with sync client always gives "locked" error (subdirs seem fine)

PVince81 · 2015-06-03T15:06:07Z

Testing #13391 (comment)
While this PR makes it a little less likely to happen, it still happened again: files lost.

It also seemed that the sync client hung in the discovery phase, maybe some changes will be required there as well to make sure it just continues.

PVince81 · 2015-06-09T08:52:49Z

@icewind1991 any update on this in regards to fixing #13391 ?

I noticed in my analysis yesterday that we need to be careful what data is returned to the client.
So even if the scanner would find that the folder on-disk has disappeared, it should not remove it from the result list if it is locked, else the client might delete it...

icewind1991 · 2015-06-09T15:03:20Z

The failure was caused by the trashbin needing the fileinfo in the wrapper for shared files which can triger a scan and run into a locking problem.

I've worked around this by making sure the scanning happens in the pre-hook instead of the unlink operation itself.
Not the cleanest solution but all I can come up with at the moment

icewind1991 · 2015-06-10T13:19:46Z

upload in root folder with sync client always gives "locked" error (subdirs seem fine)

@PVince81 can you retest?

PVince81 · 2015-06-11T13:59:28Z

Upload to root folder works now.

PVince81 · 2015-06-11T14:20:57Z

apps/files_trashbin/lib/trashbin.php

+	 * @param array $params
+	 */
+	public static function ensureFileScannedHook($params) {
+		self::getUidAndFilename($params['path']);


weird magic ?

This ensures that if the file needs to be scanned it's scanned during the pre-hook and not the actual delete operation (where it's write locked)

=> add a comment // triggers scan indirectly or something along these lines

@icewind1991 please add comment to the code as requested - THX

Expanded the phpdoc

PVince81 · 2015-06-11T14:24:59Z

This does not fix the problem here #13391

Maybe it fixes another one from the list #16664 ?

icewind1991 · 2015-06-12T11:11:02Z

@DeepDiver1975 @MorrisJobke please review

PVince81 · 2015-06-12T11:13:47Z

How did you test this ? Which one of the many issues does this fix ?

icewind1991 · 2015-06-12T11:31:31Z

It should fix issues where a file is deleted/renamed while the file is being scanned.

To test:

Add a sleep in scanFile (after the call to getData)
trigger a scan on an existing file from the cli ./occ files:scan --path /test/files/folder/asd.jpg
rename the file trough the web interface while the scan is running

Without this patch the rename completes and both the old an new file will show up in the web interface, with this patch the rename fails with a locked exception

PVince81 · 2015-06-12T11:32:58Z

Cool thanks, I'll have a try.

PVince81 · 2015-06-12T11:45:02Z

Works 👍

(side note: I completelty forgot that we had the --path option)

icewind1991 · 2015-06-12T15:32:34Z

@MorrisJobke @DeepDiver1975 please review, this is blocking some other locking work I would like to do

MorrisJobke · 2015-06-15T07:00:18Z

lib/private/connector/sabre/file.php

@@ -237,12 +239,11 @@ public function put($data) {
 				}
 			}
 			$this->refreshInfo();
+			$this->fileView->unlockFile($this->path, ILockingProvider::LOCK_SHARED);


This looks somehow weird. The lock seems to be never unlocked when an error between the lock and unlock happens. Is this valid nevertheless?

If an exception happens the unlock will be handled by the shutdown handler, since any error here ends the request there is no need to unlock the file before the end of the request

DeepDiver1975 · 2015-06-15T12:21:01Z

@icewind1991 we need a rebase here - THX

DeepDiver1975 · 2015-06-15T13:17:13Z

lib/private/files/cache/scanner.php

@@ -83,6 +89,7 @@ public function __construct(\OC\Files\Storage\Storage $storage) {
 		$this->storageId = $this->storage->getId();
 		$this->cache = $storage->getCache();
 		$this->cacheActive = !Config::getSystemValue('filesystem_cache_readonly', false);
+		$this->lockingProvider = \OC::$server->getLockingProvider();


cant without breaking changes to the storage api :(

scrutinizer-notifier · 2015-06-15T13:43:35Z

A new inspection was created.

ghost · 2015-06-15T14:58:09Z

🚀 Test PASSed.🚀

icewind1991 · 2015-06-16T12:03:33Z

@DeepDiver1975 can this be merged?

PVince81 · 2015-06-16T14:22:27Z

My 👍 still applies.

Blocks #16963.
Please review promptly.

PVince81 · 2015-06-16T14:50:33Z

@nickvergessen maybe ?

DeepDiver1975 · 2015-06-17T09:52:51Z

👍

keep a read lock while scanning a file or folder

PVince81 · 2015-06-17T15:47:32Z

Regression: #16998

icewind1991 mentioned this pull request Jun 3, 2015

add proper locking to file_put_contents when using streams #16727

Merged

icewind1991 force-pushed the scanner-read-lock branch from 8262d6b to d13b862 Compare June 3, 2015 15:19

icewind1991 mentioned this pull request Jun 11, 2015

[Experimental files locking] Files not fully uploaded to external storages should not appear. #16874

Closed

PVince81 reviewed Jun 11, 2015
View reviewed changes

PVince81 added this to the 8.1-current milestone Jun 12, 2015

PVince81 added the 3 - To Review label Jun 12, 2015

icewind1991 force-pushed the scanner-read-lock branch from ee191f6 to 9f065ff Compare June 12, 2015 12:33

MorrisJobke reviewed Jun 15, 2015
View reviewed changes

icewind1991 added 4 commits June 15, 2015 14:32

keep a read lock while scanning a file or folder

7d72f7d

properly unlock in case of failed fopen

a4f808b

ensure we dont have to scan a file in the trash wrapper

5efbab4

ensure normalized path form when changing locks

9c0d69a

icewind1991 force-pushed the scanner-read-lock branch from 9f065ff to 9c0d69a Compare June 15, 2015 12:50

DeepDiver1975 reviewed Jun 15, 2015
View reviewed changes

explain pre-hook scan

f02e110

PVince81 mentioned this pull request Jun 16, 2015

Acquire read lock when getting file or directory info #16963

Merged

SergioBertolinSG mentioned this pull request Jun 16, 2015

[Experimental files locking] Uploading files to root folder is locked. #16970

Closed

icewind1991 added the feature:locking label Jun 16, 2015

DeepDiver1975 added a commit that referenced this pull request Jun 17, 2015

Merge pull request #16729 from owncloud/scanner-read-lock

d2305f2

keep a read lock while scanning a file or folder

DeepDiver1975 merged commit d2305f2 into master Jun 17, 2015

DeepDiver1975 deleted the scanner-read-lock branch June 17, 2015 09:52

PVince81 mentioned this pull request Jun 17, 2015

Cannot browse web UI, locked, during concurrent upload #16998

Closed

nickvergessen mentioned this pull request Jun 18, 2015

Regression with locking since "keep a read lock while scanning a file or folder" #17018

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

keep a read lock while scanning a file or folder #16729

keep a read lock while scanning a file or folder #16729

icewind1991 commented Jun 3, 2015

PVince81 commented Jun 3, 2015

icewind1991 commented Jun 3, 2015

PVince81 commented Jun 3, 2015

PVince81 commented Jun 3, 2015

PVince81 commented Jun 9, 2015

icewind1991 commented Jun 9, 2015

icewind1991 commented Jun 10, 2015

PVince81 commented Jun 11, 2015

PVince81 Jun 11, 2015

icewind1991 Jun 12, 2015

PVince81 Jun 12, 2015

DeepDiver1975 Jun 15, 2015

icewind1991 Jun 15, 2015

PVince81 commented Jun 11, 2015

icewind1991 commented Jun 12, 2015

PVince81 commented Jun 12, 2015

icewind1991 commented Jun 12, 2015

PVince81 commented Jun 12, 2015

PVince81 commented Jun 12, 2015

icewind1991 commented Jun 12, 2015

MorrisJobke Jun 15, 2015

icewind1991 Jun 15, 2015

DeepDiver1975 commented Jun 15, 2015

DeepDiver1975 Jun 15, 2015

icewind1991 Jun 15, 2015

scrutinizer-notifier commented Jun 15, 2015

ghost commented Jun 15, 2015

icewind1991 commented Jun 16, 2015

PVince81 commented Jun 16, 2015

PVince81 commented Jun 16, 2015

DeepDiver1975 commented Jun 17, 2015

PVince81 commented Jun 17, 2015

keep a read lock while scanning a file or folder #16729

keep a read lock while scanning a file or folder #16729

Conversation

icewind1991 commented Jun 3, 2015

PVince81 commented Jun 3, 2015

icewind1991 commented Jun 3, 2015

PVince81 commented Jun 3, 2015

PVince81 commented Jun 3, 2015

PVince81 commented Jun 9, 2015

icewind1991 commented Jun 9, 2015

icewind1991 commented Jun 10, 2015

PVince81 commented Jun 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Jun 11, 2015

icewind1991 commented Jun 12, 2015

PVince81 commented Jun 12, 2015

icewind1991 commented Jun 12, 2015

PVince81 commented Jun 12, 2015

PVince81 commented Jun 12, 2015

icewind1991 commented Jun 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeepDiver1975 commented Jun 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scrutinizer-notifier commented Jun 15, 2015

ghost commented Jun 15, 2015

icewind1991 commented Jun 16, 2015

PVince81 commented Jun 16, 2015

PVince81 commented Jun 16, 2015

DeepDiver1975 commented Jun 17, 2015

PVince81 commented Jun 17, 2015