Hi Hin-Tak,
On Sun, 2014-01-19 at 01:50 +0000, Hin-Tak Leung wrote:
> ------------------------------
> Tested-by: Hin-Tak Leung <htl10@xxxxxxxxxxxxxxxxxxxxx>
>
> I have given the patch set a bit of light usage, and it fsck'ed clean afterwards.
> So that's the minimum it should do.I also see that you have spent substantial
> amount of effort in verifying and checking the validity and sanity of the journal
> itself (which Netgear certainly didn't do). And thanks for the good work! That part
> at least is very desirable and overdue and should land in the kernel soon.
>
Thank you for testing and reviewing of the patchset.
> About documentation and the new "norecovery" option. Should "norecovery"
> imply read-only (and also state clearly so)?
Yes, sure. The "norecovery" option means read-only state. You can see it
in hfsplus_check_journal() method:
+ if (hfsplus_journal_empty(jh)) {
+ err = hfsplus_replay_journal_header(sb);
+ if (unlikely(err)) {
+ pr_err("journal replay is failed, please run fsck\n");
+ return err;
+ }
+ /* If they're the same, we can mount, it's clean */
+ hfs_dbg(JOURNAL, "journal is empty\n");
+ return 0;
+ } else {
+ /* Try to replay journal */
+ if (test_bit(HFSPLUS_SB_NORECOVERY, &HFSPLUS_SB(sb)->flags)) {
+ pr_info("journal replay is skiped, mounting read-only.\n");
+ sb->s_flags |= MS_RDONLY;
+ return 0;
+ }
+
+ err = hfsplus_replay_journal(sb);
+ if (unlikely(err)) {
+ pr_err("journal replay is failed, please run fsck\n");
+ return err;
+ }
+ hfs_dbg(JOURNAL, "journal replay is done\n");
+ }
> The meaning of the "force" option
> also needs some updating - if we play back journal, then unclean journalled
> volumes would be mounted read-write after verify/validating and journal playback,
> correct? I see there is a need for "norecovery" in addition to "read-only" (in the typical
> convention elsewhere, the latter will occasionally write to a journalled fs,
> in the case of journal playback - so "norecovery" is "really no write, not
> a single byte, I mean it, don't even playback journal"), but I think norecovery
> should imply read-only, unless in combination with "force"?
>
Yes, you are right about "force" option. So, I need to check that all
plays well for this option. And it needs to correct documentation also
for the case of "force" option. Thank you. I'll do it.
> A somewhat minor thing - I see a few instances of
> 'pr_err("journal replay is failed\n");' - the "is" isn't needed. Just English usage.
>
Yes, I correct it. So, I'll prepare next version of the patchset with
these minor corrections ASAP.
> Now it still worries me that whichever way we implements journalling in the linux
> kernel, it may be correct according to spec, but doesn't inter-op with
> Apple's. This is especially relevant since a substantial portion of users
> who wants hfs+ journalling in their linux kernel is because they have an
> intel Mac and they are dual-booting. Now we have 3 implementations - apple's,
> this, and Netgear's. Even if we discount Netgear's, while each of them
> will create a journal during its normal mode of operation, will playback a journal
> of its own creation, we really need to test the case of playback of journals
> made by the Mac OS X darwin kernel... because I can think of this happening:
> somebody has a dual-boot machine, you pull the plug while it is in Mac OS X,
> and the boot-loader is configured to go into linux by default after a short time-out
> (or vice versa).
Yes, I am taking into account compatibility of journaling and other
functionality of HFS+ driver in Linux kernel with Mac OS X. It is very
important to keep compatibility, from my viewpoint. So, you are right
and I spend efforts on checking such compatibility when I implement and
testing code for HFS+ driver.
>
> So again, I must thank you for spending the effort on verifying and validating the
> journal entries.
>
> On separate/somewhat unrelated matters, I came across this bug report in
> the recent activities:
>
> https://bugs.launchpad.net/ubuntu/+source/hfsprogs/+bug/680606
>
> I think we addressed both of the 2nd and 3rd entries:
> errors ("Invalid volume file count", "Invalid volume free block count")
> error ("Unused node is not erased")
>
> The first entry looks suspiciously like the problem which I have/had:
> (restore a large directory from time-machine)
> Invoke "rm -r" targeting the directory
> - segfault occurs
> - Various applications freeze, and it's impossible to cleanly shut down
> - OS X Disk Utility reports that the disk can't be repaired
>
> So the disk I manually edited and fixed, still can get the kernel confused (i.e
> the kernel just get confused, if one unmount and reboot, the disk is fsck-clean still),
> just by repeatedly running "du". I found that I need to put the system under
> some memory stress: I can run "du" for a dozen of times on an idle system
> freshly rebooted, but as soon as I have google-chrome running with a few browser
> windows opening (or mozilla/firefox). The kernel driver can get confused if one transverse
> the file system quickly (with a du, or in the above case, with an r"m -rf large directory"),
> especially the system is relatively loaded.
>
I didn't try such way of reproducing the bug. So, maybe I'll try to
reproduce the issue. Unfortunately, I don't know when I'll try it
because I will be really busy during next several weeks.
> Oh, there is still the folder count issue with case-sensitive HFS+ (which AFAIK, just a
> convenience for some file system transversal usage, not actually of any critical function),
> but that's a relatively harmless issue. We can deal with it at some point.
>
Yes, sure.
Thanks,
Vyacheslav Dubeyko.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
