Created attachment 349197 [details] [review]
Disable loop auto-clear before operations needing unmount

(added bugzilla link)

Review of attachment 349197 [details] [review]:

Thanks for the patch!

The udisks_loop_call_set_autoclear_sync should be fast, however, I think we should rather use async methods if possible to prevent UI freezes...

Is the auto-clear property restored by kernel after each mount operation? Shouldn't be restored the previous setting?

Created attachment 349299 [details] [review]
Fix unmounting and probing of mounted volumes

I noticed that my code did not work if LUKS devices were unlocked and remembered that this is in fact a longer standing bug.
But these things belong all together since the part for LUKS locking was never executed because it depended on handing over only a luks pointer which was never the case. Also the return of the fixed function to deteced used volumes was never FALSE if pointers were given.
So this also fixes that the write benchmark was always preselected even if not intended by the source code.

Created attachment 349301 [details] [review]
Disable auto-clear before operations needing unmount

Thanks for the review. Good that you asked, autoclear is always set automatically again and it has the same result just as newly attaching the loop device, except one case: when the not-last partition is deleted.

Now I've added this corner case so that autoclear is only disabled if the last mounted volume has to be unmounted (depends on the other attached bug fix).
The patch also transitioned to async callbacks.

Review of attachment 349299 [details] [review]:

Thanks for the patch. It seems it fixes the problems you mentioned, however, I think it is needlessly complicated...

::: src/libgdu/gduutils.c
@@ +861,3 @@
+                           UDisksFilesystem **filesystem_to_unmount_out,
+                           UDisksEncrypted  **encrypted_to_lock_out,
+                           gboolean           encrypted_contents)

I suppose that the header change is not really needed.

@@ -963,3 @@
     }
 
- victim_found:

I think that the bugs you wanted to fix are in the "victim_found" code only and the rest of the changes you made are not really wanted....

@@ -969,3 @@
-      *filesystem_to_unmount_out = (filesystem_to_unmount != NULL) ?
-        g_object_ref (filesystem_to_unmount) : NULL;
-      ret = TRUE;

"ret" should be set independently of the input parameters to be sure that gdu_utils_is_in_use works as expected, so I would just set the ret variable outside of this if-statements.

@@ -971,3 @@
-      ret = TRUE;
-    }
-  else if (encrypted_to_lock_out != NULL)

encrypted_to_lock_out has to be set regardless of filesystem_to_unmount_out to be sure that it will be locked, so I would just remove the "else" here...

(In reply to Kai Lüke from comment #4)
> Created attachment 349301 [details] [review] [review]
> Disable auto-clear before operations needing unmount
> 
> Thanks for the review. Good that you asked, autoclear is always set
> automatically again and it has the same result just as newly attaching the
> loop device, except one case: when the not-last partition is deleted.

The question is whether we shouldn't restore the state, which was before the operation. E.g. user mount loop device, disable autoclear, run some operation, and consequently autoclear is enabled again after the operation. Shouldn't we analogically restore the autoclear state after the operation? But this is probably for another patch anyway...

Review of attachment 349301 [details] [review]:

Thanks! I don't have time to make a proper review right now, but at least some comment before weekend...

::: src/disks/gduwindow.c
@@ +3659,3 @@
+  if (loop != NULL &&
+      udisks_loop_get_autoclear (loop) &&
+      gdu_utils_get_in_use(window->client, window->current_object, FALSE) == 1)

Please fix the missing space before the opening parenthesis...

It is not probably needed to check whether is last or not, we can disable it anyway... or will it not be reenabled by kernel in that case?

Good, thanks for looking at it today!
I have to find out what is responsible for turning on the autoclear after mounting (quick search in udisks source and the loop manpages did not reveal it to me, manually mounting as root did not turn it on).
Yes, because no mount happens after deleting a partition I wanted to count the active parts and only deactivate autoclear if exactly only one is active (without counting the contents of an unlocked volume).

I'll think about how to remember the value if we don't want to rely on whatever is activating it… :D

Created attachment 349568 [details] [review]
(V2) Fix unmounting and probing of mounted volumes

Here is a version without the counting.

I wrote the first version with counting and excluding the content of encrypted devices because it is a good way to determine whether autoclear needs to be disabled before an action like deleting a partition. Anyway there are other ways possible, too. E.g. by remembering the value and only restoring it if another partition is still in use (done in the V2 patch).

Created attachment 349569 [details] [review]
(V2) Disable auto-clear before operations needing unmount

I moved the callback handling to disable autoclear into the ensure_unused function.

Only for the partition deletion the initial autoclear value is stored and only restored if meaningful. For all other cases I could store and restore it as well but since it seems to be just redundant I skipped it.

This does not use the counting but has the same effect as the previous patches.

Created attachment 349570 [details] [review]
(V2) Disable auto-clear before operations needing unmount

(forgot adding the last cases)

Review of attachment 349570 [details] [review]:

I think we have to find out at first, why is auto-clear automatically set in some case and not in another, before writing the patch...

Review of attachment 349568 [details] [review]:

Thanks! I like this patch, apart from the following:

::: src/libgdu/gduutils.c
@@ +973,2 @@
     }
+  if (encrypted_to_lock_out != NULL && filesystem_to_unmount == NULL)

I think that "else" is wrong here, because encrypted_to_lock_out should be set (at least to NULL) regardless of filesystem_to_unmount, so, "filesystem_to_unmount == NULL" is also wrong, because it has more-or-less same effect...  

filesystem_to_unmount and encrypted_to_lock are set exclusively thanks to "goto", so there shouldn't be problems with leaking references...

Vratislav, don't you know how auto-clear works and why is automatically set in some cases and not in other?

(In reply to Ondrej Holy from comment #14)
> Vratislav, don't you know how auto-clear works and why is automatically set
> in some cases and not in other?

No idea, but I don't see anything in udisks doing this. So it must be kernel.

Created attachment 349666 [details] [review]
(V2) Fix unmounting and probing of mounted volumes

Yes, that's right, I removed it :)

Created attachment 349667 [details] [review]
(V2) Disable auto-clear before operations needing unmount

(no change, just different time)

Do you think kernels might differ in setting autoclear after mount operations?

(In reply to Kai Lüke from comment #18)
> Do you think kernels might differ in setting autoclear after mount
> operations?

Could be. There was recently a kernel where loop operations caused kernel panics. So this is not rock-solid/stable as it might look.

I guess the mount binary is setting it up, not the kernel – but I might missinterpret what I found in libmount/src beginning from mnt_context_prepare_srcpath → mnt_context_setup_loopdev

Hm, no, strace shows it's rather udisks and not kernel or mount.

I run strace while formatting a loop device, first autoclear is off, then mkfs comes, then it's mounted and finally:
[pid of udisks] ioctl(16, LOOP_GET_STATUS64, {lo_offset=0, lo_number=0, lo_flags=LO_FLAGS_PARTSCAN, lo_file_name="/var/tmp/….img", ...}) = 0
[pid of udisks] ioctl(16, LOOP_SET_STATUS64, {lo_offset=0, lo_number=0, lo_flags=LO_FLAGS_AUTOCLEAR|LO_FLAGS_PARTSCAN, lo_file_name="/var/tmp/…" […]
= 0

so initially lo_flags is without AUTOCLEAR and then it's set which I think comes from udiskslinuxloop.c

udisks_linux_loop_update:
↓ which calls
disks_loop_set_autoclear (UDISKS_LOOP (loop),
                          g_udev_device_get_sysfs_attr_as_boolean (device->udev_device, "loop/autoclear"));

handle_set_autoclear (…, arg_value):
↓ which calls
loop_set_autoclear(arg_value):
ioctl (fd, LOOP_GET_STATUS64, &li64)
…
if (value)
    li64.lo_flags |= LO_FLAGS_AUTOCLEAR;
  else
    li64.lo_flags &= (~LO_FLAGS_AUTOCLEAR);
… ioctl (fd, LOOP_SET_STATUS64, &li64)


So now I wonder why g_udev_device_get_sysfs_attr_as_boolean seems to return TRUE but ioctl LOOP_GET_STATUS has no sign of AUTOCLEAR.

(In reply to Kai Lüke from comment #21)
> Hm, no, strace shows it's rather udisks and not kernel or mount.
> 
> I run strace while formatting a loop device, first autoclear is off, then
> mkfs comes, then it's mounted and finally:
> [pid of udisks] ioctl(16, LOOP_GET_STATUS64, {lo_offset=0, lo_number=0,
> lo_flags=LO_FLAGS_PARTSCAN, lo_file_name="/var/tmp/….img", ...}) = 0
> [pid of udisks] ioctl(16, LOOP_SET_STATUS64, {lo_offset=0, lo_number=0,
> lo_flags=LO_FLAGS_AUTOCLEAR|LO_FLAGS_PARTSCAN, lo_file_name="/var/tmp/…" […]
> = 0
> 
> so initially lo_flags is without AUTOCLEAR and then it's set which I think
> comes from udiskslinuxloop.c
> 
> udisks_linux_loop_update:
> ↓ which calls
> disks_loop_set_autoclear (UDISKS_LOOP (loop),
>                           g_udev_device_get_sysfs_attr_as_boolean
> (device->udev_device, "loop/autoclear"));
> 
> handle_set_autoclear (…, arg_value):

Are you sure that the udisks_loop_set_autoclear() call above triggers handle_set_autoclear(). I think it cannot be like that because otherwise this would be cycling as handle_set_autoclear() does the ioctls which would/should trigger udisks_linux_loop_update() again.

handle_set_autoclear() is a handler for the SetAutoclear() method.

Also, there should be no ioctls if there's the loop/autoclear file under /sys.

Yes, it's different, udisks_loop_set_autoclear only sets the property, so udisks works correct. ioctl is used because the /sys…loop/autoclear file is not writeable.

Riddle solved: it's gvfs which enables autoclear directly after mounting the newly discovered volumes (monitor/udisks2/gvfsudisks2volume.c)

So what does this mean? I think the current state of the patch is ok because restoring the autoclear value after the format operation is only possible when gvfs did mount it - but it takes care of setting autoclear anyway.

But for a non-gvfs environment I could also add restoring the value if the device is in use after the needed unmounts took place.

Thanks for the debugging and sorry for a late reply...

(In reply to Kai Lüke from comment #24)
> Yes, it's different, udisks_loop_set_autoclear only sets the property, so
> udisks works correct. ioctl is used because the /sys…loop/autoclear file is
> not writeable.
> 
> Riddle solved: it's gvfs which enables autoclear directly after mounting the
> newly discovered volumes (monitor/udisks2/gvfsudisks2volume.c)

Great catch! Hmm, however, I am not sure whether this is actually wanted, but that's another story...

> So what does this mean? I think the current state of the patch is ok because
> restoring the autoclear value after the format operation is only possible
> when gvfs did mount it - but it takes care of setting autoclear anyway.

Make sense, I will have to make some tests to be sure that I understand it correctly. I will try to make proper review during tomorrow...

Review of attachment 349666 [details] [review]:

Looks good to me, thanks!

Review of attachment 349667 [details] [review]:

The patch works good, but I think it needs some changes...

I think we should disable autoclear also when unmounting last partition - on_unmount_tool_button_clicked. Gnome disks provides UI for detaching loop devices, so it is pretty confusing that device disappears after clicking on unmount.

Apart from that, I think that devtab_loop_autoclear_switch should be insensitive if enabling leads to detach of loop device...

::: src/disks/gduerasemultipledisksdialog.c
@@ +300,3 @@
   gdu_window_ensure_unused (data->window,
                             object,
+                            FALSE,

This dialog is unused since commit dd6c6bd4df9cf71f6e4875125760c1700067fa2d. It would be nice to remove it completely in a separate patch...

::: src/disks/gduwindow.c
@@ +3465,3 @@
+  if (data->loop != NULL)
+    g_object_unref (data->loop);
+  g_object_unref (data->user_data);

I like reusing of the structure, but calling g_object_unref here is not really nice, although, it works in your case. You should add "GDestroyNotify user_data_notify;" struct member...

@@ +3534,3 @@
     {
       UDisksPartition *partition;
+      PartitionDeleteData *delete_data = g_slice_new0 (PartitionDeleteData);

As far as I know, it is not usual to call some function in the declaration part of code (it is ok to assign some statically allocated value, or another variable). You should declare the variable separately first:
PartitionDeleteData *data;

And call the following later:
data = g_slice_new0 (PartitionDeleteData);

@@ +3535,3 @@
       UDisksPartition *partition;
+      PartitionDeleteData *delete_data = g_slice_new0 (PartitionDeleteData);
+      delete_data->loop = NULL;

This is not needed, because g_slice_new0 already set this to 0.

@@ +3536,3 @@
+      PartitionDeleteData *delete_data = g_slice_new0 (PartitionDeleteData);
+      delete_data->loop = NULL;
+      if (data->loop != NULL && gdu_utils_is_in_use (window->client, window->current_object))

Can't be the autoclear restoration handled inside gdu_ensure_unused?

@@ +3558,3 @@
   GList *objects = NULL;
+  UDisksLoop *loop;
+  PartitionDeleteData *data = g_slice_new0 (PartitionDeleteData);

dtto

@@ +3571,3 @@
 
+  loop = udisks_object_peek_loop (window->current_object);
+  data->loop = NULL;

dtto

@@ +3637,3 @@
   gdu_window_ensure_unused (window,
                             window->current_object,
+                            FALSE,

I think that this should never be called for loop devices, so the value here is irrelevant...

@@ +3714,3 @@
   gdu_window_ensure_unused (window,
                             cleartext_object,
+                            FALSE,

I think that locking should not cause unmounting... so TRUE should be here...

@@ +4085,3 @@
+  GError *error;
+
+  error = NULL;

nitpick: I would set error to NULL directly when declaring...

@@ +4110,3 @@
                           GCancellable         *cancellable,
                           gpointer              user_data)
 {

Hmm, the gdu_window_ensure_unused machinery seems pretty redundant to me, but that's not your fault... but it would be nice to change this by a separate patch to use gdu_ensure_unused from gdu_window_ensure_unused in order to avoid the another autoclear handling here (or remove the gdu_window_ensure_unused functions at all).

@@ +4135,3 @@
+                                          ensure_unused_data);
+          return;
+        }

g_object_unref (loop);

::: src/disks/gduwindow.h
@@ +34,3 @@
 void            gdu_window_ensure_unused (GduWindow            *window,
                                           UDisksObject         *object,
+                                          gboolean              disable_autoclear,

I think that the new parameter is redundant, see my comments on places where is FALSE.

::: src/libgdu/gduutils.c
@@ +1287,3 @@
+                                          data);
+          return;
+        }

g_object_unref (loop);

::: src/libgdu/gduutils.h
@@ +79,3 @@
                               GtkWindow            *parent_window,
                               UDisksObject         *object,
+                              gboolean              disable_autoclear,

dtto

Created attachment 350205 [details] [review]
Remove obsolete multiple disk erase dialog

Created attachment 350206 [details] [review]
Use gdu_ensure_unused from gdu_window_ensure_unused

With all those suggestions a lot of callback occurrences can be avoided in the final fix :)

Created attachment 350207 [details] [review]
Disable auto-clear before operations needing unmount

The restore is now handled in ensure_unused, thanks for that idea.

Maybe one of you can tell how to solve utils_peek_top_loop_object without string manipulation through udisks alone? It is needed to go from a udisks object for /dev/loop0p2 to the object for /dev/loop0.

Best regards,
Kai

Review of attachment 350205 [details] [review]:

Looks good, thanks!

Review of attachment 350206 [details] [review]:

Looks good, thanks! Just some notes, but I will commit it as is...

::: src/disks/gduwindow.c
@@ +3943,2 @@
 static void
+ensure_unused_cb (UDisksClient *client,

nitpick: I would use GObject * here and doesn't cast the callback to GAsyncReadyCallback later, but that's up to you...

@@ +3948,3 @@
+  EnsureUnusedData *data = user_data;
+
+  data->callback (data->window, res, data->user_data);

nitpick: It would be nice to add "if (data->callback != NULL)", or something like g_return_if_fail for sure, but it should never happen currently, so it is ok...

@@ +3953,3 @@
 }
 
+/* ---------------------------------------------------------------------------------------------------- */

nitpick: I don't like separators like that, but I don't have any strong opinion about it, so ok...

Review of attachment 350207 [details] [review]:

Thanks, I like this, it is much simpler than the initial version...

::: src/disks/gduwindow.c
@@ +1603,2 @@
   gtk_switch_set_active (GTK_SWITCH (switch_), active);
+  gtk_widget_set_sensitive (switch_, sensitive);

Please make this change in a separate patch...

::: src/libgdu/gduutils.c
@@ +1037,3 @@
+UDisksObject *
+utils_peek_top_loop_object (UDisksClient *client,
+                            UDisksObject *object)

The following string-based implementation is not acceptable. But I think that something like the following should work (untested):
loop = udisks_client_get_loop_for_block (client, object);
ret = g_dbus_interface_get_object (G_DBUS_INTERFACE (loop));
g_object_unref (loop);

See udisks_client_get_loop_for_block implementation.

Please ping Vratislav if the suggested doesn't work...

@@ +1276,3 @@
+  GError *error;
+
+  error = NULL;

note: I would initialize it when declaring, but it is up to you, both styles are used in the gnome disks codes as far as I can tell...

@@ +1302,3 @@
+  if (gdu_utils_is_in_use (data->client,
+                           utils_peek_top_loop_object (client,
+                                                       UDISKS_OBJECT (g_list_first (data->objects)->data))))

Please use some helper variable like the following in order to avoid such complicated structures...
object = utils_peek_top_loop_object (client, UDISKS_OBJECT (g_list_first (data->objects)->data));
if (gdu_utils_is_in_use (data->client, object))

@@ +1307,3 @@
+                                      TRUE,
+                                      g_variant_new ("a{sv}", NULL), /* options */
+                                      NULL, /* cancellable */

Why not use data->cancellable?

@@ +1373,3 @@
+                                          FALSE,
+                                          g_variant_new ("a{sv}", NULL), /* options */
+                                          NULL, /* cancellable */

Why not use cancellable?

::: src/libgdu/gduutils.h
@@ +76,3 @@
                               UDisksObject *object);
 
+UDisksObject * utils_peek_top_loop_object (UDisksClient *client,

nitpick: You should not use space after the star:
UDisksObject *utils_peek_top_loop_object

Created attachment 350486 [details] [review]
Insensitive auto-clear switch for unused loop device

Created attachment 350489 [details] [review]
Disable auto-clear before operations needing unmount

Thanks, I used the function in a previous version but did not think about casting it now!

Hope I didn't miss anything ;)
Best regards,
Kai

Created attachment 350490 [details] [review]
Disable auto-clear before operations needing unmount

(casting with UDISKS_OBJECT instead of (UDisksObject *))

Review of attachment 350486 [details] [review]:

Thanks, looks good to me!

Comment on attachment 350486 [details] [review]
Insensitive auto-clear switch for unused loop device

Attachment 350486 [details] pushed as b13532c - Insensitive auto-clear switch for unused loop device

Attachment 349666 [details] pushed as 92e512b
Attachment 350205 [details] pushed as 7baa0e6

Review of attachment 350206 [details] [review]:

I am sorry, I advised you the wrong way... this patch is not needed, see the following review of attachment 350490 [details] [review].

::: src/disks/gduwindow.c
@@ -3976,3 @@
-gdu_window_ensure_unused_list_finish (GduWindow     *window,
-                                      GAsyncResult  *res,
-                                      GError       **error)

By the way, this function should not be removed, because it should be used in power_off_ensure_unused_cb... can you please prepare separate patch to use gdu_window_ensure_unused_list_finish in power_off_ensure_unused_cb?

Review of attachment 350490 [details] [review]:

I am sorry, that I did not realize it earlier. gdu_utils_ensure_unused is a just wrapper for gdu_utils_ensure_unused_list. gdu_utils_ensure_unused_list can be used alone (and it is actually used alone). So, we should move the autoclear handling into it. Consequently, the attachment 350206 [details] [review] is no more needed.

I would suggest adding something like "gboolean *last" parameter for gdu_utils_is_in_use_full and use that from unuse_data_iterate for checking whether autoclear disabling is really needed (this is similar what you suggested in your first patch here). 

I hope it makes sense to you, feel free to ping me on irc if you have some doubts about.

::: src/libgdu/gduutils.c
@@ +1275,3 @@
+                                      TRUE,
+                                      g_variant_new ("a{sv}", NULL), /* options */
+                                      data->cancellable, /* cancellable */

nitpick: the /* cancellable */ comment is pretty redundant

@@ +1340,3 @@
+                                          FALSE,
+                                          g_variant_new ("a{sv}", NULL), /* options */
+                                          cancellable, /* cancellable */

dtto

Created attachment 350660 [details] [review]
Use correct callback finish method

Created attachment 350661 [details] [review]
Disable auto-clear before operations needing unmount

As discussed this version disables the auto-clear flag only if the last volume is going to be unmounted. So restoring is not needed anymore.

Created attachment 350662 [details] [review]
Implicit auto-clear handling for unmount and lock buttons

Review of attachment 350660 [details] [review]:

Looks good, thanks!

Review of attachment 350662 [details] [review]:

Looks good, thanks!

Review of attachment 350661 [details] [review]:

::: src/libgdu/gduutils.c
@@ +897,3 @@
                           UDisksFilesystem **filesystem_to_unmount_out,
+                          UDisksEncrypted  **encrypted_to_lock_out,
+                          gboolean          *last)

I would rather rename it to last_out and use it analogically as filesystem_to_unmount_out/encrypted_to_lock_out. I see that your solution is a slightly more effective, but less readable...

@@ +1148,3 @@
+                            _("Error disabling autoclear for loop device"),
+                            error);
+      g_error_free (error);

I think that unuse_data_complete has to be used here, otherwise, it may end up in endless loop...

Created attachment 350762 [details] [review]
Disable auto-clear before operations needing unmount

Thanks for the review, this time I forgot to simlulate the error case ;)

Review of attachment 350762 [details] [review]:

Thanks! I meant the following with the phrase about analogical use. I think it is good practice to not touch the caller's variable unless we know the final value. Apart from that, I found one memory leak...

::: src/libgdu/gduutils.c
@@ +911,3 @@
 
+  if (last_out != NULL)
+    *last_out = TRUE;

gboolean last = TRUE;

@@ +986,3 @@
+              if (ret)
+                {
+                  *last_out = FALSE;

last = FALSE;

@@ +1007,3 @@
+              if (ret)
+                {
+                  *last_out = FALSE;

last = FALSE;

@@ +1013,1 @@
               g_object_unref (cleartext);

This needs to be called before break...

@@ +1020,3 @@
         }
     }
 

if (last_out != NULL)
  *last_out = last;

Created attachment 350846 [details] [review]
Disable auto-clear before operations needing unmount

Attachment 350660 [details] pushed as 86f414c - Use correct callback finish method
Attachment 350662 [details] pushed as 9c2a44f - Implicit auto-clear handling for unmount and lock buttons
Attachment 350846 [details] pushed as 4ea9260 - Disable auto-clear before operations needing unmount

Ah, ok, I see - thanks for finishing it!