Re: [PATCH 03/22] xfs: add helpers to collect and sift btree block pointers during repair

[Allison Henderson <allison.henderson@oracle.com>]

On 05/16/2018 02:23 PM, Dave Chinner wrote:
> On Wed, May 16, 2018 at 11:06:15AM -0700, Darrick J. Wong wrote:
>> On Wed, May 16, 2018 at 10:34:36AM -0700, Allison Henderson wrote:
>>> Ok, with the points Dave made:
>>> Reviewed by: Allison Henderson <allison.henderson@oracle.com>
>>>
>>> On 05/16/2018 12:56 AM, Dave Chinner wrote:
>>>> On Tue, May 15, 2018 at 03:33:58PM -0700, Darrick J. Wong wrote:
>>>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>>>
>>>>> Add some helpers to assemble a list of fs block extents.  Generally,
>>>>> repair functions will iterate the rmapbt to make a list (1) of all
>>>>> extents owned by the nominal owner of the metadata structure; then they
>>>>> will iterate all other structures with the same rmap owner to make a
>>>>> list (2) of active blocks; and finally we have a subtraction function to
>>>>> subtract all the blocks in (2) from (1), with the result that (1) is now
>>>>> a list of blocks that were owned by the old btree and must be disposed.
>>>>>
>>>>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>>>>> ---
>>>>>    fs/xfs/scrub/repair.c |  207 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    fs/xfs/scrub/repair.h |   31 +++++++
>>>>>    2 files changed, 238 insertions(+)
>>>>>
>>>>>
>>>>> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
>>>>> index 72f04a717150..8e8ecddd7537 100644
>>>>> --- a/fs/xfs/scrub/repair.c
>>>>> +++ b/fs/xfs/scrub/repair.c
>>>>> @@ -361,3 +361,210 @@ xfs_repair_init_btblock(
>>>>>    	return 0;
>>>>>    }
>>>>> +
>>>>> +/* Collect a dead btree extent for later disposal. */
>>>>> +int
>>>>> +xfs_repair_collect_btree_extent(
>>>>> +	struct xfs_scrub_context	*sc,
>>>>> +	struct xfs_repair_extent_list	*exlist,
>>>>> +	xfs_fsblock_t			fsbno,
>>>>> +	xfs_extlen_t			len)
>>>>> +{
>>>>> +	struct xfs_repair_extent	*rex;
>>>>> +
>>>>> +	trace_xfs_repair_collect_btree_extent(sc->mp,
>>>>> +			XFS_FSB_TO_AGNO(sc->mp, fsbno),
>>>>> +			XFS_FSB_TO_AGBNO(sc->mp, fsbno), len);
>>>>> +
>>>>> +	rex = kmem_alloc(sizeof(struct xfs_repair_extent), KM_MAYFAIL);
>>>>> +	if (!rex)
>>>>> +		return -ENOMEM;
>>>> Is this in transaction context? Regardless, I think we need to run
>>>> the entire of scrub/repair in a memalloc_nofs_save() context so we
>>>> don't have memory reclaim recursion issues...
>>>>
>>>> [...]
>>>>
>>>>> +/* Compare two btree extents. */
>>>>> +static int
>>>>> +xfs_repair_btree_extent_cmp(
>>>>> +	void				*priv,
>>>>> +	struct list_head		*a,
>>>>> +	struct list_head		*b)
>>>>> +{
>>>>> +	struct xfs_repair_extent	*ap;
>>>>> +	struct xfs_repair_extent	*bp;
>>>>> +
>>>>> +	ap = container_of(a, struct xfs_repair_extent, list);
>>>>> +	bp = container_of(b, struct xfs_repair_extent, list);
>>>>> +
>>>>> +	if (ap->fsbno > bp->fsbno)
>>>>> +		return 1;
>>>>> +	else if (ap->fsbno < bp->fsbno)
>>>>> +		return -1;
>>>> No need for the else there.
>>> Well, I think he meant to return 0 in the case of ap->fsbno == bp->fsbno?
>>> Am i reading that right?  caller expects 1 for greater than, -1 for less
>>> than and 0 on equivalence?
>>
>> Correct.  I think Dave was pointing out that else-after-return is
>> unnecessary, since the following behaves equivalently:
>>
>> if (a > b)
>> 	return 1;
>> if (a < b)
>> 	return -1;
>> return 0;
>>
>> Note that gcc (7.3, anyway) generates the same asm for either version so
>> I assume this is mostly stylistic cleanup to make the comparisons line
>> up?  I don't have a preference either way. :)
> 
> It's a style and maintenance thing. The else is redundant and could
> lead to future confusion, so IMO it is best to leave it out....
> 
> Cheers,
> 
> Dave.
> 
Alrighty then, thx for the clarification! :-)


Allison