xfs
[Top] [All Lists]

Re: [PATCH] xfs_repair: multithread phase 2

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs_repair: multithread phase 2
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Mon, 10 Jan 2011 13:55:22 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1294620248-17098-1-git-send-email-david@xxxxxxxxxxxxx>
References: <1294620248-17098-1-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
Looks good except for some trivial nitpicks below,


Reviewed-by: Christoph Hellwig <hch@xxxxxx>

Btw, your previous patch used just

"repair:" as the Subject prefix, while this one uses xfs_repair.  I
don't really care about, but we should standardize on one.  The more
recent usage seems to include the xfs_ prefix.

> +                                  scanfunc_bno : scanfunc_cnt, 0,
> +                                  (void *)agcnts);

no need for the void cast.

> +#define SCAN_THREADS 32

this is unused now.

> +     agcnts = malloc(mp->m_sb.sb_agcount * sizeof(*agcnts));
> +     if (!agcnts) {
> +             do_abort(_("no memory for ag header counts\n"));
> +             return;
> +     }
> +     memset(agcnts, 0, mp->m_sb.sb_agcount * sizeof(*agcnts));

this could use a calloc.

>                                       break;
> +                             case PHASE2_THREADS:
> +                                     phase2_threads = (int)strtol(val, NULL, 
> 0);
> +                                     break;

This option also needs to be documented in the man page.  Also shouldn't
we try to handle errors from strtol?  Also maybe strtoul would be a
better choice as we certainly don't want a negative number of threads.

<Prev in Thread] Current Thread [Next in Thread>