GNOME Bugzilla – Bug 676111
Add GMountOperation::show-unmount-progress signal
Last modified: 2012-07-16 12:45:21 UTC
The idea here is that if unmounting is slow because the OS kernel is transferring data from kernel buffers to stable storage (the writes done by the app have already completed), we want to convey this to the user so he doesn't unplug the device. This happens very frequently on Linux if you don't mount with the 'sync' option. The various GMount implementations would emit this signal when unmounting and the time spent doing the operation exceeds, say, 1.5 seconds. GMountOperation implementations, such as GtkMountOperation, would e.g. pop up a window telling the user to wait. This is not a new idea, it's related to the existing ::show-processes signal http://developer.gnome.org/gio/unstable/GMountOperation.html#GMountOperation-show-processes that is shown when there are open fd's on a device being unmounted. The signal should include parameters about estimated amount of data and estimated time left. We'd initially set this to -1 (for unknown) and ignore it in e.g. GtkMountOperation but in the future we could possibly get this information from the kernel. The signal would be allowed to be emitted several times (for updated parameters). See https://bugzilla.redhat.com/show_bug.cgi?id=819492 for some background on why we want this.
Thinking more about it, I think GMountOperation::show-unmount-progress is more appropriate than GMountOperation::unmount-pending and more in line with the existing GMountOperation signals, see http://developer.gnome.org/gio/unstable/GMountOperation.html#GMountOperation.signals
Tomas, Cosimo - is one of you looking at implementing this API ?
I'm working on this.
Created attachment 218457 [details] [review] mount-operation: add show-unmount-progress signal --- GLib part
Created attachment 218458 [details] [review] udisks2: plug a memory leak We were not free-ing the UnmountMountsOp structure when there was an error in one mount. Fix that and use g_list_free_full to make code more concise.
Created attachment 218459 [details] [review] trivial: remove unused defines
Created attachment 218460 [details] [review] udisks2: simplify some code
Created attachment 218461 [details] [review] monitor: handle show-unmount-progress signal
Created attachment 218462 [details] [review] udisks2: implement show-unmount-progress
Created attachment 218463 [details] [review] common: add support show-unmount-progress signal to GMountOperationDBus
Created attachment 218464 [details] [review] daemon: implement show-unmount-progress in GVfsJobUnmount
Review of attachment 218457 [details] [review]: LGTM, only some doc comments ::: gio/gmountoperation.c @@ +393,3 @@ + * is completed + * + * @time_left: the estimated time left before the operation completes, or -1 I would say s/1.5 seconds/some time (typically 1.5 seconds)/ @@ +399,3 @@ + * can take a considerable amount of time. This signal will be emitted several + * times as long as the unmount operation is outstanding, and then one + * is completed Suggest to use the word "may" instead of "will" since I believe the udisks2 impl will only emit it once (with bytes_left set to -1) and then one more time at the end (with bytes_left set to 0).
Review of attachment 218458 [details] [review]: Looks good to me.
Review of attachment 218459 [details] [review]: Yup.
Review of attachment 218460 [details] [review]: OK, we can do that
Review of attachment 218461 [details] [review]: Looks correct to me, it's mostly just copy-paste from the show-processes signal, right? Might be good to get Tomas' review on it as well since he touched that code last.
Review of attachment 218462 [details] [review]: This looks a bit complicated and I'm wondering if it can be simplified somewhat... isn't the basic idea just that we want to attach extra data to the GMountOperation passed to the unmount() and eject() vfuncs? If so, I think we should attach it using g_object_set_data_full() and not have the users worry about freeing it (including who frees it). I don't think we need to expose the UnmountNotifyData struct either, I mean, we can just have gvfs_udisks2_unmount_notify_data_*() functions take a GMountOperation themselves and worry about getting/creating the UnmountNotifyData struct as needed. With those changes, I think the changes to gvfsudisks2mount.c and gvfsudisks2drive.c will be very small, basically it will just be a _start() and _stop() in the beginning resp. end of the mount and eject implementations. ::: monitor/udisks2/gvfsudisks2utils.c @@ +640,3 @@ + * bytes left fields. + */ + Not sure about the TODO item, I think if bytes_left or time_left is set then the dialog showing GMountOperation would convey the time... @@ +650,3 @@ + g_free (message); + g_free (name); +unmount_notify_get_name (UnmountNotifyData *data) Not sure it's appropriate to use that language for mounts that are not udisks - suggest to use very generic language for those instead, perhaps Unmount /mnt/path/to/mountpoint Please wait... or similar. Thoughts? @@ +737,3 @@ + message = g_strdup_printf (_("You can now unplug %s\n"), name); + g_signal_emit_by_name (data->op, "show-unmount-progress", + data = g_slice_new0 (UnmountNotifyData); Same here, this is not appropriate for non-udisks mounts... Probably say /mnt/path/to/mountpoint has been unmounted or something?
Created attachment 218486 [details] [review] mount-operation: add show-unmount-progress signal --- Fixed documentation as for review comments
Created attachment 218487 [details] [review] udisks2: factor out a common function Factor out unmount_data_complete(), we'll use it later.
Created attachment 218488 [details] [review] udisks2: split a helper function We'll need to use this later.
Created attachment 218489 [details] [review] udisks2: implement show-unmount-progress --- Thanks for the review David, this approach really makes the code simpler.
(In reply to comment #17) > ::: monitor/udisks2/gvfsudisks2utils.c > @@ +640,3 @@ > + * bytes left fields. > + */ > + > > Not sure about the TODO item, I think if bytes_left or time_left is set then > the dialog showing GMountOperation would convey the time... The comment there is not about including those in the message string, but about reporting the values in the signal emission. > Not sure it's appropriate to use that language for mounts that are not udisks - > suggest to use very generic language for those instead, perhaps > > Unmount /mnt/path/to/mountpoint > Please wait... > > or similar. Thoughts? I didn't change this because I don't understand what you mean with "not udisks"...this code path is only hit for udisks mounts.... > Same here, this is not appropriate for non-udisks mounts... Probably say > > /mnt/path/to/mountpoint has been unmounted > > or something? ...and I'd feel bad talking to the user in terms of mountpoints...the reason why the notification is useful in this case is to prevent accidental data loss due to an early physical unplug, and we already special case optical media. In which scenarios would this wording not be appropriate with udisks objects?
Comment on attachment 218486 [details] [review] mount-operation: add show-unmount-progress signal Looks good to me, thanks
Review of attachment 218487 [details] [review]: Sounds good to me.
Review of attachment 218488 [details] [review]: Good idea
Review of attachment 218489 [details] [review]: This is exactly what I had in mind, thanks! Except for the question about disconnecting signal handlers, this is good to go I think ::: monitor/udisks2/gvfsudisks2utils.c @@ +723,3 @@ + g_slice_free (UnmountNotifyData, data); +} + message = g_strdup_printf (_("Writing data to %s\n" Shouldn't we disconnect the signal handlers in unmount_notify_data_free()? For example, it's not entirely impossible you get another ::reply signal...
(In reply to comment #22) > I didn't change this because I don't understand what you mean with "not > udisks"...this code path is only hit for udisks mounts.... The way I read the patch from comment 21 (the latest patch) is that it shows the notification also if filesystem == NULL in which case we will just run the umount(8) command on the mount (e.g. it's a non-udisks mount). > > Same here, this is not appropriate for non-udisks mounts... Probably say > > > > /mnt/path/to/mountpoint has been unmounted > > > > or something? > > ...and I'd feel bad talking to the user in terms of mountpoints... OK, well, just use g_mount_get_name() then. It'll be set to the last path element of the mount point anyway (overridable with x-gvfs-name mount option). That's the name the user sees anyway. > the reason > why the notification is useful in this case is to prevent accidental data loss > due to an early physical unplug, and we already special case optical media. But exactly the same reasons (physical unplug of e.g. network cable, suspend, etc.) applies to any other kind of mount since _any_ filesystem mount will have some level of client-side buffering between the upper layers (~ filesystem) through the interconnect (USB, SATA for physical devices, IP / ethernet for network mounts) and to the platters / etc. The mount could be - NFS - CIFS - Storage backed by GMail - any kind of FUSE-thing Suppose for example the mount is for my GMail account and it's mounted at /home/davidz/GMailStorage. Let's say that I don't use the x-gvfs-name option so g_mount_get_name() returns "GMailStorage). I don't think it's appropriate to say Writing data to GMailStorage Don't unplug until finished [later] You can now unplug GMailStorage I think it would be better to say Unmounting GMailStorage Please wait [later] GMailStorage has been unmounted
Created attachment 218587 [details] [review] udisks2: implement show-unmount-progress --- Use a different wording for mounts that don't have a filesystem object.
We discussed the signal stuff on IRC; I think this should cover the other comments about the strings we use.
Review of attachment 218587 [details] [review]: Looks good to me, please commit (I'm assuming you've tested it with both usb sticks and NFS fstab mounts). Thanks!
Comment on attachment 218486 [details] [review] mount-operation: add show-unmount-progress signal Attachment 218486 [details] pushed as 44375ad - mount-operation: add show-unmount-progress signal
Attachment 218458 [details] pushed as 079c5ae - udisks2: plug a memory leak Attachment 218459 [details] pushed as 9aec339 - trivial: remove unused defines Attachment 218460 [details] pushed as 17f987f - udisks2: simplify some code Attachment 218461 [details] pushed as 8c61a01 - monitor: handle show-unmount-progress signal Attachment 218487 [details] pushed as 4fccf40 - udisks2: factor out a common function Attachment 218488 [details] pushed as 8dcd2e3 - udisks2: split a helper function Attachment 218587 [details] pushed as 174f304 - udisks2: implement show-unmount-progress
(In reply to comment #30) > Review of attachment 218587 [details] [review]: > > Looks good to me, please commit (I'm assuming you've tested it with both usb > sticks and NFS fstab mounts). Thanks! Thanks...I pushed a version with two other small corner case fixes discovered in testing with various scenarios. Keeping this open for the remaining two patches for the daemon part.
Review of attachment 218464 [details] [review]: Firing notification for all GVfs backends is rather unusual but hey, why not, could be actually useful in some cases. ::: daemon/gvfsjobunmount.c @@ +141,3 @@ + g_debug ("gvfsjobunmount progress clear"); + + message = g_strdup_printf (_("Connection to %s closed"), wording: GVfs backends are not always network services, suggesting using something neutral like "%s has been unmounted" @@ +160,3 @@ + /* TODO: report estimated bytes and time left */ + g_mount_source_show_unmount_progress (op_job->mount_source, + _("Writing data"), -1, -1); In the future, we could report umount/sync progress for some backends. For example, archiving backend might be doing archive close which may be essential to ensure archive integrity and could take quite a long time. We would have to call this handler multiple times then. @@ +180,3 @@ return; + unmount_progress_start (op_job); You're calling unmount_progress_start() and unmount_progress_clear() only from within run(). However, some backends might be actually doing real unmount within try_unmount even if it should be used only for quick operations. And since there's 1.5 sec timeout, it shouldn't disrupt the UI if it returns quickly and then falls back into run(). Also, there are even more occurrences of try_unmount calls in the file for the force unmount functionality.
Review of attachment 218463 [details] [review]: Looks good to me, please commit, I'll update my gdbus-core branch afterwards (involving 75% rewrite anyway). Please also bump GIO requirement for the new signal.
Comment on attachment 218463 [details] [review] common: add support show-unmount-progress signal to GMountOperationDBus Attachment 218463 [details] pushed as 69bdea2 - common: add support show-unmount-progress signal to GMountOperationDBus
(In reply to comment #34) > ::: daemon/gvfsjobunmount.c > @@ +141,3 @@ > + g_debug ("gvfsjobunmount progress clear"); > + > + message = g_strdup_printf (_("Connection to %s closed"), > > wording: GVfs backends are not always network services, suggesting using > something neutral like "%s has been unmounted" Okay, fixed now. > @@ +160,3 @@ > + /* TODO: report estimated bytes and time left */ > + g_mount_source_show_unmount_progress (op_job->mount_source, > + _("Writing data"), -1, -1); > > In the future, we could report umount/sync progress for some backends. For > example, archiving backend might be doing archive close which may be essential > to ensure archive integrity and could take quite a long time. We would have to > call this handler multiple times then. Sure, there shouldn't be any problem in calling this many times (and it's something that can't happen ATM as far as I see), but I tweaked a bit the code to make it more robust to this case. > @@ +180,3 @@ > return; > > + unmount_progress_start (op_job); > > You're calling unmount_progress_start() and unmount_progress_clear() only from > within run(). However, some backends might be actually doing real unmount > within try_unmount even if it should be used only for quick operations. And > since there's 1.5 sec timeout, it shouldn't disrupt the UI if it returns > quickly and then falls back into run(). > > Also, there are even more occurrences of try_unmount calls in the file for the > force unmount functionality. Yeah....though my understanding of the code here was that try_unmount() is allowed to block though, since it shouldn't take a long time (if it did, the backend would have returned FALSE and then unmount() would have run in a thread), so there's little point in running the timeout if the code is going to block anyway. I don't think this is really needed by any backend ATM, so I left it as it was in the next patch.
Created attachment 218755 [details] [review] daemon: implement show-unmount-progress in GVfsJobUnmount --- Address Tomas' review.
Review of attachment 218755 [details] [review]: (In reply to comment #37) > Yeah....though my understanding of the code here was that try_unmount() is > allowed to block though, since it shouldn't take a long time (if it did, the > backend would have returned FALSE and then unmount() would have run in a > thread), so there's little point in running the timeout if the code is going to > block anyway. > I don't think this is really needed by any backend ATM, so I left it as it was > in the next patch. That's right, it's not needed for the moment, it could only serve as a safety fallback for badly written backends. Okay then, leave it as is, time will tell if it's needed or not, it's a trivial change after all. Patch looks good, please commit.
Attachment 218755 [details] pushed as 39a92eb - daemon: implement show-unmount-progress in GVfsJobUnmount Thanks Tomas, closing this as FIXED.