ext3/ext4: Factor out disk addressability check
Subsystems (4)
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.
include/linux/fs.h +2 -0
@@ -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
@@ -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
@@ -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
@@ -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; }
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.
fs/jbd2/journal.c +4 -0
@@ -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 ;
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.
fs/ocfs2/super.c +46 -5
@@ -1991,6 +1991,36 @@ static int ocfs2_setup_osb_uuid(struct ocfs2_super *osb, const unsigned char *uureturn 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 :) */
Re: [Ocfs2-devel] [PATCH 2/3] JBD2: Allow feature checks before journal recovery
In reply to Patrick J. LoPrestiQuoted 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@@ -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-8127Re: [Ocfs2-devel] [PATCH 2/3] JBD2: Allow feature checks before journal recovery
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
Re: [Ocfs2-devel] [PATCH 2/3] JBD2: Allow feature checks before journal recovery
In reply to Joel BeckerQuoted 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
Re: [Ocfs2-devel] [PATCH 2/3] JBD2: Allow feature checks before journal recovery
In reply to Joel BeckerQuoted 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
Ted Ts'o
maintainerRe: [Ocfs2-devel] [PATCH 2/3] JBD2: Allow feature checks before journal recovery
In reply to Joel BeckerQuoted 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.
- Ted
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-8127Quoted 9 lines · Patrick J. LoPresti
As part of adding support for OCFS2 to mount huge volumes, we need to check that the
sector_tand 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.cand 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
Quoted 9 lines · Patrick J. LoPresti
As part of adding support for OCFS2 to mount huge volumes, we need to check that the
sector_tand 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.cand 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@@ -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@@ -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@@ -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@@ -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
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 thesector_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
-EFBIGotherwise.
+ */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
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 thesector_t
+ * and page cache of the system. Return 0 if so and-EFBIGotherwise.
+ */
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_SIZEinternally, 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
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_SIZEinternally, 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
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
f988b04c58039ae9a65fea537656088ea56a8072Mon 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]
--- 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
@@ -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
@@ -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
@@ -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
@@ -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-8127Andreas Dilger
Acked-byYou can add my:
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
f988b04c58039ae9a65fea537656088ea56a8072Mon 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 checkAs part of adding support for OCFS2 to mount huge volumes, we need to check that the
sector_tand 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.cand modifies ext3 and ext4 to invoke it.[Edited to
-EINVALinstead ofBUG_ON()for badblocksize_bits-- Joel]Signed-off-by Patrick LoPresti <[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@@ -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@@ -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@@ -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@@ -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
Quoted 3 lines · Andreas Dilger
You can add my:
Acked-by Andreas Dilger <[email protected]>
Thanks!
Joel
Signature · 8 lines
There are morethings in heaven and earth, Horatio, Than are dreamt of in your philosophy. Joel Becker Consulting Software Developer Oracle E-mail: [email protected] Phone: (650) 506-8127
Jan Kara
reviewerQuoted 21 lines · Joel Becker
fs/libfs.c +13
-0@@ -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
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_tcheck. 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
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_tcheck. 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
Jan Kara
reviewerQuoted 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_tcheck. 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
Quoted 26 lines · Jan Kara
Quoted 25 lines · Joel Becker
fs/libfs.c +17
-0@@ -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_tcheck. 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
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_tcheck. 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
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_tcheck. 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_tis 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
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
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
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
8de5cb9164cdc179ba84a07b282a895d0eb794b0Mon 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.
--- fs/libfs.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
fs/libfs.c +4 -4
@@ -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
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_sectorwould be shifting up, which could wrap a really
largelast_fs_blocks. So I'm going to keep thesector_tcheck as-is. How's this:Quoted 2 lines
From
8de5cb9164cdc179ba84a07b282a895d0eb794b0Mon 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 ingeneric_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.
Honza
Quoted 44 lines
--- fs/libfs.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)fs/libfs.c +4
-4@@ -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
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_sectorwould be shifting up, which could wrap a really
largelast_fs_blocks. So I'm going to keep thesector_tcheck as-is. How's this:Quoted 2 lines
From
8de5cb9164cdc179ba84a07b282a895d0eb794b0Mon 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 ingeneric_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-8127Quoted 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
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_tand 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.cand 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
Ted Ts'o
maintainerQuoted 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_tand 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.cand 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.
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