xfs
[Top] [All Lists]

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

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH v3 08/11] xfs: update the finobt on inode free
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 11 Feb 2014 18:31:03 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1391536182-9048-9-git-send-email-bfoster@xxxxxxxxxx>
References: <1391536182-9048-1-git-send-email-bfoster@xxxxxxxxxx> <1391536182-9048-9-git-send-email-bfoster@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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

<Prev in Thread] Current Thread [Next in Thread>