After Linus' recent changes there were many complaints
about bugs in the isofs handling. Still, his code is fine.
I know about some things that still need to be done, and
already corrected two flaws yesterday or so, but people
sent me their isofs images and their problem was not caused
by anything I knew about, and behaved like a Heisenbug.
I never read assembler, but looking at the code produced
by gcc (2.95.2) it seemed peculiar, maybe an attempt to
optimize something combining the
if (filp->f_pos >= inode->i_size)
with the
while (filp->f_pos < inode->i_size)
slightly later.
After removal of the superfluous test
if (filp->f_pos >= inode->i_size)
return 0;
all suddenly worked well.
Below a working patch for which the isofs images I got
all are OK. (There is still a lot of silliness here -
superfluous parentheses, a rename of isofs_cmp to isofs_comp
in one file to avoid confusion with the isofs_cmp in another file..)
I have seen that there were discussions on the right compiler to use.
Is 2.95.2 wrong? Have other things to do tomorrow, so it will be
24 hours before I can look at this again.
Note: this is not yet a confirmed compiler bug - would have to check
more details. But after the indicated change all works for me.
Hope that the same is true for the people reporting problems.
Andries
diff -r -u linux-2.4.0-test11/linux/fs/isofs/dir.c linux-2.4.0-test11a/linux/fs/isofs/dir.c
--- linux-2.4.0-test11/linux/fs/isofs/dir.c Wed Nov 22 16:39:32 2000
+++ linux-2.4.0-test11a/linux/fs/isofs/dir.c Thu Nov 23 04:07:56 2000
@@ -115,11 +115,8 @@
char *p = NULL; /* Quiet GCC */
struct iso_directory_record *de;
- if (filp->f_pos >= inode->i_size)
- return 0;
-
- offset = filp->f_pos & (bufsize - 1);
- block = filp->f_pos >> bufbits;
+ offset = (filp->f_pos & (bufsize - 1));
+ block = (filp->f_pos >> bufbits);
high_sierra = inode->i_sb->u.isofs_sb.s_high_sierra;
while (filp->f_pos < inode->i_size) {
@@ -132,7 +129,8 @@
}
de = (struct iso_directory_record *) (bh->b_data + offset);
- if (first_de) inode_number = (bh->b_blocknr << bufbits) + offset;
+ if (first_de)
+ inode_number = (bh->b_blocknr << bufbits) + offset;
de_len = *(unsigned char *) de;
@@ -207,7 +205,7 @@
map = 1;
if (inode->i_sb->u.isofs_sb.s_rock) {
len = get_rock_ridge_filename(de, tmpname, inode);
- if (len != 0) {
+ if (len != 0) { /* may be -1 */
p = tmpname;
map = 0;
}
diff -r -u linux-2.4.0-test11/linux/fs/isofs/inode.c linux-2.4.0-test11a/linux/fs/isofs/inode.c
--- linux-2.4.0-test11/linux/fs/isofs/inode.c Wed Nov 22 16:39:32 2000
+++ linux-2.4.0-test11a/linux/fs/isofs/inode.c Thu Nov 23 02:15:46 2000
@@ -45,14 +45,14 @@
static int isofs_hashi(struct dentry *parent, struct qstr *qstr);
static int isofs_hash(struct dentry *parent, struct qstr *qstr);
-static int isofs_cmpi(struct dentry *dentry, struct qstr *a, struct qstr *b);
-static int isofs_cmp(struct dentry *dentry, struct qstr *a, struct qstr *b);
+static int isofs_compi(struct dentry *dentry, struct qstr *a, struct qstr *b);
+static int isofs_comp(struct dentry *dentry, struct qstr *a, struct qstr *b);
#ifdef CONFIG_JOLIET
static int isofs_hashi_ms(struct dentry *parent, struct qstr *qstr);
static int isofs_hash_ms(struct dentry *parent, struct qstr *qstr);
-static int isofs_cmpi_ms(struct dentry *dentry, struct qstr *a, struct qstr *b);
-static int isofs_cmp_ms(struct dentry *dentry, struct qstr *a, struct qstr *b);
+static int isofs_compi_ms(struct dentry *dentry, struct qstr *a, struct qstr *b);
+static int isofs_comp_ms(struct dentry *dentry, struct qstr *a, struct qstr *b);
#endif
static void isofs_put_super(struct super_block *sb)
@@ -84,20 +84,20 @@
static struct dentry_operations isofs_dentry_ops[] = {
{
d_hash: isofs_hash,
- d_compare: isofs_cmp,
+ d_compare: isofs_comp,
},
{
d_hash: isofs_hashi,
- d_compare: isofs_cmpi,
+ d_compare: isofs_compi,
},
#ifdef CONFIG_JOLIET
{
d_hash: isofs_hash_ms,
- d_compare: isofs_cmp_ms,
+ d_compare: isofs_comp_ms,
},
{
d_hash: isofs_hashi_ms,
- d_compare: isofs_cmpi_ms,
+ d_compare: isofs_compi_ms,
}
#endif
};
@@ -173,7 +173,7 @@
* Case insensitive compare of two isofs names.
*/
static int
-isofs_cmpi_common(struct dentry *dentry,struct qstr *a,struct qstr *b,int ms)
+isofs_compi_common(struct dentry *dentry,struct qstr *a,struct qstr *b,int ms)
{
int alen, blen;
@@ -197,7 +197,7 @@
* Case sensitive compare of two isofs names.
*/
static int
-isofs_cmp_common(struct dentry *dentry,struct qstr *a,struct qstr *b,int ms)
+isofs_comp_common(struct dentry *dentry,struct qstr *a,struct qstr *b,int ms)
{
int alen, blen;
@@ -230,15 +230,15 @@
}
static int
-isofs_cmp(struct dentry *dentry,struct qstr *a,struct qstr *b)
+isofs_comp(struct dentry *dentry,struct qstr *a,struct qstr *b)
{
- return isofs_cmp_common(dentry, a, b, 0);
+ return isofs_comp_common(dentry, a, b, 0);
}
static int
-isofs_cmpi(struct dentry *dentry,struct qstr *a,struct qstr *b)
+isofs_compi(struct dentry *dentry,struct qstr *a,struct qstr *b)
{
- return isofs_cmpi_common(dentry, a, b, 0);
+ return isofs_compi_common(dentry, a, b, 0);
}
#ifdef CONFIG_JOLIET
@@ -255,15 +255,15 @@
}
static int
-isofs_cmp_ms(struct dentry *dentry,struct qstr *a,struct qstr *b)
+isofs_comp_ms(struct dentry *dentry,struct qstr *a,struct qstr *b)
{
- return isofs_cmp_common(dentry, a, b, 1);
+ return isofs_comp_common(dentry, a, b, 1);
}
static int
-isofs_cmpi_ms(struct dentry *dentry,struct qstr *a,struct qstr *b)
+isofs_compi_ms(struct dentry *dentry,struct qstr *a,struct qstr *b)
{
- return isofs_cmpi_common(dentry, a, b, 1);
+ return isofs_compi_common(dentry, a, b, 1);
}
#endif
@@ -500,15 +500,13 @@
* that value.
*/
blocksize = get_hardblocksize(dev);
- if( (blocksize != 0)
- && (blocksize > opt.blocksize) )
- {
+ if(blocksize > opt.blocksize) {
/*
* Force the blocksize we are going to use to be the
* hardware blocksize.
*/
opt.blocksize = blocksize;
- }
+ }
blocksize_bits = 0;
{
@@ -605,9 +603,8 @@
pri_bh = NULL;
root_found:
- brelse(pri_bh);
- if (joliet_level && opt.rock == 'n') {
+ if (joliet_level && (pri == NULL || opt.rock == 'n')) {
/* This is the case of Joliet with the norock mount flag.
* A disc with both Joliet and Rock Ridge is handled later
*/
@@ -704,6 +701,7 @@
* We're all done using the volume descriptor, and may need
* to change the device blocksize, so release the buffer now.
*/
+ brelse(pri_bh);
brelse(bh);
/*
@@ -873,7 +871,7 @@
/* Life is simpler than for other filesystem since we never
* have to create a new block, only find an existing one.
*/
-int isofs_get_block(struct inode *inode, long iblock,
+static int isofs_get_block(struct inode *inode, long iblock,
struct buffer_head *bh_result, int create)
{
unsigned long b_off;
diff -r -u linux-2.4.0-test11/linux/fs/isofs/namei.c linux-2.4.0-test11a/linux/fs/isofs/namei.c
--- linux-2.4.0-test11/linux/fs/isofs/namei.c Wed Nov 22 16:39:32 2000
+++ linux-2.4.0-test11a/linux/fs/isofs/namei.c Thu Nov 23 00:42:11 2000
@@ -123,7 +123,7 @@
if (dir->i_sb->u.isofs_sb.s_rock &&
((i = get_rock_ridge_filename(de, tmpname, dir)))) {
- dlen = i;
+ dlen = i; /* possibly -1 */
dpnt = tmpname;
#ifdef CONFIG_JOLIET
} else if (dir->i_sb->u.isofs_sb.s_joliet_level) {
@@ -142,8 +142,9 @@
* Skip hidden or associated files unless unhide is set
*/
match = 0;
- if ((!(de->flags[-dir->i_sb->u.isofs_sb.s_high_sierra] & 5)
- || dir->i_sb->u.isofs_sb.s_unhide == 'y') && dlen)
+ if (dlen > 0 &&
+ (!(de->flags[-dir->i_sb->u.isofs_sb.s_high_sierra] & 5)
+ || dir->i_sb->u.isofs_sb.s_unhide == 'y'))
{
match = (isofs_cmp(dentry,dpnt,dlen) == 0);
}
diff -r -u linux-2.4.0-test11/linux/fs/isofs/rock.c linux-2.4.0-test11a/linux/fs/isofs/rock.c
--- linux-2.4.0-test11/linux/fs/isofs/rock.c Wed Nov 22 16:39:32 2000
+++ linux-2.4.0-test11a/linux/fs/isofs/rock.c Thu Nov 23 00:38:58 2000
@@ -152,6 +152,7 @@
return retval;
}
+/* return length of name field; 0: not found, -1: to be ignored */
int get_rock_ridge_filename(struct iso_directory_record * de,
char * retname, struct inode * inode)
{
@@ -215,7 +216,7 @@
printk("RR: RE (%x)\n", inode->i_ino);
#endif
if (buffer) kfree(buffer);
- return 0;
+ return -1;
default:
break;
}
On Thu, 23 Nov 2000 [email protected] wrote:
>
> I never read assembler, but looking at the code produced
> by gcc (2.95.2) it seemed peculiar, maybe an attempt to
> optimize something combining the
> if (filp->f_pos >= inode->i_size)
> with the
> while (filp->f_pos < inode->i_size)
> slightly later.
Can you send me the code in question? I don't have gcc-2.95.2, and I don't
want to install it. If this is truly a compiler bug, I'd like to see it
and verify it and get it reported to the gcc lists asap, as these kinds of
things are so damn nasty to find.
> I have seen that there were discussions on the right compiler to use.
> Is 2.95.2 wrong? Have other things to do tomorrow, so it will be
> 24 hours before I can look at this again.
2.95.2 should have been reasonably ok, but egcs-2.91.66 is probably
considered the most stable compiler right now.
Note that gcc has always had problems with "long long" variables. Very few
people use them as they aren't standard, and the code generation can be
much trickier, so bugs are much more likely. This (along with performance
issues) was why I refused the original LFS patches - they put "long long"
code all over the place.
Linus
On Thu, Nov 23, 2000 at 07:37:27AM -0800, Linus Torvalds wrote:
> > I have seen that there were discussions on the right compiler to use.
> > Is 2.95.2 wrong? Have other things to do tomorrow, so it will be
> > 24 hours before I can look at this again.
>
> 2.95.2 should have been reasonably ok, but egcs-2.91.66 is probably
> considered the most stable compiler right now.
>
> Note that gcc has always had problems with "long long" variables. Very few
> people use them as they aren't standard, and the code generation can be
> much trickier, so bugs are much more likely. This (along with performance
> issues) was why I refused the original LFS patches - they put "long long"
> code all over the place.
gcc 2.95.2 seems to have more problems with long long the egcs 1.1, I have
at least one test case where it miscompiles a variable long long shift.
-Andi
On Thu, Nov 23, 2000 at 04:50:22AM +0100, [email protected] wrote:
> Below a working patch for which the isofs images I got
> all are OK. (There is still a lot of silliness here -
> superfluous parentheses, a rename of isofs_cmp to isofs_comp
> in one file to avoid confusion with the isofs_cmp in another file..)
Yup, indeed it solves the dir/namei problem.
However, while my ide drive is fine, my mistumi (mcdx) still oopses and/or
aaies when doing dd on it. Here's what I was able to catch from the trace..
doesn't look too relevant, tho. I tried oopisng it again, but it spew a
list of hex addresses as long as my arm and couldn't catch the first one.
__wake_up ce-28
0xe3c9d34d
handle_scancode
del_timer
handle_scancode
do_SAK
vc_resize
Where __wake_up:
c0113f95: 89 15 4c 01 22 c0 movl %edx,0xc022014c
c0113f9b: ff 05 44 c4 25 c0 incl 0xc025c444
c0113fa1: 89 d8 movl %ebx,%eax
c0113fa3: e8 10 fa ff ff call c01139b8 <reschedule_idle>
c0113fa8: 57 pushl %edi
c0113fa9: 9d popf
c0113faa: 8b 4d f8 movl 0xfffffff8(%ebp),%ecx
c0113fad: 23 4e 00 andl 0x0(%esi),%ecx
c0113fb0: f6 c1 01 testb $0x1,%cl
c0113fb3: 75 43 jne c0113ff8 <__wake_up+0xd0>
c0113fb5: 8b 45 e8 movl 0xffffffe8(%ebp),%eax
c0113fb8: 39 45 e4 cmpl %eax,0xffffffe4(%ebp)
c0113fbb: 74 3b je c0113ff8 <__wake_up+0xd0>
c0113fbd: 8b 75 e4 movl 0xffffffe4(%ebp),%esi
c0113fc0: 8b 55 e4 movl 0xffffffe4(%ebp),%edx
c0113fc3: 83 c6 f8 addl $0xfffffff8,%esi
c0113fc6: 8b 12 movl (%edx),%edx
c0113fc8: 89 55 e4 movl %edx,0xffffffe4(%ebp)
c0113fcb: 8b 5e 04 movl 0x4(%esi),%ebx
>>> c0113fce: 8b 03 movl (%ebx),%eax
c0113fd0: 85 45 fc testl %eax,0xfffffffc(%ebp)
c0113fd3: 74 e0 je c0113fb5 <__wake_up+0x8d>
c0113fd5: 83 7d f4 00 cmpl $0x0,0xfffffff4(%ebp)
c0113fd9: 74 96 je c0113f71 <__wake_up+0x49>
c0113fdb: 8b 4d f8 movl 0xfffffff8(%ebp),%ecx
c0113fde: 23 4e 00 andl 0x0(%esi),%ecx
--
____/| Ragnar H?jland Freedom - Linux - OpenGL Fingerprint 94C4B
\ o.O| 2F0D27DE025BE2302C
=(_)= "Thou shalt not follow the NULL pointer for 104B78C56 B72F0822
U chaos and madness await thee at its end." hkp://keys.pgp.com
Handle via comment channels only.
On Thu, 23 Nov 2000, Ragnar Hojland Espinosa wrote:
>
> Yup, indeed it solves the dir/namei problem.
Can you check whether the single patch of _just_ removing the extra "f_pos
>= i_size" test in do_isofs_readdir() fixes it? The other changes of
Andries patch look like they should not affect code generation at all, but
I'd still like to verify that it's only that part. If so, it definitely
looks like a gcc-2.95.2 code generation bug - that single if() statement
does not actually matter for the end result, it's just a (misguided)
early-out optimization.
[ Btw, looking at the generated assembly is quite painful. Ugh. It reminds
me again why "long long" is to be avoided with gcc. Getting rid of the
extra test actually improves and speeds up that function probably
simply because the 64-bit arithmetic just cofuses gcc so badly. ]
Linus
On Thu, 23 Nov 2000, Linus Torvalds wrote:
> To tie two threads together again: the thread about FS corruption is one
> of my main worries right now. Do people who see this happen to use a gcc
> other than egcs-2.91.66? I know Andries apparently has 2.95.2, and he's
> one of the people who have reported corruption problems...
I am using 2.91.66. Ok, I'll get on with testing Al Viro's latest patch
now.
Regards,
Tigran
On Thu, Nov 23, 2000 at 09:20:15AM -0800, Linus Torvalds wrote:
> Can you check whether the single patch of _just_ removing the extra "f_pos
> >= i_size" test in do_isofs_readdir() fixes it? The other changes of
> Andries patch look like they should not affect code generation at all, but
> I'd still like to verify that it's only that part. If so, it definitely
Verified, it does.
--
____/| Ragnar H?jland Freedom - Linux - OpenGL Fingerprint 94C4B
\ o.O| 2F0D27DE025BE2302C
=(_)= "Thou shalt not follow the NULL pointer for 104B78C56 B72F0822
U chaos and madness await thee at its end." hkp://keys.pgp.com
On Thu, Nov 23, 2000 at 08:59:46AM -0800, Linus Torvalds wrote:
> [ Btw, I noticed that one of my machines _does_ have gcc-2.95.2, so I can
> look at the isofs code generation myself. I don't see anything obvious,
> and the code is hairy. The differences between 2.91.66 and 2.95.2 are
> big enough that a plain "diff" doesn't show anything clear. Andi, what
> does your test-case look like? ]
Just a variable width shift of long long with both arguments fetched
from deep indirection via pointers, and that all in a function with
register pressure (extracted from the XFS source, which does all kinds
of nasty things with long long)
I am actually not sure if the normal kernel contains even a variable
width long long shift.
-Andi (who also sees some strange hangs in 2.4.0test11-pre, but more
suspects his own hacks. no fs corruption, but I do not run dbench normally)
On Thu, 23 Nov 2000, Andi Kleen wrote:
>
> I am actually not sure if the normal kernel contains even a variable
> width long long shift.
Sure it does. The isofs code contains exctly that:
block = filp->f_pos >> bufbits;
In fact, almost all filesystems do this at some point. ext2 does it for
directories too, for some very similar reasons that isofs does. See
fs/ext2/dir.c:
blk = (filp->f_pos) >> EXT2_BLOCK_SIZE_BITS(sb);
(and don't ask me about the extraneous parenthesis. I bet some LISP
programmer felt alone and decided to make it a bit more homey).
Linus
On Thu, Nov 23, 2000 at 12:38:55PM -0800, Linus Torvalds wrote:
...
> In fact, almost all filesystems do this at some point. ext2 does it for
> directories too, for some very similar reasons that isofs does. See
> fs/ext2/dir.c:
>
> blk = (filp->f_pos) >> EXT2_BLOCK_SIZE_BITS(sb);
>
> (and don't ask me about the extraneous parenthesis. I bet some LISP
> programmer felt alone and decided to make it a bit more homey).
>
> Linus
Propably some programmer has been bitten once too many times with
C's operator precedence rules, which only affect more complicated
expressions -- and thus are used rarely, and not remembered well.
(Or perhaps rememberance is felt to be weak, and parenthesis solve it.)
/Matti Aarnio
On Thu, 23 Nov 2000, Matti Aarnio wrote:
> On Thu, Nov 23, 2000 at 12:38:55PM -0800, Linus Torvalds wrote:
> ...
> > In fact, almost all filesystems do this at some point. ext2 does it for
> > directories too, for some very similar reasons that isofs does. See
> > fs/ext2/dir.c:
> >
> > blk = (filp->f_pos) >> EXT2_BLOCK_SIZE_BITS(sb);
> >
> > (and don't ask me about the extraneous parenthesis. I bet some LISP
> > programmer felt alone and decided to make it a bit more homey).
> >
> > Linus
>
> Propably some programmer has been bitten once too many times with
> C's operator precedence rules, which only affect more complicated
> expressions -- and thus are used rarely, and not remembered well.
Come again? Precedence or not, how in hell could anything be stronger than
-> or . on the _right_ side? Field names are atoms, you can't have an
expression there...