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

[Christoph Hellwig <hch@infradead.org>]

On Mon, Mar 15, 2010 at 01:35:06PM +1100, Dave Chinner wrote:
> +	 * for LR headers - the space for data in an iclog is the size minus
> +	 * the space used for the headers. If we use the iclog size, then we
> +	 * undercalculate the number of headers required.
> +	 *
> +	 * Furthermore - the addition of op headers for split-recs might
> +	 * increase the space required enough to require more log and op
> +	 * headers, so take that into account too.
> +	 *
> +	 * IMPORTANT: This reservation makes the assumption that if this
> +	 * transaction is the first in an iclog and hence has the LR headers
> +	 * accounted to it, then the remaining space in the iclog is
> +	 * exclusively for this transaction.  i.e. if the transaction is larger
> +	 * than the iclog, it will be the only thing in that iclog.
> +	 * Fundamentally, this means we must pass the entire log vector to
> +	 * xlog_write to guarantee this.
> +	 */
> +	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.

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