On Wed, Dec 04, 2013 at 11:37:12AM +1100, Dave Chinner wrote:
> 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.
Christoph, what about this alignment issue?
Thanks,
Ben
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
