Re: BUG: isofs broken (2.2 and 2.4)

From: Linus Torvalds (torvalds@transmeta.com)
Date: Fri Nov 17 2000 - 17:29:25 EST


On Fri, 17 Nov 2000, Harald Koenig wrote:
>
> this seems to make things much worse: starting with ~90M free memory
> "du" again started leaking (or maybe just using memory?) down to ~80M free
> memory when the system suddently locked up completely, no console switch
> was possible anymore (but Sysrq-B did reboot).

How about this version (full patch against test10 - it includes a
slightly corrected version of my earlier dir.c patch)?

It's entirely untested, but it looks good and compiles. Ship it!

                Linus

-----
diff -u --recursive --new-file v2.4.0-test10/linux/fs/isofs/dir.c linux/fs/isofs/dir.c
--- v2.4.0-test10/linux/fs/isofs/dir.c Fri Aug 11 14:29:01 2000
+++ linux/fs/isofs/dir.c Fri Nov 17 13:38:01 2000
@@ -94,6 +94,14 @@
         return retnamlen;
 }
 
+static struct buffer_head *isofs_bread(struct inode *inode, unsigned int bufsize, unsigned int block)
+{
+ unsigned int blknr = isofs_bmap(inode, block);
+ if (!blknr)
+ return NULL;
+ return bread(inode->i_dev, blknr, bufsize);
+}
+
 /*
  * This should _really_ be cleaned up some day..
  */
@@ -105,7 +113,7 @@
         unsigned char bufbits = ISOFS_BUFFER_BITS(inode);
         unsigned int block, offset;
         int inode_number = 0; /* Quiet GCC */
- struct buffer_head *bh;
+ struct buffer_head *bh = NULL;
         int len;
         int map;
         int high_sierra;
@@ -117,46 +125,25 @@
                 return 0;
  
         offset = filp->f_pos & (bufsize - 1);
- block = isofs_bmap(inode, filp->f_pos >> bufbits);
+ block = filp->f_pos >> bufbits;
         high_sierra = inode->i_sb->u.isofs_sb.s_high_sierra;
 
- if (!block)
- return 0;
-
- if (!(bh = breada(inode->i_dev, block, bufsize, filp->f_pos, inode->i_size)))
- return 0;
-
         while (filp->f_pos < inode->i_size) {
                 int de_len;
-#ifdef DEBUG
- printk("Block, offset, f_pos: %x %x %x\n",
- block, offset, filp->f_pos);
- printk("inode->i_size = %x\n",inode->i_size);
-#endif
- /* Next directory_record on next CDROM sector */
- if (offset >= bufsize) {
-#ifdef DEBUG
- printk("offset >= bufsize\n");
-#endif
- brelse(bh);
- offset = 0;
- block = isofs_bmap(inode, (filp->f_pos) >> bufbits);
- if (!block)
- return 0;
- bh = breada(inode->i_dev, block, bufsize, filp->f_pos, inode->i_size);
+
+ if (!bh) {
+ bh = isofs_bread(inode, bufsize, block);
                         if (!bh)
                                 return 0;
- continue;
                 }
 
                 de = (struct iso_directory_record *) (bh->b_data + offset);
- if(first_de) inode_number = (block << bufbits) + (offset & (bufsize - 1));
+ if (first_de) inode_number = (bh->b_blocknr << bufbits) + offset;
 
                 de_len = *(unsigned char *) de;
 #ifdef DEBUG
                 printk("de_len = %d\n", de_len);
-#endif
-
+#endif
 
                 /* If the length byte is zero, we should move on to the next
                    CDROM sector. If we are at the end of the directory, we
@@ -164,36 +151,33 @@
 
                 if (de_len == 0) {
                         brelse(bh);
- filp->f_pos = ((filp->f_pos & ~(ISOFS_BLOCK_SIZE - 1))
- + ISOFS_BLOCK_SIZE);
+ bh = NULL;
+ filp->f_pos = ((filp->f_pos & ~(ISOFS_BLOCK_SIZE - 1)) + ISOFS_BLOCK_SIZE);
+ block = filp->f_pos >> bufbits;
                         offset = 0;
-
- if (filp->f_pos >= inode->i_size)
- return 0;
-
- block = isofs_bmap(inode, (filp->f_pos) >> bufbits);
- if (!block)
- return 0;
- bh = breada(inode->i_dev, block, bufsize, filp->f_pos, inode->i_size);
- if (!bh)
- return 0;
                         continue;
                 }
 
- offset += de_len;
- if (offset > bufsize) {
- /*
- * This would only normally happen if we had
- * a buggy cdrom image. All directory
- * entries should terminate with a null size
- * or end exactly at the end of the sector.
- */
- printk("next_offset (%x) > bufsize (%lx)\n",
- offset,bufsize);
- break;
+ offset += de_len;
+
+ /* Make sure we have a full directory entry */
+ if (offset >= bufsize) {
+ int slop = bufsize - offset + de_len;
+ memcpy(tmpde, de, slop);
+ offset &= bufsize - 1;
+ block++;
+ brelse(bh);
+ bh = NULL;
+ if (offset == bufsize) {
+ bh = isofs_bread(inode, bufsize, block);
+ if (!bh)
+ return 0;
+ memcpy((void *) tmpde + slop, bh->b_data, de_len - slop);
+ }
+ de = tmpde;
                 }
 
- if(de->flags[-high_sierra] & 0x80) {
+ if (de->flags[-high_sierra] & 0x80) {
                         first_de = 0;
                         filp->f_pos += de_len;
                         continue;
@@ -265,7 +249,7 @@
 
                 continue;
         }
- brelse(bh);
+ if (bh) brelse(bh);
         return 0;
 }
 
diff -u --recursive --new-file v2.4.0-test10/linux/fs/isofs/namei.c linux/fs/isofs/namei.c
--- v2.4.0-test10/linux/fs/isofs/namei.c Mon May 10 14:14:28 1999
+++ linux/fs/isofs/namei.c Fri Nov 17 14:22:33 2000
@@ -49,6 +49,15 @@
         return dentry->d_op->d_compare(dentry, &dentry->d_name, &qstr);
 }
 
+static struct buffer_head *isofs_bread(struct inode *inode, unsigned int bufsize, unsigned int block)
+{
+ unsigned int blknr = isofs_bmap(inode, block);
+ if (!blknr)
+ return NULL;
+ return bread(inode->i_dev, blknr, bufsize);
+}
+
+
 /*
  * isofs_find_entry()
  *
@@ -57,129 +66,82 @@
  * itself (as an inode number). It does NOT read the inode of the
  * entry - you'll have to do that yourself if you want to.
  */
-static struct buffer_head *
-isofs_find_entry(struct inode *dir, struct dentry *dentry, unsigned long *ino)
+static unsigned long
+isofs_find_entry(struct inode *dir, struct dentry *dentry,
+ char * tmpname, struct iso_directory_record * tmpde)
 {
+ unsigned long inode_number;
         unsigned long bufsize = ISOFS_BUFFER_SIZE(dir);
         unsigned char bufbits = ISOFS_BUFFER_BITS(dir);
- unsigned int block, i, f_pos, offset,
- inode_number = 0; /* shut gcc up */
- struct buffer_head * bh , * retval = NULL;
- unsigned int old_offset;
- int dlen, match;
- char * dpnt;
- unsigned char *page = NULL;
- struct iso_directory_record * de = NULL; /* shut gcc up */
- char de_not_in_buf = 0; /* true if de is in kmalloc'd memory */
- char c;
-
- *ino = 0;
-
- if (!(block = dir->u.isofs_i.i_first_extent)) return NULL;
+ unsigned int block, f_pos, offset;
+ struct buffer_head * bh = NULL;
+
+ if (!dir->u.isofs_i.i_first_extent)
+ return 0;
   
         f_pos = 0;
+ offset = 0;
+ block = 0;
 
- offset = f_pos & (bufsize - 1);
- block = isofs_bmap(dir,f_pos >> bufbits);
+ while (f_pos < dir->i_size) {
+ struct iso_directory_record * de;
+ int de_len, match, i, dlen;
+ char *dpnt, c;
 
- if (!block || !(bh = bread(dir->i_dev,block,bufsize))) return NULL;
+ if (!bh) {
+ bh = isofs_bread(dir, bufsize, block);
+ if (!bh)
+ return 0;
+ }
 
- while (f_pos < dir->i_size) {
+ de = (struct iso_directory_record *) (bh->b_data + offset);
+ inode_number = (bh->b_blocknr << bufbits) + offset;
 
- /* if de is in kmalloc'd memory, do not point to the
- next de, instead we will move to the next sector */
- if(!de_not_in_buf) {
- de = (struct iso_directory_record *)
- (bh->b_data + offset);
- }
- inode_number = (block << bufbits) + (offset & (bufsize - 1));
-
- /* If byte is zero, or we had to fetch this de past
- the end of the buffer, this is the end of file, or
- time to move to the next sector. Usually 2048 byte
- boundaries. */
-
- if (*((unsigned char *) de) == 0 || de_not_in_buf) {
- if(de_not_in_buf) {
- /* james@bpgc.com: Since we slopped
- past the end of the last buffer, we
- must start some way into the new
- one */
- de_not_in_buf = 0;
- kfree(de);
- f_pos += offset;
- }
- else {
- offset = 0;
- f_pos = ((f_pos & ~(ISOFS_BLOCK_SIZE - 1))
- + ISOFS_BLOCK_SIZE);
- }
+ de_len = *(unsigned char *) de;
+ if (!de_len) {
                         brelse(bh);
                         bh = NULL;
-
- if (f_pos >= dir->i_size)
- break;
-
- block = isofs_bmap(dir,f_pos>>bufbits);
- if (!block || !(bh = bread(dir->i_dev,block,bufsize)))
- break;
-
- continue; /* Will kick out if past end of directory */
+ f_pos = (f_pos + ISOFS_BLOCK_SIZE) & ~(ISOFS_BLOCK_SIZE - 1);
+ block = f_pos >> bufbits;
+ offset = 0;
+ continue;
                 }
 
- old_offset = offset;
- offset += *((unsigned char *) de);
- f_pos += *((unsigned char *) de);
+ offset += de_len;
+ f_pos += de_len;
 
- /* james@bpgc.com: new code to handle case where the
- directory entry spans two blocks. Usually 1024
- byte boundaries */
+ /* Make sure we have a full directory entry */
                 if (offset >= bufsize) {
- struct buffer_head *bh_next;
-
- /* james@bpgc.com: read the next block, and
- copy the split de into a newly kmalloc'd
- buffer */
- block = isofs_bmap(dir,f_pos>>bufbits);
- if (!block ||
- !(bh_next = bread(dir->i_dev,block,bufsize)))
- break;
-
- de = (struct iso_directory_record *)
- kmalloc(offset - old_offset, GFP_KERNEL);
- memcpy((char *)de, bh->b_data + old_offset,
- bufsize - old_offset);
- memcpy((char *)de + bufsize - old_offset,
- bh_next->b_data, offset - bufsize);
- brelse(bh_next);
- de_not_in_buf = 1;
- offset -= bufsize;
+ int slop = bufsize - offset + de_len;
+ memcpy(tmpde, de, slop);
+ offset &= bufsize - 1;
+ block++;
+ brelse(bh);
+ bh = NULL;
+ if (offset == bufsize) {
+ bh = isofs_bread(dir, bufsize, block);
+ if (!bh)
+ return 0;
+ memcpy((void *) tmpde + slop, bh->b_data, de_len - slop);
+ }
+ de = tmpde;
                 }
+
                 dlen = de->name_len[0];
                 dpnt = de->name;
 
- if (dir->i_sb->u.isofs_sb.s_rock ||
- dir->i_sb->u.isofs_sb.s_joliet_level ||
- dir->i_sb->u.isofs_sb.s_mapping == 'n' ||
- dir->i_sb->u.isofs_sb.s_mapping == 'a') {
- if (! page) {
- page = (unsigned char *)
- __get_free_page(GFP_KERNEL);
- if (!page) break;
- }
- }
                 if (dir->i_sb->u.isofs_sb.s_rock &&
- ((i = get_rock_ridge_filename(de, page, dir)))) {
+ ((i = get_rock_ridge_filename(de, tmpname, dir)))) {
                         dlen = i;
- dpnt = page;
+ dpnt = tmpname;
 #ifdef CONFIG_JOLIET
                 } else if (dir->i_sb->u.isofs_sb.s_joliet_level) {
- dlen = get_joliet_filename(de, dir, page);
- dpnt = page;
+ dlen = get_joliet_filename(de, dir, tmpname);
+ dpnt = tmpname;
 #endif
                 } else if (dir->i_sb->u.isofs_sb.s_mapping == 'a') {
- dlen = get_acorn_filename(de, page, dir);
- dpnt = page;
+ dlen = get_acorn_filename(de, tmpname, dir);
+ dpnt = tmpname;
                 } else if (dir->i_sb->u.isofs_sb.s_mapping == 'n') {
                         for (i = 0; i < dlen; i++) {
                                 c = dpnt[i];
@@ -190,13 +152,13 @@
                                         break;
                                 }
                                 if (c == ';') c = '.';
- page[i] = c;
+ tmpname[i] = c;
                         }
                         /* This allows us to match with and without
                          * a trailing period. */
- if(page[dlen-1] == '.' && dentry->d_name.len == dlen-1)
+ if(tmpname[dlen-1] == '.' && dentry->d_name.len == dlen-1)
                                 dlen--;
- dpnt = page;
+ dpnt = tmpname;
                 }
                 /*
                  * Skip hidden or associated files unless unhide is set
@@ -208,43 +170,32 @@
                         match = (isofs_cmp(dentry,dpnt,dlen) == 0);
                 }
                 if (match) {
- if(inode_number == -1) {
- /* Should only happen for the '..' entry */
- inode_number =
- isofs_lookup_grandparent(dir,
- find_rock_ridge_relocation(de,dir));
- }
- *ino = inode_number;
- retval = bh;
- bh = NULL;
- break;
+ if (bh) brelse(bh);
+ return inode_number;
                 }
         }
- if (page) free_page((unsigned long) page);
         if (bh) brelse(bh);
- if(de_not_in_buf)
- kfree(de);
- return retval;
+ return 0;
 }
 
 struct dentry *isofs_lookup(struct inode * dir, struct dentry * dentry)
 {
         unsigned long ino;
- struct buffer_head * bh;
         struct inode *inode;
+ struct page *page;
 
 #ifdef DEBUG
         printk("lookup: %x %s\n",dir->i_ino, dentry->d_name.name);
 #endif
         dentry->d_op = dir->i_sb->s_root->d_op;
 
- bh = isofs_find_entry(dir, dentry, &ino);
+ page = alloc_page(GFP_USER);
+ ino = isofs_find_entry(dir, dentry, page_address(page), 1024 + page_address(page));
+ __free_page(page);
 
         inode = NULL;
- if (bh) {
- brelse(bh);
-
- inode = iget(dir->i_sb,ino);
+ if (ino) {
+ inode = iget(dir->i_sb, ino);
                 if (!inode)
                         return ERR_PTR(-EACCES);
         }

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/



This archive was generated by hypermail 2b29 : Thu Nov 23 2000 - 21:00:14 EST