Re: [PATCH 9/9] xfs: log ticket reservation underestimates the number of iclogs

From: Dave Chinner <david@fromorbit.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 9/9] xfs: log ticket reservation underestimates the number of iclogs
Date: Tue, 16 Mar 2010 12:51:07 +1100	[thread overview]
Message-ID: <20100316015107.GE12369@dastard> (raw)
In-Reply-To: <20100315154435.GA29024@infradead.org>

On Mon, Mar 15, 2010 at 11:44:35AM -0400, Christoph Hellwig wrote:
> > > +	iclog_space = log->l_iclog_size - log->l_iclog_hsize;
> > > +	num_headers = (unit_bytes + iclog_space - 1) / iclog_space;
> > > +
> > > +	/* for split-recs - ophdrs added when data split over LRs */
> > > +	unit_bytes += sizeof(xlog_op_header_t) * num_headers;
> > > +
> > > +	/* add extra header reservations if we overrun */
> > > +	while (!num_headers ||
> > > +	       ((unit_bytes + iclog_space - 1) / iclog_space) > num_headers) {
> > > +		unit_bytes += sizeof(xlog_op_header_t);
> > > +		num_headers++;
> > > +	}
> > 
> > Looks good, but why do you check for a zero num_headers here?  The only way
> > this could happen after the roundup above is if unit_bytes is zero, which
> > can't ever happen - one caller has it hardcoded to 1, and the the other
> > has a conditional for it beeing bigger than 0 around the call.

Mainly because I want to be able to pass a length of zero in for the
checkpoint transaction and have it do the right thing. That is,
reserve space for the first header which, in this case, will
definitely be used. Indeed, once I made this change, i forgot to
change the CIL ticket allocation back to zero - it is still:

	tic = xlog_ticket_alloc(log, 1 /* to get a LR header */,
				1, XFS_TRANSACTION, 0);

Other than that I can't find any caller that uses unit_bytes = 1.
Maybe I missed something - can you point it out to me?

FWIW, there's more missing from the transaction reservations
themselves - none of them take into account the space used by the
log item format descriptor that describe the changed regions of an
object written to the log. They take into account the regions
themselves, but not the descriptor. Some of the hard coded
transaction reservations do - see all the single sector transactions
that reserve "<something> + 128" e.g:

	error = xfs_trans_reserve(tp, 0, mp->m_sb.sb_sectsize + 128, 0, 0,
				XFS_DEFAULT_LOG_COUNT);

That 128 bytes is for the log item format descriptor, and in some
cases I'm not sure that 128 bytes is enough. Anyway, that's a
problem for a rainy day...

> BTW: use of the howmany macro might make some of the above easier
> to read.

Good point. I'll look into using that.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

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