Skip to contentSkip to conversation

ext3/ext4: Factor out disk addressability check

PATCH In review Patrick J. LoPresti 6 participants 32 messages Jul 22, 2010 – Aug 16, 2010

Subsystems (4)
VFS ext4 file system OCFS2 Journalling layer for block devices (JBD2)
lore
Share & tools
lore

Patrick J. LoPresti

author
#0 lore raw

PATCH 1/3 ext3/ext4: Factor out disk addressability check

As part of adding support for OCFS2 to mount huge volumes, we need to check that the sector_t and page cache of the system are capable of addressing the entire volume.

An identical check already appears in ext3 and ext4. This patch moves the addressability check into its own function in fs/libfs.c and modifies ext3 and ext4 to invoke it.

Signed-off-by Patrick LoPresti <[email protected]>
include/linux/fs.h +2 -0
index 471e1ff..6aff3f5 100644
@@ -2399,6 +2399,8 @@ extern ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
 
 extern int generic_file_fsync(struct file *, int);
 
+extern int generic_check_addressable(unsigned, u64);
+
 #ifdef CONFIG_MIGRATION
 extern int buffer_migrate_page(struct address_space *,
 				struct page *, struct page *);
fs/libfs.c +32 -0
index dcaf972..b969648 100644
@@ -955,6 +955,38 @@ int generic_file_fsync(struct file *file, int datasync)
 }
 EXPORT_SYMBOL(generic_file_fsync);
 
+/**
+ * generic_check_addressable - Check addressability of file system
+ * @blocksize_bits:	log of file system block size
+ * @num_blocks:		number of blocks in file system
+ *
+ * Determine whether a file system with @num_blocks blocks (and a
+ * block size of 2**@blocksize_bits) is addressable by the sector_t
+ * and page cache of the system.  Return 0 if so and -EFBIG otherwise.
+ */
+int generic_check_addressable(unsigned blocksize_bits, u64 num_blocks)
+{
+	u64 last_fs_block = num_blocks - 1;
+
+	BUG_ON(blocksize_bits < 9);
+	BUG_ON(blocksize_bits > PAGE_CACHE_SHIFT);
+
+	if (unlikely(num_blocks == 0))
+		return 0;
+
+	printk(KERN_INFO "HERE %u %lu %u %u", blocksize_bits, last_fs_block,
+	       sizeof(sector_t), sizeof(pgoff_t));
+
+	if ((last_fs_block >
+	     (sector_t)(~0ULL) >> (blocksize_bits - 9)) ||
+	    (last_fs_block >
+	     (pgoff_t)(~0ULL) >> (PAGE_CACHE_SHIFT - blocksize_bits))) {
+		return -EFBIG;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(generic_check_addressable);
+
 /*
  * No-op implementation of ->fsync for in-memory filesystems.
  */
fs/ext3/super.c +2 -2
index 6c953bb..d0643db 100644
@@ -1862,8 +1862,8 @@ static int ext3_fill_super (struct super_block *sb, void *data, int silent)
 		goto failed_mount;
 	}
 
-	if (le32_to_cpu(es->s_blocks_count) >
-		    (sector_t)(~0ULL) >> (sb->s_blocksize_bits - 9)) {
+	if (generic_check_addressable(sb->s_blocksize_bits,
+				      le32_to_cpu(es->s_blocks_count))) {
 		ext3_msg(sb, KERN_ERR,
 			"error: filesystem is too large to mount safely");
 		if (sizeof(sector_t) < 8)
fs/ext4/super.c +3 -5
index 4e8983a..979cc57 100644
@@ -2706,15 +2706,13 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	 * Test whether we have more sectors than will fit in sector_t,
 	 * and whether the max offset is addressable by the page cache.
 	 */
-	if ((ext4_blocks_count(es) >
-	     (sector_t)(~0ULL) >> (sb->s_blocksize_bits - 9)) ||
-	    (ext4_blocks_count(es) >
-	     (pgoff_t)(~0ULL) >> (PAGE_CACHE_SHIFT - sb->s_blocksize_bits))) {
+	ret = generic_check_addressable(sb->s_blocksize_bits,
+					ext4_blocks_count(es));
+	if (ret) {
 		ext4_msg(sb, KERN_ERR, "filesystem"
 			 " too large to mount safely on this system");
 		if (sizeof(sector_t) < 8)
 			ext4_msg(sb, KERN_WARNING, "CONFIG_LBDAF not enabled");
-		ret = -EFBIG;
 		goto failed_mount;
 	}
author
#1 lore raw

PATCH 2/3 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. In particular, 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 +4 -0
index bc2ff59..5cfd8d4 100644
@@ -1365,6 +1365,10 @@ int jbd2_journal_check_used_features (journal_t *journal, unsigned long compat,
 
 	if (!compat && !ro && !incompat)
 		return 1;
+	/* Load journal superblock if it is not loaded yet. */
+	if (journal->j_format_version == 0 &&
+	    journal_get_superblock(journal) != 0)
+		return 0;
 	if (journal->j_format_version == 1)
 		return 0;
author
#2 lore raw

PATCH 3/3 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 +46 -5
index 0eaa929..76dac4c 100644
@@ -1991,6 +1991,36 @@ static int ocfs2_setup_osb_uuid(struct ocfs2_super *osb, const unsigned char *uu
 	return 0;
 }
 
+/* Make sure entire volume is addressable by our journal.  Requires
+   osb_clusters_at_boot to be valid and for the journal to have been
+   initialized by ocfs2_journal_init(). */
+static int ocfs2_journal_addressable(struct ocfs2_super *osb)
+{
+	int status = 0;
+	u64 max_block =
+		ocfs2_clusters_to_blocks(osb->sb,
+					 osb->osb_clusters_at_boot) - 1;
+
+	/* 32-bit block number is always OK. */
+	if (max_block <= (u32)~0ULL)
+		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,

@@ -2003,6 +2033,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
 	struct ocfs2_journal *journal;
 	__le32 uuid_net_key;
 	struct ocfs2_super *osb;
+	u64 total_blocks;
 
 	mlog_entry_void();
 

@@ -2215,11 +2246,15 @@ 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;
+	total_blocks = ocfs2_clusters_to_blocks(osb->sb,
+						le32_to_cpu(di->i_clusters));
+
+	status = generic_check_addressable(osb->sb->s_blocksize_bits,
+					   total_blocks);
+	if (status) {
+		mlog(ML_ERROR, "Volume too large "
+		     "to mount safely on this system");
+		status = -EFBIG;
 		goto bail;
 	}
 

@@ -2381,6 +2416,12 @@ static int ocfs2_check_volume(struct ocfs2_super *osb)
 		goto finally;
 	}
 
+	/* Now that journal has been initialized, check to make sure
+	   entire volume is addressable. */
+	status = ocfs2_journal_addressable(osb);
+	if (status)
+		goto finally;
+
 	/* If the journal was unmounted cleanly then we don't want to
 	 * recover anything. Otherwise, journal_load will do that
 	 * dirty work for us :) */
#9 lore raw
In reply to Patrick J. LoPresti
Quoted 9 lines · Patrick J. LoPresti

As part of adding support for OCFS2 to mount huge volumes, we need to check that the sector_t and page cache of the system are capable of addressing the entire volume.

An identical check already appears in ext3 and ext4. This patch moves the addressability check into its own function in fs/libfs.c and modifies ext3 and ext4 to invoke it.

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

ext4/jbd2 folks,
I would like to push these upstream. Should they go through
ocfs2.git or would you rather take them via your trees?

Joel

Signature · 10 lines
"In the arms of the angel, fly away from here,
 From this dark, cold hotel room and the endlessness that you fear.
 You are pulled from the wreckage of your silent reverie.
 In the arms of the angel, may you find some comfort here."

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
#10 lore raw
In reply to Patrick J. LoPresti
Quoted 9 lines · Patrick J. LoPresti

As part of adding support for OCFS2 to mount huge volumes, we need to check that the sector_t and page cache of the system are capable of addressing the entire volume.

An identical check already appears in ext3 and ext4. This patch moves the addressability check into its own function in fs/libfs.c and modifies ext3 and ext4 to invoke it.

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

Dear ext3/4 folks,
I've pushed this patch to the merge-window branch of ocfs2.git.
I'm ready to send it to Linus, But I need your OK.

Joel

Quoted 33 lines
include/linux/fs.h +2 -0
index 471e1ff..6aff3f5 100644
@@ -2399,6 +2399,8 @@ extern ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
 
 extern int generic_file_fsync(struct file *, int);
 
+extern int generic_check_addressable(unsigned, u64);
+
 #ifdef CONFIG_MIGRATION
 extern int buffer_migrate_page(struct address_space *,
 				struct page *, struct page *);
fs/libfs.c +11 -0
index dcaf972..b969648 100644
@@ -955,6 +955,38 @@ int generic_file_fsync(struct file *file, int datasync)
 }
 EXPORT_SYMBOL(generic_file_fsync);
 
+/**
+ * generic_check_addressable - Check addressability of file system
+ * @blocksize_bits:	log of file system block size
+ * @num_blocks:		number of blocks in file system
+ *
+ * Determine whether a file system with @num_blocks blocks (and a
+ * block size of 2**@blocksize_bits) is addressable by the sector_t
+ * and page cache of the system.  Return 0 if so and -EFBIG otherwise.
+ */
+int generic_check_addressable(unsigned blocksize_bits, u64 num_blocks)
+{

fs/libfs.c int generic_file_fsync(struct file *file, int datasync) · line 978

Quoted 10 lines
fs/libfs.c +10 -0
+	u64 last_fs_block = num_blocks - 1;
+
+	BUG_ON(blocksize_bits < 9);
+	BUG_ON(blocksize_bits > PAGE_CACHE_SHIFT);
+
+	if (unlikely(num_blocks == 0))
+		return 0;
+
+	printk(KERN_INFO "HERE %u %lu %u %u", blocksize_bits, last_fs_block,
+	       sizeof(sector_t), sizeof(pgoff_t));

Minus this printk(), of course ;-)

Quoted 57 lines
fs/libfs.c +11 -0
+
+	if ((last_fs_block >
+	     (sector_t)(~0ULL) >> (blocksize_bits - 9)) ||
+	    (last_fs_block >
+	     (pgoff_t)(~0ULL) >> (PAGE_CACHE_SHIFT - blocksize_bits))) {
+		return -EFBIG;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(generic_check_addressable);
+
 /*
  * No-op implementation of ->fsync for in-memory filesystems.
  */
fs/ext3/super.c +2 -2
index 6c953bb..d0643db 100644
@@ -1862,8 +1862,8 @@ static int ext3_fill_super (struct super_block *sb, void *data, int silent)
 		goto failed_mount;
 	}
 
-	if (le32_to_cpu(es->s_blocks_count) >
-		    (sector_t)(~0ULL) >> (sb->s_blocksize_bits - 9)) {
+	if (generic_check_addressable(sb->s_blocksize_bits,
+				      le32_to_cpu(es->s_blocks_count))) {
 		ext3_msg(sb, KERN_ERR,
 			"error: filesystem is too large to mount safely");
 		if (sizeof(sector_t) < 8)
fs/ext4/super.c +3 -5
index 4e8983a..979cc57 100644
@@ -2706,15 +2706,13 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	 * Test whether we have more sectors than will fit in sector_t,
 	 * and whether the max offset is addressable by the page cache.
 	 */
-	if ((ext4_blocks_count(es) >
-	     (sector_t)(~0ULL) >> (sb->s_blocksize_bits - 9)) ||
-	    (ext4_blocks_count(es) >
-	     (pgoff_t)(~0ULL) >> (PAGE_CACHE_SHIFT - sb->s_blocksize_bits))) {
+	ret = generic_check_addressable(sb->s_blocksize_bits,
+					ext4_blocks_count(es));
+	if (ret) {
 		ext4_msg(sb, KERN_ERR, "filesystem"
 			 " too large to mount safely on this system");
 		if (sizeof(sector_t) < 8)
 			ext4_msg(sb, KERN_WARNING, "CONFIG_LBDAF not enabled");
-		ret = -EFBIG;
 		goto failed_mount;
 	}

_______________________________________________
Ocfs2-devel mailing list
[email protected]
http://oss.oracle.com/mailman/listinfo/ocfs2-devel

Signature · 10 lines
"What does it say about a society's priorities when the time you
 spend in meetings on Monday is greater than the total number of
 hours you spent sleeping over the weekend?"
	- Nat Friedman

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
#3 lore raw

Re: [Ocfs2-devel] [PATCH 2/3] JBD2: Allow feature checks before journal recovery

In reply to Patrick J. LoPresti
Quoted 8 lines · Patrick J. LoPresti

Before we start accessing a huge (> 16 TiB) OCFS2 volume, we need to confirm that its journal supports 64-bit offsets. In particular, 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.

Quoted 5 lines

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

Dear jbd2 developers,
I've pushed this patch to the merge-window branch of ocfs2.git.
I'm ready to send it to Linus, but I need your OK.

Joel

Quoted 20 lines
fs/jbd2/journal.c +4 -0
index bc2ff59..5cfd8d4 100644
@@ -1365,6 +1365,10 @@ int jbd2_journal_check_used_features (journal_t *journal, unsigned long compat,
 
 	if (!compat && !ro && !incompat)
 		return 1;
+	/* Load journal superblock if it is not loaded yet. */
+	if (journal->j_format_version == 0 &&
+	    journal_get_superblock(journal) != 0)
+		return 0;
 	if (journal->j_format_version == 1)
 		return 0;

_______________________________________________
Ocfs2-devel mailing list
[email protected]
http://oss.oracle.com/mailman/listinfo/ocfs2-devel

Signature · 11 lines
 Brain: I shall pollute the water supply with this DNAdefibuliser,
        turning everyone into mindless slaves.
 Pinky: What about the people who drink bottled water?
 Brain: Pinky, people who pay 5 dollars for a bottle of water are
        already mindless slaves.

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
#11 lore raw
In reply to Joel Becker
Quoted 7 lines · Joel Becker
Quoted 7 lines

+/**
+ * generic_check_addressable - Check addressability of file system
+ * @blocksize_bits: log of file system block size
+ * @num_blocks: number of blocks in file system
+ *
+ * Determine whether a file system with @num_blocks blocks (and a
+ * block size of 2**@blocksize_bits) is addressable by the sector_t

Quoted 8 lines

fs/libfs.c int generic_file_fsync(struct file *file, int datasync) · line 972

Quoted 8 lines

+ * and page cache of the system. Return 0 if so and -EFBIG otherwise.
+ */

fs/libfs.c +6 -0
+int generic_check_addressable(unsigned blocksize_bits, u64 num_blocks)
+{
+	u64 last_fs_block = num_blocks - 1;
+
+	BUG_ON(blocksize_bits < 9);
+	BUG_ON(blocksize_bits > PAGE_CACHE_SHIFT);

I'd rather not have a BUG_ON() for a "check" function that may be called with on-disk values by some filesystem. Some filesystems (AFAIR) also handle blocksize > PAGE_SIZE internally, so this helper would not be useful for them.

Cheers, Andreas

#12 lore raw
Quoted 10 lines · Andreas Dilger
Quoted 9 lines · Joel Becker
Quoted 9 lines

+/**
+ * generic_check_addressable - Check addressability of file system
+ * @blocksize_bits: log of file system block size
+ * @num_blocks: number of blocks in file system
+ *
+ * Determine whether a file system with @num_blocks blocks (and a
+ * block size of 2**@blocksize_bits) is addressable by the sector_t
+ * and page cache of the system. Return 0 if so and -EFBIG otherwise.
+ */

Quoted 8 lines
Quoted 6 lines

fs/libfs.c int generic_file_fsync(struct file *file, int datasync) · line 972

Quoted 6 lines
fs/libfs.c +6 -0
+int generic_check_addressable(unsigned blocksize_bits, u64 num_blocks)
+{
+	u64 last_fs_block = num_blocks - 1;
+
+	BUG_ON(blocksize_bits < 9);
+	BUG_ON(blocksize_bits > PAGE_CACHE_SHIFT);

I'd rather not have a BUG_ON() for a "check" function that may be called with on-disk values by some filesystem. Some filesystems (AFAIR) also handle blocksize > PAGE_SIZE internally, so this helper would not be useful for them.

Filesystems that handle their own page cache certainly wouldn't
be interested in a generic helper anyway. All of our pagecache assumes blocks between 512<->PAGE_CACHE_SIZE.
If I change the BUG_ON()s to -EINVAL, does that work? Or do you
have some way you'd like to allow non-pagecache filesystems to use this as well?

Joel

Signature · 9 lines
"I am working for the time when unqualified blacks, browns, and
 women join the unqualified men in running our government."
	- Sissy Farenthold

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
#13 lore raw
Quoted 11 lines · Joel Becker
Quoted 9 lines · Andreas Dilger
Quoted 6 lines · Joel Becker

fs/libfs.c int generic_file_fsync(struct file *file, int datasync) · line 972

Quoted 6 lines
fs/libfs.c +6 -0
+int generic_check_addressable(unsigned blocksize_bits, u64 num_blocks)
+{
+	u64 last_fs_block = num_blocks - 1;
+
+	BUG_ON(blocksize_bits < 9);
+	BUG_ON(blocksize_bits > PAGE_CACHE_SHIFT);

I'd rather not have a BUG_ON() for a "check" function that may be called with on-disk values by some filesystem. Some filesystems (AFAIR) also handle blocksize > PAGE_SIZE internally, so this helper would not be useful for them.

Quoted 6 lines

Filesystems that handle their own page cache certainly wouldn't
be interested in a generic helper anyway. All of our pagecache assumes blocks between 512<->PAGE_CACHE_SIZE.
If I change the BUG_ON()s to -EINVAL, does that work? Or do you
have some way you'd like to allow non-pagecache filesystems to use this as well?

That's probably fine for now. If anyone complains, we can always change it later, since they can't possibly depend on this function yet...

Cheers, Andreas

#14 lore raw
Quoted 5 lines · Andreas Dilger
Quoted 3 lines

If I change the BUG_ON()s to -EINVAL, does that work? Or do you
have some way you'd like to allow non-pagecache filesystems to use this as well?

That's probably fine for now. If anyone complains, we can always change it later, since they can't possibly depend on this function yet...

How's this:

Quoted 2 lines

From f988b04c58039ae9a65fea537656088ea56a8072 Mon Sep 17 00:00:00 2001
From: Patrick J. LoPresti <[email protected]>

Date: Thu, 22 Jul 2010 15:03:41 -0700
Subject: [PATCH] ext3/ext4: Factor out disk addressability check

As part of adding support for OCFS2 to mount huge volumes, we need to check that the sector_t and page cache of the system are capable of addressing the entire volume.

An identical check already appears in ext3 and ext4. This patch moves the addressability check into its own function in fs/libfs.c and modifies ext3 and ext4 to invoke it.

[Edited to -EINVAL instead of BUG_ON() for bad blocksize_bits -- Joel]

Signed-off-by Patrick LoPresti <[email protected]>
Cc [email protected]
Signed-off-by Joel Becker <[email protected]>
---
 fs/ext3/super.c    |    4 ++--
 fs/ext4/super.c    |    8 +++-----
 fs/libfs.c         |   29 +++++++++++++++++++++++++++++
 include/linux/fs.h |    2 ++
 4 files changed, 36 insertions(+), 7 deletions(-)
fs/ext3/super.c +2 -2
index 6c953bb..d0643db 100644
@@ -1862,8 +1862,8 @@ static int ext3_fill_super (struct super_block *sb, void *data, int silent)
 		goto failed_mount;
 	}
 
-	if (le32_to_cpu(es->s_blocks_count) >
-		    (sector_t)(~0ULL) >> (sb->s_blocksize_bits - 9)) {
+	if (generic_check_addressable(sb->s_blocksize_bits,
+				      le32_to_cpu(es->s_blocks_count))) {
 		ext3_msg(sb, KERN_ERR,
 			"error: filesystem is too large to mount safely");
 		if (sizeof(sector_t) < 8)
fs/ext4/super.c +3 -5
index e72d323..e03a7d2 100644
@@ -2706,15 +2706,13 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	 * Test whether we have more sectors than will fit in sector_t,
 	 * and whether the max offset is addressable by the page cache.
 	 */
-	if ((ext4_blocks_count(es) >
-	     (sector_t)(~0ULL) >> (sb->s_blocksize_bits - 9)) ||
-	    (ext4_blocks_count(es) >
-	     (pgoff_t)(~0ULL) >> (PAGE_CACHE_SHIFT - sb->s_blocksize_bits))) {
+	ret = generic_check_addressable(sb->s_blocksize_bits,
+					ext4_blocks_count(es));
+	if (ret) {
 		ext4_msg(sb, KERN_ERR, "filesystem"
 			 " too large to mount safely on this system");
 		if (sizeof(sector_t) < 8)
 			ext4_msg(sb, KERN_WARNING, "CONFIG_LBDAF not enabled");
-		ret = -EFBIG;
 		goto failed_mount;
 	}
fs/libfs.c +29 -0
index dcaf972..f099566 100644
@@ -955,6 +955,35 @@ int generic_file_fsync(struct file *file, int datasync)
 }
 EXPORT_SYMBOL(generic_file_fsync);
 
+/**
+ * generic_check_addressable - Check addressability of file system
+ * @blocksize_bits:	log of file system block size
+ * @num_blocks:		number of blocks in file system
+ *
+ * Determine whether a file system with @num_blocks blocks (and a
+ * block size of 2**@blocksize_bits) is addressable by the sector_t
+ * and page cache of the system.  Return 0 if so and -EFBIG otherwise.
+ */
+int generic_check_addressable(unsigned blocksize_bits, u64 num_blocks)
+{
+	u64 last_fs_block = num_blocks - 1;
+
+	if (unlikely(num_blocks == 0))
+		return 0;
+
+	if ((blocksize_bits < 9) || (blocksize_bits > PAGE_CACHE_SHIFT))
+		return -EINVAL;
+
+	if ((last_fs_block >
+	     (sector_t)(~0ULL) >> (blocksize_bits - 9)) ||
+	    (last_fs_block >
+	     (pgoff_t)(~0ULL) >> (PAGE_CACHE_SHIFT - blocksize_bits))) {
+		return -EFBIG;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(generic_check_addressable);
+
 /*
  * No-op implementation of ->fsync for in-memory filesystems.
  */
include/linux/fs.h +2 -0
index e5106e4..7644248 100644
@@ -2414,6 +2414,8 @@ extern ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
 
 extern int generic_file_fsync(struct file *, int);
 
+extern int generic_check_addressable(unsigned, u64);
+
 #ifdef CONFIG_MIGRATION
 extern int buffer_migrate_page(struct address_space *,
 				struct page *, struct page *);
Signature · 13 lines
1.7.1


-- 

"Friends may come and go, but enemies accumulate." 
        - Thomas Jones

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
#30 lore raw
In reply to Joel Becker
Quoted 9 lines · Joel Becker
Quoted 8 lines · Patrick J. LoPresti

As part of adding support for OCFS2 to mount huge volumes, we need to check that the sector_t and page cache of the system are capable of addressing the entire volume.

An identical check already appears in ext3 and ext4. This patch moves the addressability check into its own function in fs/libfs.c and modifies ext3 and ext4 to invoke it.

Quoted 5 lines
Quoted 1 line
Signed-off-by Patrick LoPresti <[email protected]>

Dear ext3/4 folks,
I've pushed this patch to the merge-window branch of ocfs2.git.
I'm ready to send it to Linus, But I need your OK.

To be specific, I would like an Acked-by.

Joel

Signature · 9 lines
Life's Little Instruction Book #456

	"Send your loved one flowers.  Think of a reason later."

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
#4 lore raw

Re: [Ocfs2-devel] [PATCH 2/3] JBD2: Allow feature checks before journal recovery

In reply to Joel Becker
Quoted 13 lines · Joel Becker
Quoted 12 lines · Patrick J. LoPresti

Before we start accessing a huge (> 16 TiB) OCFS2 volume, we need to confirm that its journal supports 64-bit offsets. In particular, 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.

Quoted 5 lines
Quoted 1 line
Signed-off-by Patrick LoPresti <[email protected]>

Dear jbd2 developers,
I've pushed this patch to the merge-window branch of ocfs2.git.
I'm ready to send it to Linus, but I need your OK.

To be specific, I'd really like an Acked-by.

Joel

Signature · 9 lines
"The doctrine of human equality reposes on this: that there is no
 man really clever who has not found that he is stupid."
	- Gilbert K. Chesterson

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
#5 lore raw

Re: [Ocfs2-devel] [PATCH 2/3] JBD2: Allow feature checks before journal recovery

In reply to Joel Becker
Quoted 13 lines · Joel Becker
Quoted 12 lines · Patrick J. LoPresti

Before we start accessing a huge (> 16 TiB) OCFS2 volume, we need to confirm that its journal supports 64-bit offsets. In particular, 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.

Quoted 5 lines
Quoted 1 line
Signed-off-by Patrick LoPresti <[email protected]>

Dear jbd2 developers,
I've pushed this patch to the merge-window branch of ocfs2.git.
I'm ready to send it to Linus, but I need your OK.

To be specific, I'd really like an Acked-by.

Joel

Signature · 9 lines
"The doctrine of human equality reposes on this: that there is no
 man really clever who has not found that he is stupid."
	- Gilbert K. Chesterson

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
#6 lore raw

Re: [Ocfs2-devel] [PATCH 2/3] JBD2: Allow feature checks before journal recovery

In reply to Joel Becker
Quoted 13 lines · Joel Becker
Quoted 12 lines · Patrick J. LoPresti

Before we start accessing a huge (> 16 TiB) OCFS2 volume, we need to confirm that its journal supports 64-bit offsets. In particular, 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.

Quoted 5 lines
Quoted 1 line
Signed-off-by Patrick LoPresti <[email protected]>

Dear jbd2 developers,
I've pushed this patch to the merge-window branch of ocfs2.git.
I'm ready to send it to Linus, but I need your OK.

To be specific, I'd really like an Acked-by.

Joel

Signature · 9 lines
"The doctrine of human equality reposes on this: that there is no
 man really clever who has not found that he is stupid."
	- Gilbert K. Chesterson

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
Acked-by
#15 lore raw
In reply to Joel Becker

You can add my:

Acked-by Andreas Dilger <[email protected]>
Quoted 143 lines · Joel Becker
Quoted 5 lines · Andreas Dilger
Quoted 3 lines

If I change the BUG_ON()s to -EINVAL, does that work? Or do you
have some way you'd like to allow non-pagecache filesystems to use this as well?

That's probably fine for now. If anyone complains, we can always change it later, since they can't possibly depend on this function yet...

How's this:

Quoted 2 lines

From f988b04c58039ae9a65fea537656088ea56a8072 Mon Sep 17 00:00:00 2001
From: Patrick J. LoPresti <[email protected]>

Date: Thu, 22 Jul 2010 15:03:41 -0700
Subject: [PATCH] ext3/ext4: Factor out disk addressability check

As part of adding support for OCFS2 to mount huge volumes, we need to check that the sector_t and page cache of the system are capable of addressing the entire volume.

An identical check already appears in ext3 and ext4. This patch moves the addressability check into its own function in fs/libfs.c and modifies ext3 and ext4 to invoke it.

[Edited to -EINVAL instead of BUG_ON() for bad blocksize_bits -- Joel]

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

---

fs/ext3/super.c    |    4 ++--
fs/ext4/super.c    |    8 +++-----
fs/libfs.c         |   29 +++++++++++++++++++++++++++++
include/linux/fs.h |    2 ++
4 files changed, 36 insertions(+), 7 deletions(-)
fs/ext3/super.c +2 -2
index 6c953bb..d0643db 100644
@@ -1862,8 +1862,8 @@ static int ext3_fill_super (struct super_block *sb, void *data, int silent)
 		goto failed_mount;
 	}
 
-	if (le32_to_cpu(es->s_blocks_count) >
-		    (sector_t)(~0ULL) >> (sb->s_blocksize_bits - 9)) {
+	if (generic_check_addressable(sb->s_blocksize_bits,
+				      le32_to_cpu(es->s_blocks_count))) {
 		ext3_msg(sb, KERN_ERR,
 			"error: filesystem is too large to mount safely");
 		if (sizeof(sector_t) < 8)
fs/ext4/super.c +3 -5
index e72d323..e03a7d2 100644
@@ -2706,15 +2706,13 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	 * Test whether we have more sectors than will fit in sector_t,
 	 * and whether the max offset is addressable by the page cache.
 	 */
-	if ((ext4_blocks_count(es) >
-	     (sector_t)(~0ULL) >> (sb->s_blocksize_bits - 9)) ||
-	    (ext4_blocks_count(es) >
-	     (pgoff_t)(~0ULL) >> (PAGE_CACHE_SHIFT - sb->s_blocksize_bits))) {
+	ret = generic_check_addressable(sb->s_blocksize_bits,
+					ext4_blocks_count(es));
+	if (ret) {
 		ext4_msg(sb, KERN_ERR, "filesystem"
 			 " too large to mount safely on this system");
 		if (sizeof(sector_t) < 8)
 			ext4_msg(sb, KERN_WARNING, "CONFIG_LBDAF not enabled");
-		ret = -EFBIG;
 		goto failed_mount;
 	}
fs/libfs.c +29 -0
index dcaf972..f099566 100644
@@ -955,6 +955,35 @@ int generic_file_fsync(struct file *file, int datasync)
 }
 EXPORT_SYMBOL(generic_file_fsync);
 
+/**
+ * generic_check_addressable - Check addressability of file system
+ * @blocksize_bits:	log of file system block size
+ * @num_blocks:		number of blocks in file system
+ *
+ * Determine whether a file system with @num_blocks blocks (and a
+ * block size of 2**@blocksize_bits) is addressable by the sector_t
+ * and page cache of the system.  Return 0 if so and -EFBIG otherwise.
+ */
+int generic_check_addressable(unsigned blocksize_bits, u64 num_blocks)
+{
+	u64 last_fs_block = num_blocks - 1;
+
+	if (unlikely(num_blocks == 0))
+		return 0;
+
+	if ((blocksize_bits < 9) || (blocksize_bits > PAGE_CACHE_SHIFT))
+		return -EINVAL;
+
+	if ((last_fs_block >
+	     (sector_t)(~0ULL) >> (blocksize_bits - 9)) ||
+	    (last_fs_block >
+	     (pgoff_t)(~0ULL) >> (PAGE_CACHE_SHIFT - blocksize_bits))) {
+		return -EFBIG;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(generic_check_addressable);
+
 /*
 * No-op implementation of ->fsync for in-memory filesystems.
 */
include/linux/fs.h +2 -0
index e5106e4..7644248 100644
@@ -2414,6 +2414,8 @@ extern ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
 
 extern int generic_file_fsync(struct file *, int);
 
+extern int generic_check_addressable(unsigned, u64);
+
 #ifdef CONFIG_MIGRATION
 extern int buffer_migrate_page(struct address_space *,
 				struct page *, struct page *);
Signature · 17 lines
1.7.1


-- 

"Friends may come and go, but enemies accumulate." 
       - Thomas Jones

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
--
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

Cheers, Andreas

maintainer
#31 lore raw
In reply to Joel Becker
Quoted 9 lines · Joel Becker
Quoted 8 lines · Patrick J. LoPresti

As part of adding support for OCFS2 to mount huge volumes, we need to check that the sector_t and page cache of the system are capable of addressing the entire volume.

An identical check already appears in ext3 and ext4. This patch moves the addressability check into its own function in fs/libfs.c and modifies ext3 and ext4 to invoke it.

Quoted 5 lines
Quoted 1 line
Signed-off-by Patrick LoPresti <[email protected]>

Dear ext3/4 folks,
I've pushed this patch to the merge-window branch of ocfs2.git.
I'm ready to send it to Linus, But I need your OK.

Signed-off-by "Theodore Ts'o" <[email protected]>

for the ext4 bits.

(Sorry for not responding earlier to some of these patches. The fact that the merge window overlapped with LinuxCon and the Linux Storage/Filesystem workshop has been quite stressful/tiring for some of us....)

- Ted

maintainer
#7 lore raw

Re: [Ocfs2-devel] [PATCH 2/3] JBD2: Allow feature checks before journal recovery

In reply to Joel Becker
Quoted 13 lines · Joel Becker
Quoted 12 lines · Patrick J. LoPresti

Before we start accessing a huge (> 16 TiB) OCFS2 volume, we need to confirm that its journal supports 64-bit offsets. In particular, 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.

Quoted 5 lines
Quoted 1 line
Signed-off-by Patrick LoPresti <[email protected]>

Dear jbd2 developers,
I've pushed this patch to the merge-window branch of ocfs2.git.
I'm ready to send it to Linus, but I need your OK.

Signed-off-by "Theodore Ts'o" <[email protected]>

- Ted

#8 lore raw

Re: [Ocfs2-devel] [PATCH 2/3] JBD2: Allow feature checks before journal recovery

Quoted 14 lines · Ted Ts'o
Quoted 13 lines · Joel Becker
Quoted 12 lines · Patrick J. LoPresti

Before we start accessing a huge (> 16 TiB) OCFS2 volume, we need to confirm that its journal supports 64-bit offsets. In particular, 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.

Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Signed-off-by Patrick LoPresti <[email protected]>

Dear jbd2 developers,
I've pushed this patch to the merge-window branch of ocfs2.git.
I'm ready to send it to Linus, but I need your OK.

Signed-off-by "Theodore Ts'o" <[email protected]>

Thanks Ted!

Joel

Signature · 8 lines
"I don't even butter my bread; I consider that cooking."
         - Katherine Cebrian

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
reviewer
#17 lore raw
In reply to Joel Becker
Quoted 21 lines · Joel Becker
fs/libfs.c +13 -0
index dcaf972..f099566 100644
@@ -955,6 +955,35 @@ int generic_file_fsync(struct file *file, int datasync)
 }
 EXPORT_SYMBOL(generic_file_fsync);
 
+/**
+ * generic_check_addressable - Check addressability of file system
+ * @blocksize_bits:	log of file system block size
+ * @num_blocks:		number of blocks in file system
+ *
+ * Determine whether a file system with @num_blocks blocks (and a
+ * block size of 2**@blocksize_bits) is addressable by the sector_t
+ * and page cache of the system.  Return 0 if so and -EFBIG otherwise.
+ */
+int generic_check_addressable(unsigned blocksize_bits, u64 num_blocks)
+{
+	u64 last_fs_block = num_blocks - 1;
+

fs/libfs.c int generic_file_fsync(struct file *file, int datasync) · line 980

Quoted 10 lines
fs/libfs.c +10 -0
+	if (unlikely(num_blocks == 0))
+		return 0;
+
+	if ((blocksize_bits < 9) || (blocksize_bits > PAGE_CACHE_SHIFT))
+		return -EINVAL;
+
+	if ((last_fs_block >
+	     (sector_t)(~0ULL) >> (blocksize_bits - 9)) ||
+	    (last_fs_block >
+	     (pgoff_t)(~0ULL) >> (PAGE_CACHE_SHIFT - blocksize_bits))) {

^^^ I don't get the pgoff_t check. Shouldn't it rather be
(u64)(pgoff_t)(~0ULL) << (PAGE_CACHE_SHIFT - blocksize_bits)? Because on 32-bit arch we are able to address 16TB device, which is for 1KB blocksize 1<<34 blocks. But your math gives 1<<30 blocks...

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
#18 lore raw
Quoted 20 lines · Jan Kara
Quoted 19 lines · Joel Becker
fs/libfs.c +19 -0
+/**
+ * generic_check_addressable - Check addressability of file system
+ * @blocksize_bits:	log of file system block size
+ * @num_blocks:		number of blocks in file system
+ *
+ * Determine whether a file system with @num_blocks blocks (and a
+ * block size of 2**@blocksize_bits) is addressable by the sector_t
+ * and page cache of the system.  Return 0 if so and -EFBIG otherwise.
+ */
+int generic_check_addressable(unsigned blocksize_bits, u64 num_blocks)
+{
+	u64 last_fs_block = num_blocks - 1;
+
+	if (unlikely(num_blocks == 0))
+		return 0;
+
+	if ((blocksize_bits < 9) || (blocksize_bits > PAGE_CACHE_SHIFT))
+		return -EINVAL;
+
Quoted 8 lines

fs/libfs.c int generic_file_fsync(struct file *file, int datasync) · line 980

Quoted 4 lines
fs/libfs.c +4 -0
+	if ((last_fs_block >
+	     (sector_t)(~0ULL) >> (blocksize_bits - 9)) ||
+	    (last_fs_block >
+	     (pgoff_t)(~0ULL) >> (PAGE_CACHE_SHIFT - blocksize_bits))) {

^^^ I don't get the pgoff_t check. Shouldn't it rather be
(u64)(pgoff_t)(~0ULL) << (PAGE_CACHE_SHIFT - blocksize_bits)? Because on 32-bit arch we are able to address 16TB device, which is for 1KB blocksize 1<<34 blocks. But your math gives 1<<30 blocks...

This code is directly lifted from ext4. But that said, I am
starting to think you're right. 1 page == 4 x 1K blocks, rather than 4 pages == 1 1K block.

Joel

Signature · 9 lines
"I always thought the hardest questions were those I could not answer.
 Now I know they are the ones I can never ask."
			- Charlie Watkins

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
#19 lore raw
Quoted 30 lines · Joel Becker
Quoted 20 lines · Jan Kara
Quoted 19 lines · Joel Becker
fs/libfs.c +19 -0
+/**
+ * generic_check_addressable - Check addressability of file system
+ * @blocksize_bits:	log of file system block size
+ * @num_blocks:		number of blocks in file system
+ *
+ * Determine whether a file system with @num_blocks blocks (and a
+ * block size of 2**@blocksize_bits) is addressable by the sector_t
+ * and page cache of the system.  Return 0 if so and -EFBIG otherwise.
+ */
+int generic_check_addressable(unsigned blocksize_bits, u64 num_blocks)
+{
+	u64 last_fs_block = num_blocks - 1;
+
+	if (unlikely(num_blocks == 0))
+		return 0;
+
+	if ((blocksize_bits < 9) || (blocksize_bits > PAGE_CACHE_SHIFT))
+		return -EINVAL;
+
Quoted 8 lines

fs/libfs.c int generic_file_fsync(struct file *file, int datasync) · line 980

Quoted 4 lines
fs/libfs.c +4 -0
+	if ((last_fs_block >
+	     (sector_t)(~0ULL) >> (blocksize_bits - 9)) ||
+	    (last_fs_block >
+	     (pgoff_t)(~0ULL) >> (PAGE_CACHE_SHIFT - blocksize_bits))) {

^^^ I don't get the pgoff_t check. Shouldn't it rather be
(u64)(pgoff_t)(~0ULL) << (PAGE_CACHE_SHIFT - blocksize_bits)? Because on 32-bit arch we are able to address 16TB device, which is for 1KB blocksize 1<<34 blocks. But your math gives 1<<30 blocks...

Quoted 3 lines

This code is directly lifted from ext4. But that said, I am
starting to think you're right. 1 page == 4 x 1K blocks, rather than 4 pages == 1 1K block.

	Wouldn't it rather be:

	    ... ||
	    ((last_fs_block >> (PAGE_CACHE_SHIFT - blocksize_bits)) >
	     (pgoff_t)(!0ULL))) {

Joel

Signature · 9 lines
"What no boss of a programmer can ever understand is that a programmer
 is working when he's staring out of the window"
	- With apologies to Burton Rascoe

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
#21 lore raw
In reply to Jan Kara
Quoted 26 lines · Jan Kara
Quoted 25 lines · Joel Becker
fs/libfs.c +17 -0
index dcaf972..f099566 100644
@@ -955,6 +955,35 @@ int generic_file_fsync(struct file *file, int datasync)
 }
 EXPORT_SYMBOL(generic_file_fsync);
 
+/**
+ * generic_check_addressable - Check addressability of file system
+ * @blocksize_bits:	log of file system block size
+ * @num_blocks:		number of blocks in file system
+ *
+ * Determine whether a file system with @num_blocks blocks (and a
+ * block size of 2**@blocksize_bits) is addressable by the sector_t
+ * and page cache of the system.  Return 0 if so and -EFBIG otherwise.
+ */
+int generic_check_addressable(unsigned blocksize_bits, u64 num_blocks)
+{
+	u64 last_fs_block = num_blocks - 1;
+
+	if (unlikely(num_blocks == 0))
+		return 0;
+
+	if ((blocksize_bits < 9) || (blocksize_bits > PAGE_CACHE_SHIFT))
Quoted 8 lines

fs/libfs.c int generic_file_fsync(struct file *file, int datasync) · line 980

Quoted 6 lines
fs/libfs.c +6 -0
+		return -EINVAL;
+
+	if ((last_fs_block >
+	     (sector_t)(~0ULL) >> (blocksize_bits - 9)) ||
+	    (last_fs_block >
+	     (pgoff_t)(~0ULL) >> (PAGE_CACHE_SHIFT - blocksize_bits))) {

^^^ I don't get the pgoff_t check. Shouldn't it rather be
(u64)(pgoff_t)(~0ULL) << (PAGE_CACHE_SHIFT - blocksize_bits)?

Argh that was my fault... Thankfully not too many 1k-blocksize-formatted 16T devices out there, I guess.

I went through the math again and also came up with:

total fs pages is blocks / (blocks per page)
total pages is blocks / (1 << PAGE_CACHE_SHIFT / 1 << blocksize_bits) total pages is blocks / (1 << (PAGE_CACHE_SHIFT - blocksize_bits)) total pages is blocks * (1 >> (PAGE_CACHE_SHIFT - blocksize_bits)) total pages is blocks >> (PAGE_CACHE_SHIFT - blocksize_bits)

too big if total pages is > (pgoff_t)(~0ULL)
too big if blocks >> (PAGE_CACHE_SHIFT - blocksize_bits) > (pgoff_t)(~0ULL) too big if blocks > (pgoff_t)(~0ULL) << (PAGE_CACHE_SHIFT - blocksize_bits) and to not overflow:
too big if blocks > (u64)(pgoff_t)(~0ULL) << (PAGE_CACHE_SHIFT - blocksize_bits)

so seems like the test is:

last_fs_block > (u64)(pgoff_t)(~0ULL) << (PAGE_CACHE_SHIFT - blocksize_bits)

Given the density of the logic in the helper it seems like maybe breaking it up and adding specific comments might be helpful to the reader:

	/* can IO layers fit total fs sectors in a sector_t? */
	if (last_fs_block >
	    (sector_t)(~0ULL) >> (blocksize_bits - 9))
		return -EFBIG;

	/* can page cache fit total fs pages in a pgoff_t? */
	if (last_fs_block >
	    (u64)(pgoff_t)(~0ULL) << (PAGE_CACHE_SHIFT - blocksize_bits)
		return -EFBIG;

Or something like that.

Sorry for chiming in late...

-Eric

Quoted 5 lines

Because on 32-bit arch we are able to address 16TB device, which is for 1KB blocksize 1<<34 blocks. But your math gives 1<<30 blocks...

Honza

#22 lore raw
Quoted 10 lines · Eric Sandeen
Quoted 4 lines

fs/libfs.c int generic_file_fsync(struct file *file, int datasync) · line 980

Quoted 2 lines
fs/libfs.c +2 -0
+	    (last_fs_block >
+	     (pgoff_t)(~0ULL) >> (PAGE_CACHE_SHIFT - blocksize_bits))) {

^^^ I don't get the pgoff_t check. Shouldn't it rather be
(u64)(pgoff_t)(~0ULL) << (PAGE_CACHE_SHIFT - blocksize_bits)?

Argh that was my fault... Thankfully not too many 1k-blocksize-formatted 16T devices out there, I guess.

I went through the math again and also came up with:

Quoted 8 lines

total fs pages is blocks / (blocks per page)
total pages is blocks / (1 << PAGE_CACHE_SHIFT / 1 << blocksize_bits) total pages is blocks / (1 << (PAGE_CACHE_SHIFT - blocksize_bits)) total pages is blocks * (1 >> (PAGE_CACHE_SHIFT - blocksize_bits)) total pages is blocks >> (PAGE_CACHE_SHIFT - blocksize_bits)

too big if total pages is > (pgoff_t)(~0ULL)
too big if blocks >> (PAGE_CACHE_SHIFT - blocksize_bits) > (pgoff_t)(~0ULL)

Why not stop here, which is what I put in my other email?
"blocks >> SHIFT-bits" is "how many pages do I need?".

Quoted 3 lines

too big if blocks > (pgoff_t)(~0ULL) << (PAGE_CACHE_SHIFT - blocksize_bits) and to not overflow:
too big if blocks > (u64)(pgoff_t)(~0ULL) << (PAGE_CACHE_SHIFT - blocksize_bits)

This still overflows. pgoff_t is a u64 on 64bit machines,
right? So shift that left by anything and you wrap.

Joel

Signature · 15 lines
"You cannot bring about prosperity by discouraging thrift. You cannot
 strengthen the weak by weakening the strong. You cannot help the wage
 earner by pulling down the wage payer. You cannot further the
 brotherhood of man by encouraging class hatred. You cannot help the
 poor by destroying the rich. You cannot build character and courage by
 taking away a man's initiative and independence. You cannot help men
 permanently by doing for them what they could and should do for
 themselves."
	- Abraham Lincoln 

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
#23 lore raw
Quoted 16 lines · Joel Becker
Quoted 15 lines · Eric Sandeen
Quoted 4 lines

fs/libfs.c int generic_file_fsync(struct file *file, int datasync) · line 980

Quoted 2 lines
fs/libfs.c +2 -0
+	    (last_fs_block >
+	     (pgoff_t)(~0ULL) >> (PAGE_CACHE_SHIFT - blocksize_bits))) {

^^^ I don't get the pgoff_t check. Shouldn't it rather be
(u64)(pgoff_t)(~0ULL) << (PAGE_CACHE_SHIFT - blocksize_bits)?

Argh that was my fault... Thankfully not too many 1k-blocksize-formatted 16T devices out there, I guess.

I went through the math again and also came up with:

total fs pages is blocks / (blocks per page)
total pages is blocks / (1 << PAGE_CACHE_SHIFT / 1 << blocksize_bits) total pages is blocks / (1 << (PAGE_CACHE_SHIFT - blocksize_bits)) total pages is blocks * (1 >> (PAGE_CACHE_SHIFT - blocksize_bits)) total pages is blocks >> (PAGE_CACHE_SHIFT - blocksize_bits)

Quoted 5 lines
Quoted 2 lines

too big if total pages is > (pgoff_t)(~0ULL)
too big if blocks >> (PAGE_CACHE_SHIFT - blocksize_bits) > (pgoff_t)(~0ULL)

Why not stop here, which is what I put in my other email?
"blocks >> SHIFT-bits" is "how many pages do I need?".

yeah, ok. Was going for pointless symmetry w/ the other test...

Quoted 6 lines
Quoted 3 lines

too big if blocks > (pgoff_t)(~0ULL) << (PAGE_CACHE_SHIFT - blocksize_bits) and to not overflow:
too big if blocks > (u64)(pgoff_t)(~0ULL) << (PAGE_CACHE_SHIFT - blocksize_bits)

This still overflows. pgoff_t is a u64 on 64bit machines,
right? So shift that left by anything and you wrap.

Er, yeah. I had 32 bits in my head since that's the case we're checking for... whoops.

So I guess your

... ||

	    ((last_fs_block >> (PAGE_CACHE_SHIFT - blocksize_bits)) >
	     (pgoff_t)(!0ULL))) {

is right :) (my feeble brain has a hard time reading that, though, TBH)

-Eric

Quoted 2 lines

Joel

#24 lore raw
Quoted 10 lines · Eric Sandeen

Er, yeah. I had 32 bits in my head since that's the case we're checking for... whoops.

So I guess your

... ||

	    ((last_fs_block >> (PAGE_CACHE_SHIFT - blocksize_bits)) >
	     (pgoff_t)(!0ULL))) {

is right :) (my feeble brain has a hard time reading that, though, TBH)

Well, note the bug in my quickly typed version: "(!0ULL)" vs
"(~0ULL)". How about:

	u64 last_fs_page = last_fs_block >> (PAGE_CACHE_SHIFT - blocksize_bits);

	... ||
	(last_fs_page > (pgoff_t)(~0ULL))) {

Is that more readable?

Joel

Signature · 9 lines
Life's Little Instruction Book #99

	"Think big thoughts, but relish small pleasures."

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
#25 lore raw
Quoted 6 lines · Joel Becker
Quoted 5 lines · Eric Sandeen

Er, yeah. I had 32 bits in my head since that's the case we're checking for... whoops.

So I guess your

Quoted 8 lines
Quoted 5 lines

... ||

	    ((last_fs_block >> (PAGE_CACHE_SHIFT - blocksize_bits)) >
	     (pgoff_t)(!0ULL))) {

is right :) (my feeble brain has a hard time reading that, though, TBH)

Well, note the bug in my quickly typed version: "(!0ULL)" vs
"(~0ULL)".

*nod* saw that but figured it was just a typo & didn't mention it ;)

Quoted 8 lines

How about:

	u64 last_fs_page = last_fs_block >> (PAGE_CACHE_SHIFT - blocksize_bits);

	... ||
	(last_fs_page > (pgoff_t)(~0ULL))) {

Is that more readable?

To me, yes. Maybe do similar for last_fs_sector.

If it's getting too verbose I understand, but less dense is a lot easier to read, IMHO. Just style though, really, so *shrug*

Thanks,
-Eric

Quoted 2 lines

Joel

#29 lore raw
In reply to Joel Becker
Quoted 6 lines · Joel Becker
Quoted 5 lines · Eric Sandeen

Er, yeah. I had 32 bits in my head since that's the case we're checking for... whoops.

So I guess your

Quoted 8 lines
Quoted 5 lines

... ||

	    ((last_fs_block >> (PAGE_CACHE_SHIFT - blocksize_bits)) >
	     (pgoff_t)(!0ULL))) {

is right :) (my feeble brain has a hard time reading that, though, TBH)

Well, note the bug in my quickly typed version: "(!0ULL)" vs
"(~0ULL)".

*nod* saw that but figured it was just a typo & didn't mention it ;)

Quoted 8 lines

How about:

	u64 last_fs_page = last_fs_block >> (PAGE_CACHE_SHIFT - blocksize_bits);

	... ||
	(last_fs_page > (pgoff_t)(~0ULL))) {

Is that more readable?

To me, yes. Maybe do similar for last_fs_sector.

If it's getting too verbose I understand, but less dense is a lot easier to read, IMHO. Just style though, really, so *shrug*

Thanks,
-Eric

Quoted 2 lines

Joel

reviewer
#20 lore raw
In reply to Joel Becker
Quoted 35 lines · Joel Becker
Quoted 30 lines · Joel Becker
Quoted 20 lines · Jan Kara
Quoted 19 lines · Joel Becker
fs/libfs.c +19 -0
+/**
+ * generic_check_addressable - Check addressability of file system
+ * @blocksize_bits:	log of file system block size
+ * @num_blocks:		number of blocks in file system
+ *
+ * Determine whether a file system with @num_blocks blocks (and a
+ * block size of 2**@blocksize_bits) is addressable by the sector_t
+ * and page cache of the system.  Return 0 if so and -EFBIG otherwise.
+ */
+int generic_check_addressable(unsigned blocksize_bits, u64 num_blocks)
+{
+	u64 last_fs_block = num_blocks - 1;
+
+	if (unlikely(num_blocks == 0))
+		return 0;
+
+	if ((blocksize_bits < 9) || (blocksize_bits > PAGE_CACHE_SHIFT))
+		return -EINVAL;
+
Quoted 8 lines

fs/libfs.c int generic_file_fsync(struct file *file, int datasync) · line 980

Quoted 4 lines
fs/libfs.c +4 -0
+	if ((last_fs_block >
+	     (sector_t)(~0ULL) >> (blocksize_bits - 9)) ||
+	    (last_fs_block >
+	     (pgoff_t)(~0ULL) >> (PAGE_CACHE_SHIFT - blocksize_bits))) {

^^^ I don't get the pgoff_t check. Shouldn't it rather be
(u64)(pgoff_t)(~0ULL) << (PAGE_CACHE_SHIFT - blocksize_bits)? Because on 32-bit arch we are able to address 16TB device, which is for 1KB blocksize 1<<34 blocks. But your math gives 1<<30 blocks...

Quoted 3 lines

This code is directly lifted from ext4. But that said, I am
starting to think you're right. 1 page == 4 x 1K blocks, rather than 4 pages == 1 1K block.

Quoted 5 lines
	Wouldn't it rather be:

	    ... ||
	    ((last_fs_block >> (PAGE_CACHE_SHIFT - blocksize_bits)) >
	     (pgoff_t)(!0ULL))) {

Yes, this would be even better than what I suggested.

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
#26 lore raw
In reply to Eric Sandeen
Quoted 11 lines · Eric Sandeen
Quoted 8 lines · Joel Becker

How about:

	u64 last_fs_page = last_fs_block >> (PAGE_CACHE_SHIFT - blocksize_bits);

	... ||
	(last_fs_page > (pgoff_t)(~0ULL))) {

Is that more readable?

To me, yes. Maybe do similar for last_fs_sector.

last_fs_sector would be shifting up, which could wrap a really
large last_fs_blocks. So I'm going to keep the sector_t check as-is. How's this:

Quoted 2 lines

From 8de5cb9164cdc179ba84a07b282a895d0eb794b0 Mon Sep 17 00:00:00 2001
From: Joel Becker <[email protected]>

Date: Mon, 16 Aug 2010 12:10:17 -0700
Subject: [PATCH] libfs: Fix shift bug in generic_check_addressable()

generic_check_addressable() erroneously shifts pages down by a block factor when it should be shifting up. To prevent overflow, we shift blocks down to pages.

Signed-off-by Joel Becker <[email protected]>
---
 fs/libfs.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
fs/libfs.c +4 -4
index 8debe7b..62baa03 100644
@@ -925,6 +925,8 @@ EXPORT_SYMBOL(generic_file_fsync);
 int generic_check_addressable(unsigned blocksize_bits, u64 num_blocks)
 {
 	u64 last_fs_block = num_blocks - 1;
+	u64 last_fs_page =
+		last_fs_block >> (PAGE_CACHE_SHIFT - blocksize_bits);
 
 	if (unlikely(num_blocks == 0))
 		return 0;

@@ -932,10 +934,8 @@ int generic_check_addressable(unsigned blocksize_bits, u64 num_blocks)
 	if ((blocksize_bits < 9) || (blocksize_bits > PAGE_CACHE_SHIFT))
 		return -EINVAL;
 
-	if ((last_fs_block >
-	     (sector_t)(~0ULL) >> (blocksize_bits - 9)) ||
-	    (last_fs_block >
-	     (pgoff_t)(~0ULL) >> (PAGE_CACHE_SHIFT - blocksize_bits))) {
+	if ((last_fs_block > (sector_t)(~0ULL) >> (blocksize_bits - 9)) ||
+	    (last_fs_page > (pgoff_t)(~0ULL))) {
 		return -EFBIG;
 	}
 	return 0;
Signature · 13 lines
1.7.1

-- 

Life's Little Instruction Book #267

	"Lie on your back and look at the stars."

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
reviewer Reviewed-by
#27 lore raw
Quoted 19 lines · Joel Becker
Quoted 11 lines · Eric Sandeen
Quoted 8 lines · Joel Becker

How about:

	u64 last_fs_page = last_fs_block >> (PAGE_CACHE_SHIFT - blocksize_bits);

	... ||
	(last_fs_page > (pgoff_t)(~0ULL))) {

Is that more readable?

To me, yes. Maybe do similar for last_fs_sector.

last_fs_sector would be shifting up, which could wrap a really
large last_fs_blocks. So I'm going to keep the sector_t check as-is. How's this:

Quoted 2 lines

From 8de5cb9164cdc179ba84a07b282a895d0eb794b0 Mon Sep 17 00:00:00 2001
From: Joel Becker <[email protected]>

Quoted 8 lines

Date: Mon, 16 Aug 2010 12:10:17 -0700
Subject: [PATCH] libfs: Fix shift bug in generic_check_addressable()

generic_check_addressable() erroneously shifts pages down by a block factor when it should be shifting up. To prevent overflow, we shift blocks down to pages.

Signed-off-by Joel Becker <[email protected]>

Looks good.

Reviewed-by Jan Kara <[email protected]>

Honza

Quoted 44 lines
---
 fs/libfs.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
fs/libfs.c +4 -4
index 8debe7b..62baa03 100644
@@ -925,6 +925,8 @@ EXPORT_SYMBOL(generic_file_fsync);
 int generic_check_addressable(unsigned blocksize_bits, u64 num_blocks)
 {
 	u64 last_fs_block = num_blocks - 1;
+	u64 last_fs_page =
+		last_fs_block >> (PAGE_CACHE_SHIFT - blocksize_bits);
 
 	if (unlikely(num_blocks == 0))
 		return 0;

@@ -932,10 +934,8 @@ int generic_check_addressable(unsigned blocksize_bits, u64 num_blocks)
 	if ((blocksize_bits < 9) || (blocksize_bits > PAGE_CACHE_SHIFT))
 		return -EINVAL;
 
-	if ((last_fs_block >
-	     (sector_t)(~0ULL) >> (blocksize_bits - 9)) ||
-	    (last_fs_block >
-	     (pgoff_t)(~0ULL) >> (PAGE_CACHE_SHIFT - blocksize_bits))) {
+	if ((last_fs_block > (sector_t)(~0ULL) >> (blocksize_bits - 9)) ||
+	    (last_fs_page > (pgoff_t)(~0ULL))) {
 		return -EFBIG;
 	}
 	return 0;
Signature · 13 lines
1.7.1

-- 

Life's Little Instruction Book #267

	"Lie on your back and look at the stars."

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
#28 lore raw
Quoted 23 lines · Jan Kara
Quoted 22 lines · Joel Becker
Quoted 11 lines · Eric Sandeen
Quoted 8 lines · Joel Becker

How about:

	u64 last_fs_page = last_fs_block >> (PAGE_CACHE_SHIFT - blocksize_bits);

	... ||
	(last_fs_page > (pgoff_t)(~0ULL))) {

Is that more readable?

To me, yes. Maybe do similar for last_fs_sector.

last_fs_sector would be shifting up, which could wrap a really
large last_fs_blocks. So I'm going to keep the sector_t check as-is. How's this:

Quoted 2 lines

From 8de5cb9164cdc179ba84a07b282a895d0eb794b0 Mon Sep 17 00:00:00 2001
From: Joel Becker <[email protected]>

Date: Mon, 16 Aug 2010 12:10:17 -0700
Subject: [PATCH] libfs: Fix shift bug in generic_check_addressable()

Quoted 7 lines
Quoted 5 lines

generic_check_addressable() erroneously shifts pages down by a block factor when it should be shifting up. To prevent overflow, we shift blocks down to pages.

Signed-off-by Joel Becker <[email protected]>

Looks good.

Reviewed-by Jan Kara <[email protected]>

Ok, I'm going to keep this atop my existing branch, and if it
all shakes out in linux-next for a few days, send it along.

Joel

Signature · 8 lines
"When ideas fail, words come in very handy." 
         - Goethe

Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127