Re: [PATCH 2/3] xfs: better xfs_trans_alloc interface

From: Dave Chinner <david@fromorbit.com>
To: Brian Foster <bfoster@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>, xfs@oss.sgi.com
Subject: Re: [PATCH 2/3] xfs: better xfs_trans_alloc interface
Date: Thu, 18 Feb 2016 09:04:36 +1100	[thread overview]
Message-ID: <20160217220436.GI19486@dastard> (raw)
In-Reply-To: <20160217134006.GA4065@bfoster.bfoster>

On Wed, Feb 17, 2016 at 08:40:08AM -0500, Brian Foster wrote:
> On Wed, Feb 17, 2016 at 09:52:38AM +0100, Christoph Hellwig wrote:
> > Merge xfs_trans_reserve and xfs_trans_alloc into a single function call
> > that returns a transaction with all the required log and block reservations,
> > and which allows passing transaction flags directly to avoid the cumbersome
> > _xfs_trans_alloc interface.
> > 
> > While we're at it we also get rid of the transaction type argument that has
> > been superflous since we stopped supporting the non-CIL logging mode.  The
> > guts of it will be removed in another patch.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> 
> > @@ -165,7 +123,7 @@ xfs_trans_dup(
> >   * This does not do quota reservations. That typically is done by the
> >   * caller afterwards.
> >   */
> > -int
> > +static int
> >  xfs_trans_reserve(
> >  	struct xfs_trans	*tp,
> >  	struct xfs_trans_res	*resp,
> > @@ -219,7 +177,7 @@ xfs_trans_reserve(
> >  						resp->tr_logres,
> >  						resp->tr_logcount,
> >  						&tp->t_ticket, XFS_TRANSACTION,
> > -						permanent, tp->t_type);
> > +						permanent, 0);
> 
> So this looks like it effectively breaks xlog_print_tic_res()..? I see
> that is all removed in the subsequent patch, but the type still seems
> like useful information in the event that an associated problem occurs
> with the transaction. In fact, we just had a transaction overrun report
> over the weekend (on irc) where at least I thought this was useful
> (unfortunately it looks like I lost the reference to the syslog output).

I've considered doing this removal myself in the past - doing
somethign like embedding the return address of the
xfs-trans_reserve() call in the ticket that is allocated tells us
exactly where the call was made. This can be printed with %pS, and
that gives us the function (and location in the function) the
reservation was made. Hence we solve the problem of not
knowing which call path triggered the problem.

Hence I don't think we actually need to the type in every function
call.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs