Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#5660 closed Enhancement (fixed)

Daemon should use event loop

Reported by: missionsix Owned by:
Priority: Normal Milestone: 2.83
Component: Daemon Version: 2.82
Severity: Normal Keywords: daemon libevent
Cc:

Description

Background


I've been interested in improving the transmission daemon source due to recent issues I've had with watchdir on linux. I've spent some time now looking at daemon.c/watch.c and realized there's a couple optimizations that can be done.

In order to cleanup the notify stuff, I've had to first fix the daemon loop.

Proposed Solution


I'd like to replace the main loop with an event loop and timer which allows for further development using libevent as the threading model in the daemon.

For example, I can register an event socket watching the inotify FD for changes, and instrument a callback to add new torrents when they are detected in the target directory. This reduces the overhead of read()-ing the inotify fd every 1 second, and allows for edge triggered updates.

Other optimizations could be made in the daemon with the event loop including safer signal handling, SIGHUP updating, watching for settings file changes, etc.

Attachments (3)

daemon-event-loop.diff (4.2 KB) - added by missionsix 8 years ago.
Daemon eventloop patch
daemon-event-loop-v2.diff (4.5 KB) - added by missionsix 8 years ago.
version 2 of the daemon event loop changes
5660-daemon-event-loop-v3.diff (4.5 KB) - added by jordan 8 years ago.

Download all attachments as: .zip

Change History (18)

Changed 8 years ago by missionsix

Daemon eventloop patch

comment:1 Changed 8 years ago by mike.dld

I'm not exactly proficient with libevent yet, but the patch looks nice to me. I have some comments though:

  1. event_new() may return NULL and event_add() may return -1. Both are fatal in current patch version (event_add() passed NULL would crash, event_base_dispatch() with no pending events would return 1 immediately) and should be handled accordingly.
  2. Check for ev_base validity during teardown is not needed, already handled during initialization.
  3. Taking in account p.1, check for status_ev validity during teardown should also be unnecessary.
  4. tr_wait_msec(1000) in reportStatus() is not needed (looks like refactoring leftover).
  5. As we're striving for optimization, I'd also wrap systemd status reporting in reportStatus() into #ifdef USE_SYSTEMD_DAEMON to disable meaningless calls to tr_sessionGetRawSpeed_KBps() (although not the point of this patch at all).

comment:2 Changed 8 years ago by missionsix

I've attached an updated version with some of the feedback addressed.

I'm not sure I like #3, given that things should be properly shut down and not just exit(1), therefore I added the cleanup: label which should be used as the exit point. This allows things like inotify table to be properly run down, such that there are no leaks.

Good catch on #4, I did mean to remove that.

I think #5 should be addressed in another patch.

comment:3 Changed 8 years ago by mike.dld

Don't see the updated patch, please try uploading it under different name.

Changed 8 years ago by missionsix

version 2 of the daemon event loop changes

comment:4 follow-up: Changed 8 years ago by jordan

v2 of the patch looks pretty good.

One trivial readability change I'd suggest -- one wouldn't expect a function named reportStatus() to have a side-effect of pumping the watchdir code. It would be less surprising if the libevent's callback func was a wrapper function named (say) periodicUpdate() that called both dtr_watchdir_update() and reportStatus().

That would also be slightly cleaner for Mike's fifth suggestion, as reportStatus()'s body could be conditionally compiled iff USE_SYSTEMD_DAEMON is #defined.

Changed 8 years ago by jordan

comment:5 follow-ups: Changed 8 years ago by jordan

I've added 5660-daemon-event-loop-v3.diff, which is based on missionsix's v2 diff plus the change I suggested in comment:4.

missionsix, mike.dld, any feedback/suggestions on this iteration?

comment:6 Changed 8 years ago by jordan

  • Milestone changed from None Set to 2.83

comment:7 in reply to: ↑ 4 Changed 8 years ago by mike.dld

  • Milestone changed from 2.83 to None Set

Replying to jordan:

... one wouldn't expect a function named reportStatus() to have a side-effect of pumping the watchdir code ...

I decided not to treat this as an issue since missionsix said the idea was not to call dtr_watchdir_update() periodically (at least with inotify), so I thought some other patch would follow which would extract this call out to separate function anyway (and I was right, as there is #5663, although it still has pumpLogMessages() inside reportStatus()).

comment:8 Changed 8 years ago by mike.dld

  • Milestone changed from None Set to 2.83

comment:9 in reply to: ↑ 5 Changed 8 years ago by mike.dld

Replying to jordan:

I've added 5660-daemon-event-loop-v3.diff, which is based on missionsix's v2 diff plus the change I suggested in comment:4.

missionsix, mike.dld, any feedback/suggestions on this iteration?

Looks good to me.

comment:10 in reply to: ↑ 5 Changed 8 years ago by missionsix

Replying to jordan:

I've added 5660-daemon-event-loop-v3.diff, which is based on missionsix's v2 diff plus the change I suggested in comment:4.

missionsix, mike.dld, any feedback/suggestions on this iteration?

Nope, LGTM.

comment:11 Changed 8 years ago by jordan

  • Resolution set to fixed
  • Status changed from new to closed

v3 diff added to trunk in r14265

comment:12 follow-up: Changed 8 years ago by craig

I think this patch isn’t working correctly for the DLink DNS-325 storage unit. I’ve been running Transmission-2.82 for a while without any issues.

When I upgraded to Transmission-2.83 via slacker, I receive the following error.

/ffp/bin/transmission-daemon: can't load library 'libevent-2.0.so.5'

I thought it was just a path or env error but the issue is beyond my skillset. I rolled back to Transmission-2.82 and everything works fine.

Below is some info for a more knowledgeable person to possible troubleshoot for a future release.


## /ffp/bin/funpkg -u /ffp/funpkg/cache/kylek/Transmission-2.83-arm-1.txz

Scanning /ffp/funpkg/cache/kylek/Transmission-2.83-arm-1.txz ... Upgrading to Transmission-2.83-arm-1 Removing Transmission-2.82-arm-1,upgraded,20140720_104741

Searching for *.new files: /ffp/etc /ffp/start /ffp/etc/examples/lighttpd.conf-with-php.new /ffp/etc/examples/lighttpd.conf.new /ffp/etc/ssh/sshd_config.new /ffp/etc/ssh/moduli.new /ffp/etc/ssl/openssl.cnf.new /ffp/start/kickwebs.sh.new /ffp/start/rsyncd.sh.new /ffp/start/sshd.sh.new /ffp/start/kickangel.sh.new /ffp/start/lighttpd.sh.new Done.

## ./transmission.sh start Starting transmission-daemon /ffp/bin/transmission-daemon: can't load library 'libevent-2.0.so.5'

## echo $PATH PATH=/opt/sbin:/opt/bin:/ffp/sbin:/usr/sbin:/sbin:/ffp/bin:/usr/bin:/bin:/opt/lib

## locate libevent-2.0.so.5 /mnt/HD/HD_a2/ffp/opt/optware/lib/libevent-2.0.so.5 /mnt/HD/HD_a2/ffp/opt/optware/lib/libevent-2.0.so.5.1.8 /opt/lib/libevent-2.0.so.5 /opt/lib/libevent-2.0.so.5.1.8

comment:13 in reply to: ↑ 12 ; follow-up: Changed 8 years ago by mike.dld

Replying to craig:

/ffp/bin/transmission-daemon: can't load library 'libevent-2.0.so.5'

This has nothing to do with code changes. It's your environment issue.

Replying to craig:

## echo $PATH PATH=/opt/sbin:/opt/bin:/ffp/sbin:/usr/sbin:/sbin:/ffp/bin:/usr/bin:/bin:/opt/lib

## locate libevent-2.0.so.5 /mnt/HD/HD_a2/ffp/opt/optware/lib/libevent-2.0.so.5 /mnt/HD/HD_a2/ffp/opt/optware/lib/libevent-2.0.so.5.1.8 /opt/lib/libevent-2.0.so.5 /opt/lib/libevent-2.0.so.5.1.8

On *NIX systems, paths where dynamic linker looks for libraries doesn't include PATH environment variable (you can see the libraries dynamic linker is currently aware of by executing ldconfig -v). Try running transmission-daemon manually, adding path to libraries explicitly beforehand:

# export LD_LIBRARY_PATH=/opt/lib:$LD_LIBRARY_PATH
# /ffp/bin/transmission-daemon -f

(You may need to use setenv or something else instead of export depending on your system). If that helps, modify either dynamic linker configuration (/etc/ld.so.conf or similar) or transmission startup script to include that path above.

comment:14 in reply to: ↑ 13 Changed 8 years ago by mike.dld

Replying to mike.dld:

If that helps, modify either dynamic linker configuration (/etc/ld.so.conf or similar) or transmission startup script to include that path above.

I'm also not sure whether "/mnt/HD/HD_a2/ffp/opt/optware" or "/opt" are standard install prefixes on your system. If those aren't, you may as well just install libevent normally (with "/ffp" prefix? transmission-daemon is installed there for some reason) and it would work as is.

comment:15 Changed 7 years ago by craig

Thanks for the help. The dynamic library development class is coming back into focus.

I rule out my DNS-325 by building a clean OS and booting from a USB stick. I tried the suggestions from the above comments. None of them worked. However when I installed the libevent package via funpkg the issue was resolved.

I agree there is nothing wrong with the code base. It appears to be an issue the way KyleK packaged up Transmission 2.83.

Note: See TracTickets for help on using tickets.