[PATCH] nilfs2: shorten freeze period due to GC in write operation

[PATCH] nilfs2: shorten freeze period due to GC in write operation

[Jiro SEKIBA]

Hi,

This is a patch to shorten freeze period.

When GC is runnning, GC moves live block to difference segments.
Copying live blocks into memory 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.

I ran sysbench fileio test against nilfs partition.
I copied some DVD/CD images and created snapshot to create live blocks
before starting the benchmark.

Followings are summary of rc8 and rc8 w/ the patch of per-request statistics,
which is min/max and avg.  I ran each test three times and bellow is
average of those numers.

According to this benchmark result, average time is slightly degrated.
However, worstcase (max) result is significantly improved.
This can address a few seconds write freeze.

- random write per-request performance of rc8
 min   0.843ms
 max 680.406ms
 avg   3.050ms
- random write per-request performance of rc8 w/ this patch
 min   0.833ms ->  98.80%
 max 354.606ms ->  52.10%
 avg   3.226ms -> 105.70%

- sequential write per-request performance of rc8
 min   0.736ms
 max 774.343ms 
 avg   2.883ms 
- sequential write per-request performance of rc8 w/ this patch
 min   0.740ms -> 100.50%
 max  563.780ms->  72.80%
 avg   3.113ms -> 107.90%

-----8<-----8<-----nilfs_cleanerd.conf-----8<-----8<-----
protection_period       150
selection_policy        timestamp       # timestamp in ascend order
nsegments_per_clean     2
cleaning_interval       2
retry_interval          60
use_mmap
log_priority            info
-----8<-----8<-----nilfs_cleanerd.conf-----8<-----8<-----


Signed-off-by: Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>
---
 fs/nilfs2/ioctl.c     |   18 ++++++++++++------
 fs/nilfs2/the_nilfs.h |    2 ++
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
index 6ea5f87..2394353 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) {
 		/*
@@ -521,6 +515,10 @@ static int nilfs_ioctl_clean_segmentsistruct inode5 einode, struct file *filp,
 
 	nilfs = NILFS_SB(inode->i_sb)->s_nilfs;
 
+	if (nilfs_gc_running(nilfs))
+		return -EBUSY;
+	set_nilfs_gc_running(nilfs);
+
 	for (n = 0; n < 4; n++) {
 		ret = -EINVAL;
 		if (argv[n].v_size != argsz[n])
@@ -548,12 +546,20 @@ 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);
+		goto out_free;
+	}
+
 	ret = nilfs_clean_segments(inode->i_sb, argv, kbufs);
 
  out_free:
 	while (--n >= 0)
 		vfree(kbufs[n]);
 	kfree(kbufs[4]);
+	clear_nilfs_gc_running(nilfs);
 	return ret;
 }
 
diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
index 1b9caaf..97ee569 100644
--- a/fs/nilfs2/the_nilfs.h
+++ b/fs/nilfs2/the_nilfs.h
@@ -37,6 +37,7 @@ enum {
 	THE_NILFS_LOADED,       /* Roll-back/roll-forward has done and
 				   the latest checkpoint was loaded */
 	THE_NILFS_DISCONTINUED,	/* 'next' pointer chain has broken */
+	THE_NILFS_GC_RUNNING,	/* gc process is running */
 };
 
 /**
@@ -197,6 +198,7 @@ static inline int nilfs_##name(struct the_nilfs *nilfs)			\
 THE_NILFS_FNS(INIT, init)
 THE_NILFS_FNS(LOADED, loaded)
 THE_NILFS_FNS(DISCONTINUED, discontinued)
+THE_NILFS_FNS(GC_RUNNING, gc_running)
 
 /* Minimum interval of periodical update of superblocks (in seconds) */
 #define NILFS_SB_FREQ		10
-- 
1.5.6.5

Re: [PATCH] nilfs2: shorten freeze period due to GC in write operation

[Ryusuke Konishi]

Hi Sekiba-san,

On Sat, 29 Aug 2009 17:54:30 +0900, Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org> wrote:
> Hi,
> 
> This is a patch to shorten freeze period.
<snip> 
> diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
> index 6ea5f87..2394353 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) {
>  		/*
> @@ -521,6 +515,10 @@ static int nilfs_ioctl_clean_segmentsistruct inode5 einode, struct file *filp,
>  
>  	nilfs = NILFS_SB(inode->i_sb)->s_nilfs;
>  
> +	if (nilfs_gc_running(nilfs))
> +		return -EBUSY;
> +	set_nilfs_gc_running(nilfs);
> +

Well, I have two comments here on this patch:

This check and the error return should be done before memory
allocation of kbufs[4] to avoid memory leak.

And, logically these bit operations must be atomic by use of
test_and_set_bit().  So, it should be like:

	if (test_and_set_bit(THE_NILFS_GC_RUNNING, &nilfs->ns_flags))
		return -EBUSY;

I think we don't have to expand THE_NILFS_FNS macro for this.

Could you revise the patch once more?

Thank you,
Ryusuke Konishi

>  	for (n = 0; n < 4; n++) {
>  		ret = -EINVAL;
>  		if (argv[n].v_size != argsz[n])
> @@ -548,12 +546,20 @@ 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);
> +		goto out_free;
> +	}
> +
>  	ret = nilfs_clean_segments(inode->i_sb, argv, kbufs);
>  
>   out_free:
>  	while (--n >= 0)
>  		vfree(kbufs[n]);
>  	kfree(kbufs[4]);
> +	clear_nilfs_gc_running(nilfs);
>  	return ret;
>  }
>  
> diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
> index 1b9caaf..97ee569 100644
> --- a/fs/nilfs2/the_nilfs.h
> +++ b/fs/nilfs2/the_nilfs.h
> @@ -37,6 +37,7 @@ enum {
>  	THE_NILFS_LOADED,       /* Roll-back/roll-forward has done and
>  				   the latest checkpoint was loaded */
>  	THE_NILFS_DISCONTINUED,	/* 'next' pointer chain has broken */
> +	THE_NILFS_GC_RUNNING,	/* gc process is running */
>  };
>  
>  /**
> @@ -197,6 +198,7 @@ static inline int nilfs_##name(struct the_nilfs *nilfs)			\
>  THE_NILFS_FNS(INIT, init)
>  THE_NILFS_FNS(LOADED, loaded)
>  THE_NILFS_FNS(DISCONTINUED, discontinued)
> +THE_NILFS_FNS(GC_RUNNING, gc_running)
>  
>  /* Minimum interval of periodical update of superblocks (in seconds) */
>  #define NILFS_SB_FREQ		10
> -- 
> 1.5.6.5
> 
> _______________________________________________
> users mailing list
> users-JrjvKiOkagjYtjvyW6yDsg@public.gmane.org
> https://www.nilfs.org/mailman/listinfo/users

Re: [PATCH] nilfs2: shorten freeze period due to GC in write operation

[Jiro SEKIBA]

Hi,

At Mon, 31 Aug 2009 16:37:29 +0900 (JST),
Ryusuke Konishi wrote:
> 
> Hi Sekiba-san,
> 
> On Sat, 29 Aug 2009 17:54:30 +0900, Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org> wrote:
> > Hi,
> > 
> > This is a patch to shorten freeze period.
> <snip> 
> > diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
> > index 6ea5f87..2394353 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) {
> >  		/*
> > @@ -521,6 +515,10 @@ static int nilfs_ioctl_clean_segmentsistruct inode5 einode, struct file *filp,
> >  
> >  	nilfs = NILFS_SB(inode->i_sb)->s_nilfs;
> >  
> > +	if (nilfs_gc_running(nilfs))
> > +		return -EBUSY;
> > +	set_nilfs_gc_running(nilfs);
> > +
> 
> Well, I have two comments here on this patch:
> 
> This check and the error return should be done before memory
> allocation of kbufs[4] to avoid memory leak.

grr, that's right.  Sorry for the careless miss.
I was trying to modify the logic as little as possible.

> And, logically these bit operations must be atomic by use of
> test_and_set_bit().  So, it should be like:
> 
> 	if (test_and_set_bit(THE_NILFS_GC_RUNNING, &nilfs->ns_flags))
> 		return -EBUSY;

Ah, that's the one.  Thank you.

> I think we don't have to expand THE_NILFS_FNS macro for this.
>
> Could you revise the patch once more?

 I'll revise it.  Thank you for the comments.

> Thank you,
> Ryusuke Konishi
> 
> >  	for (n = 0; n < 4; n++) {
> >  		ret = -EINVAL;
> >  		if (argv[n].v_size != argsz[n])
> > @@ -548,12 +546,20 @@ 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);
> > +		goto out_free;
> > +	}
> > +
> >  	ret = nilfs_clean_segments(inode->i_sb, argv, kbufs);
> >  
> >   out_free:
> >  	while (--n >= 0)
> >  		vfree(kbufs[n]);
> >  	kfree(kbufs[4]);
> > +	clear_nilfs_gc_running(nilfs);
> >  	return ret;
> >  }
> >  
> > diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
> > index 1b9caaf..97ee569 100644
> > --- a/fs/nilfs2/the_nilfs.h
> > +++ b/fs/nilfs2/the_nilfs.h
> > @@ -37,6 +37,7 @@ enum {
> >  	THE_NILFS_LOADED,       /* Roll-back/roll-forward has done and
> >  				   the latest checkpoint was loaded */
> >  	THE_NILFS_DISCONTINUED,	/* 'next' pointer chain has broken */
> > +	THE_NILFS_GC_RUNNING,	/* gc process is running */
> >  };
> >  
> >  /**
> > @@ -197,6 +198,7 @@ static inline int nilfs_##name(struct the_nilfs *nilfs)			\
> >  THE_NILFS_FNS(INIT, init)
> >  THE_NILFS_FNS(LOADED, loaded)
> >  THE_NILFS_FNS(DISCONTINUED, discontinued)
> > +THE_NILFS_FNS(GC_RUNNING, gc_running)
> >  
> >  /* Minimum interval of periodical update of superblocks (in seconds) */
> >  #define NILFS_SB_FREQ		10
> > -- 
> > 1.5.6.5
> > 
> > _______________________________________________
> > users mailing list
> > users-JrjvKiOkagjYtjvyW6yDsg@public.gmane.org
> > https://www.nilfs.org/mailman/listinfo/users
> _______________________________________________
> users mailing list
> users-JrjvKiOkagjYtjvyW6yDsg@public.gmane.org
> https://www.nilfs.org/mailman/listinfo/users
> 
> 
> 


-- 
Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>

Re: [PATCH] nilfs2: shorten freeze period due to GC in write operation

[Ryusuke Konishi]

On Mon, 31 Aug 2009 16:37:29 +0900 (JST), Ryusuke Konishi wrote:
> On Sat, 29 Aug 2009 17:54:30 +0900, Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org> wrote:
> >  	nilfs = NILFS_SB(inode->i_sb)->s_nilfs;
> >  
> > +	if (nilfs_gc_running(nilfs))
> > +		return -EBUSY;
> > +	set_nilfs_gc_running(nilfs);
> > +
> 
> Well, I have two comments here on this patch:
> 
> This check and the error return should be done before memory
> allocation of kbufs[4] to avoid memory leak.
> 
> And, logically these bit operations must be atomic by use of
> test_and_set_bit().  So, it should be like:
> 
> 	if (test_and_set_bit(THE_NILFS_GC_RUNNING, &nilfs->ns_flags))
> 		return -EBUSY;
> 
> I think we don't have to expand THE_NILFS_FNS macro for this.

I mean we don't have to add the following template:

static inline void test_and_set_nilfs_##name(struct the_nilfs *nilfs)   \
{                                                                       \
        return test_and_set_bit(THE_NILFS_##bit, &(nilfs)->ns_flags);   \
}

But, it's OK if you prefer this.

Just in case, the use of "THE_NILFS_FNS(GC_RUNNING, gc_running)" looks
proper; you don't have to change clear_nilfs_gc_running() to
"clear_bit(THE_NILFS_GC_RUNNING, &nilfs->ns_flags)".

Thanks,
Ryusuke Konishi