Re: [PATCH 06/10] xfs: format log items write directly into the linear CIL buffer

[Ben Myers <bpm@sgi.com>]

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@lst.de>
> >  	 */
> >  	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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs