Hi, On Fri, 14 Aug 2009 10:55:55 +0900, Jiro SEKIBA wrote: > Hi, > > At Thu, 13 Aug 2009 20:06:06 +0900 (JST), > Ryusuke Konishi wrote: > > > > Hi Sekiba-san, > > On Thu, 13 Aug 2009 18:23:13 +0900, Jiro SEKIBA <[email protected]> wrote: > > > Hi, > > > > > > This is a revised patch of write performance patch. > > > > > > 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. <snip>
> > > Signed-off-by: Jiro SEKIBA <[email protected]> > > > --- > > > fs/nilfs2/ioctl.c | 14 ++++++++------ > > > 1 files changed, 8 insertions(+), 6 deletions(-) > > > > > > diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c > > > index 6ea5f87..35cd0ff 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,14 @@ 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) { > > > + nilfs_remove_all_gcinode(nilfs); > > > + 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 I noticed there is a problem in this patch. The nilfs_remove_all_gcinode() function and the nilfs_ioctl_move_blocks() function manipulate a list and a hash table of inodes which hold blocks to be moved in garbage collection. However, these are not protected outside the log-writer lock (i.e. transaction lock). Although this ioctl function is usually not called simulataneously, you have to introduce new protection in order to make a series of operations on the inode list and the hash table atomic. I think this can be simply handled by adding a new GC state bit to the nilfs->ns_flags. Could you look back the code around gcinode.c please ? Thanks, Ryusuke Konishi _______________________________________________ users mailing list [email protected] https://www.nilfs.org/mailman/listinfo/users
