On Mon, Aug 26, 2019 at 02:21:18PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> Replace the open-coded process of queueing a subdirectory for scanning
> with a single helper function.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
