Add Phillip Susi to the CC list

Ahh yes, when calling the deprecated resize apis, gparted will need to take steps to avoid blocking the ui.  It can either register the timer callback function with the resize apis, which the resize code will invoke periodically to update the ui ( the function would need to drain the main loop of events, and then return ), or it can fork a thread to call the resize api in, and have the main thread wait for it in a nested main loop.

Created attachment 287450 [details]
libparted FS resize callback timing log

Hi Phillip and Curtis,

Using libparted callbacks will make GParted much better but there will
still be some hangs in the UI.

My test case was:
 Create 10240 MiB FAT32 file system
 Create 5 GiB file using dd
 Copy linux GIT repo
 Delete 5 GiB file
 Resize in GParted to 5120 MiB

On my desktop with HDD the libparted file system resize step took
03m06s.

Libparted has timer callbacks in a few ped_file_system_*() functions of
which only ped_file_system_resize() is used by GParted.  From the timed
log I generated, there were 2 notable hangs:
1) ~9 seconds for ped_geometry_new()
2) ~42 seconds between last callback and ped_file_system_resize()
   returning.
This is much better than a 3 minute hang, but 42 seconds is still a long
time.

Thanks,
Mike

Created attachment 287451 [details] [review]
libparted FS resize callback timing debug 1

Patchset used to produced the previous debugging

Nice investigation!  Then yea, just go ahead and create a background thread to make the ped_file_system_xxx calls.

*** Bug 747131 has been marked as a duplicate of this bug. ***

Just a note from a duplicate bug, this is very confusing if the screen lock kicks in, since gparted then just appears as a blank grey window which the user is likely to interpret as a hang or crash.

Created attachment 307430 [details] [review]
Fix FAT resize hangs GUI (v1)

Thank you Mike, Phillip, and Chris for all your input on this issue.

When looking at the code I did not see a simple way to add threading to all libparted ped_file_system_xxx calls.  So I took a different tact and only added threading to the libparted ped_file_system_resize function call.  The addition of this one threading call appears to significantly reduce the UI unresponsiveness issue.

Note that I based my testing on Mike's steps in comment #3 (also outlined in the patch commit message).

I'm all ears if anyone has other suggestions on how to address this issue.

Hi Curtis,

> I'm all ears if anyone has other suggestions on how to address this
> issue.

I remember when I was first looking into this issue that having back
something like the old code which created a separate thread to apply
each operation would fully insulate the GParted UI from any OS or
library call which takes long enough for a user to notice the UI wasn't
responding.  This assumes that the external command reading code and
updating the output into the UI is thread safe.  I think it might be.
Does mean that the threading for copy blocks becomes redundant.
Possibly for external command execution too, but I'm not sure if that is
still needed somewhere, such as in View > File System Support > Rescan
For Supported Actions.  This approach would take a lot more effort to
understand.

Your approach of just solving the known worst offender of
ped_file_system_resize() is simple to understand and test.


Will be a couple of days before I can review and test your code
properly.

Thanks,
Mike

The patch looks great Curtis.  It is very clean and to the point.  The one issue I have with it is the fact that the background thread calls Gtk::Main::quit().  I can't remember why now, but when I was messing with threading before, I found I had to have the background thread post an idle event callback to the foreground and have that function call quit().  I thought it had something to do with the fact that each thread can have its own set of main loops, and so quit would quit the background thread's main loop, not the gui thread's.  Now that I glance at the gtk and glib code though, it appears it just maintains a single global list of main loops and quit always kills the most recent one, regardless of what thread it is in, which seems totally broken to me.  You might ask on the gtk mailing list about whether calling quit in a background thread is safe, or search it for an old discussion I think I had there the last time I messed with the threading in gparted.

Created attachment 307579 [details] [review]
Fix FAT resize hangs GUI (v2)

Thank you Mike and Phillip for your input.

Attached is an updated patch v2.  No code changes were made.  Instead
I removed the testing steps from the commit comment, and also added
that not only fat16/fat32 resizes are affected, but also hfs/hfs+.

For testing purposes the steps from the v1 commit message are listed
below:

1)  Create in GParted 10240 MiB FAT32 file system (/dev/sde2)

2)  Mount the file system (/mnt)
    # mount /dev/sde2 /mnt

3)  Create 5* GiB file using dd
    # dd if=/dev/zero of=/mnt/fiveGBfs.bin bs=1024 count=$((5*1024*1024))

    *Note that FAT32 supports maximum file size of 4 GiB only.

4)  Copy GIT repo (or simply lots of files)
    # git clone https://github.com/git/git.git /mnt/git

5)  Delete 5 GiB file
    # rm /mnt/fiveGBfs.bin

6)  Unmount the file system
    # umount /mnt

7)  Resize in GParted to 5120 MiB
    (Note responsiveness, or lack thereof, of GUI)


On the topic of thread safety, I believe that no other threads or
asynchronously spawned commands are running when the
ped_file_system_resize() function call is made in GParted_Core.  As
suggested by Phillip, I will search to see if I can find out more
regarding thread safety.  I will report back here with what I find.

As an aside, I tested growing a fat16 file system from 512 MiB to 8192
MiB and was correctly shown the libparted message regarding conversion
from fat16 to fat32.  To me this means that the libparted
ped_exception_handler() appears to be working with this newly added
thread.

Thanks,
Curtis

Created attachment 307583 [details] [review]
Fix FAT resize hangs GUI (v3)

As per Phillip's recommendation I created a new patch v3 that changes
one line
   FROM:
      Gtk::Main::quit();
   TO:
      g_idle_add( (GSourceFunc)_mainquit, NULL );

This aligns the thread_lp_ped_file_system_resize() code with the way
the set_devices_thread() works.

Patch v3 also includes the changes to the commit message as mentioned
in comment #11.


I don't claim to be an expert on threading.  And with the trouble
we've encountered with past changes to threading I think it prudent to
model new code after that which we know works.  Hence the change
Phillip recommended.  I also agree with Mike's observation that a
global change more akin to the original (prior to 0.15.0) threading is
more risky, so this smaller change has a reduced scope that should be
easier to understand and test.

For patch v3 in addition to successful resizing, and seeing prompt for
fat16 to fat32 conversion testing I also tested cancel functionality
with the following observations:

A) Without patch v1/2/3, it is impossible to invoke the cancel
   functionality because the GUI is frozen.  The GUI unfreezes when
   the ped_file_system_resize function call completes.

B) With patch v1/2/3, a user can choose Cancel, then Force Cancel,
   then Cancel Operation.  The GUI continues to be responsive; however
   the ped_file_system_resize thread continues to execute until it
   naturally finishes.  During this time the GUI button Force Cancel
   is greyed out.  At the end of the entire operation the file system
   is successfully resized, however the indication to cancel is shown
   in the gparted details.  This is not entirely correct, but I think
   is no worse than without the patch.

So both with and without patch v1/2/3 a user cannot actually cancel a
resize operation in the ped_file_system_resize function call.  The key
difference is that with the patch the GUI remains responsive.

In summary, I believe that patch v3 aligns with Phillips
recommendation and also permits the GUI to remain responsive while
resizing fat16/fat32/hfs/hfs+ file systems.

As always I am open to further suggestions.

Curtis

Nice.. I was actually thinking that the message was a bit long and didn't need the testing steps.  FYI, you can use the git notes feature to add extra information like that and it will be displayed in the patch following a scissors line ( <----> ) so that reviewers can see it, but it won't be part of the commit message in the main repo when pushed.

Now if only I could remember why I decided before that it wasn't safe to call Gtk::Main::quit() from a background thread... will have to search the mailing list archives.

Hi Curtis,

I've successfully tested resizing FAT16, FAT32, HFS and HFS+ across a
range of distros between CentOS 5 to Ubuntu 14.10.  As such I'm ready to
commit the change.

Do you mind if I make some white space layout changes to your patch?
To put commas immediately after parameters, rather than at the start of
the next parameter on the next line.
From this:
    function_name( parameter1
                 , parameter2
                 );
To this:
    function_name( parameter1,
                   parameter2 );
I'm trying to make the the GParted code use a more common layout style.

Thanks,
Mike

Hi Mike,

> Do you mind if I make some white space layout changes to your patch?

Sure, feel free to change the alignment.

There are many different coding styles, and I know that GParted contains several different ones.  I like when the parameters line up, so either coding style works for me.

Curtis

This patch has now been pushed to the public GParted git repository:

Add libparted ped_file_system_resize thread to avoid blocking GUI (#737022)
https://git.gnome.org/browse/gparted/commit/?id=1561d1ae7ec1bdad5c378134fc4138f346615787

Thanks,
Mike

This enhancement was included in the GParted 0.23.0 release on August 3, 2015.