Re: [PATCH][1/2] SquashFS

Andrew Morton
2005-03-15T00:55:41Z

Phillip Lougher <phillip@lougher.demon.co.uk> wrote:
>
> Please apply the following two patches which adds SquashFS to the
> kernel.

> +
> +#include <linux/types.h>
> +#include <linux/squashfs_fs.h>
> +#include <linux/module.h>
> +#include <linux/errno.h>
> +#include <linux/slab.h>
> +#include <linux/fs.h>
> +#include <linux/smp_lock.h>
> +#include <linux/slab.h>
> +#include <linux/squashfs_fs_sb.h>
> +#include <linux/squashfs_fs_i.h>
> +#include <linux/buffer_head.h>
> +#include <linux/vfs.h>
> +#include <linux/init.h>
> +#include <linux/dcache.h>
> +#include <asm/uaccess.h>
> +#include <linux/wait.h>
> +#include <asm/semaphore.h>
> +#include <linux/zlib.h>
> +#include <linux/blkdev.h>
> +#include <linux/vmalloc.h>
> +#include "squashfs.h"
> +

We normally put aam includes after linux includes:

#include <linux/a.h>
#include <linux/b.h>

#include <asm/a.h>
#Include <asm/b.h>

> +
> +DECLARE_MUTEX(read_data_mutex);
> +

Does this need to have global scope? If so, it needs a less generic name.
`squashfs_read_data_mutex' would suit.

> +static struct file_system_type squashfs_fs_type = {
> + .owner = THIS_MODULE,
> + .name = "squashfs",
> + .get_sb = squashfs_get_sb,
> + .kill_sb = kill_block_super,
> + .fs_flags = FS_REQUIRES_DEV
> + };
> +

The final brace should go in column 1.

> +
> +static struct buffer_head *get_block_length(struct super_block *s,
> + int *cur_index, int *offset, int *c_byte)
> +{
> + squashfs_sb_info *msBlk = (squashfs_sb_info *)s->s_fs_info;

s_fs_info has type void*. Hence there is no need to typecast when
assigning pointers to or from it. In fact it is a little harmful to do so.

Please search both your patches for all occurrences of s_fs_info and remove
the typecasts. There are many.

> + unsigned short temp;
> + struct buffer_head *bh;
> +
> + if (!(bh = sb_bread(s, *cur_index)))
> + return NULL;
> +
> + if (msBlk->devblksize - *offset == 1) {
> + if (msBlk->swap)
> + ((unsigned char *) &temp)[1] = *((unsigned char *)
> + (bh->b_data + *offset));
> + else
> + ((unsigned char *) &temp)[0] = *((unsigned char *)
> + (bh->b_data + *offset));

All this typecasting looks nasty. Is there a nicer way? Perhaps using a
temporary variable?

Is this code endian-safe?

> + if (msBlk->swap)
> + ((unsigned char *) &temp)[0] = *((unsigned char *)
> + bh->b_data);
> + else
> + ((unsigned char *) &temp)[1] = *((unsigned char *)
> + bh->b_data);
> + *c_byte = temp;
> + *offset = 1;
> + } else {
> + if (msBlk->swap) {
> + ((unsigned char *) &temp)[1] = *((unsigned char *)
> + (bh->b_data + *offset));
> + ((unsigned char *) &temp)[0] = *((unsigned char *)
> + (bh->b_data + *offset + 1));
> + } else {
> + ((unsigned char *) &temp)[0] = *((unsigned char *)
> + (bh->b_data + *offset));
> + ((unsigned char *) &temp)[1] = *((unsigned char *)
> + (bh->b_data + *offset + 1));
> + }

Ditto.

> +
> + if (SQUASHFS_CHECK_DATA(msBlk->sBlk.flags)) {
> + if (*offset == msBlk->devblksize) {
> + brelse(bh);
> + if (!(bh = sb_bread(s, ++(*cur_index))))
> + return NULL;
> + *offset = 0;
> + }
> + if (*((unsigned char *) (bh->b_data + *offset)) !=
> + SQUASHFS_MARKER_BYTE) {
> + ERROR("Metadata block marker corrupt @ %x\n",
> + *cur_index);
> + brelse(bh);
> + return NULL;

Multiple return statements per function are a maintainability problem,
especially if some of them are deep inside that function's logic. The old
`goto out' is preferred.

(Imagine what would happen if you later wanted to change this function to
kmalloc a bit of temp storage and you don't want it to leak).

> + }
> + (*offset) ++;

whitespace.

> +unsigned int squashfs_read_data(struct super_block *s, char *buffer,
> + unsigned int index, unsigned int length,
> + unsigned int *next_index)
> +{
> + squashfs_sb_info *msBlk = (squashfs_sb_info *)s->s_fs_info;
> + struct buffer_head *bh[((SQUASHFS_FILE_MAX_SIZE - 1) >>
> + msBlk->devblksize_log2) + 2];

Dynamically sized local storage. Deliberate? What is the upper bound on
its size?

> +block_release:
> + while (--b >= 0) brelse(bh[b]);

while (--b >= 0)
brelse(bh[b]);

please.

> +
> + if (n == 0) {
> + wait_queue_t wait;
> +
> + init_waitqueue_entry(&wait, current);
> + add_wait_queue(&msBlk->waitq, &wait);
> + set_current_state(TASK_UNINTERRUPTIBLE);
> + up(&msBlk->block_cache_mutex);
> + schedule();
> + set_current_state(TASK_RUNNING);
> + remove_wait_queue(&msBlk->waitq, &wait);
> + continue;
> + }

- Use DECLARE_WAITQUEUE (or DEFINE_WAIT)

- schedule() always returns in state TASK_RUNNING.

> + msBlk->next_cache = (i + 1) % SQUASHFS_CACHED_BLKS;
> +
> + if (msBlk->block_cache[i].block ==
> + SQUASHFS_INVALID_BLK) {
> + if (!(msBlk->block_cache[i].data =
> + (unsigned char *)
> + kmalloc(SQUASHFS_METADATA_SIZE,
> + GFP_KERNEL))) {

kmalloc() returns void*, hence no cast is needed. Please search both
patches for all instances of kmalloc(), remove the typecasts.

> + if (n == 0) {
> + wait_queue_t wait;
> +
> + init_waitqueue_entry(&wait, current);
> + add_wait_queue(&msBlk->fragment_wait_queue,
> + &wait);
> + set_current_state(TASK_UNINTERRUPTIBLE);
> + up(&msBlk->fragment_mutex);
> + schedule();
> + set_current_state(TASK_RUNNING);
> + remove_wait_queue(&msBlk->fragment_wait_queue,
> + &wait);
> + continue;
> + }

See above.

> +
> +static struct inode *squashfs_iget(struct super_block *s, squashfs_inode inode)
> +{
> + struct inode *i;
> + squashfs_sb_info *msBlk = (squashfs_sb_info *)s->s_fs_info;
> + squashfs_super_block *sBlk = &msBlk->sBlk;
> + unsigned int block = SQUASHFS_INODE_BLK(inode) +
> + sBlk->inode_table_start;
> + unsigned int offset = SQUASHFS_INODE_OFFSET(inode);
> + unsigned int ino = SQUASHFS_MK_VFS_INODE(block
> + - sBlk->inode_table_start, offset);
> + unsigned int next_block, next_offset;
> + squashfs_base_inode_header inodeb;

How much stack space is being used here? Perhaps you should run
scripts/checkstack.pl across the whole thing.

> + TRACE("Entered squashfs_iget\n");
> +
> + if (msBlk->swap) {
> + squashfs_base_inode_header sinodeb;
> +

More stack

> +
> + switch(inodeb.inode_type) {
> + case SQUASHFS_FILE_TYPE: {
> + squashfs_reg_inode_header inodep;
> + unsigned int frag_blk, frag_size;

And more

> + case SQUASHFS_DIR_TYPE: {
> + squashfs_dir_inode_header inodep;

And more!

> + if (msBlk->swap) {
> + squashfs_dir_inode_header sinodep;

> + }
> + case SQUASHFS_LDIR_TYPE: {
> + squashfs_ldir_inode_header inodep;

> + }
> + case SQUASHFS_SYMLINK_TYPE: {
> + squashfs_symlink_inode_header inodep;

> + case SQUASHFS_BLKDEV_TYPE:
> + case SQUASHFS_CHRDEV_TYPE: {
> + squashfs_dev_inode_header inodep;

More.

> +static int squashfs_symlink_readpage(struct file *file, struct page *page)
> +{
> + struct inode *inode = page->mapping->host;
> + int index = page->index << PAGE_CACHE_SHIFT, length, bytes;
> + unsigned int block = SQUASHFS_I(inode)->start_block;
> + int offset = SQUASHFS_I(inode)->offset;
> + void *pageaddr = kmap(page);
> +
> + TRACE("Entered squashfs_symlink_readpage, page index %d, start block "
> + "%x, offset %x\n", page->index,
> + SQUASHFS_I(inode)->start_block,
> + SQUASHFS_I(inode)->offset);
> +
> + for (length = 0; length < index; length += bytes) {
> + if (!(bytes = squashfs_get_cached_block(inode->i_sb, NULL,
> + block, offset, PAGE_CACHE_SIZE, &block,
> + &offset))) {
> + ERROR("Unable to read symbolic link [%x:%x]\n", block,
> + offset);
> + goto skip_read;
> + }
> + }
> +
> + if (length != index) {
> + ERROR("(squashfs_symlink_readpage) length != index\n");
> + bytes = 0;
> + goto skip_read;
> + }
> +
> + bytes = (inode->i_size - length) > PAGE_CACHE_SIZE ? PAGE_CACHE_SIZE :
> + inode->i_size - length;

One should normally use i_size_read(), but I guess thi is OK on symlinks.

> +skip_read:
> + memset(pageaddr + bytes, 0, PAGE_CACHE_SIZE - bytes);
> + kunmap(page);
> + flush_dcache_page(page);
> + SetPageUptodate(page);
> + unlock_page(page);
> +
> + return 0;
> +}

There's no need to run flush_dcache_page() against a symlink - they cannot
be mmapped.

See if you can use kmap_atomic() here - kmap() is slow, theoretically
deadlocky and is deprecated.

> +static unsigned int read_blocklist(struct inode *inode, int index,
> + int readahead_blks, char *block_list,
> + unsigned short **block_p, unsigned int *bsize)
> +{
> ...
> +
> + if (msBlk->swap) {
> + unsigned char sblock_list[SIZE];

How large is `SIZE'?

That seems to be a risky choice of identifier. I guess it's so generic
that nobody else used it, so you're safe ;)

> +
> +static int squashfs_readpage(struct file *file, struct page *page)
> +{
> + struct inode *inode = page->mapping->host;
> + squashfs_sb_info *msBlk = (squashfs_sb_info *)inode->i_sb->s_fs_info;
> + squashfs_super_block *sBlk = &msBlk->sBlk;
> + unsigned char block_list[SIZE];
> + unsigned int bsize, block, i = 0, bytes = 0, byte_offset = 0;
> + int index = page->index >> (sBlk->block_log - PAGE_CACHE_SHIFT);
> + void *pageaddr = kmap(page);
> + struct squashfs_fragment_cache *fragment = NULL;
> + char *data_ptr = msBlk->read_page;
> +
> + int mask = (1 << (sBlk->block_log - PAGE_CACHE_SHIFT)) - 1;
> + int start_index = page->index & ~mask;
> + int end_index = start_index | mask;
> +
> + TRACE("Entered squashfs_readpage, page index %x, start block %x\n",
> + (unsigned int) page->index,
> + SQUASHFS_I(inode)->start_block);
> +
> + if (page->index >= ((inode->i_size + PAGE_CACHE_SIZE - 1) >>
> + PAGE_CACHE_SHIFT)) {
> + goto skip_read;
> + }

This should use i_size_read(). Please review the patches for all instances
of open-coded i_size reads and writes.

> +
> + if (i == page->index) {
> + memcpy(pageaddr, data_ptr + byte_offset,
> + available_bytes);
> + memset(pageaddr + available_bytes, 0,
> + PAGE_CACHE_SIZE - available_bytes);
> + kunmap(page);
> + flush_dcache_page(page);
> + SetPageUptodate(page);
> + unlock_page(page);
> + } else if ((push_page =
> + grab_cache_page_nowait(page->mapping, i))) {
> + void *pageaddr = kmap(push_page);
> +
> + memcpy(pageaddr, data_ptr + byte_offset,
> + available_bytes);
> + memset(pageaddr + available_bytes, 0,
> + PAGE_CACHE_SIZE - available_bytes);
> + kunmap(push_page);
> + flush_dcache_page(push_page);
> + SetPageUptodate(push_page);
> + unlock_page(push_page);
> + page_cache_release(push_page);

kmap_atomic() can be used here.

> +static int squashfs_readpage4K(struct file *file, struct page *page)
> +{
> + struct inode *inode = page->mapping->host;
> + squashfs_sb_info *msBlk = (squashfs_sb_info *)inode->i_sb->s_fs_info;
> + squashfs_super_block *sBlk = &msBlk->sBlk;
> + unsigned char block_list[SIZE];
> + unsigned int bsize, block, bytes = 0;
> + void *pageaddr = kmap(page);

Stack space?

> +
> +
> +static int get_dir_index_using_name(struct super_block *s, unsigned int
> + *next_block, unsigned int *next_offset,
> + unsigned int index_start,
> + unsigned int index_offset, int i_count,
> + const char *name, int size)
> +{
> + squashfs_sb_info *msBlk = (squashfs_sb_info *)s->s_fs_info;
> + squashfs_super_block *sBlk = &msBlk->sBlk;
> + int i, length = 0;
> + char buffer[sizeof(squashfs_dir_index) + SQUASHFS_NAME_LEN + 1];
> + squashfs_dir_index *index = (squashfs_dir_index *) buffer;
> + char str[SQUASHFS_NAME_LEN + 1];
> +
> + TRACE("Entered get_dir_index_using_name, i_count %d\n", i_count);
> +
> + strncpy(str, name, size);
> + str[size] = '\0';

Are you sure that this strncpy() is always safe?

> + dirs_read ++;

We don't put a space before the `++'. Please review both patches for this.

> +static struct inode *squashfs_alloc_inode(struct super_block *sb)
> +{
> + struct squashfs_inode_info *ei;
> + ei = (struct squashfs_inode_info *)
> + kmem_cache_alloc(squashfs_inode_cachep, SLAB_KERNEL);

kmem_cache_alloc() returns void*.

> +static void init_once(void * foo, kmem_cache_t * cachep, unsigned long flags)
> +{
> + struct squashfs_inode_info *ei = (struct squashfs_inode_info *) foo;

Unneeded typecast.

> +static int init_inodecache(void)

This can be __init.