Jared Hulbert wrote:
quoted text
> +static void axfs_free_region(struct axfs_super *sbi,
> +			     struct axfs_region_desc *region)
> +{
> +	if (!region)
> +		return;
> +
> +	if (AXFS_IS_REGION_XIP(sbi, region))
> +		return;
> +
> +	if (region->virt_addr)
> +		vfree(region->virt_addr);
> +}
> +

No need to do

	if(xxx)
		vfree(xxx)

vfree/kfree can cope with NULL pointers.

quoted text
> +static void axfs_put_sbi(struct axfs_super *sbi)
> +{

quoted text
> +	axfs_free_region(sbi, &sbi->uids);
> +	axfs_free_region(sbi, &sbi->gids);
> +
> +	if (sbi->second_dev)
> +		kfree(sbi->second_dev);
> +
> +	if (sbi->cblock_buffer[0])
> +		vfree(sbi->cblock_buffer[0]);
> +	if (sbi->cblock_buffer[1])
> +		vfree(sbi->cblock_buffer[1]);
> +

Again, just kfree(xxx)/vfree(xxx)

quoted text
> +
> +static int axfs_copy_mem(struct super_block *sb, void *buf, u64 fsoffset,
> +			 u64 len)
> +{
> +	struct axfs_super *sbi = AXFS_SB(sb);
> +	unsigned long addr;
> +
> +	addr = sbi->virt_start_addr + (unsigned long)fsoffset;
> +	memcpy(buf, (void *)addr, (size_t) len);
> +	return 0;

Always returns 0, consider changing to static void

quoted text
> +}
> +
> +static int axfs_copy_metadata(struct super_block *sb, void *buf, u64 fsoffset,
> +			      u64 len)
> +{
> +	struct axfs_super *sbi = AXFS_SB(sb);
> +	u64 end = fsoffset + len;
> +	u64 a = sbi->mmap_size - fsoffset;
> +	u64 b = end - sbi->mmap_size;
> +	void *bb = (void *)((unsigned long)buf + (unsigned long)a);
> +	int err;
> +
> +	/* Catches case where sbi is not yet fully initialized. */
> +	if ((sbi->magic == 0) && (sbi->virt_start_addr != 0))
> +		return axfs_copy_mem(sb, buf, fsoffset, len);
> +
> +	if (fsoffset < sbi->mmap_size) {
> +		if (end > sbi->mmap_size) {
> +			err = axfs_copy_metadata(sb, buf, fsoffset, a);
> +			if (err)
> +				return err;
> +			err = axfs_copy_metadata(sb, bb, sbi->mmap_size, b);
> +		} else {
> +			if (AXFS_IS_OFFSET_MMAPABLE(sbi, fsoffset)) {
> +				err = axfs_copy_mem(sb, buf, fsoffset, len);
> +			} else if (AXFS_HAS_MTD(sb)) {
> +				err = axfs_copy_mtd(sb, buf, fsoffset, len);
> +			} else if (AXFS_HAS_BDEV(sb)) {
> +				err = axfs_copy_block(sb, buf, fsoffset, len);
> +			} else {
> +				err = -EINVAL;

Consider initialising err to -EINVAL at declaration time, and get rid of 
this else,

quoted text
> +			}
> +		}
> +	} else {
> +		if (AXFS_NODEV(sb)) {
> +			err = axfs_copy_mem(sb, buf, fsoffset, len);
> +		} else if (AXFS_HAS_BDEV(sb)) {
> +			err = axfs_copy_block(sb, buf, fsoffset, len);
> +		} else if (AXFS_HAS_MTD(sb)) {
> +			err = axfs_copy_mtd(sb, buf, fsoffset, len);
> +		} else {
> +			err = -EINVAL;

and this one.

quoted text
> +		}
> +	}
> +	return err;
> +}
> +
> +static int axfs_fill_region_data(struct super_block *sb,
> +				 struct axfs_region_desc *region, int force)
> +{
> +
> +	if (AXFS_IS_REGION_INCORE(region))
> +		goto incore;
> +
> +	if (AXFS_IS_REGION_COMPRESSED(region))
> +		goto incore;
> +
> +	if (AXFS_IS_REGION_XIP(sbi, region)) {
> +		if ((end > sbi->mmap_size) && (force))
> +			goto incore;
> +		addr = sbi->virt_start_addr;
> +		addr += (unsigned long)fsoffset;
> +		region->virt_addr = (void *)addr;
> +		return 0;
> +	}
> +
> +incore:
> +	region->virt_addr = vmalloc(size);
> +	if (!region->virt_addr)
> +		goto out;
> +	vaddr = region->virt_addr;
> +
> +	if (AXFS_IS_REGION_COMPRESSED(region)) {
> +		buff = vmalloc(c_size);
> +		if (!buff)
> +			goto out;
> +		axfs_copy_metadata(sb, buff, fsoffset, c_size);
> +		err = axfs_uncompress_block(vaddr, size, buff, c_size);
> +		if (!err)
> +			goto out;
> +		vfree(buff);
> +	} else {
> +		axfs_copy_metadata(sb, vaddr, fsoffset, size);
> +	}
> +
> +	return 0;


 From this it would appear that if the region data can't be mapped XIP 
(i.e. it is compressed or on a block device), it is cached in its 
entirety in RAM?

This implies for block devices that the entire filesystem metadata has 
to be cached in RAM.  This severely limits the size of AXFS filesystems 
when using block devices, or the else memory usage will be excessive.


quoted text
> +
> +out:
> +	if (buff)
> +		vfree(buff);
> +	if (region->virt_addr)
> +		vfree(region->virt_addr);
> +	return err;
> +}
> +

Just do kfree(xxx)


quoted text
> +
> +static int axfs_get_onmedia_super(struct super_block *sb)
> +{
> +	int err;
> +	struct axfs_super *sbi = AXFS_SB(sb);
> +	struct axfs_super_onmedia *sbo;
> +
> +	sbo = kmalloc(sizeof(*sbo), GFP_KERNEL);
> +	if (!sbo)
> +		return -ENOMEM;
> +
> +	axfs_copy_metadata(sb, (void *)sbo, 0, sizeof(*sbo));
> +
> +	/* Do sanity checks on the superblock */
> +	if (be32_to_cpu(sbo->magic) != AXFS_MAGIC) {
> +		printk(KERN_ERR "axfs: wrong magic\n");
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* verify the signiture is correct */
> +	if (strncmp(sbo->signature, AXFS_SIGNATURE, sizeof(AXFS_SIGNATURE))) {
> +		printk(KERN_ERR "axfs: wrong axfs signature,"
> +		       " read %s, expected %s\n",
> +		       sbo->signature, AXFS_SIGNATURE);
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	sbi->magic = be32_to_cpu(sbo->magic);
> +	sbi->version_major = sbo->version_major;
> +	sbi->version_minor = sbo->version_minor;
> +	sbi->version_sub = sbo->version_sub;
> +	sbi->files = be64_to_cpu(sbo->files);
> +	sbi->size = be64_to_cpu(sbo->size);
> +	sbi->blocks = be64_to_cpu(sbo->blocks);
> +	sbi->mmap_size = be64_to_cpu(sbo->mmap_size);
> +	sbi->cblock_size = be32_to_cpu(sbo->cblock_size);
> +	sbi->timestamp.tv_sec = be64_to_cpu(sbo->timestamp);
> +	sbi->timestamp.tv_nsec = 0;
> +	sbi->compression_type = sbo->compression_type;
> +
> +	err = axfs_set_compression_type(sbi);
> +	if (err)
> +		goto out;
> +
> +	/* If no block or MTD device, adjust mmapable to cover all image */
> +	if (AXFS_NODEV(sb))
> +		sbi->mmap_size = sbi->size;
> +
> +	err = axfs_fill_region_descriptors(sb, sbo);
> +	if (err)
> +		goto out;
> +
> +	err = 0;

Redundant code

quoted text
> +out:
> +	kfree(sbo);
> +	return err;
> +}
> +

Phillip


--
unsubscribe notice
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/