[PATCH][1/2] SquashFS

13 views
Skip to first unread message

Phillip Lougher

unread,
Mar 14, 2005, 12:45:37 PM3/14/05
to Andrew Morton, linux-...@vger.kernel.org, Greg KH
Andrew, Greg,

Please apply the following two patches which adds SquashFS to the
kernel. SquashFS is self contained compressed filesystem, and it
has been used by a large amount of projects over the last few years
without problems.

A number of people have started to ask me to submit it to the kernel.
Please consider adding it.

The SquashFS patch has been split into two. This email contains
inode.c, the next email contains everything else.

Thanks

Phillip Lougher

Signed-Off-By: Phillip Lougher (phi...@lougher.demon.co.uk)


patch1

Matt Mackall

unread,
Mar 14, 2005, 7:48:17 PM3/14/05
to Phillip Lougher, Andrew Morton, linux-...@vger.kernel.org, Greg KH
A quick skim...

> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + *
> + * inode.c
> + */
> +
> +#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"

Please put all the asm/ bits after all the linux/ bits.
> +
> +static void squashfs_put_super(struct super_block *);
> +static int squashfs_statfs(struct super_block *, struct kstatfs *);
> +static int squashfs_symlink_readpage(struct file *file, struct page *page);
> +static int squashfs_readpage(struct file *file, struct page *page);
> +static int squashfs_readpage4K(struct file *file, struct page *page);
> +static int squashfs_readdir(struct file *, void *, filldir_t);
> +static void squashfs_put_super(struct super_block *s);
> +static struct inode *squashfs_alloc_inode(struct super_block *sb);
> +static void squashfs_destroy_inode(struct inode *inode);
> +static int init_inodecache(void);
> +static void destroy_inodecache(void);
> +static struct dentry *squashfs_lookup(struct inode *, struct dentry *,
> + struct nameidata *);
> +static struct inode *squashfs_iget(struct super_block *s, squashfs_inode inode);
> +static unsigned int read_blocklist(struct inode *inode, int index,
> + int readahead_blks, char *block_list,
> + unsigned short **block_p, unsigned int *bsize);
> +static struct super_block *squashfs_get_sb(struct file_system_type *, int,
> + const char *, void *);

Would be nice to reorder things so that fewer forward declarations
were needed.

> +static z_stream stream;
> +
> +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
> + };
^^^^
Weird whitespace.

> +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;

Needless cast from void *. Mixed case identifiers are discouraged.

> + if (!(bh = sb_bread(s, *cur_index)))
> + return NULL;

Please don't do assignment inside if().

> + 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));

That's rather ugly, what's going on here? There seems to be a lot of
this swapping going on. At the very least, let's use u8.

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

Linebreak.

> + for (i = msBlk->next_cache, n = SQUASHFS_CACHED_BLKS;
> + n ; n --, i = (i + 1) %
> + SQUASHFS_CACHED_BLKS)

Messy. Do n-- (no space), handle i outside the for control structures.
Perhaps break this piece out into a separate function, the indenting
is making things cramped.

> + 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);

I suspect you'll find there's a much cleaner way to do whatever it is you're
trying to do here.

> + if (!(msBlk->block_cache[i].data =
> + (unsigned char *)
> + kmalloc(SQUASHFS_METADATA_SIZE,
> + GFP_KERNEL))) {

Another class of unnecessary cast.

> + msBlk->fragment_index[SQUASHFS_FRAGMENT_INDEX(fragment)];
> + int offset = SQUASHFS_FRAGMENT_INDEX_OFFSET(fragment);

Feel free to make these defines a little less unwieldy. So long as
they're internal to Squashfs.

> + for (;;) {

while (1)

> + for (i = 0; i < SQUASHFS_CACHED_FRAGMENTS &&
> + msBlk->fragment[i].block != start_block; i++);

';' on its own line. Better is

for (i = 0; i < SQUASHFS_CACHED_FRAGMENTS; i++)
if (msBlk->fragment[i].block != start_block)
break;

> + WARNING("Mounting a different endian SQUASHFS "
> + "filesystem on %s\n", bdevname(s->s_bdev, b));

Can't we just use printk for this stuff?

> + bytes = (inode->i_size - length) > PAGE_CACHE_SIZE ? PAGE_CACHE_SIZE :
> + inode->i_size - length;

min()

> + if (page->index >= ((inode->i_size + PAGE_CACHE_SIZE - 1) >>
> + PAGE_CACHE_SHIFT)) {
> + goto skip_read;
> + }

Skip braces for single statements.

> + block = (msBlk->read_blocklist)(inode, page->index, 1,
> + block_list, NULL, &bsize);

Unnecessary parens around the function pointer.

...

This file could use quite a bit of work in terms of reducing nesting
depth and the like.

--
Mathematics is the supreme nostalgia of our time.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Andrew Morton

unread,
Mar 14, 2005, 7:58:47 PM3/14/05
to Phillip Lougher, linux-...@vger.kernel.org, gr...@kroah.com
Phillip Lougher <phi...@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.

Nick Piggin

unread,
Mar 14, 2005, 8:48:38 PM3/14/05
to Matt Mackall, Phillip Lougher, Andrew Morton, linux-...@vger.kernel.org, Greg KH
Matt Mackall wrote:

>>+ for (;;) {
>>
>
>while (1)
>
>

I always thought for (;;) was preferred. Or at least acceptable?

Matt Mackall

unread,
Mar 14, 2005, 9:36:55 PM3/14/05
to Nick Piggin, Phillip Lougher, Andrew Morton, linux-...@vger.kernel.org, Greg KH
On Tue, Mar 15, 2005 at 12:47:23PM +1100, Nick Piggin wrote:
> Matt Mackall wrote:
>
> >>+ for (;;) {
> >
> >while (1)
>
> I always thought for (;;) was preferred. Or at least acceptable?

The for (;;) form has always struck me as needlessly clever and I've
known it to puzzle coworkers. I try to make my for loops fall into the
mold of simple initialize/test/advance. But no, I'm not aware of any
LKML concensus opinion on this particular point.

The assignment-in-if problem is a bit more serious as it exacerbates
the jammed-up-against-the-right-margin formatting issues.

--
Mathematics is the supreme nostalgia of our time.

Paul Jackson

unread,
Mar 15, 2005, 3:52:31 AM3/15/05
to Nick Piggin, m...@selenic.com, phi...@lougher.demon.co.uk, ak...@osdl.org, linux-...@vger.kernel.org, gr...@kroah.com
In the overall kernel (Linus's bk tree) I count:

733 lines matching 'for *( *; *; *)'
718 lines matching 'while *( *1 *)'

In the kernel/*.c files, I count 15 of the 'for(;;)' style and 1 of the
'while(1)' style.

Certainly the 'for(;;)' style is acceptable, and even slightly to
substantially dominant, depending on which piece of code you're in.

--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <p...@engr.sgi.com> 1.650.933.1373, 1.925.600.0401

Phillip Lougher

unread,
Mar 15, 2005, 12:07:23 PM3/15/05
to Paul Jackson, Nick Piggin, m...@selenic.com, ak...@osdl.org, linux-...@vger.kernel.org, gr...@kroah.com
Paul Jackson wrote:
> In the overall kernel (Linus's bk tree) I count:
>
> 733 lines matching 'for *( *; *; *)'
> 718 lines matching 'while *( *1 *)'
>
> In the kernel/*.c files, I count 15 of the 'for(;;)' style and 1 of the
> 'while(1)' style.
>
> Certainly the 'for(;;)' style is acceptable, and even slightly to
> substantially dominant, depending on which piece of code you're in.
>

I prefer the 'while' style, and only used 'for' because that's what I
thought the kernel used.

If no-one objects I'll change it back to while...

Shouldn't issues like this be in the coding style document?

Phillip

Matt Mackall

unread,
Mar 15, 2005, 12:35:40 PM3/15/05
to Phillip Lougher, Paul Jackson, Nick Piggin, ak...@osdl.org, linux-...@vger.kernel.org, gr...@kroah.com
On Tue, Mar 15, 2005 at 03:50:26PM +0000, Phillip Lougher wrote:
> Paul Jackson wrote:
> >In the overall kernel (Linus's bk tree) I count:
> >
> > 733 lines matching 'for *( *; *; *)'
> > 718 lines matching 'while *( *1 *)'
> >
> >In the kernel/*.c files, I count 15 of the 'for(;;)' style and 1 of the
> >'while(1)' style.
> >
> >Certainly the 'for(;;)' style is acceptable, and even slightly to
> >substantially dominant, depending on which piece of code you're in.
> >
>
> I prefer the 'while' style, and only used 'for' because that's what I
> thought the kernel used.
>
> If no-one objects I'll change it back to while...
>
> Shouldn't issues like this be in the coding style document?

This particular point is rather trivial. Do whatever suits you.

--
Mathematics is the supreme nostalgia of our time.

Phillip Lougher

unread,
Mar 15, 2005, 12:44:48 PM3/15/05
to Matt Mackall, Paul Jackson, Nick Piggin, ak...@osdl.org, linux-...@vger.kernel.org, gr...@kroah.com
Matt Mackall wrote:
> On Tue, Mar 15, 2005 at 03:50:26PM +0000, Phillip Lougher wrote:
>
>>Paul Jackson wrote:
>>
>>>In the overall kernel (Linus's bk tree) I count:
>>>
>>> 733 lines matching 'for *( *; *; *)'
>>> 718 lines matching 'while *( *1 *)'
>>>
>>>In the kernel/*.c files, I count 15 of the 'for(;;)' style and 1 of the
>>>'while(1)' style.
>>>
>>>Certainly the 'for(;;)' style is acceptable, and even slightly to
>>>substantially dominant, depending on which piece of code you're in.
>>>
>>
>>I prefer the 'while' style, and only used 'for' because that's what I
>>thought the kernel used.
>>
>>If no-one objects I'll change it back to while...
>>
>>Shouldn't issues like this be in the coding style document?
>
>
> This particular point is rather trivial. Do whatever suits you.
>

It's a shame the 'rather trivial' issue got picked up in the first place
then :-) I didn't raise the issue.

Paul Jackson

unread,
Mar 15, 2005, 2:25:35 PM3/15/05
to Phillip Lougher, m...@selenic.com, nickp...@yahoo.com.au, ak...@osdl.org, linux-...@vger.kernel.org, gr...@kroah.com
> >>Shouldn't issues like this be in the coding style document?

There is not a concensus (nor a King Penguin dictate) between the
"while(1)" and "for(;;)" style to document. If this were a
frequently asked question, I suppose someone would eventually
note in a coding style doc that either style is acceptable.


> It's a shame the 'rather trivial' issue got picked up in the first place

Not a shame at all. Such coding style issues are well known to be a
proper subject for discussion here.

--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <p...@engr.sgi.com> 1.650.933.1373, 1.925.600.0401

Junio C Hamano

unread,
Mar 15, 2005, 8:51:58 PM3/15/05
to linux-...@vger.kernel.org
>>>>> "PJ" == Paul Jackson <p...@engr.sgi.com> writes:

PJ> There is not a concensus (nor a King Penguin dictate) between the
PJ> "while(1)" and "for(;;)" style to document.

FWIW, linux-0.01 has four uses of "while (1)" and two uses of
"for (;;)" ;-).

./fs/inode.c: while (1) {
./fs/namei.c: while (1) {
./fs/namei.c: while (1) {
./kernel/sched.c: while (1) {

./init/main.c: for(;;) pause();
./kernel/panic.c: for(;;);

What is interesting here is that the King Penguin used these two
constructs with consistency. The "while (1)" form was used with
normal exit routes with "if (...) break" inside; while the
"for(;;)" form was used only in unusual "the thread of control
should get stuck here forever" cases.

So, Phillip's decision to go back to his original while(1) style
seems to be in line with the style used in the original Linux
kernel ;-).

Paul Jackson

unread,
Mar 16, 2005, 2:17:12 AM3/16/05
to Junio C Hamano, linux-...@vger.kernel.org
> the King Penguin used these two constructs with consistency:

Nice distinction - thanks.

--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <p...@engr.sgi.com> 1.650.933.1373, 1.925.600.0401

jerome lacoste

unread,
Mar 17, 2005, 3:13:44 PM3/17/05
to Junio C Hamano, linux-...@vger.kernel.org
On Tue, 15 Mar 2005 17:50:02 -0800, Junio C Hamano <jun...@cox.net> wrote:
> >>>>> "PJ" == Paul Jackson <p...@engr.sgi.com> writes:
>
> PJ> There is not a concensus (nor a King Penguin dictate) between the
> PJ> "while(1)" and "for(;;)" style to document.
>
> FWIW, linux-0.01 has four uses of "while (1)" and two uses of
> "for (;;)" ;-).
>
> ./fs/inode.c: while (1) {
> ./fs/namei.c: while (1) {
> ./fs/namei.c: while (1) {
> ./kernel/sched.c: while (1) {
>
> ./init/main.c: for(;;) pause();
> ./kernel/panic.c: for(;;);
>
> What is interesting here is that the King Penguin used these two
> constructs with consistency. The "while (1)" form was used with
> normal exit routes with "if (...) break" inside; while the
> "for(;;)" form was used only in unusual "the thread of control
> should get stuck here forever" cases.
>
> So, Phillip's decision to go back to his original while(1) style
> seems to be in line with the style used in the original Linux
> kernel ;-).

After the Pinguin janitors, now comes the Pinguin archeologists.

This starts to be lemmingesque :)

J

Phillip Lougher

unread,
Mar 18, 2005, 10:27:36 PM3/18/05
to Andrew Morton, linux-...@vger.kernel.org, gr...@kroah.com
Andrew Morton wrote in relation to my initial SquashFS patch:

> Phillip Lougher <phi...@lougher.demon.co.uk> wrote:
>
>>+skip_read:
>>+ memset(pageaddr + bytes, 0, PAGE_CACHE_SIZE - bytes);
>>+ kunmap(page);
>>+ flush_dcache_page(page);
>>+ SetPageUptodate(page);
>>+ unlock_page(page);
>>+
>>+ return 0;
>>+}

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

From some browsing of the kernel source and a useful article on lwn.net
I think I can replace all the readpage_xxx (symlink, readpage &
readpage4k) kmap/kunmaps with kmap_atomic(page, KM_USER0)/kunmap(kaddr,
KM_USER0)...

The lwn.net article mentions that k[un]map_atomic is the new alternative
to kmap (as Andrew Morton mentioned).

I've noticed in asm-ppc/highmem.h the following comment

/*
* The use of kmap_atomic/kunmap_atomic is discouraged - kmap/kunmap
* gives a more generic (and caching) interface. But kmap_atomic can
* be used in IRQ contexts, so in some (very limited) cases we need
* it.
*/

Given what Andrew and the lwn.net article says, this comment is rather
confusing. Is it wrong?

Regards

Phillip Lougher

Andrew Morton

unread,
Mar 18, 2005, 10:44:32 PM3/18/05
to Phillip Lougher, linux-...@vger.kernel.org, gr...@kroah.com
Phillip Lougher <phi...@lougher.demon.co.uk> wrote:
>
> Andrew Morton wrote in relation to my initial SquashFS patch:
> > Phillip Lougher <phi...@lougher.demon.co.uk> wrote:
> >
> >>+skip_read:
> >>+ memset(pageaddr + bytes, 0, PAGE_CACHE_SIZE - bytes);
> >>+ kunmap(page);
> >>+ flush_dcache_page(page);
> >>+ SetPageUptodate(page);
> >>+ unlock_page(page);
> >>+
> >>+ return 0;
> >>+}
>
> > See if you can use kmap_atomic() here - kmap() is slow, theoretically
> > deadlocky and is deprecated.
> >
>
> From some browsing of the kernel source and a useful article on lwn.net
> I think I can replace all the readpage_xxx (symlink, readpage &
> readpage4k) kmap/kunmaps with kmap_atomic(page, KM_USER0)/kunmap(kaddr,
> KM_USER0)...

Yes.

Generally the only reason the kernel needs to modify pagecache pages is to
do a quick memset or to copy a string in there or something like that. So
a quick kmap_atomic/memory copy/kunmap_atomic() cycle is a fairly common
pattern.

The one exception is the directory-in-pagecache code where we tend to do a
lot of stuff while holding the kmap. Not that there's any particular
reason why we needed to do it that way.

> The lwn.net article mentions that k[un]map_atomic is the new alternative
> to kmap (as Andrew Morton mentioned).
>
> I've noticed in asm-ppc/highmem.h the following comment
>
> /*
> * The use of kmap_atomic/kunmap_atomic is discouraged - kmap/kunmap
> * gives a more generic (and caching) interface. But kmap_atomic can
> * be used in IRQ contexts, so in some (very limited) cases we need
> * it.
> */
>
> Given what Andrew and the lwn.net article says, this comment is rather
> confusing. Is it wrong?

I'd say so, yes. For a start, kmap_high() takes a global lock.

Phillip Lougher

unread,
Mar 20, 2005, 7:52:05 PM3/20/05
to Andrew Morton, linux-...@vger.kernel.org
Andrew Morton wrote:

> Phillip Lougher <phi...@lougher.demon.co.uk> wrote:
>>
>>+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.
>

A lot of the functions use a fair amount of stack (I never thought it
was excessive)... This is the result of running checkstack.pl against
the code on Intel.

0x00003a3c get_dir_index_using_name: 596
0x00000d80 squashfs_iget: 488
0x000044d8 squashfs_lookup: 380
0x00003d00 squashfs_readdir: 372
0x000020fe squashfs_fill_super: 316
0x000031b8 squashfs_readpage: 308
0x00002f5c read_blocklist: 296
0x00003634 squashfs_readpage4K: 284

A couple of these functions show a fair amount of stack use. What is
the maximum acceptable usage, i.e. do any of the above functions need
work to reduce their stack usage?

Thanks

Phillip

Andrew Morton

unread,
Mar 20, 2005, 8:01:16 PM3/20/05
to Phillip Lougher, linux-...@vger.kernel.org

There's no hard-and-fast rule. The conditions running up to a stack
overrun are necessarily complex, and rare. But you can see that for a
twenty or thirty function deep call stack, 500 bytes is a big bite out of
4k.

> i.e. do any of the above functions need
> work to reduce their stack usage?

I'd say so, yes. If at all possible.

Reply all
Reply to author
Forward
0 new messages