Date: Sat, 26 Jan 2008 22:04:04 -0800
From: Andrew Morton <akpm@linux-foundation.org>
To: Kyungmin Park <kmpark@infradead.org>
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] CRAMFS: Uncompressed files support
Message-Id: <20080126220404.5c128ca3.akpm@linux-foundation.org>
In-Reply-To: <20080122022345.GA3678@party>
References: <20080122022345.GA3678@party>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3057
Lines: 79

> On Tue, 22 Jan 2008 11:23:45 +0900 Kyungmin Park <kmpark@infradead.org> wrote:
> Hi,
> 
> This patch enables the uncompressed files support in cramfs.
> 
> The word 'uncompressed file' is from linear cramfs (aka Application XIP).
> In linear cramfs, it is used to suport XIP on NOR. However it is also helpful on OneNAND. It makes a filesystem faster by removing compression overhead.
> In XIP mode it runs XIP, But non-XIP mode. It copies data to ram and runs.
> 
> In my simple test, copy busybox (compressed or uncompressed).
> It reduces the about 50% time saving from 0.40s to 0.19s.
> Yes, it incrases the file system size, but nowadays flash has big capacity.
> It's trade-off between size and performance.
> 
> Also this patch uses the page cache directly.
> In previous implementation, it used the local buffer. why?
> It's already uncompressed and fits to page size. So It uses the page directly to remove useless memory copy.
> 
> It's compatible the existing linear cramfs image and original one.
> 
> Any comments are welcome.
> 
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index 3d194a2..edba28f 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c

I don't know what kernel this is against but it generates a lot of rejects
against present mainline.

> @@ -40,6 +40,7 @@ static DEFINE_MUTEX(read_mutex);
>  #define CRAMINO(x)	(((x)->offset && (x)->size)?(x)->offset<<2:1)
>  #define OFFSET(x)	((x)->i_ino)
>  
> +#define CRAMFS_INODE_IS_XIP(x)	((x)->i_mode & S_ISVTX)
>  
>  static int cramfs_iget5_test(struct inode *inode, void *opaque)
>  {
> @@ -143,8 +144,9 @@ static int next_buffer;
>  /*
>   * Returns a pointer to a buffer containing at least LEN bytes of
>   * filesystem starting at byte offset OFFSET into the filesystem.
> + * If the @pg has the page, it returns the page buffer address
>   */
> -static void *cramfs_read(struct super_block *sb, unsigned int offset, unsigned int len)
> +static void *cramfs_read(struct super_block *sb, unsigned int offset, unsigned int len, struct page **pg)
>  {
>  	struct address_space *mapping = sb->s_bdev->bd_inode->i_mapping;
>  	struct page *pages[BLKS_PER_BUF];
> @@ -174,6 +176,22 @@ static void *cramfs_read(struct super_block *sb, unsigned int offset, unsigned i
>  
>  	devsize = mapping->host->i_size >> PAGE_CACHE_SHIFT;
>  
> +	/*
> +	 * Use page directly either 
> +	 * - uncompressed page or
> +	 * - comprssed page which has all required data
> +	 */
> +	if (pg && offset + len <= PAGE_CACHE_SIZE) {
> +		struct page *page = NULL;
> +		page = read_mapping_page(mapping, blocknr, NULL);
> +		if (!IS_ERR(page)) {
> +			*pg = page;
> +			data = kmap(page);
> +			data += offset;
> +			return data;
> +		}
> +	}

Are you sure about the kmap/kunmap handling in this patch?  It looks like
it might be unbalanced.


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