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@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
