Re: [PATCH 04/10] AXFS: axfs_inode.c

[Arnd Bergmann <arnd@arndb.de>]

On Thursday 21 August 2008, Jared Hulbert wrote:
> +       array_index = AXFS_GET_INODE_ARRAY_INDEX(sbi, ino_number);
> +       array_index += page->index;
> +
> +       node_index = AXFS_GET_NODE_INDEX(sbi, array_index);
> +       node_type = AXFS_GET_NODE_TYPE(sbi, array_index);
> +
> +       if (node_type == Compressed) {
> +               /* node is in compessed region */
> +               cnode_offset = AXFS_GET_CNODE_OFFSET(sbi, node_index);
> +               cnode_index = AXFS_GET_CNODE_INDEX(sbi, node_index);
> +               down_write(&sbi->lock);
> +               if (cnode_index != sbi->current_cnode_index) {
> +                       /* uncompress only necessary if different cblock */
> +                       ofs = AXFS_GET_CBLOCK_OFFSET(sbi, cnode_index);
> +                       len = AXFS_GET_CBLOCK_OFFSET(sbi, cnode_index + 1);
> +                       len -= ofs;
> +                       axfs_copy_data(sb, cblk1, &(sbi->compressed), ofs, len);
> +                       axfs_uncompress_block(cblk0, cblk_size, cblk1, len);
> +                       sbi->current_cnode_index = cnode_index;
> +               }
> +               downgrade_write(&sbi->lock);
> +               max_len = cblk_size - cnode_offset;
> +               len = max_len > PAGE_CACHE_SIZE ? PAGE_CACHE_SIZE : max_len;
> +               src = (void *)((unsigned long)cblk0 + cnode_offset);
> +               memcpy(pgdata, src, len);
> +               up_read(&sbi->lock);

This looks very nice, but could use some comments about how the data is
actually stored on disk. It took me some time to figure out that it actually
allows to do tail merging into compressed blocks, which I was about to suggest
you implement ;-). Cramfs doesn't have them, and I found that they are the
main reason why squashfs compresses better than cramfs, besides the default
block size, which you can change on either one.

Have you seen any benefit of the rwsem over a simple mutex? I would guess
that you can never even get into the situation where you get concurrent
readers since I haven't found a single down_read() in your code, only
downgrade_write().

	Arnd <><