Hi Sekiba-san,
On Sat, 29 Aug 2009 17:54:30 +0900, Jiro SEKIBA <[email protected]> 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
> [email protected]
> https://www.nilfs.org/mailman/listinfo/users
_______________________________________________
users mailing list
[email protected]
https://www.nilfs.org/mailman/listinfo/users
