xfs
[Top] [All Lists]

Re: [PATCH 06/10 v2] xfs: format log items write directly into the linea

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 06/10 v2] xfs: format log items write directly into the linear CIL buffer
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 12 Dec 2013 11:05:30 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131211120341.GA6116@xxxxxxxxxxxxx>
References: <20131129083919.207915844@xxxxxxxxxxxxxxxxxxxxxx> <20131129084001.028583376@xxxxxxxxxxxxxxxxxxxxxx> <20131211120341.GA6116@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Wed, Dec 11, 2013 at 04:03:41AM -0800, Christoph Hellwig wrote:
> Instead of setting up pointers to memory locations in iop_format which then
> get copied into the CIL linear buffer after return move the copy into
> the individual inode items.  This avoids the need to always have a memory
> block in the exact same layout that gets written into the log around, and
> allow the log items to be much more flexible in their in-memory layouts.
> 
> The only caveat is that we need to properly align the data for each
> iovec so that don't have structures misaligned in subsequent iovecs.

.....

> diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
> index 384c6c4..65e054a 100644
> --- a/fs/xfs/xfs_log.h
> +++ b/fs/xfs/xfs_log.h
> @@ -31,18 +31,49 @@ struct xfs_log_vec {
>  #define XFS_LOG_VEC_ORDERED  (-1)
>  
>  static inline void *
> -xlog_copy_iovec(struct xfs_log_iovec **vecp, uint type, void *data, int len)
> +xlog_prepare_iovec(struct xfs_log_vec *lv, struct xfs_log_iovec **vecp,
> +             uint type)
>  {
>       struct xfs_log_iovec *vec = *vecp;
>  
> +     if (vec) {
> +             ASSERT(vec - lv->lv_iovecp < lv->lv_niovecs);
> +             vec++;
> +     } else {
> +             vec = &lv->lv_iovecp[0];
> +     }
> +
>       vec->i_type = type;
> -     vec->i_addr = data;
> -     vec->i_len = len;
> +     vec->i_addr = lv->lv_buf + lv->lv_buf_len;
>  
> -     *vecp = vec + 1;
> +     *vecp = vec;
>       return vec->i_addr;

Can you add an assert here like this:

        ASSERT(IS_ALIGNED(vec->i_addr, sizeof(uint64_t));

So we catch any situation where the alignment ends up wrong?

Otherwise, it looks good, so consider it:

Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>

-- 
Dave Chinner
david@xxxxxxxxxxxxx

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