From: Eric Sandeen <sandeen@redhat.com> To: Jan Kara <jack@suse.cz> Cc: Joel Becker <Joel.Becker@oracle.com>, Andreas Dilger <adilger@dilger.ca>, "Ted Ts'o" <tytso@mit.edu>, ocfs2-devel@oss.oracle.com, linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, "linux-kernel@vger.kernel.org Patrick J. LoPresti" <lopresti@gmail.com> Subject: Re: [Ocfs2-devel] [PATCH 1/3] ext3/ext4: Factor out disk addressability check Date: Sun, 15 Aug 2010 12:19:36 -0500 [thread overview] Message-ID: <4C6821A8.5080805@redhat.com> (raw) In-Reply-To: <20100813163006.GB4329@quack.suse.cz> 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 >
WARNING: multiple messages have this Message-ID (diff)
From: Eric Sandeen <sandeen@redhat.com> To: Jan Kara <jack@suse.cz> Cc: Joel Becker <Joel.Becker@oracle.com>, Andreas Dilger <adilger@dilger.ca>, Ted Ts'o <tytso@mit.edu>, ocfs2-devel@oss.oracle.com, linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, "linux-kernel@vger.kernel.org Patrick J. LoPresti" <lopresti@gmail.com> Subject: [Ocfs2-devel] [PATCH 1/3] ext3/ext4: Factor out disk addressability check Date: Sun, 15 Aug 2010 12:19:36 -0500 [thread overview] Message-ID: <4C6821A8.5080805@redhat.com> (raw) In-Reply-To: <20100813163006.GB4329@quack.suse.cz> 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 >
next prev parent reply other threads:[~2010-08-15 17:19 UTC|newest] Thread overview: 63+ messages / expand[flat|nested] mbox.gz Atom feed top 2010-07-22 22:03 [PATCH 1/3] ext3/ext4: Factor out disk addressability check Patrick J. LoPresti 2010-07-22 22:03 ` [Ocfs2-devel] " Patrick J. LoPresti 2010-07-22 22:04 ` [PATCH 2/3] JBD2: Allow feature checks before journal recovery Patrick J. LoPresti 2010-07-22 22:04 ` [Ocfs2-devel] " Patrick J. LoPresti 2010-07-22 22:05 ` [PATCH 3/3] OCFS2: Allow huge (> 16 TiB) volumes to mount Patrick J. LoPresti 2010-07-22 22:05 ` [Ocfs2-devel] " Patrick J. LoPresti 2010-08-12 17:43 ` [Ocfs2-devel] [PATCH 2/3] JBD2: Allow feature checks before journal recovery Joel Becker 2010-08-12 17:43 ` Joel Becker 2010-08-12 23:03 ` Joel Becker 2010-08-12 23:03 ` Joel Becker 2010-08-12 23:03 ` Joel Becker 2010-08-12 23:03 ` Joel Becker 2010-08-12 23:03 ` [Ocfs2-devel] " Joel Becker 2010-08-13 3:39 ` Ted Ts'o 2010-08-13 3:39 ` Ted Ts'o 2010-08-13 7:17 ` Joel Becker 2010-08-13 7:17 ` Joel Becker 2010-08-13 7:17 ` Joel Becker 2010-08-10 15:15 ` [Ocfs2-devel] [PATCH 1/3] ext3/ext4: Factor out disk addressability check Joel Becker 2010-08-10 15:15 ` Joel Becker 2010-08-12 17:42 ` Joel Becker 2010-08-12 17:42 ` Joel Becker 2010-08-12 18:45 ` Andreas Dilger 2010-08-12 18:45 ` Andreas Dilger 2010-08-12 20:15 ` Joel Becker 2010-08-12 20:15 ` Joel Becker 2010-08-12 21:32 ` Andreas Dilger 2010-08-12 21:32 ` Andreas Dilger 2010-08-12 22:29 ` Joel Becker 2010-08-12 22:29 ` Joel Becker 2010-08-12 23:07 ` Andreas Dilger 2010-08-12 23:07 ` Andreas Dilger 2010-08-12 23:13 ` Joel Becker 2010-08-12 23:13 ` Joel Becker 2010-08-13 16:30 ` Jan Kara 2010-08-13 16:30 ` Jan Kara 2010-08-13 20:47 ` Joel Becker 2010-08-13 20:47 ` Joel Becker 2010-08-13 22:52 ` Joel Becker 2010-08-13 22:52 ` Joel Becker 2010-08-16 15:09 ` Jan Kara 2010-08-16 15:09 ` Jan Kara 2010-08-15 17:19 ` Eric Sandeen [this message] 2010-08-15 17:19 ` Eric Sandeen 2010-08-16 2:54 ` Joel Becker 2010-08-16 2:54 ` [Ocfs2-devel] " Joel Becker 2010-08-16 3:36 ` Eric Sandeen 2010-08-16 3:36 ` Eric Sandeen 2010-08-16 9:21 ` Joel Becker 2010-08-16 9:21 ` [Ocfs2-devel] " Joel Becker 2010-08-16 14:44 ` Eric Sandeen 2010-08-16 14:44 ` Eric Sandeen 2010-08-16 14:44 ` Eric Sandeen 2010-08-16 19:13 ` Joel Becker 2010-08-16 19:13 ` [Ocfs2-devel] " Joel Becker 2010-08-16 19:21 ` Jan Kara 2010-08-16 19:21 ` Jan Kara 2010-08-16 20:45 ` Joel Becker 2010-08-16 20:45 ` [Ocfs2-devel] " Joel Becker 2010-08-12 23:03 ` Joel Becker 2010-08-12 23:03 ` [Ocfs2-devel] " Joel Becker 2010-08-13 3:39 ` Ted Ts'o 2010-08-13 3:39 ` Ted Ts'o
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4C6821A8.5080805@redhat.com \ --to=sandeen@redhat.com \ --cc=Joel.Becker@oracle.com \ --cc=adilger@dilger.ca \ --cc=jack@suse.cz \ --cc=linux-ext4@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=lopresti@gmail.com \ --cc=ocfs2-devel@oss.oracle.com \ --cc=tytso@mit.edu \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.