Re: [PATCH v3 08/11] xfs: update the finobt on inode free

[Dave Chinner <david@fromorbit.com>]

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@fromorbit.com

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