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]>
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 471e1ff..6aff3f5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -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 *);
diff --git a/fs/libfs.c b/fs/libfs.c
index dcaf972..b969648 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -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.
*/
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index 6c953bb..d0643db 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -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)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 4e8983a..979cc57 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -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;
}
On Thu, Jul 22, 2010 at 03:03:41PM -0700, Patrick J. LoPresti wrote:
> 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
--
"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
On Thu, Jul 22, 2010 at 03:03:41PM -0700, Patrick J. LoPresti wrote:
> 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
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 471e1ff..6aff3f5 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -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 *);
> diff --git a/fs/libfs.c b/fs/libfs.c
> index dcaf972..b969648 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -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));
Minus this printk(), of course ;-)
> +
> + 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.
> */
> diff --git a/fs/ext3/super.c b/fs/ext3/super.c
> index 6c953bb..d0643db 100644
> --- a/fs/ext3/super.c
> +++ b/fs/ext3/super.c
> @@ -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)
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 4e8983a..979cc57 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -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
--
"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
On 2010-08-12, at 11:42, Joel Becker wrote:
>> +/**
>> + * 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);
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
On Thu, Aug 12, 2010 at 12:45:41PM -0600, Andreas Dilger wrote:
> On 2010-08-12, at 11:42, Joel Becker wrote:
> >> +/**
> >> + * 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);
>
> 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
--
"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
On 2010-08-12, at 14:15, Joel Becker wrote:
> On Thu, Aug 12, 2010 at 12:45:41PM -0600, Andreas Dilger wrote:
>> On 2010-08-12, at 11:42, Joel Becker wrote:
>>>> +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?
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
On Thu, Aug 12, 2010 at 03:32:27PM -0600, Andreas Dilger wrote:
> > 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:
>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(-)
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index 6c953bb..d0643db 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -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)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index e72d323..e03a7d2 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -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;
}
diff --git a/fs/libfs.c b/fs/libfs.c
index dcaf972..f099566 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -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.
*/
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e5106e4..7644248 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -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 *);
--
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
On Thu, Aug 12, 2010 at 10:42:16AM -0700, Joel Becker wrote:
> On Thu, Jul 22, 2010 at 03:03:41PM -0700, Patrick J. LoPresti wrote:
> > 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.
To be specific, I would like an Acked-by.
Joel
--
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
You can add my:
Acked-by: Andreas Dilger <[email protected]>
On 2010-08-12, at 16:29, Joel Becker wrote:
> On Thu, Aug 12, 2010 at 03:32:27PM -0600, Andreas Dilger wrote:
>>> 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:
>
>> 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(-)
>
> diff --git a/fs/ext3/super.c b/fs/ext3/super.c
> index 6c953bb..d0643db 100644
> --- a/fs/ext3/super.c
> +++ b/fs/ext3/super.c
> @@ -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)
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index e72d323..e03a7d2 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -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;
> }
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index dcaf972..f099566 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -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.
> */
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e5106e4..7644248 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -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 *);
> --
> 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
On Thu, Aug 12, 2010 at 05:07:36PM -0600, Andreas Dilger wrote:
> You can add my:
>
> Acked-by: Andreas Dilger <[email protected]>
Thanks!
Joel
--
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
On Thu, Aug 12, 2010 at 10:42:16AM -0700, Joel Becker wrote:
> On Thu, Jul 22, 2010 at 03:03:41PM -0700, Patrick J. LoPresti wrote:
> > 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.
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
On Thu 12-08-10 15:29:49, Joel Becker wrote:
> diff --git a/fs/libfs.c b/fs/libfs.c
> index dcaf972..f099566 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -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))) {
^^^ 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
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Fri, Aug 13, 2010 at 06:30:07PM +0200, Jan Kara wrote:
> On Thu 12-08-10 15:29:49, Joel Becker wrote:
> > +/**
> > + * 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))) {
> ^^^ 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
--
"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
On Fri, Aug 13, 2010 at 01:47:01PM -0700, Joel Becker wrote:
> On Fri, Aug 13, 2010 at 06:30:07PM +0200, Jan Kara wrote:
> > On Thu 12-08-10 15:29:49, Joel Becker wrote:
> > > +/**
> > > + * 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))) {
> > ^^^ 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.
Wouldn't it rather be:
... ||
((last_fs_block >> (PAGE_CACHE_SHIFT - blocksize_bits)) >
(pgoff_t)(!0ULL))) {
Joel
--
"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 wrote:
> On Thu 12-08-10 15:29:49, Joel Becker wrote:
>> diff --git a/fs/libfs.c b/fs/libfs.c
>> index dcaf972..f099566 100644
>> --- a/fs/libfs.c
>> +++ b/fs/libfs.c
>> @@ -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))) {
> ^^^ 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
> 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
>
On Sun, Aug 15, 2010 at 12:19:36PM -0500, Eric Sandeen wrote:
> >> + (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)
Why not stop here, which is what I put in my other email?
"blocks >> SHIFT-bits" is "how many pages do I need?".
> 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
--
"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
Joel Becker wrote:
> On Sun, Aug 15, 2010 at 12:19:36PM -0500, Eric Sandeen wrote:
>>>> + (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)
>
> 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...
>> 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
> Joel
>
On Sun, Aug 15, 2010 at 10:36:36PM -0500, Eric Sandeen wrote:
> 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
--
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
Joel Becker wrote:
> On Sun, Aug 15, 2010 at 10:36:36PM -0500, Eric Sandeen wrote:
>> 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)".
*nod* saw that but figured it was just a typo & didn't mention it ;)
> 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
> Joel
>
On Fri 13-08-10 15:52:46, Joel Becker wrote:
> On Fri, Aug 13, 2010 at 01:47:01PM -0700, Joel Becker wrote:
> > On Fri, Aug 13, 2010 at 06:30:07PM +0200, Jan Kara wrote:
> > > On Thu 12-08-10 15:29:49, Joel Becker wrote:
> > > > +/**
> > > > + * 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))) {
> > > ^^^ 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.
>
> 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
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon, Aug 16, 2010 at 09:44:35AM -0500, Eric Sandeen wrote:
> Joel Becker wrote:
> > 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:
>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(-)
diff --git a/fs/libfs.c b/fs/libfs.c
index 8debe7b..62baa03 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -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;
--
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
On Mon 16-08-10 12:13:19, Joel Becker wrote:
> On Mon, Aug 16, 2010 at 09:44:35AM -0500, Eric Sandeen wrote:
> > Joel Becker wrote:
> > > 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:
>
> >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]>
Looks good.
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/libfs.c | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 8debe7b..62baa03 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -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;
> --
> 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
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon, Aug 16, 2010 at 09:21:00PM +0200, Jan Kara wrote:
> On Mon 16-08-10 12:13:19, Joel Becker wrote:
> > On Mon, Aug 16, 2010 at 09:44:35AM -0500, Eric Sandeen wrote:
> > > Joel Becker wrote:
> > > > 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:
> >
> > >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]>
> 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
--
"When ideas fail, words come in very handy."
- Goethe
Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127