After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 642842 - nilfs is not detected
nilfs is not detected
Status: RESOLVED FIXED
Product: gparted
Classification: Other
Component: application
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: gparted maintainers alias
gparted maintainers alias
Depends on:
Blocks:
 
 
Reported: 2011-02-21 00:46 UTC by Michael Buckley
Modified: 2012-02-22 00:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Minimal nilfs2 detection (2.82 KB, patch)
2011-11-20 21:46 UTC, Mike Fleetwood
none Details | Review
Minimal nilfs2 detection v2 (2.77 KB, patch)
2011-11-22 09:20 UTC, Mike Fleetwood
none Details | Review
Draft GParted NILFS2 support patch (9.30 KB, patch)
2011-11-22 23:09 UTC, Mike Fleetwood
none Details | Review
Minimal nilfs2 detection v3 (2.84 KB, patch)
2011-11-28 15:20 UTC, Mike Fleetwood
none Details | Review
Draft GParted NILFS2 support v2 patch (9.34 KB, patch)
2011-11-28 15:22 UTC, Mike Fleetwood
none Details | Review
Draft GParted NILFS2 support v4 patch (11.53 KB, patch)
2011-12-08 22:38 UTC, Mike Fleetwood
none Details | Review
nilfs2 with resize, shallow detail tree (7.56 KB, text/x-c++src)
2011-12-12 19:17 UTC, Mike Fleetwood
  Details
nilfs2 resize operation details picture with 100,000s hours (66.98 KB, image/png)
2011-12-12 19:20 UTC, Mike Fleetwood
  Details
nilfs2 with resize, deep detail tree (7.71 KB, text/x-c++src)
2011-12-12 19:21 UTC, Mike Fleetwood
  Details
Diff file with minor format and colour changes (3.34 KB, text/plain)
2011-12-16 18:53 UTC, Curtis Gedak
  Details
Fix uninitialised read in OperationDetail::set_status() patch (1.40 KB, patch)
2011-12-18 12:43 UTC, Mike Fleetwood
none Details | Review
Add helper functions for mounted file system resizing operations (8.00 KB, patch)
2011-12-28 22:16 UTC, Mike Fleetwood
none Details | Review
Add resize support to nilfs2 (#642842) (3.37 KB, patch)
2011-12-28 22:17 UTC, Mike Fleetwood
none Details | Review
Update jfs resize to use new helper functions (4.97 KB, patch)
2011-12-28 22:18 UTC, Mike Fleetwood
none Details | Review
Update btrfs resize to use new helper functions (5.84 KB, patch)
2011-12-28 22:18 UTC, Mike Fleetwood
none Details | Review
Update xfs resize and copy to use new helper functions (12.16 KB, patch)
2011-12-28 22:19 UTC, Mike Fleetwood
none Details | Review
nilfs2 resize 5 patch set (v2) (34.56 KB, patch)
2012-01-11 00:12 UTC, Mike Fleetwood
none Details | Review

Description Michael Buckley 2011-02-21 00:46:53 UTC
The nilfs file system is not detected by gparted.  It was added to the kernel in release 2.6.30.
Comment 1 Curtis Gedak 2011-02-21 16:54:30 UTC
Thank you Michael for bringing this to my attention.

Do you have some good links to information about nilfs that you would like to share?

Do you know if there are free software tools that run on GNU/Linux to work with nilfs file systems?
Comment 2 Michael Buckley 2011-02-22 00:44:02 UTC
There is of course the nilfs home page http://www.nilfs.org/en/ and http://www.nilfs.org/en/about_nilfs.html is particularly helpful if you wanted to create a partition with it.

My interest in the file system is, unlike any other supported by gparted currently, it is more SSD friendly in the way it works.  Another is logfs which was added in 2.6.34, but I will leave filing a bug about that to someone with more interest in it then me.
Comment 3 Curtis Gedak 2011-02-22 17:36:41 UTC
Thanks for the extra info Michael.
Comment 4 Mike Fleetwood 2011-11-20 21:46:55 UTC
Created attachment 201775 [details] [review]
Minimal nilfs2 detection
Comment 5 Mike Fleetwood 2011-11-20 21:49:40 UTC
Just attached minimal nilfs2 detection patch (gparted-nilfs2-01.patch)
for consideration.  Wondering if the colour I have chosen is OK though.
I intent to write 1 or 2 more patches to provide full nilfs2 support
utilising nilfs-utils.  Can then produced just 1 patch containing all
changes if preferred.

Mike
Comment 6 Mike Fleetwood 2011-11-22 09:20:37 UTC
Created attachment 201903 [details] [review]
Minimal nilfs2 detection v2

1) Softened the colour for NILFS2 slightly to better fit the colour palette.
2) Tidied up the commit message.
Comment 7 Mike Fleetwood 2011-11-22 23:09:14 UTC
Created attachment 201958 [details] [review]
Draft GParted NILFS2 support patch

Here's a draft of the full nilfs2 support patch.

Not sure of relevant copyright etiquette.  I've just put my name at the
top of the 2 new files, although nilfs2.h is only really s/xxx/nilfs2/
from any other FS header.

Seems better to let blkid read the fs label because its faster and
less code than running nilfs-tune -l /dev/DEVICE and parsing:
        Filesystem volume name:   LABEL
or      Filesystem volume name:   (none)
(colon, tab, space, space then LABEL) unless it is needed because of
Display ext2/3/4 unicode volume labels properly (#662537).

Don't know how to query the file systems used/free space when unmounted
so I haven't coded set_used_sectors().  Therefore the
        Unable to read the contents of the file system! ...
warning is displayed.  Could mount, statvfs(), umount but that doesn't
seem desirable.
Comment 8 Mike Fleetwood 2011-11-28 15:20:31 UTC
Created attachment 202300 [details] [review]
Minimal nilfs2 detection v3

Updated minimal nilfs2 detection patch with improved message reporting
libparted can also detect nilfs2.
Comment 9 Mike Fleetwood 2011-11-28 15:22:42 UTC
Created attachment 202301 [details] [review]
Draft GParted NILFS2 support v2 patch

Updated nilfs2 support patch with updated commit message and README
tweek.  Previous 3 questions about copyright etiquette, use blkid to
read label or parse nilfs-tune output and not implementing
set_used_sectors() still apply.
Comment 10 Curtis Gedak 2011-12-03 22:36:47 UTC
Thank you Mike for your work to add support for the NILFS file system.

(In reply to comment #7)
> Not sure of relevant copyright etiquette.  I've just put my name at the
> top of the 2 new files, although nilfs2.h is only really s/xxx/nilfs2/
> from any other FS header.

Since you created the file, I belive that placing your name in the copyright is the proper thing to do.  At least that is what I have done when adding new file system support to GParted.  :-)


> Seems better to let blkid read the fs label because its faster and
> less code than running nilfs-tune -l /dev/DEVICE and parsing:
>         Filesystem volume name:   LABEL
> or      Filesystem volume name:   (none)
> (colon, tab, space, space then LABEL) unless it is needed because of
> Display ext2/3/4 unicode volume labels properly (#662537).

Inside the file system class, I prefer to use the file system specific tools to read the label.

From my testing, the problem with blkid (older versions) reading unicode labels does occur for nilfs.  Hence even though it is slower, I think that accuracy in reading the label is more important than speed in this case.


> Don't know how to query the file systems used/free space when unmounted
> so I haven't coded set_used_sectors().  Therefore the
>         Unable to read the contents of the file system! ...
> warning is displayed.  Could mount, statvfs(), umount but that doesn't
> seem desirable.

The "nilfs-tune -l /path-to-partition" command appears to display enough information to calculate the free sectors.  Specifically I think that the "Block size:" and "Free blocks count:" lines contain the necessary information.

I think something similar in nature to the xfs::set_used_sectors() code would work.

The free space is mostly needed when resizing partitions.
Do you know if there are resize commands available for nilfs?

Also since we haven't committed the nilfs detection patch, would you be able to create a single patch for nilfs support?
Comment 11 Mike Fleetwood 2011-12-04 10:26:58 UTC
(In reply to comment #10)
> (In reply to comment #7)
> > Seems better to let blkid read the fs label because its faster and
> > less code than running nilfs-tune -l /dev/DEVICE and parsing:
> >         Filesystem volume name:   LABEL
> > or      Filesystem volume name:   (none)
> > (colon, tab, space, space then LABEL) unless it is needed because of
> > Display ext2/3/4 unicode volume labels properly (#662537).
> 
> Inside the file system class, I prefer to use the file system specific tools to
> read the label.
> 
> From my testing, the problem with blkid (older versions) reading unicode labels
> does occur for nilfs.  Hence even though it is slower, I think that accuracy in
> reading the label is more important than speed in this case.

OK.  Will use nilfs specific method.

> > Don't know how to query the file systems used/free space when unmounted
> > so I haven't coded set_used_sectors().  Therefore the
> >         Unable to read the contents of the file system! ...
> > warning is displayed.  Could mount, statvfs(), umount but that doesn't
> > seem desirable.
> 
> The "nilfs-tune -l /path-to-partition" command appears to display enough
> information to calculate the free sectors.  Specifically I think that the
> "Block size:" and "Free blocks count:" lines contain the necessary information.
> 
> I think something similar in nature to the xfs::set_used_sectors() code would
> work.

I remember manually testing it but for some reason deciding the figures
between df and nilfs-tune didn't make sence.  Will look at it again and
code it up.

> The free space is mostly needed when resizing partitions.
> Do you know if there are resize commands available for nilfs?

I didn't think that it was functional (hence the comment in the code),
but I have just tested it and it works!  Will code this too and try and
work out the minimum requirements.

> Also since we haven't committed the nilfs detection patch, would you be able to
> create a single patch for nilfs support?

It'll take me a few days, but I'll code and test the above changes and
submit a single patch.
Comment 12 Mike Fleetwood 2011-12-08 22:38:18 UTC
Created attachment 203124 [details] [review]
Draft GParted NILFS2 support v4 patch

Here's a new draft of a merged nilfs2 support patch.  Does fs specific
label reading and sets used sectors.  Does NOT do resizing.  Still
working on that.
Comment 13 Curtis Gedak 2011-12-10 19:03:15 UTC
Thank you for the updated patch Mike.  I plan to test and review it mid week.
Comment 14 Mike Fleetwood 2011-12-12 19:17:22 UTC
Created attachment 203288 [details]
nilfs2 with resize, shallow detail tree
Comment 15 Mike Fleetwood 2011-12-12 19:20:34 UTC
Created attachment 203289 [details]
nilfs2 resize operation details picture with 100,000s hours
Comment 16 Mike Fleetwood 2011-12-12 19:21:28 UTC
Created attachment 203290 [details]
nilfs2 with resize, deep detail tree
Comment 17 Mike Fleetwood 2011-12-12 19:22:30 UTC
Support v4 patch is ready for committing, subject to your testing and
review of cause, but just doesn't contain any resizing code.

Resizing code ...

I initially tried to create a shallow operation detail as btrfs does,
but also marking each command as either successful or errored.  [code#1]
+ Shrink /dev/sda10 from x.xx GiB to y.yy GiB
  + shrink file system
    + "mkdir ..."
    + "mount ..."
    + "nilfs-resize ..."
    + "umount ..."
    + "rmdir ..."
However this resulted in each command reportedly taking 100,000s of
hours.  [pic#1].

So instead I switched to a deeper operation detail as xfs does.  I
followed the tree as close as I could, but not so closely with the code.
[code#2]
+ Shrink /dev/sda10 from x.xx GiB to y.yy GiB
  + shrink file system
    + create temporary mount point ...
      + "mkdir ..."
    + mount /dev/sda10 on ...
      + "mount ..."
    + resize mounted file system
      + "nilfs-resize ..."
    + unmount ...
      + "umount ..."
    + remove temporary mount point ...
    + "rmdir ..."
I have a couple of outstanding issues with this code:
1) When performing a grow the final rmdir operation is displayed as a
   fail, however it must have succeeded because: 1) the directory is
   removed, 2) it is not displayed as a warning, 3) no child details
   with strerror text is added.  Probably means I don't understand
   OperationDetails() yet.
2) About 1 in 10 times of performing a shrink the unmount operation is
   displayed as a fail, but the following rmdir is performed
   successfully and the fs is umounted.  Every time I add debugging
   code it never happens.  Really strange.

[code#1] nilfs2-shallow.cc
[pic#1] nilfs2-resize-shallow.png
[code#2] nilfs2-deep.cc

Suggestions welcome.
Mike
Comment 18 Curtis Gedak 2011-12-14 19:05:50 UTC
Thank you Mike for the new patch and testing information.

My preliminary testing with the patch in comment #12 (w/o resizing) is going well.  I want to do some more testing, and perform a source code review before this is committed to the source code repository.

I have not yet looked closely into your notes from comment #14 onwards.

Since you mentioned thousands of seconds of elapsed time is being reported, this might be a problem with initializing the timer.  Again I haven't had time to look into this yet.

My plan is to thoroughly review the patch (w/o resizing) before looking into the resizing problems you have encountered.
Comment 19 Curtis Gedak 2011-12-16 18:53:40 UTC
Created attachment 203692 [details]
Diff file with minor format and colour changes

Great work on the patch Mike!

I hope you don't mind but I made some minor format and colour changes before committing your patch to the gnome git repository.

The colour change is a personal preference because the light yellow-orange colour appeared to be very close to the accent yellow colour used for XFS file systems.

The relevant git commit can be viewed at the following link:
http://git.gnome.org/browse/gparted/commit/?id=df20b54d00b0560707961255d3521d0c602e4f53

On the topic of nilfs2 resizing, it might be a while before I can look at these closely.  Currently there are some bug reports, such as bug #664050, that I should focus on first before investigating new functionality.

Thanks again for your work to improve GParted.  :)
Comment 20 Mike Fleetwood 2011-12-18 12:43:15 UTC
Created attachment 203793 [details] [review]
Fix uninitialised read in OperationDetail::set_status() patch

Hi Curtis, when you have a moment I found this bug.  From the commit
message ...

    Fix uninitialised read in OperationDetail::set_status()

    Setting the status and controlling the timing of operation details
    initialised with OperationDetail(desc, status, font) uses a conditional
    branch depending on the uninitialised variable this->status.
    ...

This uninitialised read is triggered in lots places in the operation
details.  Just the ones I saw were: format partition as ext2, shrink
ext2, resize btrfs, format partition as xfs, grow xfs.  By chance it
hasn't caused any harm until now where the undefined behaviour bites my
nilfs2 resize code.  For me this patch fixes issue #1 I was having with
the deeper operation tree resize code I mentioned in comment #17.
> 1) When performing a grow the final rmdir operation is displayed as a
>    fail, however it must have succeeded because: 1) the directory is
>    removed, 2) it is not displayed as a warning, 3) no child details
>    with strerror text is added.  Probably means I don't understand
>    OperationDetails() yet.

I'm continuing to look into the other issues I mentioned in comment #17
with my nilfs2 resizing code, so don't let me keep you from other bugs.
Comment 21 Mike Fleetwood 2011-12-18 21:55:48 UTC
Uninitialised read in set_status() patch in comment #20 also definitely
resolves issue #2 I was having with the deeper operation tree resize
code I mentioned in comment #17.
> 2) About 1 in 10 times of performing a shrink the unmount operation is
>    displayed as a fail, but the following rmdir is performed
>    successfully and the fs is umounted.  Every time I add debugging
>    code it never happens.  Really strange.

It's coming along much better now.
Comment 22 Curtis Gedak 2011-12-19 16:35:45 UTC
It is good to hear that you found and fixed a bug, and are making good progress on nilfs resizing.  I hope to take a look at the patch in comment #20 soon.
Comment 23 Mike Fleetwood 2011-12-28 22:16:50 UTC
Created attachment 204299 [details] [review]
Add helper functions for mounted file system resizing operations
Comment 24 Mike Fleetwood 2011-12-28 22:17:36 UTC
Created attachment 204300 [details] [review]
Add resize support to nilfs2 (#642842)
Comment 25 Mike Fleetwood 2011-12-28 22:18:10 UTC
Created attachment 204301 [details] [review]
Update jfs resize to use new helper functions
Comment 26 Mike Fleetwood 2011-12-28 22:18:45 UTC
Created attachment 204302 [details] [review]
Update btrfs resize to use new helper functions
Comment 27 Mike Fleetwood 2011-12-28 22:19:27 UTC
Created attachment 204303 [details] [review]
Update xfs resize and copy to use new helper functions
Comment 28 Mike Fleetwood 2011-12-28 22:20:26 UTC
Hi Curtis,  Here's the nilfs2 resizing code.  I added some helper
functions to make resizing easier to implement and then went on to use
them update btrfs, jfs and xfs resizing and xfs copying code.

Comments welcome, Mike
Comment 29 Curtis Gedak 2012-01-04 21:13:18 UTC
Great catch Mike on the uninitialised read in OperationDetail::set_status()!
This patch (comment #20) has been committed to the gnome git repository.

Thank you too for your continued work on this enhancement.  My to-do list had included making a helper mount/umount function. It was a nice holiday gift to see that you had gone ahead and done created such a helper function.  :)

I appreciate your patience too.  I am working through a backlog of bug reports, forum requests, and patches, and will continue working through the patches you have supplied.  If I run into any problems I will be sure to let you know.
Comment 30 Curtis Gedak 2012-01-09 20:34:03 UTC
Hi Mike, My review and testing of your set of 5 patches in comments 23 through 27 has gone very well.  The code looks good and my testing on Ubuntu 11.10 resulted in no problems at all.

Thank you again for all of your hard work to enhance GParted.

There is one minor item that we should address in patch 1/5 in comment #23.  Since English words have been used in two of the log messages, these should either be made available for translation to other languages, or else remove these words.

The two text messages in question are:
     "mkdir: created directory: "
and
     "rmdir: removed directory: "

I would be fine going either way on this.  For example if you wished to delete the words "created directory: " and "removed directory: " that would be okay with me.

If we want to make these words available for translation, it is better practice to make the entire sentence available for translation.

This is an example from GParted_Core::update_bootsector:

operationdetail .add_child( OperationDetail( 
	/*TO TRANSLATORS: update boot sector of ntfs file system on /dev/sdd1 */
	String::ucompose( _("update boot sector of %1 file system on %2"),
			  Utils::get_filesystem_string( partition .filesystem ),
			  partition .get_path() ) ) ) ;

You can read more about language translation in the following GNU 'gettext' Utilities link:
http://www.gnu.org/software/gettext/manual/gettext.html

The short and quick instruction is:

1)  To make text available for translation place it between the
    indicators _( and )

2)  If the text string is not constant, then use String::ucompose and
    % parameters for the substitution because this enables translators
    to create a better translation.

3)  When using String::ucompose for translation, it is helpful to place
    a comment immediately above with an example of the sentence with the
    % parameters filled in.

For fixing this problem if it is easier to create a sixth patch that deals with the "create directory" and "remove directory" strings, then that would be fine with me.  I would then commit all six patches at the same time.
Comment 31 Mike Fleetwood 2012-01-11 00:12:40 UTC
Created attachment 204995 [details] [review]
nilfs2 resize 5 patch set (v2)

Hi Curtis,  I just made the text translateable in the end.

Learnt some more Git Kung Fu (editing midstream patches), so here's
version 2 of the resize 5 patch set, concatenated together.  Just in
case, apply with:
  git am gparted-nilfs2-resize-v2.mbox
Comment 32 Curtis Gedak 2012-01-11 20:12:47 UTC
Thanks Mike for this updated patch set.  My testing went very smoothly with these new patches.  Great job!

And thank you for the tip about editing midstream patches.  After some Internet searches I learned about using "git rebase bbc643cd^ --interactive".  Hopefully you don't mind, but I tried it out to add translation comments to the first of the five patches.  It worked wonderfully.

The patch set in comment #31 has been committed to the Gnome git repository for inclusion in the next release of GParted.

The relevant git commits can be viewed at the following links:

Add helper functions for mounted file system resizing operations
http://git.gnome.org/browse/gparted/commit/?id=4ab3fe0ee75ed35d049ba349628fb8ea9f6c9d15

Add resize support to nilfs2 (#642842)
http://git.gnome.org/browse/gparted/commit/?id=15bca17a46e990f4757b8b7a9a83c5c1496ba3f2

Update jfs resize to use new helper functions
http://git.gnome.org/browse/gparted/commit/?id=b0d818b8f9d364d27e0d370fded158393eb771d1

Update btrfs resize to use new helper functions
http://git.gnome.org/browse/gparted/commit/?id=a13e1a38636ba76468023d14f3dcf9f43d48c42c

Update xfs resize and copy to use new helper functions
http://git.gnome.org/browse/gparted/commit/?id=e414b71b73d42c97fd0abc03f859288a1eff19f6


Please note that I added the files src/FileSystem.cc and src/nilfs2.cc to POTFILES.in so that these files will be included in the translator .po files.

Add files for translation to POTFILES.in
http://git.gnome.org/browse/gparted/commit/?id=08357b6e81f05eb11da34aad3551366db9658949
Comment 33 Curtis Gedak 2012-02-22 00:44:06 UTC
This enhancement has been included in GParted 0.12.0 released on February, 21, 2012.