xfs
[Top] [All Lists]

Re: [PATCH 2/2] xfs: inode log reservations are still too small

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/2] xfs: inode log reservations are still too small
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Wed, 5 Mar 2014 11:06:02 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1393981893-2497-3-git-send-email-david@xxxxxxxxxxxxx>
References: <1393981893-2497-1-git-send-email-david@xxxxxxxxxxxxx> <1393981893-2497-3-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Wed, Mar 05, 2014 at 12:11:33PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> Back in commit 23956703 ("xfs: inode log reservations are too
> small"), the reservation size was increased to take into account the
> difference in size between the in-memory BMBT block headers and the
> on-disk BMDR headers. This solved a transaction overrun when logging
> the inode size.
> 
> Recently, however, we've seen a number of these same overruns on
> kernels with the above fix in it. All of them have been by 4 bytes,
> so we must still not be accounting for something correctly.
> 
> Through inspection it turns out the above commit didn't take into
> account everything it should have. That is, it only accounts for a
> single log op_hdr structure, when it can actually require up to four
> op_hdrs - one for each region (log iovec) that is formatted. These
> regions are the inode log format header, the inode core, and the two
> forks that can be held in the literal area of the inode.
> 
> This means we are not accounting for 36 bytes of log space that the
> transaction can use, and hence when we get inodes in certain formats
> with particular fragmentation patterns we can overrun the
> transaction. Fix this by adding the correct accounting for log
> op_headers in the transaction.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---

You can add:

Tested-by: Brian Foster <bfoster@xxxxxxxxxx>

... to Eric's review as well. I'm still throwing a workload at it, but
this has survived a few hours of a reproducer that consistently caused
the overrun in 5-10 seconds. Thanks for catching this.

Brian

>  fs/xfs/xfs_trans_resv.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans_resv.c b/fs/xfs/xfs_trans_resv.c
> index 8515b04..d2c8e4a 100644
> --- a/fs/xfs/xfs_trans_resv.c
> +++ b/fs/xfs/xfs_trans_resv.c
> @@ -81,20 +81,28 @@ xfs_calc_buf_res(
>   * on disk. Hence we need an inode reservation function that calculates all 
> this
>   * correctly. So, we log:
>   *
> - * - log op headers for object
> + * - 4 log op headers for object
> + *   - for the ilf, the inode core and 2 forks
>   * - inode log format object
> - * - the entire inode contents (core + 2 forks)
> - * - two bmap btree block headers
> + * - the inode core
> + * - two inode forks containing bmap btree root blocks.
> + *   - the btree data contained by both forks will fit into the inode size,
> + *     hence when combined with the inode core above, we have a total of the
> + *     actual inode size.
> + *   - the BMBT headers need to be accounted separately, as they are
> + *     additional to the records and pointers that fit inside the inode
> + *     forks.
>   */
>  STATIC uint
>  xfs_calc_inode_res(
>       struct xfs_mount        *mp,
>       uint                    ninodes)
>  {
> -     return ninodes * (sizeof(struct xlog_op_header) +
> -                       sizeof(struct xfs_inode_log_format) +
> -                       mp->m_sb.sb_inodesize +
> -                       2 * XFS_BMBT_BLOCK_LEN(mp));
> +     return ninodes *
> +             (4 * sizeof(struct xlog_op_header) +
> +              sizeof(struct xfs_inode_log_format) +
> +              mp->m_sb.sb_inodesize +
> +              2 * XFS_BMBT_BLOCK_LEN(mp));
>  }
>  
>  /*
> -- 
> 1.9.0
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

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