xfs
[Top] [All Lists]

Re: [PATCH 3/3] xfs: push down inactive transaction mgmt for ifree

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 3/3] xfs: push down inactive transaction mgmt for ifree
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Thu, 19 Sep 2013 08:44:06 -0400
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130918230620.GE9901@dastard>
References: <1379520960-22972-1-git-send-email-bfoster@xxxxxxxxxx> <1379520960-22972-4-git-send-email-bfoster@xxxxxxxxxx> <20130918230620.GE9901@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130805 Thunderbird/17.0.8
On 09/18/2013 07:06 PM, Dave Chinner wrote:
> On Wed, Sep 18, 2013 at 12:16:00PM -0400, Brian Foster wrote:
>> Push the inode free work performed during xfs_inactive() down into
>> a new xfs_inactive_ifree() helper. This clears xfs_inactive() from
>> all inode locking and transaction management more directly
>> associated with freeing the inode xattrs, extents and the inode
>> itself.
>>
>> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
>> ---
>>  fs/xfs/xfs_inode.c | 121 
>> +++++++++++++++++++++++++++++++----------------------
>>  1 file changed, 71 insertions(+), 50 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index 9416462..a6ed69d 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -1710,6 +1710,74 @@ error0:
>>  }
>>  
>>  /*
>> + * xfs_inactive_ifree()
>> + *
>> + * Perform the inode free when an inode is unlinked.
>> + */
>> +STATIC int
>> +xfs_inactive_ifree(
>> +    struct xfs_inode *ip)
>> +{
>> +    xfs_bmap_free_t         free_list;
>> +    xfs_fsblock_t           first_block;
>> +    int                     committed;
>> +    struct xfs_mount        *mp = ip->i_mount;
>> +    struct xfs_trans        *tp;
>> +    int                     error;
>> +
>> +    tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
>> +    error = xfs_trans_reserve(tp, &M_RES(mp)->tr_ifree, 0, 0);
>> +    if (error) {
>> +            ASSERT(XFS_FORCED_SHUTDOWN(mp));
>> +            xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
>> +            return error;
>> +    }
>> +
>> +    xfs_ilock(ip, XFS_ILOCK_EXCL);
>> +    xfs_trans_ijoin(tp, ip, 0);
>> +
>> +    xfs_bmap_init(&free_list, &first_block);
>> +    error = xfs_ifree(tp, ip, &free_list);
>> +    if (error) {
>> +            /*
>> +             * If we fail to free the inode, shut down.  The cancel
>> +             * might do that, we need to make sure.  Otherwise the
>> +             * inode might be lost for a long time or forever.
>> +             */
>> +            if (!XFS_FORCED_SHUTDOWN(mp)) {
>> +                    xfs_notice(mp, "%s: xfs_ifree returned error %d",
>> +                            __func__, error);
>> +                    xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR);
>> +            }
>> +            xfs_iunlock(ip, XFS_ILOCK_EXCL);
>> +            xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
>> +            return error;
>> +    }
>> +
>> +    /*
>> +     * Credit the quota account(s). The inode is gone.
>> +     */
>> +    xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_ICOUNT, -1);
>> +
>> +    /*
>> +     * Just ignore errors at this point.  There is nothing we can
>> +     * do except to try to keep going. Make sure it's not a silent
>> +     * error.
>> +     */
>> +    error = xfs_bmap_finish(&tp,  &free_list, &committed);
>> +    if (error)
>> +            xfs_notice(mp, "%s: xfs_bmap_finish returned error %d",
>> +                    __func__, error);
>> +    error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
>> +    if (error)
>> +            xfs_notice(mp, "%s: xfs_trans_commit returned error %d",
>> +                    __func__, error);
>> +
>> +    xfs_iunlock(ip, XFS_ILOCK_EXCL);
>> +    return 0;
>> +}
> 
> I suspect we can clean up the error handling here now, and make it
> look like the symlink remove inactive handle where we cancel bmaps
> and abort transactions and trigger shutdowns appropriately. I'd
> leave that to a separate patchset, though ;)
> 

Hmm, well I follow what you mean as far as changing the code I think.
But what changed that makes this safe? Or are you suggesting to shutdown
on a bmap_finish/trans_commit failure instead of just "being noisy?"

(Regardless, a separate patchset sounds good..)

>> -                            __func__, error);
>> -    }
>> +    error = xfs_inactive_ifree(ip);
>> +    if (error)
>> +            goto out;
>>  
>>      /*
>>       * Release the dquots held by inode, if any.
>>       */
>>      xfs_qm_dqdetach(ip);
>> -    xfs_iunlock(ip, XFS_ILOCK_EXCL);
>>  out:
>>      return VN_INACTIVE_CACHE;
>>  }
> 
> I think it's time to kill VN_INACTIVE_CACHE, as well. It's a
> hold-over from Irix, and it serves no purpose what-so-ever on
> Linux...
> 

Ok, looks like a trivial patch to append to the series. In fact, there
appears to be no reason to return a value from xfs_inactive() at all
(along with the out label being unnecessary)... Thanks.

Brian

> Otherwise it looks good.
> 
> Cheers,
> 
> Dave.
> 

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