hfsplus journal detection - try 4

hfsplus journal detection - try 4

[Warren Turkal]

Here's another shot at this patch. I found a small issue with sparse.
If this is suitable for inclusion, could someone please pull it into
their tree so that it can be merged upstream?

Thanks,
wt

[PATCH] Identify journal info block in volume header.

[Warren Turkal]

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] Fix journal detection on HFS+.

[Warren Turkal]

The code was unconditionally assumming that the volume had a jounal if the
jounal 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.

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

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index eb74531..128101b 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -17,9 +17,16 @@
 
 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)
 {
 	struct hfs_find_data fd;
@@ -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

[PATCH] Fix header include.

[Warren Turkal]

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

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 128101b..4f00a84 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -15,12 +15,12 @@
 #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) &&
-- 
1.5.6.3

Re: [PATCH] Fix header include.

[Andrew Morton]

On Tue, 11 Nov 2008 12:56:48 -0800
Warren Turkal <wt@penguintechs.org> wrote:

> Signed-off-by: Warren Turkal <wt@penguintechs.org>
> ---
>  fs/hfsplus/super.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index 128101b..4f00a84 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -15,12 +15,12 @@
>  #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) &&

again: what was wrong with the old code??

If it was causing some compilation problem then please quote the compiler
output in the changelog.

Re: [PATCH] Fix header include.

[Brad Boyer]

On Wed, Nov 19, 2008 at 02:51:40PM -0800, Andrew Morton wrote:
> again: what was wrong with the old code??
> 
> If it was causing some compilation problem then please quote the compiler
> output in the changelog.

I didn't write this patch, but I was the original author of this driver.
The issue here is that Apple added a journal feature after the fact and
the current Linux driver doesn't support that. The current code just
checks for the equivalent of a feature bit, while this change makes it
also confirm that there really is a journal file. Without this change,
an HFS+ file system that has been setup to do transactional changes but
doesn't currently have an actual journal file allocated will not be
mountable with the Linux driver.

Just as a side note, although I am the original author I have never
claimed to maintain this code. In fact, I wasn't the one that submitted
it for inclusion. In any case, I don't plan on adding myself as maintainer.

	Brad Boyer
	flar@allandria.com

Re: [PATCH] Fix header include.

[Brad Boyer]

On Wed, Nov 19, 2008 at 02:51:40PM -0800, Andrew Morton wrote:
> On Tue, 11 Nov 2008 12:56:48 -0800
> Warren Turkal <wt@penguintechs.org> wrote:
> > @@ -15,12 +15,12 @@
> >  #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) &&
> 
> again: what was wrong with the old code??

I just realized what you were asking after I sent my other reply. The
include needs to move so 'struct hfsplus_vh' is declared before the
function prototype. That happens in hfsplus_fs.h.

My apologies for the noise, although I stand by my statement that I
have no intention of actively maintaining this code myself.

> If it was causing some compilation problem then please quote the compiler
> output in the changelog.

The original submitter would need to include the actual compiler output.

	Brad Boyer
	flar@allandria.com

Re: [PATCH] Fix header include.

[Warren Turkal]

I just submitted a patchset with more detailed commit comments.. It's
the same change wrapped in a, hopefully, prettier candy coating.

wt

On Wed, Nov 19, 2008 at 4:41 PM, Brad Boyer <flar@allandria.com> wrote:
> On Wed, Nov 19, 2008 at 02:51:40PM -0800, Andrew Morton wrote:
>> On Tue, 11 Nov 2008 12:56:48 -0800
>> Warren Turkal <wt@penguintechs.org> wrote:
>> > @@ -15,12 +15,12 @@
>> >  #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) &&
>>
>> again: what was wrong with the old code??
>
> I just realized what you were asking after I sent my other reply. The
> include needs to move so 'struct hfsplus_vh' is declared before the
> function prototype. That happens in hfsplus_fs.h.
>
> My apologies for the noise, although I stand by my statement that I
> have no intention of actively maintaining this code myself.
>
>> If it was causing some compilation problem then please quote the compiler
>> output in the changelog.
>
> The original submitter would need to include the actual compiler output.
>
>        Brad Boyer
>        flar@allandria.com
>
>

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

[Andrew Morton]

On Tue, 11 Nov 2008 12:56:47 -0800
Warren Turkal <wt@penguintechs.org> wrote:

> The code was unconditionally assumming that the volume had a jounal if the
> jounal 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.

OK, but so what?

Presumably there is some situation in which this is causing you a
problem, but what is that situation, and what was the kernel's
behaviour in that situation?

> Signed-off-by: Warren Turkal <wt@penguintechs.org>
> ---
>  fs/hfsplus/super.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index eb74531..128101b 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -17,9 +17,16 @@
>  
>  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)
>  {
>  	struct hfs_find_data fd;
> @@ -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;

The patch itself looks OK to me.

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

[Warren Turkal]

The kernel was mounting an HFS+ FS ro if the volume header indicated
that it had a journal and the journal info block was 0. The kernel
should just treat the volume as not having a journal in that case.

wt

On Wed, Nov 19, 2008 at 2:51 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 11 Nov 2008 12:56:47 -0800
> Warren Turkal <wt@penguintechs.org> wrote:
>
>> The code was unconditionally assumming that the volume had a jounal if the
>> jounal 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.
>
> OK, but so what?
>
> Presumably there is some situation in which this is causing you a
> problem, but what is that situation, and what was the kernel's
> behaviour in that situation?
>
>> Signed-off-by: Warren Turkal <wt@penguintechs.org>
>> ---
>>  fs/hfsplus/super.c |   11 +++++++++--
>>  1 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
>> index eb74531..128101b 100644
>> --- a/fs/hfsplus/super.c
>> +++ b/fs/hfsplus/super.c
>> @@ -17,9 +17,16 @@
>>
>>  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)
>>  {
>>       struct hfs_find_data fd;
>> @@ -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;
>
> The patch itself looks OK to me.
>

Re: [PATCH] Identify journal info block in volume header.

[Andrew Morton]

On Tue, 11 Nov 2008 12:56:46 -0800
Warren Turkal <wt@penguintechs.org> wrote:

> 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;

Please include a changelog which fully describes the reason for making
this change.

Re: hfsplus journal detection - try 4

[Warren Turkal]

Did anyone pick up this patch set?

wt

On Tue, Nov 11, 2008 at 12:56 PM, Warren Turkal <wt@penguintechs.org> wrote:
> Here's another shot at this patch. I found a small issue with sparse.
> If this is suitable for inclusion, could someone please pull it into
> their tree so that it can be merged upstream?
>
> Thanks,
> wt
>

Re: hfsplus journal detection - try 4

[Jörn Engel]

On Tue, 18 November 2008 23:07:31 -0800, Warren Turkal wrote:
> 
> Did anyone pick up this patch set?

You could try sending it to someone who cares.  Roman apparently
doesn't, hfsplus is officially unmaintained.  Guess that leaves Andrew
Morton.

Jörn

-- 
There's nothing better for promoting creativity in a medium than
making an audience feel "Hmm ­ I could do better than that!"
-- Douglas Adams in a slashdot interview
--
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: hfsplus journal detection - try 4

[Warren Turkal]

Andrew,

I am sending this to you since it's a small patchset and I'm not
getting any traction with the alleged HFS+ maintainer. Is this small
enough to make it into 2.6.28 or will it have to wait for the next
cycle?

BTW, I am the guy who just came to your cube to bug you.

Thanks,
wt

On Tue, Nov 11, 2008 at 12:56 PM, Warren Turkal <wt@penguintechs.org> wrote:
> Here's another shot at this patch. I found a small issue with sparse.
> If this is suitable for inclusion, could someone please pull it into
> their tree so that it can be merged upstream?

Re: hfsplus journal detection - try 4

[Andrew Morton]

On Tue, 11 Nov 2008 12:56:45 -0800
Warren Turkal <wt@penguintechs.org> wrote:

> Here's another shot at this patch. I found a small issue with sparse.
> If this is suitable for inclusion, could someone please pull it into
> their tree so that it can be merged upstream?

I don't follow linux-fsdevel much.  cc'ing linux-kernel gets my attention.

This patchset is missing an overall description of the problem which is
being solved.  I can kinda see what the changes do, but it would be
much better if we were provided with a description of the kernel
behaviour before and after the patches.