Created attachment 131032 [details] [review]
Fix for gvfs

Created attachment 131039 [details] [review]
Updated patch, do not rely on GMount::pre-unmount being there

+static void
+real_mount_pre_unmount_cb (GVolumeMonitor    *volume_monitor,
+                           GMount            *mount,
+                           GProxyShadowMount *shadow_mount)
+{
+  if (mount == shadow_mount->real_mount)
+    signal_emit_in_idle (shadow_mount->volume_monitor, "mount-pre-unmount", shadow_mount);
+}
+

I don't think you need to do that in an idle. You just got an emission of mount-pre-unmount, so we should be in a safe place for emissions. (Less idle stuff, less risk for weird reordering of signals)

Otherwise it looks good to commit.

Not sure this is going to work so please hold off committing that patch.

Keep in mind that the hal volume monitor runs in a separate process. So there are at least two issues here. 

 1. The mount-pre-unmount signals aren't being listened to by the D-Bus
    server for the volume monitor process and not propagated to any
    process

 2. It's pretty hard to fix 1.

The idea behind mount-pre-unmount is that users can subscribe to that signal and then they have a window (in the signal handler) to do stuff (which I guess would involve closing files synchronously).

Now, in order for this to really work we would need the D-Bus server to get an ACK back from *each* process using the volume monitor. This ACK would get sent when the signal handler for the process in question is done. Then when the volume monitor has ACK's for all processes it can go ahead and continue the unmount operation. 

It seems to me that mount-pre-unmount is mostly something used in Nautilus so I don't know how interesting it is to try and support the ACK scheme described above since it's pretty complex to get right. Note that before we moved volume monitors out of process mount-pre-unmount didn't really work except for only a single process....

So maybe it's better just to deprecate mount-pre-unmount and make Nautilus do it's own thing. I don't know.

FWIW, Totem uses the pre-umount to remove DVDs and files from the playlist before the device is unmounted.

Another way to resolve this bug is to 

 a. only emit mount-pre-unmount in-process (e.g. only from the proxy
    and daemon monitors) and just ignore it in the remove volume monitors.

 b. amend the ::mount-pre-unmount docs to say that this signal is only
    guaranteed to be emitted in the process initiating the unmount operation

which would make it work for the Nautilus case. 

Another (and better I think) way to resolve this is to just remove/deprecate mount-pre-unmount altogether and just fix up applications so they don't have open files unless needed. Not sure how realistic this is.

Also, this may screw over Nautilus when using fam instead of inotify (which I think BSD is doing), see the nautilus list for details

http://mail.gnome.org/archives/nautilus-list/2009-February/msg00022.html

(In reply to comment #5)
> FWIW, Totem uses the pre-umount to remove DVDs and files from the playlist
> before the device is unmounted.

Why not simply use ::unmounted? Or do you have a open fd on the mount? If so, do you need to do that?

(In reply to comment #7)
> (In reply to comment #5)
> > FWIW, Totem uses the pre-umount to remove DVDs and files from the playlist
> > before the device is unmounted.
> 
> Why not simply use ::unmounted? Or do you have a open fd on the mount? If so,
> do you need to do that?

It's mainly for discs, where the libraries underneath us aren't so keen on having the media go away from under them.

The use case I'm mostly interested in is in making Tracker cleanly skip files from just unmounted media. As it could be running through the device files' when it's being unmounted, it should:

1) Remove any file in the unmounted device from the queue
2) Cancel the ongoing indexing operation if it's also affected by the unmount.

So I think fixing Tracker not to allow clean unmounting without requiring the ::mount-pre-unmount signal is going to be really hard at least.

David, I think you're right that it'd be better to have applications notify back before actually unmounting, but that would also make unmount operations prone to get stuck with failing apps (blocking, crashing, ...). IMHO just documenting this signal as a "hurry up, save yourself" notification would be enough, as it certainly is interesting outside Nautilus :).

(In reply to comment #9)
> The use case I'm mostly interested in is in making Tracker cleanly skip files
> from just unmounted media. As it could be running through the device files'
> when it's being unmounted, it should:
> 
> 1) Remove any file in the unmounted device from the queue
> 2) Cancel the ongoing indexing operation if it's also affected by the unmount.
> 
> So I think fixing Tracker not to allow clean unmounting without requiring the
> ::mount-pre-unmount signal is going to be really hard at least.
> 
> David, I think you're right that it'd be better to have applications notify
> back before actually unmounting, but that would also make unmount operations
> prone to get stuck with failing apps (blocking, crashing, ...). IMHO just
> documenting this signal as a "hurry up, save yourself" notification would be
> enough, as it certainly is interesting outside Nautilus :).
> 

I 100% agree with Carlos. It seems to me the obvious solution is to do that and have some fall-back time-out which assumes applications didn't get their ACK together quick enough (pun intended) :)

This is quite a big issues for the Maemo platform right now because they do things when you plug the USB cable into the PC and if Tracker play nice by releasing files we have open/etc then they have problems unmounting. 

We never blocked the unmount process on each app handling pre-unmount before (in gnome-vfs), and I don't think that is a good idea. I much prefer it to be a simple hint for apps with a timeout. Now, its clearly just a hint, and won't happen if e.g. things are unmounted with non-gio apis. However, in the vast majority of cases we can get things to work fine.

It is not realistic to not have this and yet claim to support dnotify or fam, which we obviously do. Nor do I think its useless for other apps (totem, etc as per above).

David, what do you mean by:
> 1. The mount-pre-unmount signals aren't being listened to by the D-Bus
>    server for the volume monitor process and not propagated to any
>    process

g_vfs_proxy_volume_monitor_daemon_main() connects to the hal monitors mount-pre-unmount and sends MountPreUnmount signals, which are proxied to the proxy volume monitor by gproxyvolumemonitor.c:filter_function()

Where exactly do you see this failing?

(In reply to comment #11)
> We never blocked the unmount process on each app handling pre-unmount before
> (in gnome-vfs), and I don't think that is a good idea. I much prefer it to be a
> simple hint for apps with a timeout. Now, its clearly just a hint, and won't
> happen if e.g. things are unmounted with non-gio apis. However, in the vast
> majority of cases we can get things to work fine.
> 
> It is not realistic to not have this and yet claim to support dnotify or fam,
> which we obviously do. Nor do I think its useless for other apps (totem, etc as
> per above).

I was under the impression that you just needed to do your thing (close all open fd's) before returning from the signal handler for GVolumeMonitor::mount-pre-unmount ... I mean, for a single process I think you are guaranteed that all signal handlers complete before

 g_signal_emit (monitor, "mount-pre-unmount", mount_to_be_unmounted);

returns. Isn't that right?

> David, what do you mean by:
> > 1. The mount-pre-unmount signals aren't being listened to by the D-Bus
> >    server for the volume monitor process and not propagated to any
> >    process
> 
> g_vfs_proxy_volume_monitor_daemon_main() connects to the hal monitors
> mount-pre-unmount and sends MountPreUnmount signals, which are proxied to the
> proxy volume monitor by gproxyvolumemonitor.c:filter_function()

I should have checked it then, I just remembered not adding this because of the fact that it would never really work anyway when the volume monitor is a separate process.

> Where exactly do you see this failing?

Well, as I tried to explain. In order for GVolumeMonitor::mount-pre-unmount to actually work, then

 - every recipient of GVolumeMonitor::mount-pre-unmount needs to close all
   files in the signal handler as described above [1]

 - we need to wait until all signal handlers complete

 - we need to handle this for all GIO using processes

otherwise you have a race condition.

Maybe a better solution, instead of a complicated and fragile ACK system, is just for the volume monitor to retry the unmount operation a couple of times (sleeping a few hundred milliseconds between the attempts) if it gets G_IO_ERROR_BUSY? Is that what you are suggesting Alex?

     David

[1] : the raises an interesting question of whether this is even feasible with current GIO APIs... presumably, for GLocalFile, there is only open fd's when ops are pending. So the guarantees we need are

 - when g_cancellable_cancel() returns then the corresponding fd is closed
 - ditto for g_file_monitor_cancel()
 - and probably others...

What I'm suggesting (and what the patch and gnome-vfs implements) is that we'll send the pre-unmount signal, wait a short time and then try to unmount. In a typical situation this is good enough to get anything that keeps the mount busy to release its stuff, while guaranteeing that we'll never block the unmount forever waiting on some hanged app.

Now, its true that there is a race condition (I've never argued that there isn't), but I don't think that is a huge problem. What happens in the case of a race? It means some process will release its access on the to-be-unmounted mount a bit later, so we get a BUSY error, but if you try to unmount again it will succeed. While a bit ugly, this is imho better than both a) risking blocking forever or b) don't do the pre-mount meaning you'll never be able to avoid busying unmounts.

The current code does a 500 msec wait and then tries to unmount. The approach you propose, trying a coule of times retrying on BUSY with shorter timeouts may actually be a nicer approach, as it will result in less delay in the non-busy case. In fact, we could try unmounting immediately, then delaying a bit if busy.

Alright... so I've tried to implement this patch as suggested, but gnome-mount is standing in my way. As called by GVfs, it's gnome-mount showing the error dialog if the mount is busy, so that error is seen several times.

According to gnome-mount manpage, file browsers shouldn't use the GUI, so the -n flag should be passed, but even with the -t switch, it doesn't output any error through stderr, so GVfs is just able to emit the G_IO_ERROR_FAILED_HANDLED error.

My conclusion is that fixing this in gnome-mount is too much pain for little gain, since GVfs should also depend on newer behavior, etc... IMHO 0.5secs is a not too long delay for humans, and a reasonably big window for apps to handle this, and the operation would still fail anyway if there's some faulty app not releasing the removable device.

carlos: thats a bit sucky. 
But, I agree, lets go with 500msec and one try. So, please commit with the change i requested above.

I've just committed the patch with the change suggested on comment #3, thanks!

2009-03-26  Carlos Garnacho  <carlosg@gnome.org>

        Bug 576083 – pre-unmount signals not being triggered

        * monitor/hal/ghalmount.c (unmount_do) (unmount_do_cb): Emit
        ::mount-pre-unmount and wait 500msec before actually trying to
        unmount.
        * monitor/proxy/gproxyshadowmount.c (real_mount_pre_unmount_cb): Proxy
        the shadowed mount pre-unmount signal.
        * monitor/proxy/gproxyvolumemonitor.c (filter_function): GMount
        doesn't have the ::pre-unmount signal yet, so don't emit it.

JFYI I just fixed the same bug in the gdu volume monitor (which will be in the next GVfs release and take priority over the hal volume monitor). Unlike the hal volume monitor we'll attempt the unmount right away and retry a couple of times if we get G_IO_ERROR_BUSY.

http://cgit.freedesktop.org/~david/gvfs/commit/?h=gdu-volume-monitor&id=8d9fc8a777fb1b00ae371e276a14416a0987f4af

I'll attach a small test program used to test this; it may be useful to test other volume monitors.

Created attachment 132605 [details]
test program