Re: [PATCH 16/71] xfs: log refcount intent items

On Tue, Sep 06, 2016 at 08:21:55AM -0700, Christoph Hellwig wrote:
> > +	__uint64_t			cui_id;
> > +	struct xfs_ail_cursor		cur;
> > +	struct xfs_ail			*ailp = log->l_ailp;
> > +
> > +	cud_formatp = item->ri_buf[0].i_addr;
> > +	ASSERT(item->ri_buf[0].i_len == sizeof(struct xfs_cud_log_format));
> 
> Should we return -EFSCORRUPTED here instead?

Yes.  The RUD recovery routine should probably get that change too.

> > +	/* XXX: do nothing for now */
> 
> What else would be do in the future here?
> 
> > +static void
> > +xfs_trans_set_refcount_flags(
> > +	struct xfs_phys_extent		*refc,
> > +	enum xfs_refcount_intent_type	type)
> > +{
> > +	refc->pe_flags = 0;
> > +	switch (type) {
> > +	case XFS_REFCOUNT_INCREASE:
> > +		refc->pe_flags |= XFS_REFCOUNT_EXTENT_INCREASE;
> > +		break;
> > +	case XFS_REFCOUNT_DECREASE:
> > +		refc->pe_flags |= XFS_REFCOUNT_EXTENT_DECREASE;
> > +		break;
> > +	case XFS_REFCOUNT_ALLOC_COW:
> > +		refc->pe_flags |= XFS_REFCOUNT_EXTENT_ALLOC_COW;
> > +		break;
> > +	case XFS_REFCOUNT_FREE_COW:
> > +		refc->pe_flags |= XFS_REFCOUNT_EXTENT_FREE_COW;
> > +		break;
> 
> Is there any good reasons to use a type enum in core, but flags on
> disk?

I suppose since log structures aren't guaranteed to be platform agnostic
it's fine to just copy in the in-core enum here.

> > +int
> > +xfs_trans_log_finish_refcount_update(
> > +	struct xfs_trans		*tp,
> > +	struct xfs_cud_log_item		*cudp,
> > +	enum xfs_refcount_intent_type	type,
> > +	xfs_fsblock_t			startblock,
> > +	xfs_extlen_t			blockcount,
> > +	struct xfs_btree_cur		**pcur)
> > +{
> > +	int				error;
> > +
> > +	/* XXX: leave this empty for now */
> > +	error = -EFSCORRUPTED;
> 
> Lift might be a lot easier if this patch and "xfs: connect refcount
> adjust functions to upper layers" were merged into one.  It's not like
> they are testable independently anyway.

I'll think about it.  I think it might not be too difficult to push "xfs: log
refcount intent items" down one and merge it with "xfs: connect refcount adjust
functions to upper layers", though my preference biases towards not stirring
things up just to reduce commit count.

(Basically, I'll give it a try after I'm done making the other fixes and commit
it if it doesn't make a total mess of things.)

--D

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs