GNOME Bugzilla – Bug 694622
Add support for online resize
Last modified: 2013-12-09 18:13:27 UTC
Linux now has the ability to change the size of mounted partitions. Patches to add this ability to libparted can be found on the parted-devel mailing list. I have backported them to Ubuntu 13.04 and am now uploading it to my ppa, which you can use for testing. The attached patch adds support to gparted.
Created attachment 237314 [details] [review] 0001-Add-online-resize-support-694622.patch
Created attachment 255254 [details] [review] Add online resize support (v2) Just updated Phillip's patch so that it applies to current GParted HEAD.
Created attachment 255255 [details] File System Support dialog before and after screen shot Hi Curtis and Phillip, Initial UI questions: 1) The File System Support dialog becomes wider than our 800 pixels wide target. Also dialog looks a bit odd with wide columns for Online Grow and Online Shrink. Some ideas: a) Use two row headers like this: .... | Online | Online | ... | Grow | Shrink | b) Reorder columns like this: ... | Grow | Online | Shrink | Online | ... c) Have 2 ticks for Grow and Shrink, first for offline and second for online. 2) At the moment the extra columns in the Fiel System Support dialog are dependant on GParted being configured with --enable-online-resize. GParted normally shows all columns regardless and displays Apply and Cancel icons depending on capabilities. I think that for (1) choice (b) is simple and will do and for (2) GParted should always show the new online grow/shrink columns/icons. Thanks, Mike
'Just a quick comment on the UI question. 1) Another possible way to handle grow/shrink headers d) Add a three-way indicator. Current indicator is "Yes|No". Possible have something like "Both|Offline|No" My sample wording is not great, but a three-way indicator would remove the need for making the support dialog grow too wide. Curtis
The question is how to show a three-way indicator. If the columns had "Yes|No" text instead of the icons, then it would be simple... use "Offline|Online|No". I suppose only this column could be changed to show text, but then it would look out of place with all of the rest being icons. What do you think of doing away with the icons and using text instead?
Hi Phillip and Mike, When using a web browser I really detest having to scroll sideways to view a web site. As such I believe we should choose a solution that avoids the need to scroll sideways to view the File System Support dialog on an 800x600 screen (our minimum screen resolution target). Regarding text versus icons, there is a text "Yes | No" sample available for view at Wikipedia. See: https://en.wikipedia.org/wiki/GParted#Supported_features Unfortunately I have not come up with good text words to use for a potential three-way indicator solution. At the moment I think Mike's proposed solution in comment #3 might be our best path forward. Specifically: 1) b) Reorder columns like this: ... | Grow | Online | Shrink | Online | ... and 2) GParted should always show the new online grow/shrink columns/icons Ideally for 1(b) it would help if we could somehow visually pair these columns together so that it is obvious that "| Grow | Online |" are related, and "| Shrink | Online |" are related. Please note that we might need another column for online labelling in the near future. See: Bug #600496 - Allow changing ext2/3 label without unmounting partition Please also note that Mike is actively working on a solution to address a height issue with the File System Support Dialog. See: Bug #342682 - too much information in 'features' dialog Curtis
The text is simply to change the word "Yes" to "Online" when it is available. Thus you have a 3 way indicator in the exiting column rather than new columns. In other words you would see: Grow | Shrink Yes | Yes No | No Yes | No Online | Yes Online | Online
My concern with the word "Online" is that it does not imply that the operation can be performed both Offline and Online. Perhaps something like "Online" or "Online+" would work if we also add a note in the legend that this works both online and offline. I think it would also help to use either background colour or text colour to enhance the indication if the feature is not supported, or supported offline and/or online. I would suggest green to indicate support, and red to indicate not supported.
Created attachment 256014 [details] File System Support dialog screen shot options Hi Curtis and Phillip, I've coded a couple of the File System Support dialog UI options I suggested to see what they look like. Here is a screen shot showing the following options: * Original (632 pixels wide) * Extra columns with long headers (818 pixels wide) (code from comment #2) * Extra columns with short headers (736 pixels wide) (comment #3, option 1.b) * Second ticks in existing columns (651 pixels wide) (comment #3, option 1.c) I think that I prefer option (1.c). What do you prefer? Thanks, Mike
Hi Phillip, Of the three proposed options, I prefer option (1.c). To help clarify that the groups of two check marks are in the same column, it might help to add gridlines to the chart. Did you investigate the text option of using "Yes|No|Online"? Curtis
I'm not sure whether it is better to keep graphical checks instead of a Yes|No|Online text, but option C should be fine.
Hi Phillip, Can you tell me what distro / packages has parted / libparted online resizing so I can give it a test. Thanks, Mike
Debian testing and Ubuntu saucy. It went in parted 2.3-14.
Hi Mike and Phillip, Thanks Mike for coding and building the sample screen shots in comment #10. It seems that we all agree on (1.c) so let's go with that option. Curtis
Created attachment 256135 [details] gparted_details.htm log file from failed online ext2 grow operation Hi Phillip and Mike, Using Debian Testing (Jessie) in a virtual machine, I was able to successfully perform the following operations: Online grow of btrfs, ext3, ext4, jfs, and xfs. Online shrink of btrfs. Since the GUI also permits online resize of ext2, I tried growing a mounted 128 MiB ext2 partition to 256 MiB and received the following error (also contained in the attached gparted_detail.htm log file). resize2fs /dev/sdb1 Filesystem at /dev/sdb1 is mounted on /mnt/mymnt; on-line resizing required old_desc_blocks = 1, new_desc_blocks = 1 resize2fs 1.42.8 (20-Jun-2013) resize2fs: Kernel does not support online resizing The kernel version is: $ uname -a Linux jessie 3.10-3-686-pae #1 SMP Debian 3.10.11-1 (2013-09-10) i686 GNU/Linux Does ext2 need an even more recent version of the kernel to permit online resizing? Curtis
Does the filesystem have the resize_inode feature enabled? Check with tune2fs -l.
I created the 128 MiB ext2 partition using GParted. The output from tune2fs -l is as follows: tune2fs 1.42.8 (20-Jun-2013) Filesystem volume name: ext2-test Last mounted on: <not available> Filesystem UUID: 18132b38-ef80-410c-8a93-20700977f5a0 Filesystem magic number: 0xEF53 Filesystem revision #: 1 (dynamic) Filesystem features: ext_attr resize_inode dir_index filetype sparse_super Filesystem flags: signed_directory_hash Default mount options: user_xattr acl Filesystem state: clean Errors behavior: Continue Filesystem OS type: Linux Inode count: 32768 Block count: 131072 Reserved block count: 6553 Free blocks: 125381 Free inodes: 32757 First block: 1 Block size: 1024 Fragment size: 1024 Reserved GDT blocks: 256 Blocks per group: 8192 Fragments per group: 8192 Inodes per group: 2048 Inode blocks per group: 256 Filesystem created: Mon Sep 30 18:55:46 2013 Last mount time: n/a Last write time: Mon Sep 30 18:55:46 2013 Mount count: 0 Maximum mount count: -1 Last checked: Mon Sep 30 18:55:46 2013 Check interval: 0 (<none>) Reserved blocks uid: 0 (user root) Reserved blocks gid: 0 (group root) First inode: 11 Inode size: 128 Default directory hash: half_md4 Directory Hash Seed: 74feb88e-acf1-4ef9-a19a-4ad1a3563b16
Strange, it worked for me on my server at work, but now doesn't. It seems to be a problem with the ext2 driver. If you mount the ext2 filesystem using the ext4 driver it works fine. If the kernel is built without the ext2 or 3 drivers then the ext4 driver will automatically handle those filesystems. I've been meaning to push for this to be done.
Ahh yes, I had forgot my server is running a custom built kernel where I did disable ext2 and ext3, so the ext4 driver automatically takes over for them.
Hi Phillip, Is it possible to enhance the patch to detect this ext2 online resize problem so that our users will avoid this problem? Curtis
Not that I can see. We can probably just disable it for ext2 since nobody should really be using it still anyway.
It is possible to detect which ext kernel driver is mounting an ext file system like this: # mkfs.ext2 /dev/sda11 # mount /dev/sda11 /mnt/1 # fgrep /dev/sda11 /proc/mounts /dev/sda11 /mnt/1 ext2 rw,relatime,errors=continue 0 0 ^^^^ # umount /dev/sda11 # mount -t ext4 /dev/sda11 /mnt/1 # fgrep /dev/sda11 /proc/mounts /dev/sda11 /mnt/1 ext4 rw,relatime,barrier=1,data=writeback 0 0 ^^^^ The manual page for resize2fs says only the kernel module for ext3 and ext4 can resize online. It doesn't say that the ext4 module can mount the ext2/3 file systems too. So a user could easily assume that ext2 can't be grown online at all. I don't think any distributions have yet stopped building ext2/3 kernel modules, although I think Fedora has discussed it. So by default ext2 file system will be mounted by the ext2 kernel module and not be online growable. Also GParted currently rediscovers capabilities only when [Rescan for Supported Actions] is clicked, but merely unmounting ext2 and remounting using the ext4 module would allow online grow to be possible. For these reasons I think it best to keep it simple and never mark ext2 as online growable. I have also confirmed that CentOS 5.9 with kernel 2.6.18 using resize2fs from e2fsprogs-1.39 can online grow an ext3 file system so we shouldn't need any kernel >= specific version as nilfs2 does.
Also I don't think the /proc/mounts check works when you have a kernel without the ext2/3 modules and ext4 takes over automatically.
> For these reasons I think it best to keep it simple and never mark ext2 > as online growable. I agree. Let's disable online ext2 grow for now. We should also place a mention of the --enable-online-resize option in the README with the other configure options.
(In reply to comment #23) > Also I don't think the /proc/mounts check works when you have a kernel without > the ext2/3 modules and ext4 takes over automatically. FYI: The kernel reports the driver owning the mount point so a kernel without ext2/3 will still show ext4 as the driver owning the mount point like it does for every other type of file system. e.g. # grep sda /proc/mounts /dev/sda3 / ext4 rw,relatime,barrier=1,data=ordered 0 0 /dev/sda1 /boot ext4 rw,nosuid,nodev,relatime,barrier=1,data=ordered 0 0 /dev/sda5 /home ext3 rw,nosuid,nodev,relatime,errors=continue,barrier=1,... /dev/sda10 /mnt/1 fuseblk rw,relatime,user_id=0,group_id=0,allow_other,... /dev/sda11 /mnt/2 xfs rw,relatime,attr2,noquota 0 0 /dev/sda12 /mnt/3 jfs rw,relatime 0 0 /dev/sda13 /mnt/4 reiserfs rw,relatime 0 0 /dev/sda14 /mnt/5 hfsplus rw,relatime,umask=22,uid=0,gid=0,nls=utf8 0 0
I have done some more research. Online resizing of partitions requires: 1) Kernel >= 3.6 for BLKPG_RESIZE_PARTITION support [1] [2] 2) parted with patches to issue ioctl() of BLKPG_RESIZE_PARTITION [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c83f6bf98dc1f1a194118b3830706cebbebda8c4 [2] http://kernelnewbies.org/Linux_3.6 While online resizing of btrfs, ext2/3/4, jfs and xfs has been available for a long time in the kernel, resizing of partitions requires a new kernel. GParted currently equates one file system with one partition and doesn't manage file systems inside LVM volumes. I therefore think that online grow and shrink should be dependant on kernel >= 3.6. (See use of kernel_version_at_least in nilfs2.cc for an example). Is there anyway to test that parted provides online resize so that it doesn't have to be manually defined at build time?
I think that when ext4 takes over automatically for ext2 and ext3, it registers those filesystem names and has them just point to its own methods, thus when you mount an ext2 filesystem, /proc/mounts thinks it is using the ext2 driver. I suppose the runtime kernel version check wouldn't hurt but unfortunately there's no way to check libparted's support other than to try and see if it fails, at least until it gets into mainline parted and then maybe we can do a runtime version check.
I agree that we should add a check for kernel >= 3.6. Regarding the parted version, we should ensure that the README indicates that the --enable-online-resize also requires a specific patch to parted.
Hi Phillip, Can you explain your thinking for the use of #ifdef ENABLE_ONLINE_RESIZE conditional code. Much of the added code uses it but some doesn't. I would much prefer #ifdef ENABLE_ONLINE_RESIZE was only used the absolute minimum because it's just an extra level of conditions to understand when reading the code which doesn't need to be there. I think the absolute minimum is just around the setting of fs.online_grow/ shrink in get_filesystem_support() for each file system. Also please use FS::EXTERNAL not GParted::FS::EXTERNAL. The whole of the GParted code in already in the GParted name space. Thanks, Mike
It is used everywhere it makes a material difference. It is a habit from the kernel: if the feature is disabled, then there's no sense wasting space and time having code or variables around that won't be used. It also makes it easier to remove it in the future if need be. It looks like jfs.cc was already referring to everything using the explicit GParted:: scope for some reason, so I guess I just followed suit for consistency.
Created attachment 256329 [details] [review] Dialog update (draft 1) Here's the work-in-progress code I used to create the second check marks in the File System Support dialog. It's currently based on top of master. When we commit Phillip's patch I will update this to just make the needed changes in the DialogFeatures class.
Done a bit of testing and LVM2 PVs can also be resized, both grown and shrunk, online.
Your patch did not apply cleanly on top of mine, largely due to the fact that you seemed to duplicate some of the changes I made, minus the #ifdefs. It also added some trailing whitespace errors. I have merged the two and am happy with the result. One further thought is that rather than have a second check mark for online support, I think a third icon would be preferable. Maybe a star. But as it is, this looks good and works well, so hopefully it can be applied for the next release.
Created attachment 258509 [details] [review] 0001-Add-online-resize-support-694622.patch
Just to let you know I am working on testing and reviewing this. Mike
Thanks for the heads up Mike. From a quick look, I think the patch set could be enhanced further by adding a check for minimum required kernel version, and a note added to the README about the new --enable-online-resize configure option. I haven't done any testing yet on Phillip's update patch in comment #34. Curtis
Created attachment 259951 [details] [review] 0001-Add-online-resize-support-694622.patch Cleaned up some more trailing whitespace errors and added the kernel version runtime check.
Hi Phillip, I like the way fs.online_grow/shrink = fs.grow/shrink. Simple and understandable and handles tool dependency. Online resize now depends on kernel >= 3.6. Good. I've tested this on Ubuntu 13.10 (kernel 3.11 with online ptn resize capability and parted 2.3 with online resize distro patches). Required changes: 1) Remove all other #ifdef ENABLE_ONLINE_RESIZEs. Those except around setting fs.online_grow/shrink and 4) below. There not necessary; make the code harder to read; performance of an extra if statement is not a problem; keeps the UI identical to with and without #define. 2) Online resize of nilfs2 does a normal mount-resize-unmount cycle and fails. 3) Add a note into the README file as per Curtis' comment #36. 4) Make saved details write that ENABLE_ONLINE_RESIZE is set. See Dialog_Progress::on_save() and USE_LIBPARTED_DMRAID. Explains why, when reading saved details, resizing didn't include a check. Discussion points: A) Should the manual mention about online resize? The manual doesn't say anything about the operations being performed offline at the moment. Possibly not. B) Question of using a single icon for online and offline resize, such as star, as per Phillip's comment #33? One of us should knock up some code and produce a screen shot to compare with my others from comment #9. C) Should the name of the operations in the Pending Operations pane be "Online Grow ..." and "Online Shrink ..." to make it clearer to the user? Rather than just "Grow ..." and "Shrink ...". Code is in OperationResizeMove::create_description(). I think so. Thanks, Mike
Created attachment 260032 [details] File System Support dialog screen shot options (v3) Hi Curtis and Phillip, Here's a picture of Phillip's suggestion of a using a star from comment #33 to compare with all the others from comment #9. What do you think between star and second check? Thanks, Mike
I would really rather keep the #ifdefs. I think it is only the one spot that it makes it a bit more difficult to read, and it isn't that bad, and if you really need to, most editors have a way to hide them with the inactive branch. I also intended for the ui to look different depending on whether the configure option is enabled or not. This makes it easy to tell the difference between whether online support was not compiled in, or not detected at runtime, and there's no sense showing the user the extra key information when it is not available. Ooops on the nilfs2... fixing it now. Good point about the saved details. The manual probably should describe online resize. I'm not sure that changing the name of the operation adds anything since the user generally doesn't care whether it is online or not, as far as they are concerned grow is grow and online or not is just a matter of when the operation is allowed or not.
Created attachment 260055 [details] [review] 0001-Add-online-resize-support-694622.patch I added the log entry, the README entry, and fixed nilfs2. I can't figure out how to get the docs to build though. They aren't being built as part of a full make, and when I go into the help directory and run make html, or pdf, it just says nothing needs done, and looking at the Makefile, it doesn't *do* anything for those targets.
Hi Phillip and Mike, 'Just started looking at the patch in comment #41. Thanks for adding the README entry. ENHANCEMENT SUGGESTIONS: 1) From a quick look, it would appear that the "dnl" comment in configure.ac for checking for online resize incorrectly refers to USE_LIBPARTED_DMRAID. 'Probably a simple copy/paste mistake. ;) DOC BUILD CLARIFICATION: The docs (./help/ subdirectory) _should_ be built by default when running the usual ./autogen.sh; ./configure --enable-online-resize; make; sudo make install. If the last step is omitted (sudo make install), then the docs are built, but are not installed the default location for yelp to see. In this case you can view the docs by providing the absolute path to yelp. E.g., yelp /home/gedakc/workspace/gparted/help/C/gparted.xml Phillip, does that explanation help or is something else occurring? ICON QUESTION: The current icons are part of the standard libraries included with distros and the look and feel vary from distro to distro. As such I think it best to either use all stock icons, or to make all custom icons. Mike, in your tests did you use a stock icon, or include a custom one? One thing I struggle with when using the star icon is that it does not have an obvious meaning like a check mark and an "X" do. I think I prefer the double check mark, but if both of you like the star better then perhaps we should make the legend display mandatory (i.e., can't be collapsed). Curtis
Hi Phillip, The patch in comment #41 does not compile for me. A snippet of the error message follows: nilfs2.cc: In member function ‘virtual bool GParted::nilfs2::resize(const GParted::Partition&, GParted::OperationDetail&, bool)’: nilfs2.cc:196:48: error: ‘mount_point’ was not declared in this scope make[2]: *** [nilfs2.o] Error 1 Would you be able to fix and re-post the patch including the enhancement suggested in comment #42? Curtis
Hi Curtis, Re: ICON QUESTION I used stock GTK ABOUT icon to match the others. Yes there is loads of variation between distros. (In reply to comment #42) > One thing I struggle with when using the star icon is that it does not have an > obvious meaning like a check mark and an "X" do. I think I prefer the double > check mark, ... Agreed. Lets stick with double check mark. Thanks, Mike
Ohh, I thought that was the source file and the build process had to convert it into other formats, including html to go on the web, and some other format for the online help system. I suppose the double check mark is clear and simple, it just takes up a bit more horizontal space than a star and I know you were worried about the horizontal space.
Created attachment 260059 [details] [review] 0001-Add-online-resize-support-694622.patch Cleaned up and added to the documentation.
Hi Phillip and Curtis, Re: #ifdefs (In reply to comment #40) > I also intended for the ui to look different depending on whether the configure > option is enabled or not. This makes it easy to tell the difference between > whether online support was not compiled in, or not detected at runtime, and > there's no sense showing the user the extra key information when it is not > available. This is understandable. I prefer that the UI was the same in both cases and just either show 2 check marks or 1. (At the moment the only difference in the UI is a slightly wider "Grow" and "Shrink" columns). Curtis what do you prefer? (In reply to comment #40) > I would really rather keep the #ifdefs. I think it is only the one spot that > it makes it a bit more difficult to read, and it isn't that bad, and if you > really need to, most editors have a way to hide them with the inactive branch. This is a show stopper for me. (1) It introduces conditional code which does the same as the normal code and more. It's therefore unnecessary. (2) It doubles the combination of tests to any change to the relevant area of code. (3) Code which isn't in use by default has a tendancy to bit rot. Therefore remove #ifdefs from: include/DialogFeatures.h include/Utils.h src/Dialog_Partition_Resize_Move.cc src/Win_GParted.cc File system specific resize() methods for btrfs, jfs, nilfs2, xfs. Thanks, Mike
Created attachment 260143 [details] [review] 0001-Add-online-resize-support-694622.patch Ok, I removed the #ifdefs.
(In reply to comment #47) > (In reply to comment #40) > > I also intended for the ui to look different depending on whether the configure > > option is enabled or not. This makes it easy to tell the difference between > > whether online support was not compiled in, or not detected at runtime, and > > there's no sense showing the user the extra key information when it is not > > available. > > This is understandable. I prefer that the UI was the same in both cases > and just either show 2 check marks or 1. > > (At the moment the only difference in the UI is a slightly wider "Grow" > and "Shrink" columns). > > Curtis what do you prefer? My preference is the same. Let's keep the UI the same in both cases and just either show 2 check marks or 1.
Hi Phillip, I was just looking through the GParted help manual updates and see some changes I would like to make. Specifically get rid of all the tabs, fix some typo's, and re-word some sections. As such feel free to either drop the help manual changes from the patch in comment #48 and I will do a separate patch, or place the changes in a separate commit and I will update from there. Note that I tend to be a bit of a control freak regarding the help manual. :) Curtis
Hi Mike, I have started reviewing the patch in comment #48 and have noticed a few minor changes I'd like to make. However, I want to make sure that we don't step on each other's toes while reviewing. Are you reviewing the patch and making minor changes? If so I will hold off until you have finished. I can then review your updated patch. If not, then I will proceed with reviewing and then post an updated patch set. That way you can review to see if I missed anything you wished changed. For this patch set I would like both of us to have had a chance to review before one of us commits this to the master branch. Curtis
I'm not reviewing yet. Go ahead and make your changes. I'll review later.
Created attachment 260393 [details] [review] Online resize support patch set v8 (or so ;-) Thank you Phillip and Mike for your work on this patch. In an effort to minimize the introduction of any new bugs, I have pored over this patch and extensively compared it to the original code. This has led me to make several changes. These changes were made in an effort to simplify the logic, preserve the previous behaviour, and more closely match the previous coding style (especially to get rid of lines marked for change only because a space was removed between the end of a command and the semicolon). The end result is that this new patch set should have much fewer changed lines. PATCH CHANGE SUMMARY -------------------- Summary of changes to patch from comment #48: README: Reword --enable-online-resize configure description configure.ac: Align new option in final config and add question mark Add comment for enable-online-resize check help/C/gparted.xml: Remove updates (I will update help manual separately) include/DialogFeatures.h: Move online_grow immediately after grow include/Utils.h: Re-add blank line before Byte_Value MIN src/DialogFeatures.cc: Move online_grow immediately after grow src/Dialog_Partition_Resize_move.cc: Re-organize and simplify Set MIN logic src/Win_GParted.cc: Add comment and remove tab from blank line src/btrfs.cc: Change fs.online_shrink to be set to fs.shrink, not fs.grow Remove do_unmount, add success check to unmount check src/ext2.cc: Minor whitespace matching src/jfs.cc: Move online_grow check immediately after online_read Remove do_unmount, add success check to unmount check src/lvm2_pv.cc: Move online_grow/shrink check inside is_lvm2_pv_supported src/nilfs2.cc: Move online_grow check immediately after online_read Alter logic for whether to mount / unmount src/reiserfs.cc: Move online_grow check immediately after online_read src/xfs.cc: Move online_grow check immediately after online_read Remove do_unmount, add success check to unmount check UBUNTU 13.10 TESTING RESULTS ---------------------------- Successfully tested: btrfs - both grow and shrink while unmounted or mounted ext2 - grow while mounted is not supported ext3 - grow while unmounted or mounted ext4 - grow while unmounted or mounted jfs - grow while unmounted or mounted lvm2_pv - both grow and shrink while deactivated or activated nilfs2 - both grow and shrink while mounted *** FAIL: both grow and shrink FAILED while unmounted - nilfs_cleanerd process starts after mount on ubuntu 13.10 and prevents unmouning. reiserfs - grow while unmounted or mounted xfs - grow while unmounted or mounted QUESTIONS --------- 1) Any ideas on how to deal with nilfs_cleanerd process preventing unmount of nilfs2 file systems? Even a simple mount followed by an unmount does not work. Only after killing the nilfs_cleanerd process will the unmount work. Sample Session: # mount -t nilfs2 /dev/sdb1 /mnt/mymnt # umount /mnt/mymnt user.nilfs2: /dev/sdb1: device is busy user.nilfs2: /dev/sdb1: device is busy # ps -e | grep nilfs 3518 ? 00:00:00 nilfs_cleanerd # kill 3518 # umount /mnt/mymnt # Perhaps this is a bug in nilfs2 or the Ubuntu package? 2) Phillip, has the libparted online resize patch made it into parted upstream? NEXT STEP --------- Mike, when you have time please review this updated patch set.
According to the man page, umount is supposed to shut it down. I just tried setting up a loop device with nilfs, mounting it, and unmounting it, and it seems to work fine here. No, the online resize patches have not made it into upstream parted yet. I'm trying to get a release done soon with all of the other bug fixes, then get on pushing the resize patches for the next release.
Thank you Phillip for testing nilfs2 mount/unmount and your efforts to get the online resize patch into upstream parted. If you need some help testing a beta release of upstream parted with GParted then just let me know. Phillip, What distro did you use for testing? If it wasn't Ubuntu 13.10, would you be able to try it in 13.10? NILFS2 MOUNT/UNMOUNT TESTING ============================ Both of the following Debian Jessie and Ubuntu 13.10 Saucy Salamander tests were performed in virtual machines built using VMWare Player 6.0.1. DEBIAN JESSIE ------------- Based on Phillip's results I tried nilfs2 mount/unmount in Debian Jessie (testing). No problems there as shown in the following session log: root@jessie:~# gparted /dev/sdb # Create 512 MiB nilfs2 sdb1 ====================== libparted : 2.3 ====================== root@jessie:~# mount /dev/sdb1 /mnt/mymnt root@jessie:~# umount /mnt/mymnt root@jessie:~# UBUNTU 13.10 ------------ The same cannot be said of Ubuntu 13.10 Saucy Salamander. In saucy both my tests with /dev/sdb1 and /dev/loop0p1 failed the unmount step as shown in the following session log: root@saucy:~# gparted /dev/sdb # Create 512 MiB nilfs2 sdb1 ====================== libparted : 2.3 ====================== root@saucy:~# mount -t nilfs2 /dev/sdb1 /mnt/mymnt root@saucy:~# umount /mnt/mymnt umount.nilfs2: /dev/sdb1: device is busy umount.nilfs2: /dev/sdb1: device is busy root@saucy:~# ps -e | grep nilfs 2242 ? 00:00:00 nilfs_cleanerd root@saucy:~# kill 2242 root@saucy:~# umount /mnt/mymnt root@saucy:~# truncate -s 512M loopdisk root@saucy:~# losetup /dev/loop0 loopdisk root@saucy:~# gparted /dev/loop0 # Create 128 MiB nilfs2 loop0p1 ====================== libparted : 2.3 ====================== /dev/loop0: unrecognised disk label /dev/loop0: unrecognised disk label root@saucy:~# mount -t nilfs2 /dev/loop0p1 /mnt/mymnt root@saucy:~# umount /mnt/mymnt umount.nilfs2: /dev/loop0p1: device is busy umount.nilfs2: /dev/loop0p1: device is busy root@saucy:~# ps -e | grep nilfs 2573 ? 00:00:00 nilfs_cleanerd root@saucy:~# kill 2573 root@saucy:~# umount /mnt/mymnt root@saucy:~# losetup -d /dev/loop0 root@saucy:~# rm loopdisk root@saucy:~# If anyone can duplicate this problem in Ubuntu 13.10 then it would appear that there is a bug in the Ubuntu 13.10 with nilfs2. Curtis
Tested on Ubuntu 13.10, but I didn't do anything with gparted. I just ran: truncate -s 1g img losetup -d /dev/loop0 img mkfs -t nilfs2 /dev/loop0 mount /dev/loop0 /mnt umount /mnt
Hi Curtis, I've had a quick test of mounting and umounting nilfs2. In Fedora 19 VM it worked, but in Ubuntu 13.10 VM it failed the same way you experienced. It'll probably be Saturday until I can really review the code. Mike
Thank you Mike and Phillip for testing nilfs2 mount/unmount. It looks like Mike and I are not the only ones experiencing this nilfs2 unmount problem in Ubuntu. nilfs2 cannot umount, device is always busy https://bugs.launchpad.net/ubuntu/+source/nilfs-tools/+bug/1252063 Since this problem is not unique to GParted (and would happen with previous versions of GParted running on Ubuntu 13.10), the nilfs2 unmount problem should not prevent us from reviewing, updating, and eventually committing this patch set. No rush Mike on reviewing the code. I would like to make a GParted release early to mid December but this is not a hard deadline. Curtis
I wonder why it works for me? You might try calling umount.nilfs2 directly. umount should do that for you, but if for some reason, it did not, that would explain it.
Hi Phillip, Even calling umount.nilfs2 directly results in a "device is busy" error. Since this problem likely affects many more people, appears to be Ubuntu specific and is not related to GParted, I think that any future communication on the nilfs unmount problem should be recorded in the report: nilfs2 cannot umount, device is always busy https://bugs.launchpad.net/ubuntu/+source/nilfs-tools/+bug/1252063 Curtis
Created attachment 261299 [details] [review] Online resize support (v9) Hi Curtis and Phillip, I have reviewed the patchset v8 from comment #53. Review and testing passed. Resize() methods for file systems which have to be mounted, btrfs, jfs, nilfs2 and xfs, are much more readable now. I have made the following cosmetic / white space changes in patchset v9: 1st commit message - Mentioned need for kernel >= 3.6 for online partition resize - Minor grammar and layout changes 2nd commit message - Line wrapping update Add bug footer src/GParted_Core.cc - Minor white space change src/Win_GParted.cc - Split determining whether partition can be online resized from early return if busy src/nilfs2.cc - Removed unneeded and inconsistent {}'s from around rm_temp_dir() at the end of resize() method I will be committing patchset v9 by tomorrow if I don't hear otherwise. Thanks, Mike
Comment on attachment 261299 [details] [review] Online resize support (v9) Mark as patch
Thank you Mike for reviewing patchset v8. I like the patchset v9 improvements you made with the commit messages and splitting out the online-resize check. I found one other white space change to make in GParted_Core.cc::resize() to match the surrounding code. Specifically: FROM: if ( partition_new.busy || check_repair_filesystem( partition_new, ... TO: if ( partition_new .busy || check_repair_filesystem( partition_new, ... However this is really splitting hairs. I think if I'm worrying about this level of change then the patchset must be pretty darned good. :-) I am okay if you add or ignore this white space change when you commit the patchset. In summary, all looks good to me. Curtis
Phillip and Mike, With all my focus on testing online resizing of file systems, I didn't even think about the extended partition. Currently if the extended partition contains any active/mounted partitions, then GParted will disable the extended partition resize option. Can you think of any reason why we would not be able to perform an "online resize" of the extended partition? Is so could the start as well as the end of the partition be changed? If this is possible then I suggest we consider such an enhancement in a new commit, or maybe even a new bug report. Curtis
Oh yea, I totally forgot about the extended partitions. Yes, I think it should be fine to remove the busy check from them and just allow them to be resized at will. As far as the whitespace before the dot and semicolons goes, I just find them ugly and wasteful and have *NEVER* seen code like that before, so I try to get rid of it when I'm touching the line anyhow.
Phillip, I agree the white space is purely a preference and this varies from person to person. GParted was the first code I saw that did this, so I try to ensure that the code matches the surrounding code. On the topic of online resizing of the extended partition, I would think that there should be no problem moving the end of the partition. Since logical partitions are tracked by Extended Boot Records within the extended partition, I wonder if there would be problems if the start of the extended partition is moved? Curtis
No problems beyond what gparted already handles ( like leaving room for the EBR ). The fact that there's a logical partition that is mounted shouldn't make any difference.
I was thinking about the kernel and it finding the logical partition as an offset read from the first EBR in the extended partition. If an extended partition is moved, then at least one EBR will be affected. Do you foresee this as a problem Phillip?
libparted rewrites all of the EBRs when you sync the new table to the disk so it will make sure that they are updated as needed, so it should not be a problem.
Hi Phillip and Curtis, Thank you for your work on this change. The following changes have been commited to GParted git repo: Add online resize support (#694622) https://git.gnome.org/browse/gparted/commit/?id=de2844d02df02ad6ccf0f2caf7f63622cf4d25de Update help manual for online resize support (#694622) https://git.gnome.org/browse/gparted/commit/?id=00409ef51baf8adc52ee67bc143bfd8163190c5a Thanks, Mike
Created attachment 263433 [details] [review] Fix minor Utils::get_kernel_version logic flaw Hi Mike, The Utils::get_kernel_version was failing to set the major, minor, and patch versions if there was not patch version. For example if /proc/version contained: Linux version 3.10-3-686-pae... then the missing patch version would prevent the major, minor, and patch versions from being set to "3", "10", and the default of "-1". The attached patch should address this situation. Please review this patch to see if my reasoning is sound. This patch is needed so that the --enable-online-resize configure option will work on Debian 8 Jessie which has only a major and minor number in the Linux kernel. Curtis
Created attachment 263449 [details] [review] Handle 2 component kernel versions (draft 2) Hi Curtis, The change you made doesn't work in the boundary case. For example "Linux version 3.10-3" would be read by get_kernel_verion() as 3, 10, -1 but kernel_version_at_least() would find it too low to satisfy the requirement of 3, 10, 0. Possibly irrelevant question: What does "Linux version 3.10-3-686-pae..." mean by the Debian packers? Does it mean kernel 3.10 with no upstream patches, only Debian ones; or does it mean kernel 3.10.3? If the later then mayby "3.10-3" should be read as 3, 10, 3 not just 3, 10. Anyway here is a patch I have just thrown together. Needs a testing properly and a proper commit message. Thanks, Mike
Created attachment 263474 [details] [review] Fix minor Utils::get_kernel_version logic flaw (v3) Hi Mike, Good catch on the logic flaw in my patch. It certainly would have failed in the situation you mentioned. > What does "Linux version 3.10-3-686-pae..." mean by the Debian packers? > Does it mean kernel 3.10 with no upstream patches, only Debian ones; or > does it mean kernel 3.10.3? Some debian kernels appear to only have a major and minor kernel version from upstream and no upstream patches, only debian patches. For examples, see the following link with a list of debian linux images. http://packages.debian.org/search?suite=default§ion=all&arch=any&searchon=names&keywords=linux-image For example note that the package linux-image-2.6-486 shows a 3.2+46 for wheezy (stable). The logic in the patch in comment #72 made complete sense to me. However when testing I discovered strange behaviour that I could consistently reproduce on both debian 8 Jessie and ubuntu 13.10. The steps to see the strange behaviour are: 1) Start gparted 2) Choose "View -> File System Support" Note that only btrfs is shown to have double-check-marks 3) Click "Rescan for supported actions" Note that all of the double-check-marks disappear. When I put a few printf's in the code it seemed that the assigned variable was not working as I would expect. In order to avoid a possible problem with the assigned variable or sscanf not correctly returning the number of arguments read, I opted to make a minor change to the patch in comment #71. In this patch I am more open and allow just the major number of the Linux kernel version to be sufficient. Please review this patch. Fixing this problem appears to be much harder than it should be. Especially the strange behaviour with the patch in comment #72. Your logic looked perfect to me. Curtis
Created attachment 263549 [details] [review] Handle 2 component kernel versions (v4) Hi Curtis, > Fixing this problem appears to be much harder than it should be. Especially > the strange behaviour with the patch in comment #72. Your logic looked perfect > to me. While I was in bed last night I realised that I had made a mistake in my code in comment #72. It also explains your testing results of it only working on the first ever call to get_kernel_version(). Need to make success variable static: static bool success = false; So success is not initialised to false on each call yet only ever updated to true on the first call when the version file is read. Instead it needs to be remembered in subsequent calls to get_kernel_version(). If you don't mind I would like to use my patch because yours uses read_major_ver = 0 to mean unknown, yet sscanf() can assign 0 to the first read number. However the era of Linux 0.x is long gone. Here's my corrected patch now tested and with a suitable commit message. Thanks, Mike
Hi Mike, Thank you for persevering on the problem. In programming there are often many ways to address a challenge. The important thing is to fix the problem. I successfully tested your patch with and without --enable-online-resize on kubuntu 12.04, ubuntu 13.10, and debian 8 jessie and observed the expected results. As such I have committed the patch from comment #74 to the master branch of the git repository. The relevant git commit can be viewed at the following link: Successfully read kernel versions with only 2 components https://git.gnome.org/browse/gparted/commit/?id=a4b82a93053d9482dd6d126820b97ddec8563013 Curtis
This enhancement was included in the GParted 0.17.0 release on December 9, 2013.