On Thu, Dec 02, 2010 at 01:01:32PM +1100, Dave Chinner wrote:
> 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...
I don't think obsfucating the code is a good idea to reach this goal.
What might be better is a helper like:
static inline void xlog_crack_lsn(atomic64_t *lsn, int *cycle, int *block)
{
xfs_lsn_t = atomic64_read(lsn);
*cycle = CYCLE_LSN(tail_lsn);
*block = BLOCK_LSN(tail_lsn);
}
and a long comment explaining how it needs to be used.
> 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.
Heh, when looking at the patch I actually found this part pretty hard
to read already. So moving the factoring of the helpers out into a
separate patch might indeed be a good idea, and that patch can also
remove the wrappers.
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
