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

From: Brian Foster <bfoster@redhat.com>
To: Dave Chinner <david@fromorbit.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 07:10:21 -0500	[thread overview]
Message-ID: <20160218121021.GB16962@bfoster.bfoster> (raw)
In-Reply-To: <20160217220436.GI19486@dastard>

On Thu, Feb 18, 2016 at 09:04:36AM +1100, Dave Chinner wrote:
> 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.
> 

That sounds like a good idea to me.

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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