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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Sep 04, 2019 at 06:12:52PM +1000, Dave Chinner wrote:
> 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.

nr_dirs is (as you've observed) fixed in the next patch.

Yes, we need to free the memory.  Good catch.

> > @@ -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...

A worker thread signals the condition variable when nr_dirs hits zero.
There should only be one worker left when this happens (assuming the
accounting is correct) and the worker doesn't do anything with sft after
it unlocks it, so this should be safe.

Will add comment to that effect.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com