* [PATCH] fs/minix: bugfix, number of indirect block ptrs per block depends on block size
@ 2010-05-26 10:03 Erik van der Kouwe
2010-05-26 21:54 ` Jan Kara
2010-05-27 4:35 ` Al Viro
0 siblings, 2 replies; 6+ messages in thread
From: Erik van der Kouwe @ 2010-05-26 10:03 UTC (permalink / raw)
To: Alexander Viro, linux-fsdevel, linux-kernel; +Cc: Fam Zheng
From: Erik van der Kouwe <vdkouwe@cs.vu.nl>
The MINIX filesystem driver used a constant number of indirect block
pointers in an indirect block. This worked only for filesystems with 1kb
block, while the MINIX default block size is now 4kb. As a consequence,
large files were read incorrectly on such filesystems and writing a
large file would cause the filesystem to become corrupted. This patch
computes the number of indirect block pointers based on the block size,
making the driver work for each block size.
I would like to thank Feiran Zheng ('Fam') for pointing out the cause
of the corruption.
Signed-off-by: Erik van der Kouwe <vdkouwe@cs.vu.nl>
---
--- fs/minix/itree_v2.c.orig 2010-05-26 14:10:15.000000000 +0200
+++ fs/minix/itree_v2.c 2010-05-26 13:44:53.000000000 +0200
@@ -20,6 +20,9 @@ static inline block_t *i_data(struct ino
return (block_t *)minix_i(inode)->u.i2_data;
}
+#define DIRCOUNT 7
+#define INDIRCOUNT(sb) ((sb)->s_blocksize / 4)
+
static int block_to_path(struct inode * inode, long block, int offsets[DEPTH])
{
int n = 0;
@@ -34,21 +37,21 @@ static int block_to_path(struct inode *
printk("MINIX-fs: block_to_path: "
"block %ld too big on dev %s\n",
block, bdevname(sb->s_bdev, b));
- } else if (block < 7) {
+ } else if (block < DIRCOUNT) {
offsets[n++] = block;
- } else if ((block -= 7) < 256) {
- offsets[n++] = 7;
+ } else if ((block -= DIRCOUNT) < INDIRCOUNT(sb)) {
+ offsets[n++] = DIRCOUNT;
offsets[n++] = block;
- } else if ((block -= 256) < 256*256) {
- offsets[n++] = 8;
- offsets[n++] = block>>8;
- offsets[n++] = block & 255;
+ } else if ((block -= INDIRCOUNT(sb)) < INDIRCOUNT(sb) * INDIRCOUNT(sb)) {
+ offsets[n++] = DIRCOUNT + 1;
+ offsets[n++] = block / INDIRCOUNT(sb);
+ offsets[n++] = block % INDIRCOUNT(sb);
} else {
- block -= 256*256;
- offsets[n++] = 9;
- offsets[n++] = block>>16;
- offsets[n++] = (block>>8) & 255;
- offsets[n++] = block & 255;
+ block -= INDIRCOUNT(sb) * INDIRCOUNT(sb);
+ offsets[n++] = DIRCOUNT + 2;
+ offsets[n++] = (block / INDIRCOUNT(sb)) / INDIRCOUNT(sb);
+ offsets[n++] = (block / INDIRCOUNT(sb)) % INDIRCOUNT(sb);
+ offsets[n++] = block % INDIRCOUNT(sb);
}
return n;
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fs/minix: bugfix, number of indirect block ptrs per block depends on block size
2010-05-26 10:03 [PATCH] fs/minix: bugfix, number of indirect block ptrs per block depends on block size Erik van der Kouwe
@ 2010-05-26 21:54 ` Jan Kara
2010-05-27 16:01 ` Al Viro
2010-05-27 17:11 ` Erik van der Kouwe
2010-05-27 4:35 ` Al Viro
1 sibling, 2 replies; 6+ messages in thread
From: Jan Kara @ 2010-05-26 21:54 UTC (permalink / raw)
To: Erik van der Kouwe; +Cc: Alexander Viro, linux-fsdevel, linux-kernel, Fam Zheng
On Wed 26-05-10 12:03:23, Erik van der Kouwe wrote:
> From: Erik van der Kouwe <vdkouwe@cs.vu.nl>
>
> The MINIX filesystem driver used a constant number of indirect block
> pointers in an indirect block. This worked only for filesystems with
> 1kb block, while the MINIX default block size is now 4kb. As a
> consequence, large files were read incorrectly on such filesystems
> and writing a large file would cause the filesystem to become
> corrupted. This patch computes the number of indirect block pointers
> based on the block size, making the driver work for each block size.
Out of curiosity: Are you really using MINIX? The code is dead for a
long time...
> I would like to thank Feiran Zheng ('Fam') for pointing out the
> cause of the corruption.
>
> Signed-off-by: Erik van der Kouwe <vdkouwe@cs.vu.nl>
> ---
>
> --- fs/minix/itree_v2.c.orig 2010-05-26 14:10:15.000000000 +0200
> +++ fs/minix/itree_v2.c 2010-05-26 13:44:53.000000000 +0200
> @@ -20,6 +20,9 @@ static inline block_t *i_data(struct ino
> return (block_t *)minix_i(inode)->u.i2_data;
> }
>
> +#define DIRCOUNT 7
> +#define INDIRCOUNT(sb) ((sb)->s_blocksize / 4)
> +
> static int block_to_path(struct inode * inode, long block, int offsets[DEPTH])
> {
> int n = 0;
> @@ -34,21 +37,21 @@ static int block_to_path(struct inode *
> printk("MINIX-fs: block_to_path: "
> "block %ld too big on dev %s\n",
> block, bdevname(sb->s_bdev, b));
> - } else if (block < 7) {
> + } else if (block < DIRCOUNT) {
> offsets[n++] = block;
> - } else if ((block -= 7) < 256) {
> - offsets[n++] = 7;
> + } else if ((block -= DIRCOUNT) < INDIRCOUNT(sb)) {
This modification of 'block' in the if condition is a blatant violation
of the kernel coding style. If you feel like it, you could fix it when changing
the conditions anyway. Something like:
if (block < DIRCOUNT) {
offsets[n++] = block;
return n;
}
block -= DIRCOUNT;
if (block < INDIRCOUNT(sb)) {
...
> + offsets[n++] = DIRCOUNT;
> offsets[n++] = block;
> - } else if ((block -= 256) < 256*256) {
> - offsets[n++] = 8;
> - offsets[n++] = block>>8;
> - offsets[n++] = block & 255;
> + } else if ((block -= INDIRCOUNT(sb)) < INDIRCOUNT(sb) * INDIRCOUNT(sb)) {
> + offsets[n++] = DIRCOUNT + 1;
> + offsets[n++] = block / INDIRCOUNT(sb);
> + offsets[n++] = block % INDIRCOUNT(sb);
This division and modulo aren't really necessary. You can define
INDIRCOUNT_BITS(sb) as (sb->s_blocksize_bits - 2) and use it instead
of the division and use & (INDIRCOUNT(sb)-1) instead of the modulo. Not
that it would seriously matter but it's an improvement.
> } else {
> - block -= 256*256;
> - offsets[n++] = 9;
> - offsets[n++] = block>>16;
> - offsets[n++] = (block>>8) & 255;
> - offsets[n++] = block & 255;
> + block -= INDIRCOUNT(sb) * INDIRCOUNT(sb);
> + offsets[n++] = DIRCOUNT + 2;
> + offsets[n++] = (block / INDIRCOUNT(sb)) / INDIRCOUNT(sb);
> + offsets[n++] = (block / INDIRCOUNT(sb)) % INDIRCOUNT(sb);
> + offsets[n++] = block % INDIRCOUNT(sb);
> }
> return n;
> }
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fs/minix: bugfix, number of indirect block ptrs per block depends on block size
2010-05-26 21:54 ` Jan Kara
@ 2010-05-27 16:01 ` Al Viro
2010-05-27 17:11 ` Erik van der Kouwe
1 sibling, 0 replies; 6+ messages in thread
From: Al Viro @ 2010-05-27 16:01 UTC (permalink / raw)
To: Jan Kara; +Cc: Erik van der Kouwe, linux-fsdevel, linux-kernel, Fam Zheng
On Wed, May 26, 2010 at 11:54:43PM +0200, Jan Kara wrote:
> > - } else if ((block -= 7) < 256) {
> > - offsets[n++] = 7;
> > + } else if ((block -= DIRCOUNT) < INDIRCOUNT(sb)) {
> This modification of 'block' in the if condition is a blatant violation
> of the kernel coding style.
The hell it is. [...said the guy who'd written it that way in the first place]
It _is_ idiomatic C for that kind of thing ("we've got several adjacent ranges,
find which one had we hit and how far from its start").
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fs/minix: bugfix, number of indirect block ptrs per block depends on block size
2010-05-26 21:54 ` Jan Kara
2010-05-27 16:01 ` Al Viro
@ 2010-05-27 17:11 ` Erik van der Kouwe
2010-05-27 17:17 ` Jan Kara
1 sibling, 1 reply; 6+ messages in thread
From: Erik van der Kouwe @ 2010-05-27 17:11 UTC (permalink / raw)
To: Jan Kara; +Cc: Alexander Viro, linux-fsdevel, linux-kernel, Fam Zheng
Dear all,
> Out of curiosity: Are you really using MINIX? The code is dead for a
> long time...
MINIX is very much alive, see http://www.minix3.org/.
I work in the team developing MINIX. Being able to exchange data between
Linux and MINIX is very useful for us and there is no other filesystem
that the two share. We have been aware of (and annoyed by) this problem
for a long time, but didn't know the cause (and didn't have much time to
check). Feiran is a GSoC student working on our project and he found the
issue when setting up his development environment so then we decided to
bring out a patch.
With kind regards,
Erik
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs/minix: bugfix, number of indirect block ptrs per block depends on block size
2010-05-27 17:11 ` Erik van der Kouwe
@ 2010-05-27 17:17 ` Jan Kara
0 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2010-05-27 17:17 UTC (permalink / raw)
To: Erik van der Kouwe
Cc: Jan Kara, Alexander Viro, linux-fsdevel, linux-kernel, Fam Zheng
On Thu 27-05-10 19:11:41, Erik van der Kouwe wrote:
> Dear all,
>
> > Out of curiosity: Are you really using MINIX? The code is dead for a
> >long time...
>
> MINIX is very much alive, see http://www.minix3.org/.
>
> I work in the team developing MINIX. Being able to exchange data
> between Linux and MINIX is very useful for us and there is no other
> filesystem that the two share. We have been aware of (and annoyed
> by) this problem for a long time, but didn't know the cause (and
> didn't have much time to check). Feiran is a GSoC student working on
> our project and he found the issue when setting up his development
> environment so then we decided to bring out a patch.
OK, I was more speaking about the Linux MINIX filesystem driver but it's
good to know someone is still using it :) Thanks for education.
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs/minix: bugfix, number of indirect block ptrs per block depends on block size
2010-05-26 10:03 [PATCH] fs/minix: bugfix, number of indirect block ptrs per block depends on block size Erik van der Kouwe
2010-05-26 21:54 ` Jan Kara
@ 2010-05-27 4:35 ` Al Viro
1 sibling, 0 replies; 6+ messages in thread
From: Al Viro @ 2010-05-27 4:35 UTC (permalink / raw)
To: Erik van der Kouwe; +Cc: linux-fsdevel, linux-kernel, Fam Zheng
On Wed, May 26, 2010 at 12:03:23PM +0200, Erik van der Kouwe wrote:
> +#define INDIRCOUNT(sb) ((sb)->s_blocksize / 4)
ITYM (1 << ((sb)->s_blocksize_bits - 2))
Give gcc at least a chance to turn the uses of that into shifts...
Applied with such change.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-05-27 17:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-05-26 10:03 [PATCH] fs/minix: bugfix, number of indirect block ptrs per block depends on block size Erik van der Kouwe
2010-05-26 21:54 ` Jan Kara
2010-05-27 16:01 ` Al Viro
2010-05-27 17:11 ` Erik van der Kouwe
2010-05-27 17:17 ` Jan Kara
2010-05-27 4:35 ` Al Viro
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).