[PATCH 0/2] HFS+ journal improvement - take 5

[PATCH 0/2] HFS+ journal improvement - take 5

[Warren Turkal]

This is a patchset to change the way that the HFS+ filesystem detects
whether a volume has a journal or not.

The code currently mounts an HFS+ volume read-only by default when a
journal is detected. One can force a read/write mount by giving the
"force" mount option. The current code has this behavior since there is
no support for the HFS+ journal.

My problem is that the detection of the journal could be better. The
current code tests the attribute bit in the volume header that
indicates there is a journal. If that bit is set, the code assumes
that there is a journal.

Unfortunately, this is not enough to really determine if there is a
journal or not. When that bit is set, one must also examine the journal
info block field of the volume header. If this field is 0, there is no
journal, and the volume can be mounted read/write.

Thanks,
wt

Warren Turkal (2):
  Identify journal info block in volume header.
  Fix journal detection on HFS+.

 fs/hfsplus/hfsplus_raw.h |    2 +-
 fs/hfsplus/super.c       |   13 ++++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

[PATCH 1/2] Identify journal info block in volume header.

[Warren Turkal]

The HFS+ support in the kernel currently will mount an HFS+ volume read-only if
the volume header has the attribute bit set that indicates there is a journal.
The kernel does this because there is no support for a journalled HFS+ volume.

The problem is that this is only half of what needs to be checked to see if
there really is a journal. There is also an entry in the volume header that
tells you where to find the journal info block. In the kernel version of the
kernel, this 4 byte block is labeled reserved. This patch identifies the journal
info block in the header.

Signed-off-by: Warren Turkal <wt@penguintechs.org>
---
 fs/hfsplus/hfsplus_raw.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/hfsplus/hfsplus_raw.h b/fs/hfsplus/hfsplus_raw.h
index fe99fe8..14f1dd8 100644
--- a/fs/hfsplus/hfsplus_raw.h
+++ b/fs/hfsplus/hfsplus_raw.h
@@ -94,7 +94,7 @@ struct hfsplus_vh {
 	__be16 version;
 	__be32 attributes;
 	__be32 last_mount_vers;
-	u32 reserved;
+	__be32 journal_info_block;
 
 	__be32 create_date;
 	__be32 modify_date;
-- 
1.5.6.3

[PATCH 2/2] Fix journal detection on HFS+.

[Warren Turkal]

The code currently mounts an HFS+ volume read-only (unless given the "force"
mount option) if it detects that the HFS+ volume has a journal. The code was
unconditionally assuming that the volume had a jounal if the journal
attribute was set in the volume header. However, the volume also has to have
a non-zero journal info block to actually have a journal.

In this patch, I refactored the journal detection into a function since the
logic is used twice. The journal detection also uses the better logic to
determine if there is a journal.

Signed-off-by: Warren Turkal <wt@penguintechs.org>
---
 fs/hfsplus/super.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index eb74531..4f00a84 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -15,10 +15,17 @@
 #include <linux/vfs.h>
 #include <linux/nls.h>
 
+#include "hfsplus_fs.h"
+
 static struct inode *hfsplus_alloc_inode(struct super_block *sb);
 static void hfsplus_destroy_inode(struct inode *inode);
+static bool hfsplus_vol_has_journal(struct hfsplus_vh *vhdr);
 
-#include "hfsplus_fs.h"
+static bool hfsplus_vol_has_journal(struct hfsplus_vh *vhdr)
+{
+	return (vhdr->attributes & cpu_to_be32(HFSPLUS_VOL_JOURNALED) &&
+				vhdr->journal_info_block);
+}
 
 struct inode *hfsplus_iget(struct super_block *sb, unsigned long ino)
 {
@@ -260,7 +267,7 @@ static int hfsplus_remount(struct super_block *sb, int *flags, char *data)
 			printk(KERN_WARNING "hfs: filesystem is marked locked, leaving read-only.\n");
 			sb->s_flags |= MS_RDONLY;
 			*flags |= MS_RDONLY;
-		} else if (vhdr->attributes & cpu_to_be32(HFSPLUS_VOL_JOURNALED)) {
+		} else if (hfsplus_vol_has_journal(vhdr)) {
 			printk(KERN_WARNING "hfs: filesystem is marked journaled, leaving read-only.\n");
 			sb->s_flags |= MS_RDONLY;
 			*flags |= MS_RDONLY;
@@ -356,7 +363,7 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 	} else if (vhdr->attributes & cpu_to_be32(HFSPLUS_VOL_SOFTLOCK)) {
 		printk(KERN_WARNING "hfs: Filesystem is marked locked, mounting read-only.\n");
 		sb->s_flags |= MS_RDONLY;
-	} else if ((vhdr->attributes & cpu_to_be32(HFSPLUS_VOL_JOURNALED)) && !(sb->s_flags & MS_RDONLY)) {
+	} else if (hfsplus_vol_has_journal(vhdr) && !(sb->s_flags & MS_RDONLY)) {
 		printk(KERN_WARNING "hfs: write access to a journaled filesystem is not supported, "
 		       "use the force option at your own risk, mounting read-only.\n");
 		sb->s_flags |= MS_RDONLY;
-- 
1.5.6.3

Re: [PATCH 2/2] Fix journal detection on HFS+.

[Jörn Engel]

On Wed, 19 November 2008 18:26:00 -0800, Warren Turkal wrote:
> 
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index eb74531..4f00a84 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -15,10 +15,17 @@
>  #include <linux/vfs.h>
>  #include <linux/nls.h>
>  
> +#include "hfsplus_fs.h"
> +
>  static struct inode *hfsplus_alloc_inode(struct super_block *sb);
>  static void hfsplus_destroy_inode(struct inode *inode);
> +static bool hfsplus_vol_has_journal(struct hfsplus_vh *vhdr);

Sorry for not noticing this before.  The function declaration is
unnecessary.  There are no callers before the actual function
definition, which is just below.

> -#include "hfsplus_fs.h"
> +static bool hfsplus_vol_has_journal(struct hfsplus_vh *vhdr)
> +{
> +	return (vhdr->attributes & cpu_to_be32(HFSPLUS_VOL_JOURNALED) &&
> +				vhdr->journal_info_block);
> +}

You're going through a huge number of iterations for such a simple
change.  Others would have lost patience some time ago. :(

Reviewed-By: Joern Engel <joern@logfs.org>

Jörn

-- 
Courage is not the absence of fear, but rather the judgement that
something else is more important than fear.
-- Ambrose Redmoon
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Fix journal detection on HFS+.

[Warren Turkal]

On Thu, Nov 20, 2008 at 3:29 AM, Jörn Engel <joern@logfs.org> wrote:
> On Wed, 19 November 2008 18:26:00 -0800, Warren Turkal wrote:
>>
>> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
>> index eb74531..4f00a84 100644
>> --- a/fs/hfsplus/super.c
>> +++ b/fs/hfsplus/super.c
>> @@ -15,10 +15,17 @@
>>  #include <linux/vfs.h>
>>  #include <linux/nls.h>
>>
>> +#include "hfsplus_fs.h"
>> +
>>  static struct inode *hfsplus_alloc_inode(struct super_block *sb);
>>  static void hfsplus_destroy_inode(struct inode *inode);
>> +static bool hfsplus_vol_has_journal(struct hfsplus_vh *vhdr);
>
> Sorry for not noticing this before.  The function declaration is
> unnecessary.  There are no callers before the actual function
> definition, which is just below.

I will have to get rid of this when I get home tonight.

>> -#include "hfsplus_fs.h"
>> +static bool hfsplus_vol_has_journal(struct hfsplus_vh *vhdr)
>> +{
>> +     return (vhdr->attributes & cpu_to_be32(HFSPLUS_VOL_JOURNALED) &&
>> +                             vhdr->journal_info_block);
>> +}
>
> You're going through a huge number of iterations for such a simple
> change.  Others would have lost patience some time ago. :(

Bah...I'm playing in someone else's sandbox, and I have a lot to learn
around here. I figure I probably shouldn't second guess the more
experienced folks.

wt
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] Fix journal detection on HFS+.

[Warren Turkal]

The code currently mounts an HFS+ volume read-only (unless given the "force"
mount option) if it detects that the HFS+ volume has a journal. The code was
unconditionally assuming that the volume had a jounal if the journal
attribute was set in the volume header. However, the volume also has to have
a non-zero journal info block to actually have a journal.

In this patch, I refactored the journal detection into a function since the
logic is used twice. The journal detection also uses the better logic to
determine if there is a journal.

Signed-off-by: Warren Turkal <wt@penguintechs.org>
---
 fs/hfsplus/super.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index eb74531..dcbcc24 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -20,6 +20,12 @@ static void hfsplus_destroy_inode(struct inode *inode);
 
 #include "hfsplus_fs.h"
 
+static bool hfsplus_vol_has_journal(struct hfsplus_vh *vhdr)
+{
+	return (vhdr->attributes & cpu_to_be32(HFSPLUS_VOL_JOURNALED) &&
+				vhdr->journal_info_block);
+}
+
 struct inode *hfsplus_iget(struct super_block *sb, unsigned long ino)
 {
 	struct hfs_find_data fd;
@@ -260,7 +266,7 @@ static int hfsplus_remount(struct super_block *sb, int *flags, char *data)
 			printk(KERN_WARNING "hfs: filesystem is marked locked, leaving read-only.\n");
 			sb->s_flags |= MS_RDONLY;
 			*flags |= MS_RDONLY;
-		} else if (vhdr->attributes & cpu_to_be32(HFSPLUS_VOL_JOURNALED)) {
+		} else if (hfsplus_vol_has_journal(vhdr)) {
 			printk(KERN_WARNING "hfs: filesystem is marked journaled, leaving read-only.\n");
 			sb->s_flags |= MS_RDONLY;
 			*flags |= MS_RDONLY;
@@ -356,7 +362,7 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 	} else if (vhdr->attributes & cpu_to_be32(HFSPLUS_VOL_SOFTLOCK)) {
 		printk(KERN_WARNING "hfs: Filesystem is marked locked, mounting read-only.\n");
 		sb->s_flags |= MS_RDONLY;
-	} else if ((vhdr->attributes & cpu_to_be32(HFSPLUS_VOL_JOURNALED)) && !(sb->s_flags & MS_RDONLY)) {
+	} else if (hfsplus_vol_has_journal(vhdr) && !(sb->s_flags & MS_RDONLY)) {
 		printk(KERN_WARNING "hfs: write access to a journaled filesystem is not supported, "
 		       "use the force option at your own risk, mounting read-only.\n");
 		sb->s_flags |= MS_RDONLY;
-- 
1.5.6.3

Re: [PATCH 2/2] Fix journal detection on HFS+.

[Roman Zippel]

Hi,

On Friday 21. November 2008, Warren Turkal wrote:

(Sorry for taking so long to get to this.)

> +static bool hfsplus_vol_has_journal(struct hfsplus_vh *vhdr)
> +{
> +	return (vhdr->attributes & cpu_to_be32(HFSPLUS_VOL_JOURNALED) &&
> +				vhdr->journal_info_block);
> +}

I'm curious how common it is to have the journal bit set but no journal block, 
I haven't seen this case so far.
IMO more useful would be to read the journal block and check if there is 
anything that needs to be replayed.
If you're interested in a second step you could replay the journal, it's not 
that difficult to do, it's pretty much just copying blocks around.

bye, Roman

Re: [PATCH 2/2] Fix journal detection on HFS+.

[Warren Turkal]

On Sat, Nov 22, 2008 at 8:52 PM, Roman Zippel <zippel@linux-m68k.org> wrote:
> I'm curious how common it is to have the journal bit set but no journal block,
> I haven't seen this case so far.

It's so uncommon that the technote for HFS+ doesn't mention it.
However, I did find [1], and it's (c) by Apple. Look at the comment at
[2] to see what tipped me off.

> IMO more useful would be to read the journal block and check if there is
> anything that needs to be replayed.
> If you're interested in a second step you could replay the journal, it's not
> that difficult to do, it's pretty much just copying blocks around.

I am actually looking into doing the journal replaying, but i haven't
gotten around to it yet.

[1]http://fxr.watson.org/fxr/source/bsd/hfs/hfs_format.h?v=xnu-1228#L626
[2]http://fxr.watson.org/fxr/source/bsd/hfs/hfs_format.h?v=xnu-1228#L633

wt

Re: [PATCH 2/2] Fix journal detection on HFS+.

[Roman Zippel]

Hi,

On Tue, 25 Nov 2008, Warren Turkal wrote:

> On Sat, Nov 22, 2008 at 8:52 PM, Roman Zippel <zippel@linux-m68k.org> wrote:
> > I'm curious how common it is to have the journal bit set but no journal block,
> > I haven't seen this case so far.
> 
> It's so uncommon that the technote for HFS+ doesn't mention it.
> However, I did find [1], and it's (c) by Apple. Look at the comment at
> [2] to see what tipped me off.

That likely also means the journal bit is cleared. If you want to verify 
this, you should check the actual kernel source and there I can't find 
anywhere, that they handle a zero journal block specially, thus it will 
result in a failure to replay the journal, so I don't see why we should 
allow write access to the volume in this case.

> > IMO more useful would be to read the journal block and check if there is
> > anything that needs to be replayed.
> > If you're interested in a second step you could replay the journal, it's not
> > that difficult to do, it's pretty much just copying blocks around.
> 
> I am actually looking into doing the journal replaying, but i haven't
> gotten around to it yet.

That's why I suggested to do the replay check first, as it's relatively 
simple to do.

bye, Roman

Re: [PATCH 2/2] Fix journal detection on HFS+.

[Warren Turkal]

I have a drive that has the property of having the journal bit set and
having 0 in the journal_info_block field that mounts in rw on MacOS X.
That's how I discovered the problem.

wt

On Thu, Nov 27, 2008 at 8:21 AM, Roman Zippel <zippel@linux-m68k.org> wrote:
> Hi,
>
> On Tue, 25 Nov 2008, Warren Turkal wrote:
>
>> On Sat, Nov 22, 2008 at 8:52 PM, Roman Zippel <zippel@linux-m68k.org> wrote:
>> > I'm curious how common it is to have the journal bit set but no journal block,
>> > I haven't seen this case so far.
>>
>> It's so uncommon that the technote for HFS+ doesn't mention it.
>> However, I did find [1], and it's (c) by Apple. Look at the comment at
>> [2] to see what tipped me off.
>
> That likely also means the journal bit is cleared. If you want to verify
> this, you should check the actual kernel source and there I can't find
> anywhere, that they handle a zero journal block specially, thus it will
> result in a failure to replay the journal, so I don't see why we should
> allow write access to the volume in this case.
>
>> > IMO more useful would be to read the journal block and check if there is
>> > anything that needs to be replayed.
>> > If you're interested in a second step you could replay the journal, it's not
>> > that difficult to do, it's pretty much just copying blocks around.
>>
>> I am actually looking into doing the journal replaying, but i haven't
>> gotten around to it yet.
>
> That's why I suggested to do the replay check first, as it's relatively
> simple to do.
>
> bye, Roman
>

Re: [PATCH 2/2] Fix journal detection on HFS+.

[Roman Zippel]

Hi,

On Mon, 1 Dec 2008, Warren Turkal wrote:

> I have a drive that has the property of having the journal bit set and
> having 0 in the journal_info_block field that mounts in rw on MacOS X.
> That's how I discovered the problem.

Which OS X version? How was that drive created?
Is the journal info block still zero after being mounted in OS X?
Does Disk Utility report the drive as journaled?

The main point I don't understand is how it became zero in first place.
OS X will certainly mount such a drive, but I'd expect it'll simply 
reinitialize the journal.

bye, Roman

Re: [PATCH 2/2] Fix journal detection on HFS+.

[Roman Zippel]

Hi,

On Mon, 1 Dec 2008, Warren Turkal wrote:

> I have a drive that has the property of having the journal bit set and
> having 0 in the journal_info_block field that mounts in rw on MacOS X.
> That's how I discovered the problem.

Another possibility: Was the drive mounted under any other OS? Maybe some 
other OS cleared that block while mounting and continued without journal, 
so that OS X would be forced to reinitialize the journal.

bye, Roman