Re: [PATCH] nilfs2: tune write performance

[Ryusuke Konishi <ryusuke-sG5X7nlA6pw@public.gmane.org>]

On Wed,  5 Aug 2009 23:37:03 +0900, Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org> wrote:
> Hi,
> 
> This is a candidate patch to improve write performance.
> 
> When GC is runnning, GC moves live block to difference segments.
> Moving live blocks to different segment is done in a transaction,
> but it is not necessarily to be in the transaction.
> This patch will get the nilfs_ioctl_move_blocks() out from
> transaction lock and put it before the transaction.

Ah, this description may be misleading.
nilfs_ioctl_move_blocks() does not actually move blocks, it just reads
live blocks in the target segments into own caches (gc-cache).

The move operation is consisting of the read operation and a write
operation to new logs, which is carried out in nilfs_clean_segments().
The patch move the former out from the lock section, but it doesn't
for the latter.

I think the confusion is due to name of the function.  Maybe, it
should be named nilfs_ioctl_cache_copy_blocks() or something.  But, I
would like to separate it off as usual.
 
> Here are the bonnie++ bench mark results against rc5.
> I used sata disk so performace difference is not big.
> So I modified nilfs_cleanerd.conf to ensure lock will be long enough.
> 
> I ran "bonnie++ -b" three times in each environment.
> Create clean nilfs2 partition each time before test run.
> 
> Results
> - write performance of rc5
>  * Char|17472.333 K/sec
>  * Block|30748.000 K/sec
>  * Rewrite|18506.000 K/sec
> 
> - write performance of the patched rc5
>  * Char|17961.000 K/sec -> 102.700%
>  * Block|30987.666 K/sec -> 100.700%
>  * Rewrite|18775.666 K/sec -> 101.400%
<snip>
> 
>  fs/nilfs2/ioctl.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> Signed-off-by: Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>
> ---
>  fs/nilfs2/ioctl.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
> index 6ea5f87..83a885c 100644
> --- a/fs/nilfs2/ioctl.c
> +++ b/fs/nilfs2/ioctl.c
> @@ -442,12 +442,6 @@ int nilfs_ioctl_prepare_clean_segments(struct the_nilfs *nilfs,
>  	const char *msg;
>  	int ret;
>  
> -	ret = nilfs_ioctl_move_blocks(nilfs, &argv[0], kbufs[0]);
> -	if (ret < 0) {
> -		msg = "cannot read source blocks";
> -		goto failed;
> -	}
> -
>  	ret = nilfs_ioctl_delete_checkpoints(nilfs, &argv[1], kbufs[1]);
>  	if (ret < 0) {
>  		/*
> @@ -548,6 +542,12 @@ static int nilfs_ioctl_clean_segments(struct inode *inode, struct file *filp,
>  		}
>  	}
>  
> +	ret = nilfs_ioctl_move_blocks(nilfs, &argv[0], kbufs[0]);
> +	if (ret < 0) {
> +		printk(KERN_ERR "NILFS: GC failed during preparation: cannot read source blocks: err=%d\n", ret);

You should call nilfs_remove_all_gcinode() here before escaping from
the normal route.

And, break the line please.  You can do it for C-strings just as
follows:

    printk(KERN_ERR "NILFS: GC failed during preparation: "
           "cannot read source blocks: err=%d\n", ret);

> +		goto out_free;
> +	}
> +
>  	ret = nilfs_clean_segments(inode->i_sb, argv, kbufs);
>  
>   out_free:
> -- 
> 1.5.6.5

For the rest, it looks good to me.

Thanks,
Ryusuke Konishi