Re: [RFC PATCH 2/4] xfs: introduce a new helper xfs_inobt_lookup_grab_ichunk()

[Jeff Liu <jeff.liu@oracle.com>]

On 11/16 2013 01:13 AM, Christoph Hellwig wrote:
>> Refactor xfs_bulkstat() to make use of the founction.
> 
>                                          ... function.
> 
>> +int
>> +xfs_inobt_lookup_grab_ichunk(
>> +	struct xfs_btree_cur		*cur,	/* btree cursor */
>> +	xfs_agino_t			agino,	/* starting inode of chunk */
>> +	struct xfs_inobt_rec_incore	*irec,	/* btree record */
>> +	int				*stat)	/* success/failure */
>> +{
>> +	int				idx;	/* Index into inode chunk */
>> +	int				error;
>> +
>> +	/* Lookup the inode chunk that this inode lives in */
>> +	error = xfs_inobt_lookup(cur, agino, XFS_LOOKUP_LE, stat);
>> +	if (error || !*stat)
>> +		return error;
>> +
>> +	/* Get the record, should always work */
>> +	error = xfs_inobt_get_rec(cur, irec, stat);
>> +	ASSERT(!error && *stat);
> 
> If it wails it's a corruption error, so this shouldn't be an assert,
> but rather an XFS_WANT_CORRUPTED_GOTO/RETURN
Indeed.
> 
>> +	if (error || !*stat)
>> +		return error;
>> +
>> +	*stat = 0;
> 
> Btw, I don't think we need to pass around the status pointer here,
> the caller doesn't care about the internal lookup status, just if
> we succeeded or failed.
Yes, I'll fix it in next version.
> 
>> +	if (irec->ir_startino + XFS_INODES_PER_CHUNK <= agino)
>> +		return error;
> 
> error will usually be zero here, shouldn't we return a real error?
In xfs_imap_lookup(), it return EINVAL if the given inode is not inside the
returned record, looks we should follow up the same rule here.

> 
>> +	idx = agino - irec->ir_startino + 1;
>> +	/*
>> +	 * We got a right chunk and there are some left inodes allocated at it.
>> +	 */
>> +	if (idx < XFS_INODES_PER_CHUNK &&
>> +	    (xfs_inobt_maskn(idx, XFS_INODES_PER_CHUNK - idx) & ~irec->ir_free)) {
>> +		int	i;
>> +
>> +		/*
>> +		 * Grab the chunk record.  Mark all the uninteresting inodes free
>> +		 * (because they're before our start point).
>> +		 */
>> +		for (i = 0; i < idx; i++) {
>> +			if (XFS_INOBT_MASK(i) & ~irec->ir_free)
>> +				irec->ir_freecount++;
>> +		}
>> +
>> +		irec->ir_free |= xfs_inobt_maskn(0, idx);
>> +		*stat = 1;
>> +	}
> 
> 
> At this point the function is so bulkstate specific that it should stay
> in xfs_itable.c and have a name more like xfs_bulkstate_grab_ichunk.
Nice point. :)

Thanks,
-Jeff

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs