Re: [PATCH 1/3] xfs_scrub: refactor queueing of subdir scan work item

[Dave Chinner <david@fromorbit.com>]

On Mon, Aug 26, 2019 at 02:21:18PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Replace the open-coded process of queueing a subdirectory for scanning
> with a single helper function.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  scrub/vfs.c |   94 +++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 52 insertions(+), 42 deletions(-)
> 
> 
> diff --git a/scrub/vfs.c b/scrub/vfs.c
> index 7b0b5bcd..ea2866d9 100644
> --- a/scrub/vfs.c
> +++ b/scrub/vfs.c
> @@ -43,6 +43,49 @@ struct scan_fs_tree_dir {
>  	bool			rootdir;
>  };
>  
> +static void scan_fs_dir(struct workqueue *wq, xfs_agnumber_t agno, void *arg);
> +
> +/* Queue a directory for scanning. */
> +static bool
> +queue_subdir(
> +	struct scrub_ctx	*ctx,
> +	struct scan_fs_tree	*sft,
> +	struct workqueue	*wq,
> +	const char		*path,
> +	bool			is_rootdir)
> +{
> +	struct scan_fs_tree_dir	*new_sftd;
> +	int			error;
> +
> +	new_sftd = malloc(sizeof(struct scan_fs_tree_dir));
> +	if (!new_sftd) {
> +		str_errno(ctx, _("creating directory scan context"));
> +		return false;
> +	}
> +
> +	new_sftd->path = strdup(path);
> +	if (!new_sftd->path) {
> +		str_errno(ctx, _("creating directory scan path"));
> +		free(new_sftd);
> +		return false;
> +	}
> +
> +	new_sftd->sft = sft;
> +	new_sftd->rootdir = is_rootdir;
> +
> +	pthread_mutex_lock(&sft->lock);
> +	sft->nr_dirs++;
> +	pthread_mutex_unlock(&sft->lock);
> +	error = workqueue_add(wq, scan_fs_dir, 0, new_sftd);
> +	if (error) {
> +		str_info(ctx, ctx->mntpoint,
> +_("Could not queue subdirectory scan work."));
> +		return false;

Need to drop sft->nr_dirs here, probably free the memory, too.

> @@ -177,41 +203,25 @@ scan_fs_tree(
>  	pthread_mutex_init(&sft.lock, NULL);
>  	pthread_cond_init(&sft.wakeup, NULL);
>  
> -	sftd = malloc(sizeof(struct scan_fs_tree_dir));
> -	if (!sftd) {
> -		str_errno(ctx, ctx->mntpoint);
> -		return false;
> -	}
> -	sftd->path = strdup(ctx->mntpoint);
> -	sftd->sft = &sft;
> -	sftd->rootdir = true;
> -
>  	ret = workqueue_create(&wq, (struct xfs_mount *)ctx,
>  			scrub_nproc_workqueue(ctx));
>  	if (ret) {
>  		str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
> -		goto out_free;
> +		return false;
>  	}
> -	ret = workqueue_add(&wq, scan_fs_dir, 0, sftd);
> -	if (ret) {
> -		str_info(ctx, ctx->mntpoint,
> -_("Could not queue directory scan work."));
> +
> +	sft.moveon = queue_subdir(ctx, &sft, &wq, ctx->mntpoint, true);
> +	if (!sft.moveon)
>  		goto out_wq;
> -	}

sft is a stack varable that is stuffed into the structure passed to
work run on the workqueue. Is that safe to do here?

>  	pthread_mutex_lock(&sft.lock);
>  	pthread_cond_wait(&sft.wakeup, &sft.lock);

maybe it is because of this, but it's not immediately obvious what
condition actually triggers and that all the work is done...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com