drop-upload: also monitor subdirectories

[davidsarah]

The prototype implementation of drop-upload in #1429 monitors only a single directory, and ignores creation of subdirectories or changes to files within them. It is possible to monitor subdirectories as well, although the efficiency and complexity of this varies depending on the design of the platform's filesystem monitoring API (if we support multiple platforms with this feature as suggested in #1431 and #1432).

[daira]

Note that the IN_ONLYDIR constant in the current event mask doesn't do what you might think it does. It only ensures that the root being monitored is a directory.

[daira]

On 07/04/15 21:05, David Stainton wrote:>

a couple of my initial and trife observations: 1. Looks like the latest Twisted inotify class has got a recursive watch. I'm not saying we need to use that version of Twisted; it's a simple loop we can implement. ​https://twistedmatrix.com/trac/browser/tags/releases/twisted-15.0.0/twisted/internet/inotify.py#L350

This has been there since the inotify support was added in Twisted 10.1.0, in fact.

2. I looked at the source code of the twisted INotify class to see if it's watch method could be called more than once and behave properly. It looks like it does support this usage.

Yes, it does, but the recursive/autoAdd support looks sufficient so we shouldn't need this. (We also don't need it for Windows because ReadDirectoryChangesW has a similar flag to watch a subdirectory tree.)

3. I am not clear on why drop_upload.py uses this inotify mask:

mask = inotify.IN_CLOSE_WRITE | inotify.IN_MOVED_TO | inotify.IN_ONLYDIR

Combining all these definitions: IN_MOVED_TO - File moved into watched directory IN_CLOSE_WRITE - File opened for writing was closed IN_ONLYDIR - Only watch pathname if it is a directory.

This means we only watch subdirectories that have been created or moved into the directory. Did I understand things correctly?

See comment:5 .

4. the inotify man page states:

If monitoring an entire directory subtree, and a new subdirectory is created in that tree, be aware that by the time you create a watch for the new subdirectory, new files may already have been created in the subdirectory. Therefore, you may want to scan the contents of the subdirectory immediately after adding the watch.

This obviously means we should construct a mask specifically to catch newly added directories like this:

mask = inotify.IN_MOVED_TO | inotify.IN_ONLYDIR

and then add a specific inotify watch for that subdirectory... AND add recursively add that directory and its contents to the upload queue.

The autoAdd flag already adds watches for children of newly added directories (and has done so since Twisted 10.1.0):

​https://twistedmatrix.com/trac/browser/tags/releases/twisted-10.1.0/twisted/internet/inotify.py#L259

It also synthesizes IN_CREATE events for files and subdirectories found in those newly added directories (see the _addChildren method in inotify.py).

[dawuud]

sounds like we need a simple one line change like this: ​https://github.com/david415/tahoe-lafs/tree/david-1433-1

Does this satisfy our spec?: ​https://github.com/tahoe-lafs/tahoe-lafs/blob/master/docs/proposed/magic-folder/filesystem-integration.rst

which states that we need to do a full scan of new directories...

I am wondering what would happen if a user moved a directory tree into the magic folder? Would the subdirectories be scanned? It looks like to me they would not be. If this is the case then maybe we'll have to not use the autoAdd feature.

[daira]

The spec says that a scan of new directories needs to be done, it doesn't say what code is responsible for that. At the time, I hadn't looked at the relevant Twisted code.

We do need to check what happens in the move-one-directory-into-another case. I suspect it Just Works though, since _addChildren doesn't distinguish that case, and shouldn't need to.

[daira]

I suggest first writing tests for these cases, rather than worrying about the implementation.

[dawuud]

i added some code to david-1433-1 branch that handles the symlink and directory cases.

if symlink then ignore file and print error to the log. if dir then create the subdirectory.

[dawuud]

here's my working code AND unit tests! tests: ​https://github.com/david415/tahoe-lafs/blob/d9153c482eb30eb15117d758fc7303f4a90076d3/src/allmydata/test/test_drop_upload.py#L71-L114

code: ​https://github.com/david415/tahoe-lafs/tree/2406.otf-objective-2.3.1-fix-upload-deque

[daira]

Excellent! Will review now.

[dawuud]

Ok recently fixed some problems with these tests... and I think it exposes a bug... must look into it...

[dawuud]

fixed unit tests and code. awaiting code review from daira for my recent changes regarding the addition of a notifier watch upon directory placement in the uploader directory. (the watch is recursive and a recursive scan of the new directory is also applied)

[daira]

I reviewed this (and we made a few other fixes in our Tuesday pairing session); it looks good. I still need to review the rest of the queue/scanning code.

[daira]

Closing this and using ticket #2406 for any further review comments.

[warner]

Milestone renamed