Re: [PATCH 03/14] xfs: convert log grant heads to LSN notation

[Dave Chinner <david@fromorbit.com>]

On Wed, Dec 01, 2010 at 08:05:04AM -0500, Christoph Hellwig wrote:
> > -STATIC int	 xlog_space_left(xlog_t *log, int cycle, int bytes);
> > +STATIC int	 xlog_space_left(int logsize, xfs_lsn_t tail_lsn,
> > +				xfs_lsn_t head);
> 
> Looking further through the series I have to say I really hate
> passing in the logsize instead of the log structure.  Passing the
> log pointer from higher level functions just makes a lot more sense.
> 
> Also in this case passing the tail_lsn explicitly doesn't make any sense
> - it becomes atomic later and thus there is no locking requirement for
> it.

What I wanted to make clear is that the calculation works on fixed
values and doesn't sample values internally itself. I guess that's
not important for the log size, but for stuff like the tail lsn
it avoids  needing to sample inside xlog_space_left() before we
crack it. i.e. something like this is wrong:

	cycle = CYCLE_LSN(atomic64_read(&log->l_tail_lsn));
	block = BLOCK_LSN(atomic64_read(&log->l_tail_lsn));

and this is correct:

	tail_lsn = atomic64_read(&log->l_tail_lsn);
	cycle = CYCLE_LSN(tail_lsn);
	block = BLOCK_LSN(tail_lsn);

So it makes sense to me to have the value of of the tail lsn and
other variables that should only be sampled once passed into the
function. That avoids misunderstandings further down the track
because it is obvious that the calculation works on constant values.
Perhaps I should add "const" to the parameter declarations to help
make my intentions clear...

> 
> > -xlog_grant_sub_space(struct log *log, int bytes)
> > +__xlog_grant_sub_space(
> > +	xfs_lsn_t	*head,
> > +	int		bytes,
> > +	int		logsize)
> 
> Same thing here - the calling convention would be a lot more regular
> by still passing the log as first argument.

Ok, it's only for the logsize again, but that's not the important
part of the calculation...

> After the factoring I also think we could cut down on the amount of
> wrappers in this area.  Just rename __xlog_grant_sub_space and
> __xlog_grant_add_space to not have the __ prefix, and kill the wrappers
> around it - they only have one or two callers, and just having two
> helpers and passing the heads we're interested in is a lot simpler and
> cleaner.

I thought about that - my first version even did this. I thought it
was easier to understand the changes if I didn't change the calling
conventions for modifying the grant heads. As such, I'd prefer to
make this change to the wrappers in a separate patch.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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