JBD2: Allow feature checks before journal recovery

PATCH In review Patrick J. LoPresti 5 participants 12 messages Jul 11, 2010 – Jul 21, 2010

Subsystems (3)

VFS OCFS2 Journalling layer for block devices (JBD2)

lore

Share & tools

lore

Patrick J. LoPresti

author

2010-07-11 17:03 UTC #0 lore raw

PATCH 1/2 JBD2: Allow feature checks before journal recovery

Before we start accessing a huge (> 16 TiB) OCFS2 volume, we need to confirm that its journal supports 64-bit offsets. So we need to check the journal's feature bits before recovering the journal.

This is not possible with JBD2 at present, because the journal superblock (where the feature bits reside) is not loaded from disk until the journal is recovered.

This patch loads the journal superblock in jbd2_journal_check_used_features() if it has not already been loaded, allowing us to check the feature bits before journal recovery.

Signed-off-by Patrick LoPresti <[email protected]>

fs/jbd2/journal.c +2 -0

index bc2ff59..c5a864f 100644
@@ -1365,6 +1365,8 @@ int jbd2_journal_check_used_features (journal_t *journal, unsigned long compat,
 
 	if (!compat && !ro && !incompat)
 		return 1;
+	if (journal_get_superblock(journal))
+		return 0;
 	if (journal->j_format_version == 1)
 		return 0;

Patrick J. LoPresti

author

2010-07-11 17:04 UTC #1 lore raw

PATCH 2/2 OCFS2: Allow huge (> 16 TiB) volumes to mount

The OCFS2 developers have already done all of the hard work to allow volumes larger than 16 TiB. But there is still a "sanity check" in fs/ocfs2/super.c that prevents the mounting of such volumes, even when the cluster size and journal options would allow it.

This patch replaces that sanity check with a more sophisticated one to mount a huge volume provided that (a) it is addressable by the raw word/address size of the system (borrowing a test from ext4); (b) the volume is using JBD2; and (c) the JBD2_FEATURE_INCOMPAT_64BIT flag is set on the journal.

I factored out the sanity check into its own function. I also moved it from ocfs2_initialize_super() down to ocfs2_check_volume(); any earlier, and the journal will not have been initialized yet.

This patch is one of a pair, and it depends on the other ("JBD2: Allow feature checks before journal recovery").

I have tested this patch on small volumes, huge volumes, and huge volumes without 64-bit block support in the journal. All of them appear to work or to fail gracefully, as appropriate.

Signed-off-by Patrick LoPresti <[email protected]>

fs/ocfs2/super.c +41 -7

index 0eaa929..b809508 100644
@@ -1991,6 +1991,47 @@ static int ocfs2_setup_osb_uuid(struct ocfs2_super *osb, const unsigned char *uu
 	return 0;
 }
 
+/* Check to make sure entire volume is addressable on this system.
+   Requires osb_clusters_at_boot to be valid and for the journal to
+   have been initialized by ocfs2_journal_init(). */
+static int ocfs2_check_addressable(struct ocfs2_super *osb)
+{
+	int status = 0;
+	u64 max_block =
+		ocfs2_clusters_to_blocks(osb->sb,
+					 osb->osb_clusters_at_boot) - 1;
+
+	/* Absolute addressability check (borrowed from ext4/super.c) */
+	if ((max_block >
+	     (sector_t)(~0LL) >> (osb->sb->s_blocksize_bits - 9)) ||
+	    (max_block > (pgoff_t)(~0LL) >> (PAGE_CACHE_SHIFT -
+					     osb->sb->s_blocksize_bits))) {
+		mlog(ML_ERROR, "Volume too large "
+		     "to mount safely on this system");
+		status = -EFBIG;
+		goto out;
+	}
+
+	/* 32-bit block number is always OK. */
+	if (max_block <= (u32)~0UL)
+		goto out;
+
+	/* Volume is "huge", so see if our journal is new enough to
+	   support it. */
+	if (!(OCFS2_HAS_COMPAT_FEATURE(osb->sb,
+				       OCFS2_FEATURE_COMPAT_JBD2_SB) &&
+	      jbd2_journal_check_used_features(osb->journal->j_journal, 0, 0,
+					       JBD2_FEATURE_INCOMPAT_64BIT))) {
+		mlog(ML_ERROR, "The journal cannot address the entire volume. "
+		     "Enable the 'block64' journal option with tunefs.ocfs2");
+		status = -EFBIG;
+		goto out;
+	}
+
+ out:
+	return status;
+}
+
 static int ocfs2_initialize_super(struct super_block *sb,
 				  struct buffer_head *bh,
 				  int sector_size,

@@ -2215,14 +2256,6 @@ static int ocfs2_initialize_super(struct super_block *sb,
 		goto bail;
 	}
 
-	if (ocfs2_clusters_to_blocks(osb->sb, le32_to_cpu(di->i_clusters) - 1)
-	    > (u32)~0UL) {
-		mlog(ML_ERROR, "Volume might try to write to blocks beyond "
-		     "what jbd can address in 32 bits.\n");
-		status = -EINVAL;
-		goto bail;
-	}

Andreas Dilger

2010-07-13 00:21 UTC #2 lore raw

Re: [PATCH 2/2] OCFS2: Allow huge (> 16 TiB) volumes to mount

Quoted 5 lines · Patrick J. LoPresti

fs/ocfs2/super.c +5 -0

+/* Check to make sure entire volume is addressable on this system.
+   Requires osb_clusters_at_boot to be valid and for the journal to
+   have been initialized by ocfs2_journal_init(). */
+static int ocfs2_check_addressable(struct ocfs2_super *osb)
+{

fs/ocfs2/super.c static int ocfs2_setup_osb_uuid(struct ocfs2_super *osb, const unsigned char *uu · line 2013

Quoted 10 lines

fs/ocfs2/super.c +10 -0

+	/* Absolute addressability check (borrowed from ext4/super.c) */
+	if ((max_block >
+	     (sector_t)(~0LL) >> (osb->sb->s_blocksize_bits - 9)) ||
+	    (max_block > (pgoff_t)(~0LL) >> (PAGE_CACHE_SHIFT -
+					     osb->sb->s_blocksize_bits))) {
+		mlog(ML_ERROR, "Volume too large "
+		     "to mount safely on this system");
+		status = -EFBIG;
+		goto out;
+	}

This hunk of code is actually in several filesystems. It wouldn't be a bad idea to make it a library function that can be called by the filesystem to check the kernel page cache and block layer can handle these large filesystems.

Cheers, Andreas

Patrick J. LoPresti

author

2010-07-13 01:08 UTC #3 lore raw

Re: [PATCH 2/2] OCFS2: Allow huge (> 16 TiB) volumes to mount

Quoted 6 lines · Andreas Dilger

Quoted 5 lines · Patrick J. LoPresti
+ /* Absolute addressability check (borrowed from ext4/super.c) */
+ if ((max_block >
+ (sector_t)(~0LL) >> (osb->sb->s_blocksize_bits - 9)) ||
+ (max_block > (pgoff_t)(~0LL) >> (PAGE_CACHE_SHIFT

Dave Chinner

2010-07-13 01:25 UTC #4 lore raw

Re: [PATCH 2/2] OCFS2: Allow huge (> 16 TiB) volumes to mount

Quoted 14 lines · Patrick J. LoPresti

Quoted 13 lines · Andreas Dilger
Quoted 11 lines · Patrick J. LoPresti
+ /* Absolute addressability check (borrowed from ext4/super.c) */
+ if ((max_block >
+ (sector_t)(~0LL) >> (osb->sb->s_blocksize_bits - 9)) ||
+ (max_block > (pgoff_t)(~0LL) >> (PAGE_CACHE_SHIFT -
+ osb->sb->s_blocksize_bits))) {
+ mlog(ML_ERROR, "Volume too large "
+ "to mount safely on this system");
+ status = -EFBIG;
+ goto out;
+ }

Quoted 5 lines

Quoted 1 line
This hunk of code is actually in several filesystems. It wouldn't be a bad idea to make it a library function that can be called by the filesystem to check the kernel page cache and block layer can handle these large filesystems.
True, but some of them do it differently (e.g. see the #if switch in xfs_sb_validate_fsb_count). Tracking down all variants and changing them is a much larger task than my simple patch.

The XFS code is different to the above because there is still a 16TB size limit on 32 bit systemsi (i.e. page cache address limits). IOWs, you can't just remove the above 16TB check unless you (i.e. OCFS2) handle >16TB block devices on 32 bit systems correctly...

Cheers,

Dave.

Signature · 6 lines

Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patrick J. LoPresti

author

2010-07-13 01:37 UTC #5 lore raw

Re: [PATCH 2/2] OCFS2: Allow huge (> 16 TiB) volumes to mount

Quoted 5 lines · Dave Chinner

The XFS code is different to the above because there is still a 16TB size limit on 32 bit systemsi (i.e. page cache address limits). IOWs, you can't just remove the above 16TB check unless you (i.e. OCFS2) handle >16TB block devices on 32 bit systems correctly...

If you look at my patch, you will see that is precisely what it does. As the comments indicate, it uses the exact same check as ext4, which will correctly refuse to mount huge volumes on 32-bit systems.

The XFS test appears to be the same thing written a little differently. Andreas is suggesting that somebody should factor out this check into a common library routine. That sounds like a fine idea, but it also sounds orthogonal to the (simple and useful) patch I am attempting to submit.

- Pat

Andreas Dilger

2010-07-13 04:46 UTC #6 lore raw

Re: [PATCH 2/2] OCFS2: Allow huge (> 16 TiB) volumes to mount

In reply to Patrick J. LoPresti

Quoted 14 lines · Patrick J. LoPresti

Quoted 13 lines · Andreas Dilger

Quoted 11 lines · Patrick J. LoPresti

fs/ocfs2/super.c +10 -0

+     /* Absolute addressability check (borrowed from ext4/super.c) */
+     if ((max_block >
+          (sector_t)(~0LL) >> (osb->sb->s_blocksize_bits - 9)) ||
+         (max_block > (pgoff_t)(~0LL) >> (PAGE_CACHE_SHIFT -
+                                          osb->sb->s_blocksize_bits))) {
+             mlog(ML_ERROR, "Volume too large "
+                  "to mount safely on this system");
+             status = -EFBIG;
+             goto out;
+     }

Quoted 8 lines

Quoted 1 line
This hunk of code is actually in several filesystems. It wouldn't be a bad idea to make it a library function that can be called by the filesystem to check the kernel page cache and block layer can handle these large filesystems.
True, but some of them do it differently (e.g. see the #if switch in xfs_sb_validate_fsb_count). Tracking down all variants and changing them is a much larger task than my simple patch.
Are you suggesting I need to do this before my patch is accepted at all? Or is this a refactoring that can happen later?

I'm just suggesting it should be done at some point. I thought it would be better to do it first, rather than add yet another copy of this code. That said, I hate to block useful fixes because of cleanup (and I have no control over OCFS2 anyway :-). However, I've found that once the fix is in people usually forget (or become too busy) to do the cleanup and it just lingers on unseen.

Cheers, Andreas

Patrick J. LoPresti

author

2010-07-13 05:00 UTC #7 lore raw

Re: [PATCH 2/2] OCFS2: Allow huge (> 16 TiB) volumes to mount

Quoted 6 lines · Andreas Dilger

Quoted 3 lines · Patrick J. LoPresti
Are you suggesting I need to do this before my patch is accepted at all? Or is this a refactoring that can happen later?
I'm just suggesting it should be done at some point. I thought it would be better to do it first, rather than add yet another copy of this code. That said, I hate to block useful fixes because of cleanup (and I have no control over OCFS2 anyway :-). However, I've found that once the fix is in people usually forget (or become too busy) to do the cleanup and it just lingers on unseen.

I hear you.

I do not object to factoring out the basic addressability test and using it in my patch, leaving it for others -- like yourself :-) -- to modify other file systems to invoke it.

Does that sound like a reasonable compromise? If so, where should the function live and what should it be called, do you think?

- Pat
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Joel Becker

2010-07-13 08:10 UTC #8 lore raw

Re: [Ocfs2-devel] [PATCH 2/2] OCFS2: Allow huge (> 16 TiB) volumes to mount

Quoted 6 lines · Patrick J. LoPresti

Quoted 5 lines · Andreas Dilger
Quoted 3 lines · Patrick J. LoPresti
Are you suggesting I need to do this before my patch is accepted at all? Or is this a refactoring that can happen later?

Quoted 7 lines

Quoted 1 line
I'm just suggesting it should be done at some point. I thought it would be better to do it first, rather than add yet another copy of this code. That said, I hate to block useful fixes because of cleanup (and I have no control over OCFS2 anyway :-). However, I've found that once the fix is in people usually forget (or become too busy) to do the cleanup and it just lingers on unseen.
I hear you.
I do not object to factoring out the basic addressability test and using it in my patch, leaving it for others -- like yourself :-) -- to modify other file systems to invoke it.

I think you should modify ext3 and xfs, as they clearly are
partaking of this functionality. I'll happily review it for you. Put the call in fs/libfs.c. Call it generic_check_addressable(struct super_block *super).

Joel

Signature · 12 lines

"The only way to get rid of a temptation is to yield to it."
         - Oscar Wilde 

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jan Kara

reviewer

2010-07-21 17:27 UTC #9 lore raw

In reply to Patrick J. LoPresti

Quoted 19 lines

Before we start accessing a huge (> 16 TiB) OCFS2 volume, we need to confirm that its journal supports 64-bit offsets. So we need to check the journal's feature bits before recovering the journal.
This is not possible with JBD2 at present, because the journal superblock (where the feature bits reside) is not loaded from disk until the journal is recovered.
This patch loads the journal superblock in jbd2_journal_check_used_features() if it has not already been loaded, allowing us to check the feature bits before journal recovery.
Signed-off-by Patrick LoPresti <[email protected]>
fs/jbd2/journal.c +0 -0
index bc2ff59..c5a864f 100644

fs/jbd2/journal.c int jbd2_journal_check_used_features (journal_t *journal, unsigned long compat, · line 1371

Quoted 7 lines

fs/jbd2/journal.c +2 -0

@@ -1365,6 +1365,8 @@ int jbd2_journal_check_used_features (journal_t *journal, unsigned long compat,
 
 	if (!compat && !ro && !incompat)
 		return 1;
+	if (journal_get_superblock(journal))
+		return 0;
 	if (journal->j_format_version == 1)
 		return 0;

This looks OK in principle. It would be even nicer to avoid all the checks
journal_get_superblock() when the superblock is actually loaded so that we don't do them each time jbd2_journal_check_used_features is called...

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SuSE CR Labs

Patrick J. LoPresti

author

2010-07-21 17:42 UTC #10 lore raw

Quoted 8 lines · Jan Kara

Quoted 8 lines
Signed-off-by Patrick LoPresti <[email protected]>
fs/jbd2/journal.c +0 -0
index bc2ff59..c5a864f 100644
@@ -1365,6 +1365,8 @@ int jbd2_journal_check_used_features (journal_t *journal, unsigned long compat,
 

Quoted 10 lines

fs/jbd2/journal.c int jbd2_journal_check_used_features (journal_t *journal, unsigned long compat, · line 1371
Quoted 6 lines
fs/jbd2/journal.c +2 -0
       if (!compat && !ro && !incompat)
               return 1;
+     if (journal_get_superblock(journal))
+             return 0;
       if (journal->j_format_version == 1)
               return 0;
This looks OK in principle. It would be even nicer to avoid all the checks journal_get_superblock() when the superblock is actually loaded so that we don't do them each time jbd2_journal_check_used_features is called...

How about this?

         if (!compat && !ro && !incompat)
                return 1;
+        if (journal->j_format_version == 0 &&
journal_get_superblock(journal) != 0)
+               return 0;
        if (journal->j_format_version == 1)
                return 0;

journal_init_common() uses kzalloc() to allocate the journal_t, and journal_get_superblock() fills it in, so I believe this is a valid test.

- Pat
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jan Kara

reviewer

2010-07-21 17:50 UTC #11 lore raw

Quoted 30 lines · Jan Kara

Quoted 8 lines
Quoted 8 lines
Signed-off-by Patrick LoPresti <[email protected]>
fs/jbd2/journal.c +0 -0
index bc2ff59..c5a864f 100644
@@ -1365,6 +1365,8 @@ int jbd2_journal_check_used_features (journal_t *journal, unsigned long compat,
 
Quoted 10 lines
fs/jbd2/journal.c int jbd2_journal_check_used_features (journal_t *journal, unsigned long compat, · line 1371
Quoted 6 lines
fs/jbd2/journal.c +2 -0
       if (!compat && !ro && !incompat)
               return 1;
+     if (journal_get_superblock(journal))
+             return 0;
       if (journal->j_format_version == 1)
               return 0;
This looks OK in principle. It would be even nicer to avoid all the checks journal_get_superblock() when the superblock is actually loaded so that we don't do them each time jbd2_journal_check_used_features is called...
How about this?
         if (!compat && !ro && !incompat)
                return 1;
+        if (journal->j_format_version == 0 &&
journal_get_superblock(journal) != 0)
+               return 0;
        if (journal->j_format_version == 1)
                return 0;

Quoted 3 lines

journal_init_common() uses kzalloc() to allocate the journal_t, and journal_get_superblock() fills it in, so I believe this is a valid test.

Yes, this looks OK. Just add a comment before the check like
/* Load journal super block if it isn't loaded yet */

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SuSE CR Labs