xfs
[Top] [All Lists]

Re: [RFC PATCH 2/4] xfs: introduce a new helper xfs_inobt_lookup_grab_ic

To: Jeff Liu <jeff.liu@xxxxxxxxxx>
Subject: Re: [RFC PATCH 2/4] xfs: introduce a new helper xfs_inobt_lookup_grab_ichunk()
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Fri, 15 Nov 2013 09:13:56 -0800
Cc: "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <5281F518.3080106@xxxxxxxxxx>
References: <5281F518.3080106@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
> 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

> +     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.

> +     if (irec->ir_startino + XFS_INODES_PER_CHUNK <= agino)
> +             return error;

error will usually be zero here, shouldn't we return a real error?

> +     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.

I still like factoring it out though, thanks for the work!

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