On Fri, Nov 29, 2013 at 12:39:25AM -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.
>
> Note that all log item format routines now need to be careful to modify
> the copy of the item that was placed into the CIL after calls to
> xlog_copy_iovec instead of the in-memory copy.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> */
> base_size = xfs_buf_log_format_size(blfp);
>
> - nvecs = 0;
> first_bit = xfs_next_bit(blfp->blf_data_map, blfp->blf_map_size, 0);
> if (!(bip->bli_flags & XFS_BLI_STALE) && first_bit == -1) {
> /*
> * If the map is not be dirty in the transaction, mark
> * the size as zero and do not advance the vector pointer.
> */
> - goto out;
> + return;
> }
>
> - xlog_copy_iovec(vecp, XLOG_REG_TYPE_BFORMAT, blfp, base_size);
> - nvecs = 1;
> + blfp = xlog_copy_iovec(lv, vecp, XLOG_REG_TYPE_BFORMAT, blfp, base_size);
> + blfp->blf_size = 1;
Hmmmm. What guarantee do we have blf_size is now natuarally aligned?
We've returned a pointer that could have any offset into the logvec
buffer, and so some platforms are going to have problems if blfp is
at an address that is not a multiple of 4 or 8, right?
> xfs_inode_item_format_data_fork(
> struct xfs_inode_log_item *iip,
> - struct xfs_log_iovec **vecp,
> - int *nvecs)
> + struct xfs_inode_log_format *ilf,
> + struct xfs_log_vec *lv,
> + struct xfs_log_iovec **vecp)
> {
> struct xfs_inode *ip = iip->ili_inode;
> size_t data_bytes;
> @@ -239,19 +241,19 @@ xfs_inode_item_format_data_fork(
> * extents, so just point to the
> * real extents array.
> */
> - xlog_copy_iovec(vecp, XLOG_REG_TYPE_IEXT,
> + xlog_copy_iovec(lv, vecp, XLOG_REG_TYPE_IEXT,
> ip->i_df.if_u1.if_extents,
> ip->i_df.if_bytes);
> - iip->ili_format.ilf_dsize = ip->i_df.if_bytes;
> + ilf->ilf_dsize = ip->i_df.if_bytes;
And by the looks of it, we could have the same problems here?
> diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
> index 384c6c4..e04bd0c 100644
> --- a/fs/xfs/xfs_log.h
> +++ b/fs/xfs/xfs_log.h
> @@ -31,18 +31,44 @@ 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;
We could at least check here that the alignment is good...
>
> - *vecp = vec + 1;
> + *vecp = vec;
> return vec->i_addr;
> }
>
> +static inline void
> +xlog_finish_iovec(struct xfs_log_vec *lv, struct xfs_log_iovec *vec, int len)
> +{
> + lv->lv_buf_len += len;
And if we need to guarantee alignment, then maybe roundup here to
ensure we don't end up with bad offsets? That would require padding
the allocation of the buffer to take it into account, too....
Other than this concern, the code looks fine.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
