[Cluster-devel] [GFS2 PATCH] [TRY #3] GFS2: Prevent recovery before the local journal is set

[Steven Whitehouse]

Hi,

On 22/05/14 14:17, Bob Peterson wrote:
> Hi,
>
> This is my third attempt at a recovery patch that prevents recovery
> before the journal is set by GFS2. Steve Whitehouse pointed out that
> there were error paths in the code that could leave recovery permanently
> hung, waiting for the completion. It turns out there were lots of error
> paths with this problem, which prompted me to completely rewrite the
> patch. This version keeps track of when there might possibly be waiters
> for the completion, and if so, the error path does a complete_all.
>
> Patch description:
>
> This patch uses a completion to prevent dlm's recovery process from
> referencing and trying to recover a journal before a journal has been
> opened.
>
> Regards,
>
> Bob Peterson
> Red Hat File Systems
>
> Signed-off-by: Bob Peterson <rpeterso at redhat.com>
> ---
> diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
> index 2434a96..67d310c 100644
> --- a/fs/gfs2/incore.h
> +++ b/fs/gfs2/incore.h
> @@ -728,6 +728,8 @@ struct gfs2_sbd {
>   	struct gfs2_holder sd_sc_gh;
>   	struct gfs2_holder sd_qc_gh;
>   
> +	struct completion sd_journal_ready;
> +
>   	/* Daemon stuff */
>   
>   	struct task_struct *sd_logd_process;
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index be45c79..75310a4 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -94,6 +94,7 @@ static struct gfs2_sbd *init_sbd(struct super_block *sb)
>   	INIT_LIST_HEAD(&sdp->sd_jindex_list);
>   	spin_lock_init(&sdp->sd_jindex_spin);
>   	mutex_init(&sdp->sd_jindex_mutex);
> +	init_completion(&sdp->sd_journal_ready);
>   
>   	INIT_LIST_HEAD(&sdp->sd_quota_list);
>   	mutex_init(&sdp->sd_quota_mutex);
> @@ -796,6 +797,7 @@ static int init_inodes(struct gfs2_sbd *sdp, int undo)
>   		goto fail_qinode;
>   
>   	error = init_journal(sdp, undo);
> +	complete_all(&sdp->sd_journal_ready);
>   	if (error)
>   		goto fail;
>   
> @@ -1063,6 +1065,7 @@ static int fill_super(struct super_block *sb, struct gfs2_args *args, int silent
>   	struct gfs2_sbd *sdp;
>   	struct gfs2_holder mount_gh;
>   	int error;
> +	int poss_j_waiters = 0; /* Possible journal completion waiters */
>   
>   	sdp = init_sbd(sb);
>   	if (!sdp) {
> @@ -1138,6 +1141,9 @@ static int fill_super(struct super_block *sb, struct gfs2_args *args, int silent
>   	if (error)
>   		goto fail_debug;
>   
> +	poss_j_waiters = 1; /* After this point, we could have lock_dlm
> +			       waiting for our sd_journal_ready completion. */
> +
I don't think this is true... as soon as sysfs is set up (before this 
point) there can be processes waiting on the completion. Also, I think 
you can just call the complete_all() unconditionally at fail_lm time, so 
that the poss_j_waiters thing is not really needed I think. Otherwise it 
looks good though,

Steve.

>   	error = init_locking(sdp, &mount_gh, DO);
>   	if (error)
>   		goto fail_lm;
> @@ -1171,6 +1177,10 @@ static int fill_super(struct super_block *sb, struct gfs2_args *args, int silent
>   		snprintf(sdp->sd_fsname, GFS2_FSNAME_LEN, "%s.%u",
>   			 sdp->sd_table_name, sdp->sd_lockstruct.ls_jid);
>   
> +	poss_j_waiters = 0; /* init_inodes calls init_journal which satisfies
> +			       the sd_journal_ready completion, regardless
> +			       of whether it's successful. */
> +
>   	error = init_inodes(sdp, DO);
>   	if (error)
>   		goto fail_sb;
> @@ -1212,6 +1222,8 @@ fail_sb:
>   fail_locking:
>   	init_locking(sdp, &mount_gh, UNDO);
>   fail_lm:
> +	if (poss_j_waiters)
> +		complete_all(&sdp->sd_journal_ready);
>   	gfs2_gl_hash_clear(sdp);
>   	gfs2_lm_unmount(sdp);
>   fail_debug:
> diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
> index 7bc17ed..0e049f9 100644
> --- a/fs/gfs2/sys.c
> +++ b/fs/gfs2/sys.c
> @@ -407,6 +407,9 @@ int gfs2_recover_set(struct gfs2_sbd *sdp, unsigned jid)
>   	struct gfs2_jdesc *jd;
>   	int rv;
>   
> +	/* Wait for our primary journal to be initialized */
> +	wait_for_completion(&sdp->sd_journal_ready);
> +
>   	spin_lock(&sdp->sd_jindex_spin);
>   	rv = -EBUSY;
>   	if (sdp->sd_jdesc->jd_jid == jid)
>