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@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
