Re: [PATCH 01/15] xfs: Add a new transaction for changing ag state

[Dave Chinner <david@fromorbit.com>]

On Fri, Nov 16, 2012 at 02:44:55PM +0800, Jeff Liu wrote:
> This a new tranaction which would be use for Changing AG state via ioctl(2)
> 
> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
> ---
>  fs/xfs/xfs_mount.h |    1 +
>  fs/xfs/xfs_trans.c |   12 ++++++++++++
>  fs/xfs/xfs_trans.h |    8 +++++---
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index deee09e..4fe2232 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -40,6 +40,7 @@ typedef struct xfs_trans_reservations {
>  	uint	tr_growrtalloc;	/* grow realtime allocations */
>  	uint	tr_growrtzero;	/* grow realtime zeroing */
>  	uint	tr_growrtfree;	/* grow realtime freeing */
> +	uint	tr_setagstate;	/* set a.g. state trans */
>  } xfs_trans_reservations_t;
>  
>  #ifndef __KERNEL__
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 06ed520..5a5c4d8 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -531,6 +531,17 @@ xfs_calc_clear_agi_bucket_reservation(
>  }
>  
>  /*
> + * Setting the state of an allocation group.
> + *	agf of the ag: sector size
> + */
> +STATIC uint
> +xfs_calc_set_agstate_reservation(
> +	struct xfs_mount	*mp)
> +{
> +	return mp->m_sb.sb_sectsize + 128;
> +}

I know you are just copying other code, but what we really need to
do here is get rid of all these magic "128" numbers in the
transaction reservations.

What the 128 is actually reserving is space for the
struct xfs_buf_log_format that gets written into the log for every
buffer. i.e. the space needed for this structure:

struct xfs_buf_log_format {
        short unsigned int         blf_type;             /*     0     2 */
        short unsigned int         blf_size;             /*     2     2 */
        ushort                     blf_flags;            /*     4     2 */
        ushort                     blf_len;              /*     6     2 */
        __int64_t                  blf_blkno;            /*     8     8 */
        unsigned int               blf_map_size;         /*    16     4 */
        unsigned int               blf_data_map[16];     /*    20    64 */
        /* --- cacheline 1 boundary (64 bytes) was 20 bytes ago --- */

        /* size: 88, cachelines: 2, members: 7 */
        /* padding: 4 */
        /* last cacheline: 24 bytes */
};

It's rounded up to give a little bit of extra space because there's
it also needs a log opheader:

struct xlog_op_header {
        __be32                     oh_tid;               /*     0     4 */
        __be32                     oh_len;               /*     4     4 */
        __u8                       oh_clientid;          /*     8     1 */
        __u8                       oh_flags;             /*     9     1 */
        __u16                      oh_res2;              /*    10     2 */

        /* size: 12, cachelines: 1, members: 5 */
        /* last cacheline: 12 bytes */
};

So there's at least 100 bytes of extra information per buffer that
needs to be recorded with the buffer itself. Rounding it up to 128
bytes is probably just fine, though it should probably be done
programatically rather than hand coded.

As such I'd like to see this sort of thing encoded in a macro or
inline function so the above code becomes something like:

	return mp->m_sb.sb_sectsize + xfs_buf_log_overhead(mp);


and

/*
 * A buffer has a format structure overhead in the log in addition
 * to the data, so we need to take this into account when reserving
 * space in a transaction for a buffer. Round the space required up
 * to a multiple of 128 bytes so that we don't change the historical
 * reservation that has ben used for this overhead.
 */
static inline int
xfs_buf_log_overhead()
{
	return round_up(sizeof(struct xlog_op_header) +
			sizeof(struct xfs_buf_log_format), 128);
}

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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