On Tue, Feb 04, 2014 at 12:49:39PM -0500, Brian Foster wrote:
> An inode free operation can have several effects on the finobt. If
> all inodes have been freed and the chunk deallocated, we remove the
> finobt record. If the inode chunk was previously full, we must
> insert a new record based on the existing inobt record. Otherwise,
> we modify the record in place.
>
> Create the xfs_ifree_finobt() function to identify the potential
> scenarios and update the finobt appropriately.
Couple of minor things....
> + * Read and update the existing record.
> + */
> + error = xfs_inobt_get_rec(cur, &rec, &i);
> + if (error)
> + goto error;
> + XFS_WANT_CORRUPTED_GOTO(i == 1, error);
> +
> + rec.ir_free |= XFS_INOBT_MASK(offset);
> + rec.ir_freecount++;
> +
> + XFS_WANT_CORRUPTED_GOTO((rec.ir_free == ibtrec->ir_free) &&
> + (rec.ir_freecount == ibtrec->ir_freecount),
> + error);
I'd add a small comment here saying:
/*
* We could just copy the ibtrec across here, but that
* defeats the purpose of having redundant metadata. By
* doing the modifications independently, we can catch
* corruptions that we wouldn't see if we just copied from
* one record to another.
*/
> + /*
> + * The content of inobt records should always match between the inobt
> + * and finobt. The lifecycle of records in the finobt is different from
> + * the inobt in that the finobt only tracks records with at least one
> + * free inode. This is to optimize lookup for inode allocation purposes.
> + * The following checks determine whether to update the existing record or
> + * remove it entirely.
> + */
> +
extra whitespace line
> + if (rec.ir_freecount == XFS_IALLOC_INODES(mp) &&
> + !(mp->m_flags & XFS_MOUNT_IKEEP)) {
> + /*
> + * If all inodes are free and we're in !ikeep mode, the entire
> + * inode chunk has been deallocated. Remove the record from the
> + * finobt.
> + */
> + error = xfs_btree_delete(cur, &i);
> + if (error)
> + goto error;
> + ASSERT(i == 1);
> + } else {
> + /*
> + * The existing finobt record was modified and has a combination
> + * of allocated and free inodes or is completely free and ikeep
> + * is enabled. Update the record.
> + */
> + error = xfs_inobt_update(cur, &rec);
> + if (error)
> + goto error;
> + }
All th comments say pretty much the same thing, and repeat what the
code does. Hence I think this is sufficient:
/*
* The content of inobt records should always match between the inobt
* and finobt. The lifecycle of records in the finobt is different from
* the inobt in that the finobt only tracks records with at least one
* free inode. Hence if all the inodes are free and we aren't keeping
* inode chunks permanently on disk, remove them. Otherwise just
* update the record with the new information.
*/
if (rec.ir_freecount == XFS_IALLOC_INODES(mp) &&
!(mp->m_flags & XFS_MOUNT_IKEEP)) {
error = xfs_btree_delete(cur, &i);
if (error)
goto error;
ASSERT(i == 1);
} else {
error = xfs_inobt_update(cur, &rec);
if (error)
goto error;
}
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
