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

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

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>