[Ocfs2-tools-devel] [PATCH 05/10] libocfs2: read/write now know inline data.

[Joel Becker]

[Patches 1-4 look good]

On Fri, Jul 11, 2008 at 02:20:58PM +0800, Tao Ma wrote:
> +void ocfs2_dinode_new_extent_list(ocfs2_filesys *fs,
> +				  struct ocfs2_dinode *di)
> +{
> +	memset(&di->id2, 0,
> +	       fs->fs_blocksize - offsetof(struct ocfs2_dinode, id2));

	Let's move this memset to a function ocfs2_clear_dinode_id2()
like the kernel.

> +static errcode_t ocfs2_inline_data_write(struct ocfs2_dinode *di, void *buf,
> +					 uint32_t count, uint64_t offset)
> +{
> +	struct ocfs2_inline_data *id;
> +	uint64_t isize;
> +	uint8_t *p;
> +
> +	if (!(di->i_dyn_features & OCFS2_INLINE_DATA_FL))
> +		return OCFS2_ET_INVALID_ARGUMENT;
> +
> +	id = &(di->id2.i_data);
> +	isize = di->i_size;
> +
> +	if (offset > id->id_count ||
> +	    (offset + count) > id->id_count)
> +		return OCFS2_ET_NO_SPACE;

	Probably only need to check offset+count, as it will always be
farther out than offset itself.

> +	written = 1;
> +out:
> +	return written ? written : ret;

	This isn't pretty.  Why do it this way?

> +errcode_t ocfs2_file_write(ocfs2_cached_inode *ci,
> +			   void *buf, uint32_t count,
> +			   uint64_t offset, uint32_t *wrote)
> +{
> +	errcode_t ret;
> +	ocfs2_filesys *fs = ci->ci_fs;
> +	struct ocfs2_dinode *di = ci->ci_inode;
> +
> +	if (ocfs2_support_inline_data(OCFS2_RAW_SB(fs->fs_super))) {
> +		ret = ocfs2_try_to_write_inline_data(ci, buf, count, offset);
> +		if (ret == 1) {
> +			ret = ocfs2_write_cached_inode(fs, ci);

	Why not lift this call to ocfs2_write_cached_inode back up into
ocfs2_try_to_write_inline_data()?  It's coupled with that anyway.

Joel

-- 

"Vote early and vote often." 
        - Al Capone

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127