* Re: [PATCH 01/10] AXFS: exporting xip_file_fault() for use with AXFS
From: Paulius Zaleckas @ 2008-08-21 7:32 UTC (permalink / raw)
Cc: jaredeh, Jörn Engel, linux-mtd, Linux-kernel, linux-embedded,
tim.bird
In-Reply-To: <48AD1860.4040001@teltonika.lt>
Paulius Zaleckas wrote:
> Jared Hulbert wrote:
>> We'll need this for AXFS. The equivalent function in filemap.c is exported.
>>
>> Signed-off-by: Jared Hulbert <jaredeh@gmail.com>
>> ---
>> include/linux/mm.h | 4 ++++
>> mm/filemap_xip.c | 3 ++-
>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 72a15dc..2be5c22 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -161,6 +161,10 @@ struct vm_fault {
>> */
>> };
>>
>> +#ifdef CONFIG_FS_XIP
>> +extern int xip_file_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
>> +#endif
>> +
>
> No need for #ifdef here as this function is compiled and exported in .c
> regardless of CONFIG_FS_XIP
Sorry I am wrong... Have to get more sleep :)
>> /*
>> * These are the virtual MM functions - opening of an area, closing and
>> * unmapping it (needed to keep files on disk up-to-date etc), pointer
>> diff --git a/mm/filemap_xip.c b/mm/filemap_xip.c
>> index 380ab40..ab2f174 100644
>> --- a/mm/filemap_xip.c
>> +++ b/mm/filemap_xip.c
>> @@ -206,7 +206,7 @@ __xip_unmap (struct address_space * mapping,
>> *
>> * This function is derived from filemap_fault, but used for execute in place
>> */
>> -static int xip_file_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>> +int xip_file_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>> {
>> struct file *file = vma->vm_file;
>> struct address_space *mapping = file->f_mapping;
>> @@ -262,6 +262,7 @@ found:
>> return 0;
>> }
>> }
>> +EXPORT_SYMBOL_GPL(xip_file_fault);
>>
>> static struct vm_operations_struct xip_file_vm_ops = {
>> .fault = xip_file_fault,
>>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
^ permalink raw reply
* Re: [PATCH 01/10] AXFS: exporting xip_file_fault() for use with AXFS
From: Carsten Otte @ 2008-08-21 7:42 UTC (permalink / raw)
To: jaredeh
Cc: Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, nickpiggin
In-Reply-To: <48AD00D5.5010904@gmail.com>
Jared Hulbert wrote:
> We'll need this for AXFS. The equivalent function in filemap.c is exported.
>
> Signed-off-by: Jared Hulbert <jaredeh@gmail.com>
> ---
> include/linux/mm.h | 4 ++++
> mm/filemap_xip.c | 3 ++-
> 2 files changed, 6 insertions(+), 1 deletion(-)
Acked-by: Carsten Otte <cotte@de.ibm.com>
^ permalink raw reply
* Re: [PATCH 03/10] AXFS: axfs.h
From: Carsten Otte @ 2008-08-21 7:51 UTC (permalink / raw)
To: jaredeh
Cc: Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, nickpiggin
In-Reply-To: <48AD00E6.2070505@gmail.com>
Jared Hulbert wrote:
> +#define AXFS_GET_BYTETABLE_VAL(desc,index) \
> + axfs_bytetable_stitch(((struct axfs_region_desc)(desc)).table_byte_depth,\
> + (u8 *)((struct axfs_region_desc)(desc)).virt_addr, index)
> +
> +#define AXFS_GET_NODE_TYPE(sbi,node_index) \
> + AXFS_GET_BYTETABLE_VAL(((struct axfs_super *)(sbi))->node_type,\
> + (node_index))
> +
> +#define AXFS_GET_NODE_INDEX(sbi,node__index) \
> + AXFS_GET_BYTETABLE_VAL(((struct axfs_super *)(sbi))->node_index,\
> + (node__index))
> +
> +#define AXFS_IS_NODE_XIP(sbi,node_index) \
> + (AXFS_GET_NODE_TYPE(sbi, (node_index)) == XIP)
> +
> +#define AXFS_GET_CNODE_INDEX(sbi,node_index) \
> + AXFS_GET_BYTETABLE_VAL(((struct axfs_super *)(sbi))->cnode_index,\
> + (node_index))
> +
> +#define AXFS_GET_CNODE_OFFSET(desc,node_index) \
> + AXFS_GET_BYTETABLE_VAL(((struct axfs_super *)(sbi))->cnode_offset,\
> + (node_index))
> +
> +#define AXFS_GET_BANODE_OFFSET(desc,node_index) \
> + AXFS_GET_BYTETABLE_VAL(((struct axfs_super *)(sbi))->banode_offset,\
> + (node_index))
> +
> +#define AXFS_GET_CBLOCK_OFFSET(desc,node_index) \
> + AXFS_GET_BYTETABLE_VAL(((struct axfs_super *)(sbi))->cblock_offset,\
> + (node_index))
> +
> +#define AXFS_GET_INODE_FILE_SIZE(sbi,inode_index) \
> + AXFS_GET_BYTETABLE_VAL(((struct axfs_super *)(sbi))->inode_file_size,\
> + (inode_index))
> +
> +#define AXFS_GET_INODE_NAME_OFFSET(sbi,inode_index) \
> + AXFS_GET_BYTETABLE_VAL(((struct axfs_super *)(sbi))->inode_name_offset,\
> + (inode_index))
> +
> +#define AXFS_GET_INODE_NUM_ENTRIES(sbi,inode_index) \
> + AXFS_GET_BYTETABLE_VAL(((struct axfs_super *)(sbi))->inode_num_entries,\
> + (inode_index))
> +
> +#define AXFS_GET_INODE_MODE_INDEX(sbi,inode_index) \
> + AXFS_GET_BYTETABLE_VAL(((struct axfs_super *)(sbi))->inode_mode_index,\
> + (inode_index))
> +
> +#define AXFS_GET_INODE_ARRAY_INDEX(sbi,inode_index) \
> + AXFS_GET_BYTETABLE_VAL(((struct axfs_super *)(sbi))->inode_array_index,\
> + (inode_index))
> +
> +#define AXFS_GET_MODE(sbi,mode_index) \
> + AXFS_GET_BYTETABLE_VAL(((struct axfs_super *)(sbi))->modes,\
> + (AXFS_GET_INODE_MODE_INDEX(sbi, (mode_index))))
> +
> +#define AXFS_GET_UID(sbi,mode_index) \
> + AXFS_GET_BYTETABLE_VAL(((struct axfs_super *)(sbi))->uids,\
> + (AXFS_GET_INODE_MODE_INDEX(sbi, (mode_index))))
> +
> +#define AXFS_GET_GID(sbi,mode_index) \
> + AXFS_GET_BYTETABLE_VAL(((struct axfs_super *)(sbi))->gids,\
> + (AXFS_GET_INODE_MODE_INDEX(sbi, (mode_index))))
> +
> +#define AXFS_IS_REGION_COMPRESSED(_region) \
> + (( \
> + ((struct axfs_region_desc *)(_region))->compressed_size > \
> + 0 \
> + ) ? TRUE : FALSE)
> +
> +#define AXFS_PHYSADDR_IS_VALID(sbi) \
> + (((((struct axfs_super *)(sbi))->phys_start_addr) > 0 \
> + ) ? TRUE : FALSE)
> +
> +#define AXFS_VIRTADDR_IS_VALID(sbi) \
> + (((((struct axfs_super *)(sbi))->virt_start_addr) > 0 \
> + ) ? TRUE : FALSE)
> +
> +#define AXFS_IS_IOMEM(sbi) \
> + (((((struct axfs_super *)(sbi))->iomem_size) > 0) ? TRUE : FALSE)
> +
> +#define AXFS_IS_POINTED(sbi) \
> + (((((struct axfs_super *)(sbi))->mtd_pointed) > 0) ? TRUE : FALSE)
> +
> +#define AXFS_IS_PHYSMEM(sbi) \
> + (( \
> + AXFS_PHYSADDR_IS_VALID(sbi) \
> + && !AXFS_IS_IOMEM(sbi) \
> + && !AXFS_IS_POINTED(sbi) \
> + ) ? TRUE : FALSE)
> +
> +#define AXFS_IS_MMAPABLE(sbi,offset) \
> + ((\
> + (((struct axfs_super *)(sbi))->mmap_size) > (offset) \
> + ) ? TRUE : FALSE)
> +
> +#define AXFS_IS_OFFSET_MMAPABLE(sbi,offset) \
> + (( \
> + AXFS_IS_MMAPABLE(sbi, offset) && AXFS_VIRTADDR_IS_VALID(sbi) \
> + ) ? TRUE : FALSE)
> +
> +#define AXFS_IS_REGION_MMAPABLE(sbi,_region) \
> + (( \
> + AXFS_IS_MMAPABLE(sbi, ((struct axfs_region_desc *)(_region))->fsoffset) \
> + && AXFS_VIRTADDR_IS_VALID(sbi) \
> + ) ? TRUE : FALSE)
> +
> +#define AXFS_IS_REGION_INCORE(_region) \
> + (((_region)->incore > 0) ? TRUE : FALSE)
> +
> +#define AXFS_IS_REGION_XIP(sbi,_region) \
> + (( \
> + !AXFS_IS_REGION_COMPRESSED(_region) && \
> + !AXFS_IS_REGION_INCORE(_region) && \
> + AXFS_IS_REGION_MMAPABLE(sbi,_region) \
> + ) ? TRUE : FALSE)
> +
> +#define AXFS_GET_XIP_REGION_PHYSADDR(sbi) \
> + (unsigned long)((sbi)->phys_start_addr + (sbi)->xip.fsoffset)
> +
> +#define AXFS_GET_INODE_NAME(sbi,inode_index) \
> + (char *)( \
> + (sbi)->strings.virt_addr \
> + + AXFS_GET_INODE_NAME_OFFSET(sbi,inode_index) \
> + )
> +
> +#define AXFS_GET_CBLOCK_ADDRESS(sbi, cnode_index)\
> + (unsigned long)( \
> + (sbi)->compressed.virt_addr \
> + + AXFS_GET_CBLOCK_OFFSET(sbi, cnode_index) \
> + )
> +
> +#define AXFS_GET_NODE_ADDRESS(sbi,node__index) \
> + (unsigned long)( \
> + (sbi)->node_index.virt_addr \
> + + AXFS_GET_NODE_INDEX(sbi, node__index) \
> + )
> +
> +#define AXFS_GET_BANODE_ADDRESS(sbi,banode_index) \
> + (unsigned long)( \
> + (sbi)->byte_aligned.virt_addr \
> + + AXFS_GET_BANODE_OFFSET(sbi, banode_index) \
> + )
> +
> +#define AXFS_FSOFFSET_2_DEVOFFSET(sbi,fsoffset) \
> + (( \
> + ((sbi)->phys_start_addr == 0) && ((sbi)->virt_start_addr == 0) \
> + ) ? (fsoffset) : (fsoffset - (sbi)->mmap_size) \
> + )
> +
> +#define AXFS_GET_CBLOCK_LENGTH(sbi,cblock_index) \
> + (u64)( \
> + (u64)AXFS_GET_CBLOCK_OFFSET(sbi, ((u64)(cblock_index)+(u64)1)) \
> + - (u64)AXFS_GET_CBLOCK_OFFSET(sbi, (cblock_index)) \
> + )
> +
> +#ifndef TRUE
> +#define TRUE 1
> +#endif
> +#ifndef FALSE
> +#define FALSE 0
> +#endif
*Shrug*. That part reads scary. Are all those casts and macros really
needed? Maybe it is worth considering to clean this up a little.
^ permalink raw reply
* Re: [PATCH 04/10] AXFS: axfs_inode.c
From: Carsten Otte @ 2008-08-21 8:35 UTC (permalink / raw)
To: jaredeh
Cc: Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, nickpiggin
In-Reply-To: <48AD00F0.5030403@gmail.com>
Jared Hulbert wrote:
> +/***************** functions in other axfs files ******************************/
> +int axfs_get_sb(struct file_system_type *, int, const char *, void *,
> + struct vfsmount *);
This is neither implemented nor used in axfs_inode.c - why define it here?
> +void axfs_kill_super(struct super_block *);
Same for this one.
> +void axfs_profiling_add(struct axfs_super *, unsigned long, unsigned int);
> +int axfs_copy_mtd(struct super_block *, void *, u64, u64);
> +int axfs_copy_block(struct super_block *, void *, u64, u64);
These are used, but not implemented here. Please consider putting them
in a header file
> +static int axfs_copy_data(struct super_block *sb, void *dst,
> + struct axfs_region_desc *region, u64 offset, u64 len)
> +{
> + u64 mmapped = 0;
> + u64 end = region->fsoffset + offset + len;
> + u64 begin = region->fsoffset + offset;
> + u64 left;
> + void *addr;
> + void *newdst;
> + struct axfs_super *sbi = AXFS_SB(sb);
> +
> + if (len == 0)
> + return 0;
> +
> + if (region->virt_addr) {
> + if (sbi->mmap_size >= end) {
> + mmapped = len;
> + } else if (sbi->mmap_size > begin) {
> + mmapped = sbi->mmap_size - begin;
> + }
> + }
You can save braces and make the code more readable here:
=> if (sbi->mmap_size >= end)
mmapped = len;
else if (sbi->mmap_size > begin)
mmapped = si->mmap_size - begin;
> +struct inode *axfs_create_vfs_inode(struct super_block *sb, int ino)
> +{
> + struct axfs_super *sbi = AXFS_SB(sb);
> + struct inode *inode;
> + u64 size;
[SNIP]
> + size = AXFS_GET_INODE_FILE_SIZE(sbi, ino);
> + inode->i_size = size;
The variable size is not needed. Do
inode->i_size = AXFS_GET_INODE_FILE_SIZE(sbi, ino);
> +static struct dentry *axfs_lookup(struct inode *dir, struct dentry *dentry,
> + struct nameidata *nd)
> +{
> + struct super_block *sb = dir->i_sb;
> + struct axfs_super *sbi = AXFS_SB(sb);
> + u64 ino_number = dir->i_ino;
> + u64 dir_index = 0;
> + u64 entry;
> + char *name;
> + int namelen, err;
> +
> + while (dir_index < AXFS_GET_INODE_NUM_ENTRIES(sbi, ino_number)) {
> + entry = AXFS_GET_INODE_ARRAY_INDEX(sbi, ino_number);
> + entry += dir_index;
> +
> + name = AXFS_GET_INODE_NAME(sbi, entry);
> + namelen = strlen(name);
> +
> + /* fast test, the entries are sorted alphabetically and the
> + * first letter is smaller than the first letter in the search
> + * name then it isn't in this directory. Keeps this loop from
> + * needing to scan through always.
> + */
> + if (dentry->d_name.name[0] < name[0])
> + break;
> +
> + dir_index++;
> +
> + /* Quick check that the name is roughly the right length */
> + if (dentry->d_name.len != namelen)
> + continue;
> +
> + err = memcmp(dentry->d_name.name, name, namelen);
> + if (err > 0)
> + continue;
> +
> + /* The file name isn't present in the directory. */
> + if (err < 0)
> + break;
Very ingenious way to compare strings. strncmp also stops after the
first character if it does'nt fit. I doubt this has a measurable
performance advantage over using strncmp, please consider to replace
this logic to make the code smaller and more readable. See lib/string.c.
> +static int axfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
> +{
> + struct inode *inode = filp->f_dentry->d_inode;
> + struct super_block *sb = inode->i_sb;
> + struct axfs_super *sbi = AXFS_SB(sb);
> + u64 ino_number = inode->i_ino;
> + u64 entry;
> + loff_t dir_index;
> + char *name;
> + int namelen, mode;
> + int err = 0;
> +
> + /* Get the current index into the directory and verify it is not beyond
> + the end of the list */
> + dir_index = filp->f_pos;
> + if (dir_index >= AXFS_GET_INODE_NUM_ENTRIES(sbi, ino_number))
> + goto out;
> +
> + /* Verify the inode is for a directory */
> + if (!(S_ISDIR(inode->i_mode))) {
> + err = -EINVAL;
> + goto out;
> + }
Well, -ENOTDIR would be the correct return code. You can remove that
sanity check alltogether, vfs_readdir makes sure this is the right
file type. If you really want to check, make it
BUG_ON(!S_ISDIR(inode->i_mode));
> +static int axfs_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> +{
> + struct file *file = vma->vm_file;
> + struct inode *inode = file->f_dentry->d_inode;
> + struct super_block *sb = inode->i_sb;
> + struct axfs_super *sbi = AXFS_SB(sb);
> + u64 ino_number = inode->i_ino;
> + u64 array_index;
> +
> + array_index = AXFS_GET_INODE_ARRAY_INDEX(sbi, ino_number) + vmf->pgoff;
> +
> + /* if that pages are marked for write they will probably end up in RAM
> + therefore we don't want their counts for being XIP'd */
> + if (!(vma->vm_flags & VM_WRITE))
> + axfs_profiling_add(sbi, array_index, ino_number);
Thats very inacurate profiling, it does never count MAP_PRIVATE
mappings which is the regular case for executables and libraries. When
booting an enterprise distro, my sniff test shows that only about 5%
of the MAP_PRIVATE mappings get COW'ed. To get correct statistics, it
might be a good idea to find a way to add here and substract during
cow. Or to scan these mappings when the profiling information is being
retrieved - the readonly bit in the pte gives the right indication for
MIXEDMAP mappings.
^ permalink raw reply
* Re: [PATCH 00/10] AXFS: Advanced XIP filesystem
From: Dave Chinner @ 2008-08-21 8:39 UTC (permalink / raw)
To: Jared Hulbert
Cc: Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, cotte, nickpiggin
In-Reply-To: <48AD00C4.6060302@gmail.com>
On Wed, Aug 20, 2008 at 10:44:36PM -0700, Jared Hulbert wrote:
> I'd like to get a first round of review on my AXFS filesystem. This is a simple
> read only compressed filesystem like Squashfs and cramfs. AXFS is special
> because it also allows for execute-in-place of your applications. It is a major
> improvement over the cramfs XIP patches that have been floating around for ages.
> The biggest improvement is in the way AXFS allows for each page to be XIP or
> not. First, a user collects information about which pages are accessed on a
> compressed image for each mmap()ed region from /proc/axfs/volume0. That
> 'profile' is used as an input to the image builder. The resulting image has
> only the relevant pages uncompressed and XIP. The result is smaller memory
> sizes and faster launches.
FWIW, I'm not sure it's a good idea to name this new filesystem AXFS.
People are almost certainly going to confuse it with XFS despite
the filesystems being aimed at diammetrically opposed ends of the
storage spectrum. ;)
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* Re: Adding a new platform
From: David Woodhouse @ 2008-08-21 8:40 UTC (permalink / raw)
To: Charles Manning; +Cc: vb, Paul Gortmaker, Linux Embedded Maillist, corbet
In-Reply-To: <200808211502.58206.manningc2@actrix.gen.nz>
On Thu, 2008-08-21 at 15:02 +1200, Charles Manning wrote:
>
> In-tree is no silver bullet.
>
> When people modify internal APIs they will likely fix anything that breaks
> compilation. However many things are more subtle than that and it is very
> easy to end up with a driver or other code that compiles but does not work
> properly.
>
> Luckily APIs for drivers (the most common stuff that people work on) don't
> change that much, and the interfaces are reasonably clear. If you want some
> hell then try working on file systems :-).
File systems show an excellent example of why you're wrong, in fact. The
API for file systems changes quite a lot, and the people who make those
changes do tend to fix up all the in-tree file systems simultaneously,
-- and since they're doing it with a full understanding of the
implications of the change they're making, they usually manage to get
the change _right_.
I find it's more common to introduce subtle errors when the person
modifying the core API _hasn't_ also modified the driver/fs code which
uses that API. Which is what happens when you have out-of-tree code.
I'd rather remove my gonads with a rusty spoon than maintain an
out-of-tree file system.
--
dwmw2
^ permalink raw reply
* Re: [PATCH 05/10] AXFS: axfs_profiling.c
From: Carsten Otte @ 2008-08-21 8:44 UTC (permalink / raw)
To: jaredeh
Cc: Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, nickpiggin
In-Reply-To: <48AD00F8.1030004@gmail.com>
Jared Hulbert wrote:
> Profiling is a fault instrumentation and /proc formating system.
> This is used to get an accurate picture of what the pages are actually used.
> Using this info the image can be optimized for XIP
Exporting profiling data for a file system in another file system
(/proc) seems not very straigtforward to me. I think it is worth
considering to export this information via the same mount point.
^ permalink raw reply
* Re: Adding a new platform
From: Geert Uytterhoeven @ 2008-08-21 8:46 UTC (permalink / raw)
To: Charles Manning; +Cc: vb, Paul Gortmaker, Linux Embedded Maillist, corbet
In-Reply-To: <200808211502.58206.manningc2@actrix.gen.nz>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2095 bytes --]
On Thu, 21 Aug 2008, Charles Manning wrote:
> On Wednesday 20 August 2008 17:15:01 vb wrote:
> > On Tue, Aug 19, 2008 at 9:29 PM, Paul Gortmaker
> >
> > <paul.gortmaker@gmail.com> wrote:
> > > On Tue, Aug 19, 2008 at 11:57 PM, vb <vb@vsbe.com> wrote:
> > >> so, say a developer submits a proprietary driver and it gets accepted.
> > >
> > > Doesn't happen. By design. If the driver is proprietary then it is
> > > presumably not meant for open distribution, and hence not compatible with
> > > GPL and widespread distribution into 100,000 public git repositories. So
> > > it won't get submitted and it won't get accepted.
> >
> > I guess 'proprietary' is not the right term then, how do you call a
> > driver which is not a secret and not a problem to release, but
> > controls some hardware present in only in certain devices of a certain
> > company.
> >
> > Would such a driver be accepted? Wouldn't such a driver get stale
> > after a few kernel releases?
>
> In-tree is no silver bullet.
>
> When people modify internal APIs they will likely fix anything that breaks
> compilation. However many things are more subtle than that and it is very
> easy to end up with a driver or other code that compiles but does not work
> properly.
My experience with Linux/m68k tells me that +95% of the breakage is visible
by compiler warnings and errors. If it still compiles, it still works ;-)
> Luckily APIs for drivers (the most common stuff that people work on) don't
> change that much, and the interfaces are reasonably clear. If you want some
> hell then try working on file systems :-).
Really? So how come so few changes are needed to keep squashfs working?
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010
^ permalink raw reply
* Re: Adding a new platform
From: David Woodhouse @ 2008-08-21 8:48 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Charles Manning, vb, Paul Gortmaker, Linux Embedded Maillist,
corbet
In-Reply-To: <Pine.LNX.4.64.0808211044590.20704@vixen.sonytel.be>
On Thu, 2008-08-21 at 10:46 +0200, Geert Uytterhoeven wrote:
> Really? So how come so few changes are needed to keep squashfs
> working?
It's read-only.
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
^ permalink raw reply
* Re: [PATCH 05/10] AXFS: axfs_profiling.c
From: David Woodhouse @ 2008-08-21 8:49 UTC (permalink / raw)
To: carsteno
Cc: jaredeh, Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, nickpiggin
In-Reply-To: <48AD2AFF.5060609@de.ibm.com>
On Thu, 2008-08-21 at 10:44 +0200, Carsten Otte wrote:
> Jared Hulbert wrote:
> > Profiling is a fault instrumentation and /proc formating system.
> > This is used to get an accurate picture of what the pages are actually used.
> > Using this info the image can be optimized for XIP
> Exporting profiling data for a file system in another file system
> (/proc) seems not very straigtforward to me. I think it is worth
> considering to export this information via the same mount point.
I would have said sysfs, rather than 'the same mount point'.
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
^ permalink raw reply
* Re: [PATCH 06/10] AXFS: axfs_super.c
From: Carsten Otte @ 2008-08-21 8:55 UTC (permalink / raw)
To: jaredeh
Cc: Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, nickpiggin
In-Reply-To: <48AD0101.4020505@gmail.com>
Jared Hulbert wrote:
> diff --git a/fs/axfs/axfs_super.c b/fs/axfs/axfs_super.c
> new file mode 100644
> index 0000000..5efab38
> --- /dev/null
> +++ b/fs/axfs/axfs_super.c
> +/***************** functions in other axfs files ***************************/
> +int axfs_get_sb_bdev(struct file_system_type *, int, const char *,
> + struct axfs_super *, struct vfsmount *, int *);
> +void axfs_kill_block_super(struct super_block *);
> +int axfs_copy_block(struct super_block *, void *, u64, u64);
> +int axfs_is_dev_bdev(char *);
> +int axfs_map_mtd(struct super_block *);
> +void axfs_unmap_mtd(struct super_block *);
> +int axfs_copy_mtd(struct super_block *, void *, u64, u64);
> +int axfs_get_sb_mtd(struct file_system_type *, int, const char *,
> + struct axfs_super *, struct vfsmount *, int *);
> +int axfs_is_dev_mtd(char *, int *);
> +void axfs_kill_mtd_super(struct super_block *);
> +struct inode *axfs_create_vfs_inode(struct super_block *, int);
> +int axfs_get_uml_address(char *, unsigned long *, unsigned long *);
> +int axfs_init_profiling(struct axfs_super *);
> +int axfs_shutdown_profiling(struct axfs_super *);
> +void axfs_profiling_add(struct axfs_super *, unsigned long, unsigned int);
> +struct inode *axfs_create_vfs_inode(struct super_block *, int);
Please put these definitions into a header file.
^ permalink raw reply
* Re: [PATCH 10/10] AXFS: axfs_uncompress.c
From: Sven Wegener @ 2008-08-21 9:01 UTC (permalink / raw)
To: Jared Hulbert
Cc: Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, cotte, nickpiggin
In-Reply-To: <48AD0126.1050609@gmail.com>
On Wed, 20 Aug 2008, Jared Hulbert wrote:
> Handles the decompression for axfs, modeled after fs/cramfs/uncompress.c.
>
> Signed-off-by: Jared Hulbert <jaredeh@gmail.com>
> ---
> diff --git a/fs/axfs/axfs_uncompress.c b/fs/axfs/axfs_uncompress.c
> new file mode 100644
> index 0000000..b7a2060
> --- /dev/null
> +++ b/fs/axfs/axfs_uncompress.c
> @@ -0,0 +1,97 @@
> +/*
> + * Advanced XIP File System for Linux - AXFS
> + * Readonly, compressed, and XIP filesystem for Linux systems big and small
> + *
> + * Modified in 2006 by Eric Anderson
> + * from the cramfs sources fs/cramfs/uncompress.c
> + *
> + * (C) Copyright 1999 Linus Torvalds
> + *
> + * axfs_uncompress.c -
> + * axfs interfaces to the uncompression library. There's really just
> + * three entrypoints:
> + *
> + * - axfs_uncompress_init() - called to initialize the thing.
> + * - axfs_uncompress_exit() - tell me when you're done
> + * - axfs_uncompress_block() - uncompress a block.
> + *
> + * NOTE NOTE NOTE! The uncompression is entirely single-threaded. We
> + * only have one stream, and we'll initialize it only once even if it
> + * then is used by multiple filesystems.
> + *
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/vmalloc.h>
> +#include <linux/zlib.h>
> +#include <linux/mutex.h>
> +
> +static z_stream stream;
> +static int initialized;
> +static struct mutex axfs_uncmp_mutex;
Use DEFINE_MUTEX and drop the mutex_init() down in the init function.
> +
> +int axfs_uncompress_block(void *dst, int dstlen, void *src, int srclen)
> +{
> + int err;
> + int out;
> +
> + mutex_lock(&axfs_uncmp_mutex);
> +
> + stream.next_in = src;
> + stream.avail_in = srclen;
> +
> + stream.next_out = dst;
> + stream.avail_out = dstlen;
> +
> + err = zlib_inflateReset(&stream);
> + if (err != Z_OK) {
> + printk(KERN_ERR "zlib_inflateReset error %d\n", err);
> + zlib_inflateEnd(&stream);
> + zlib_inflateInit(&stream);
> + }
> +
> + err = zlib_inflate(&stream, Z_FINISH);
> + if (err != Z_STREAM_END)
> + goto err;
> +
> + out = stream.total_out;
> +
> + mutex_unlock(&axfs_uncmp_mutex);
> +
> + return out;
> +
> +err:
> +
> + mutex_unlock(&axfs_uncmp_mutex);
> +
> + printk(KERN_ERR "Error %d while decompressing!\n", err);
> + printk(KERN_ERR "%p(%d)->%p(%d)\n", src, srclen, dst, dstlen);
> + return 0;
> +}
> +
> +int axfs_uncompress_init(void)
> +{
> + if (!initialized++) {
> +
> + mutex_init(&axfs_uncmp_mutex);
> +
> + stream.workspace = vmalloc(zlib_inflate_workspacesize());
> + if (!stream.workspace) {
> + initialized = 0;
> + return -ENOMEM;
> + }
> + stream.next_in = NULL;
> + stream.avail_in = 0;
> + zlib_inflateInit(&stream);
> + }
> + return 0;
> +}
> +
> +int axfs_uncompress_exit(void)
> +{
> + if (!--initialized) {
> + zlib_inflateEnd(&stream);
> + vfree(stream.workspace);
> + }
> + return 0;
> +}
axfs_uncompress_init() and axfs_uncompress_exit() are only called during
init and exit of the module, no need for the initialized variable and the
functions can be annotated with __init and __exit.
^ permalink raw reply
* Re: [PATCH 06/10] AXFS: axfs_super.c
From: Sven Wegener @ 2008-08-21 9:04 UTC (permalink / raw)
To: Jared Hulbert
Cc: Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, cotte, nickpiggin
In-Reply-To: <48AD0101.4020505@gmail.com>
On Wed, 20 Aug 2008, Jared Hulbert wrote:
> The many different devices AXFS can mount to and the various
> dual device mounting schemes are supported here.
>
> Signed-off-by: Jared Hulbert <jaredeh@gmail.com>
> ---
> diff --git a/fs/axfs/axfs_super.c b/fs/axfs/axfs_super.c
> new file mode 100644
> index 0000000..5efab38
> --- /dev/null
> +++ b/fs/axfs/axfs_super.c
> @@ -0,0 +1,864 @@
> +/*
> + * Advanced XIP File System for Linux - AXFS
> + * Readonly, compressed, and XIP filesystem for Linux systems big and small
> + *
> + * Copyright(c) 2008 Numonyx
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * Authors:
> + * Eric Anderson
> + * Jared Hulbert <jaredeh@gmail.com>
> + * Sujaya Srinivasan
> + * Justin Treon
> + *
> + * More info and current contacts at http://axfs.sourceforge.net
> + *
> + * axfs_super.c -
> + * Contains the core code used to mount the fs.
> + *
> + */
> +
> +#include <linux/axfs.h>
> +#include <linux/fs.h>
> +#include <linux/vmalloc.h>
> +#include <linux/parser.h>
> +#include <linux/statfs.h>
> +#include <linux/module.h>
> +#include <linux/mount.h>
> +#include <linux/mtd/mtd.h>
> +
> +/******************** Function Declarations ****************************/
> +static struct super_operations axfs_sops;
> +static struct axfs_super *axfs_get_sbi(void);
> +static void axfs_put_sbi(struct axfs_super *);
If you reorder the code slightly you can avoid these static forward
declarations.
> +/***************** functions in other axfs files ***************************/
> +int axfs_get_sb_bdev(struct file_system_type *, int, const char *,
> + struct axfs_super *, struct vfsmount *, int *);
> +void axfs_kill_block_super(struct super_block *);
> +int axfs_copy_block(struct super_block *, void *, u64, u64);
> +int axfs_is_dev_bdev(char *);
> +int axfs_map_mtd(struct super_block *);
> +void axfs_unmap_mtd(struct super_block *);
> +int axfs_copy_mtd(struct super_block *, void *, u64, u64);
> +int axfs_get_sb_mtd(struct file_system_type *, int, const char *,
> + struct axfs_super *, struct vfsmount *, int *);
> +int axfs_is_dev_mtd(char *, int *);
> +void axfs_kill_mtd_super(struct super_block *);
> +struct inode *axfs_create_vfs_inode(struct super_block *, int);
> +int axfs_get_uml_address(char *, unsigned long *, unsigned long *);
> +int axfs_init_profiling(struct axfs_super *);
> +int axfs_shutdown_profiling(struct axfs_super *);
> +void axfs_profiling_add(struct axfs_super *, unsigned long, unsigned int);
> +struct inode *axfs_create_vfs_inode(struct super_block *, int);
> +/******************************************************************************/
These should probably be in axfs.h.
> +static int axfs_fill_region_descriptors(struct super_block *sb,
> + struct axfs_super_onmedia *sbo)
> +{
> + struct axfs_super *sbi = AXFS_SB(sb);
> + struct axfs_region_desc_onmedia *out;
> +
> + out = kmalloc(sizeof(*out), GFP_KERNEL);
> + if (!out)
> + return -ENOMEM;
> + memset(out, 0, sizeof(*out));
kzalloc()
> +
> + axfs_fill_region_desc(sb, out, sbo->strings, &sbi->strings);
> + axfs_fill_region_desc(sb, out, sbo->xip, &sbi->xip);
> + axfs_fill_region_desc(sb, out, sbo->compressed, &sbi->compressed);
> + axfs_fill_region_desc(sb, out, sbo->byte_aligned, &sbi->byte_aligned);
> + axfs_fill_region_desc(sb, out, sbo->node_type, &sbi->node_type);
> + axfs_fill_region_desc(sb, out, sbo->node_index, &sbi->node_index);
> + axfs_fill_region_desc(sb, out, sbo->cnode_offset, &sbi->cnode_offset);
> + axfs_fill_region_desc(sb, out, sbo->cnode_index, &sbi->cnode_index);
> + axfs_fill_region_desc(sb, out, sbo->banode_offset, &sbi->banode_offset);
> + axfs_fill_region_desc(sb, out, sbo->cblock_offset, &sbi->cblock_offset);
> + axfs_fill_region_desc(sb, out, sbo->inode_file_size,
> + &sbi->inode_file_size);
> + axfs_fill_region_desc(sb, out, sbo->inode_name_offset,
> + &sbi->inode_name_offset);
> + axfs_fill_region_desc(sb, out, sbo->inode_num_entries,
> + &sbi->inode_num_entries);
> + axfs_fill_region_desc(sb, out, sbo->inode_mode_index,
> + &sbi->inode_mode_index);
> + axfs_fill_region_desc(sb, out, sbo->inode_array_index,
> + &sbi->inode_array_index);
> + axfs_fill_region_desc(sb, out, sbo->modes, &sbi->modes);
> + axfs_fill_region_desc(sb, out, sbo->uids, &sbi->uids);
> + axfs_fill_region_desc(sb, out, sbo->gids, &sbi->gids);
> +
> + kfree(out);
> +
> + return 0;
> +}
> +int axfs_set_compression_type(struct axfs_super *sbi)
static
And this function actually doesn't set anything, check might be a better
name.
> +{
> + if (sbi->compression_type != ZLIB)
> + return -EINVAL;
> +
> + return 0;
> +}
> +/* Read the last four bytes of the volume and make sure the AXFS magic is
> + present. */
> +int axfs_verify_eofs_magic(struct super_block *sb)
> +{
> + struct axfs_super *sbi = AXFS_SB(sb);
> + u32 buf = 0;
> + int err;
> + u32 fsoffset = sbi->size - sizeof(u32);
> + int len = sizeof(u32);
> +
> + err = axfs_copy_metadata(sb, &buf, fsoffset, len);
sizeof(buf) instead of an additional variable named len.
> +
> + if (err)
> + return -EINVAL;
> +
> + if (be32_to_cpu(buf) != AXFS_MAGIC) {
> + printk(KERN_ERR "READ: 0x%x\n", be32_to_cpu(buf));
> + printk(KERN_ERR "ERROR: Filesystem is incomplete and cannot be "
> + "mounted!\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +int axfs_fill_super(struct super_block *sb, void *data, int silent)
> +{
> + struct axfs_super *sbi_in = (struct axfs_super *)data;
> + struct axfs_super *sbi;
> + struct inode *root;
> + int err;
> +
> + sbi = axfs_get_sbi();
missing check for failure
> + sb->s_fs_info = (void *)sbi;
> + memcpy(sbi, sbi_in, sizeof(*sbi));
> +
> + /* fully populate the incore superblock structures */
> + err = axfs_do_fill_super(sb);
> + if (err)
> + goto out;
> +
> + sb->s_flags |= MS_RDONLY;
> +
> + /* Setup the VFS super block now */
> + sb->s_op = &axfs_sops;
> + root = axfs_create_vfs_inode(sb, 0);
> + if (!root) {
> + err = -EINVAL;
> + goto out;
> + }
> +
> + sb->s_root = d_alloc_root(root);
> + if (!sb->s_root) {
> + iput(root);
> + err = -EINVAL;
> + goto out;
> + }
> +
> + err = axfs_init_profiling(sbi);
> + if (err)
> + goto out;
> +
> + return 0;
> +
> +out:
> + axfs_put_super(sb);
> + return err;
> +}
> +static struct axfs_super *axfs_get_sbi(void)
> +{
> + struct axfs_super *sbi;
> +
> + sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> + if (sbi)
> + return sbi;
> +
> + axfs_put_sbi(sbi);
Looks useless, sbi is NULL here.
> + return ERR_PTR(-ENOMEM);
> +}
> +static int __init init_axfs_fs(void)
> +{
> + axfs_uncompress_init();
> + return register_filesystem(&axfs_fs_type);
> +}
> +
> +static void __exit exit_axfs_fs(void)
> +{
> + axfs_uncompress_exit();
> + unregister_filesystem(&axfs_fs_type);
Order looks wrong compared to init_axfs_fs()
> +}
> +
> +module_init(init_axfs_fs);
> +module_exit(exit_axfs_fs);
> +MODULE_LICENSE("GPL");
^ permalink raw reply
* Re: [PATCH 05/10] AXFS: axfs_profiling.c
From: Carsten Otte @ 2008-08-21 10:20 UTC (permalink / raw)
To: David Woodhouse
Cc: carsteno, jaredeh, Linux-kernel, linux-embedded, linux-mtd,
Jörn Engel, tim.bird, nickpiggin
In-Reply-To: <1219308560.2988.247.camel@pmac.infradead.org>
David Woodhouse wrote:
> On Thu, 2008-08-21 at 10:44 +0200, Carsten Otte wrote:
>> Jared Hulbert wrote:
>>> Profiling is a fault instrumentation and /proc formating system.
>>> This is used to get an accurate picture of what the pages are actually used.
>>> Using this info the image can be optimized for XIP
>
>> Exporting profiling data for a file system in another file system
>> (/proc) seems not very straigtforward to me. I think it is worth
>> considering to export this information via the same mount point.
>
> I would have said sysfs, rather than 'the same mount point'.
Well, filesystems are usually not represented in the device model.
It'd be possible to add a system device for it, but that does'nt feel
like the right solution to me.
^ permalink raw reply
* Re: [PATCH 00/10] AXFS: Advanced XIP filesystem
From: Carsten Otte @ 2008-08-21 10:25 UTC (permalink / raw)
To: jaredeh
Cc: Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, nickpiggin
In-Reply-To: <48AD00C4.6060302@gmail.com>
Jared Hulbert wrote:
> I'd like to get a first round of review on my AXFS filesystem.
I like the general approach of it. It's much more flexible than the
ext2 extension I've done, and the possibility to select XIP vs.
compression per page is really really neat. I can imagine that people
will prefer this over the ext2 implementation on s390. It is unclear
to me how the "secondary block device" thing is supposed to work.
Could you elaborate a bit on that?
^ permalink raw reply
* Re: [PATCH 00/10] AXFS: Advanced XIP filesystem
From: Nick Piggin @ 2008-08-21 10:43 UTC (permalink / raw)
To: carsteno
Cc: jaredeh, Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird
In-Reply-To: <48AD42AE.4030107@de.ibm.com>
On Thursday 21 August 2008 20:25, Carsten Otte wrote:
> Jared Hulbert wrote:
> > I'd like to get a first round of review on my AXFS filesystem.
>
> I like the general approach of it. It's much more flexible than the
> ext2 extension I've done, and the possibility to select XIP vs.
> compression per page is really really neat. I can imagine that people
> will prefer this over the ext2 implementation on s390. It is unclear
> to me how the "secondary block device" thing is supposed to work.
> Could you elaborate a bit on that?
Agreed. I haven't had a good look through it yet, but at a glance it
looks pretty neat. The VM side of things looks pretty reasonable
(I fear XIP faulting might have another race or two, but that's a
core mm issue rather than filesystem specific).
^ permalink raw reply
* Re: [PATCH 00/10] AXFS: Advanced XIP filesystem
From: Jamie Lokier @ 2008-08-21 11:07 UTC (permalink / raw)
To: Jared Hulbert
Cc: Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, cotte, nickpiggin
In-Reply-To: <48AD00C4.6060302@gmail.com>
Jared Hulbert wrote:
> The biggest improvement is in the way AXFS allows for each page to be XIP or
> not. First, a user collects information about which pages are accessed on a
> compressed image for each mmap()ed region from /proc/axfs/volume0. That
> 'profile' is used as an input to the image builder. The resulting image has
> only the relevant pages uncompressed and XIP. The result is smaller memory
> sizes and faster launches.
Sounds great, really nice idea.
How does it fare with no MMU? Can the profiler and image builder lay
out the XIP pages in such a way that no-MMU mmaps can map those regions?
No complaint if not, it would be a nice bonus though.
-- Jamie
^ permalink raw reply
* Re: [PATCH 02/10] AXFS: Kconfig and Makefiles
From: Arnd Bergmann @ 2008-08-21 11:24 UTC (permalink / raw)
To: jaredeh
Cc: Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, cotte, nickpiggin
In-Reply-To: <48AD00DF.9080207@gmail.com>
On Thursday 21 August 2008, Jared Hulbert wrote:
> The Kconfig edits and Makefiles required for AXFS.
>
> Signed-off-by: Jared Hulbert <jaredeh@gmail.com>
If you split out this patch separate from the files, please make it the
*last* patch so that you cannot get build errors during a later git-bisect
through the middle of your series.
Arnd <><
^ permalink raw reply
* Re: [PATCH 06/10] AXFS: axfs_super.c
From: Sven Wegener @ 2008-08-21 11:27 UTC (permalink / raw)
To: Jared Hulbert
Cc: Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, cotte, nickpiggin
In-Reply-To: <48AD0101.4020505@gmail.com>
On Wed, 20 Aug 2008, Jared Hulbert wrote:
> The many different devices AXFS can mount to and the various
> dual device mounting schemes are supported here.
>
> Signed-off-by: Jared Hulbert <jaredeh@gmail.com>
> ---
> +static int __init init_axfs_fs(void)
> +{
> + axfs_uncompress_init();
one more thing, axfs_uncompress_init() can fail
> + return register_filesystem(&axfs_fs_type);
> +}
> +
> +static void __exit exit_axfs_fs(void)
> +{
> + axfs_uncompress_exit();
> + unregister_filesystem(&axfs_fs_type);
> +}
> +
> +module_init(init_axfs_fs);
> +module_exit(exit_axfs_fs);
> +MODULE_LICENSE("GPL");
^ permalink raw reply
* Re: [PATCH 03/10] AXFS: axfs.h
From: Arnd Bergmann @ 2008-08-21 11:31 UTC (permalink / raw)
To: jaredeh
Cc: Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, cotte, nickpiggin
In-Reply-To: <48AD00E6.2070505@gmail.com>
On Thursday 21 August 2008, Jared Hulbert wrote:
>
> +#ifndef __AXFS_H
> +#define __AXFS_H
> +
> +
> +#ifdef __KERNEL__
> +#include <linux/rwsem.h>
> +#endif
> +#include <linux/errno.h>
> +#include <linux/time.h>
> +
> +#define AXFS_MAGIC 0x48A0E4CD /* some random number */
> +#define AXFS_SIGNATURE "Advanced XIP FS"
> +#define AXFS_MAXPATHLEN 255
> +
> +/* Uncompression interfaces to the underlying zlib */
> +int axfs_uncompress_block(void *, int, void *, int);
> +int axfs_uncompress_init(void);
> +int axfs_uncompress_exit(void);
You already have both __KERNEL__ and user space parts in your header.
Function declarations clearly don't belong in the user visible parts.
I suggest you only leave on-disk data structure definitions and user
interfaces in the user-visible include/linux/axfs.h file, and move
all internal interfaces to fs/axfs/axfs.h.
> +/*
> + * on media struct describing a data region
> + */
> +struct axfs_region_desc_onmedia {
> + u64 fsoffset;
> + u64 size;
> + u64 compressed_size;
> + u64 max_index;
> + u8 table_byte_depth;
> + u8 incore;
> +};
For on-disk data structures, I would use __be64 or __le64 types,
preferrably the opposite of your native CPU byte order on the system
you are developing most code on, to make sure that you get endianess
right.
> +#define AXFS_PAGE_SIZE 4096
What happens on systems where AXFS_PAGE_SIZE != PAGE_SIZE?
> +#ifndef TRUE
> +#define TRUE 1
> +#endif
> +#ifndef FALSE
> +#define FALSE 0
> +#endif
> +
These are already defined in <linux/kernel.h>, so please use the
existing definitions.
Arnd <><
^ permalink raw reply
* Re: [PATCH 04/10] AXFS: axfs_inode.c
From: Arnd Bergmann @ 2008-08-21 11:35 UTC (permalink / raw)
To: jaredeh
Cc: Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, cotte, nickpiggin
In-Reply-To: <48AD00F0.5030403@gmail.com>
On Thursday 21 August 2008, Jared Hulbert wrote:
> +/***************** functions in other axfs files ******************************/
> +int axfs_get_sb(struct file_system_type *, int, const char *, void *,
> + struct vfsmount *);
> +void axfs_kill_super(struct super_block *);
> +void axfs_profiling_add(struct axfs_super *, unsigned long, unsigned int);
> +int axfs_copy_mtd(struct super_block *, void *, u64, u64);
> +int axfs_copy_block(struct super_block *, void *, u64, u64);
*Never* put extern declarations into a .c file, that's what headers are for.
If you ever change the definition, the compiler doesn't get a chance to
warn you otherwise.
> +/******************************************************************************/
> +static int axfs_readdir(struct file *, void *, filldir_t);
> +static int axfs_mmap(struct file *, struct vm_area_struct *);
> +static ssize_t axfs_file_read(struct file *, char __user *, size_t, loff_t *);
> +static int axfs_readpage(struct file *, struct page *);
> +static int axfs_fault(struct vm_area_struct *, struct vm_fault *);
> +static struct dentry *axfs_lookup(struct inode *, struct dentry *,
> + struct nameidata *);
> +static int axfs_get_xip_mem(struct address_space *, pgoff_t, int, void **,
> + unsigned long *);
For style reasons, also please don't put static forward declarations anywhere,
but define the functions in the right order so you don't need them.
Arnd <><
^ permalink raw reply
* Re: [PATCH 05/10] AXFS: axfs_profiling.c
From: Arnd Bergmann @ 2008-08-21 11:39 UTC (permalink / raw)
To: David Woodhouse
Cc: carsteno, jaredeh, Linux-kernel, linux-embedded, linux-mtd,
Jörn Engel, tim.bird, nickpiggin
In-Reply-To: <1219308560.2988.247.camel@pmac.infradead.org>
On Thursday 21 August 2008, David Woodhouse wrote:
> On Thu, 2008-08-21 at 10:44 +0200, Carsten Otte wrote:
> >
> > Exporting profiling data for a file system in another file system
> > (/proc) seems not very straigtforward to me. I think it is worth
> > considering to export this information via the same mount point.
>
> I would have said sysfs, rather than 'the same mount point'.
>
Let me throw in debugfs as my preferred option. sysfs is for stable
interfaces, while profiling generally fits into the debugging category.
Arnd <><
^ permalink raw reply
* Re: [PATCH 10/10] AXFS: axfs_uncompress.c
From: Artem Bityutskiy @ 2008-08-21 11:40 UTC (permalink / raw)
To: jaredeh
Cc: Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, cotte, nickpiggin
In-Reply-To: <48AD0126.1050609@gmail.com>
On Wed, 2008-08-20 at 22:46 -0700, Jared Hulbert wrote:
> + err = zlib_inflateReset(&stream);
> + if (err != Z_OK) {
> + printk(KERN_ERR "zlib_inflateReset error %d\n", err);
> + zlib_inflateEnd(&stream);
> + zlib_inflateInit(&stream);
> + }
Jared,
just FYI, are you aware that LZO which is also present in the kernel is
much faster on decompress than zlib, while its compression is only
slightly worse?
I do not remember the digits, but last time I tested UBIFS, LZO
decompression was about 3 times faster than zlib on OMAP3. This depends
on architecture, etc of course. This may matter a lot if one is fighting
for faster system boot-up. So you might consider supporting LZO as well.
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)
^ permalink raw reply
* Re: [PATCH 04/10] AXFS: axfs_inode.c
From: Arnd Bergmann @ 2008-08-21 12:17 UTC (permalink / raw)
To: jaredeh
Cc: Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, cotte, nickpiggin
In-Reply-To: <48AD00F0.5030403@gmail.com>
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 <><
^ permalink raw reply
* Re: [PATCH 03/10] AXFS: axfs.h
From: Arnd Bergmann @ 2008-08-21 12:24 UTC (permalink / raw)
To: jaredeh
Cc: Linux-kernel, linux-embedded, linux-mtd, Jörn Engel,
tim.bird, cotte, nickpiggin
In-Reply-To: <48AD00E6.2070505@gmail.com>
On Thursday 21 August 2008, Jared Hulbert wrote:
> +static inline u64 axfs_bytetable_stitch(u8 depth, u8 *table, u64 index)
> +{
> + u64 i;
> + u64 output = 0;
> + u64 byte = 0;
> + u64 j;
> + u64 bits;
> +
> + for (i = 0; i < depth; i++) {
> + j = index * depth + i;
> + bits = 8 * (depth - i - 1);
> + byte = table[j];
> + output += byte << bits;
> + }
> + return output;
> +}
> +
> +#define AXFS_GET_BYTETABLE_VAL(desc,index) \
> + axfs_bytetable_stitch(((struct axfs_region_desc)(desc)).table_byte_depth,\
> + (u8 *)((struct axfs_region_desc)(desc)).virt_addr, index)
> +
This bytetable stuff looks overly complicated, both the data structure and
the access method. It seems like you are implementing your own custom Huffman
compression with this.
Is the reasonn for the bytetable just to pack numbers efficiently, or do you
have a different intention?
Did you see a significant size benefit over simply storing all metadata as
uncompressed data structures like in cramfs?
Have you considered storing simple dentry/inode data in node_type==Compressed
nodes?
Arnd <><
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox