On Tue, 13 Oct 2009, Christoph Hellwig wrote:
However the way this is written is a bit confusing. Probably less
confusing than some of the surrounding code, but it adds up.
We calculate the offset for one case above, and then after we're done
with the reading that never touches offset we calculate it if we don't
So this at very least needs a big comment describing it, or even better
moving up the xlog_align call so that we can simply always use
offset directly, which could also get rid of the now superflous bufaddr
variable by always using offset.
Exactly the same also applies for the second use.
Anyway, thanks a lot again and if you can fix this up I'd welcome it,
otherwise I'll do it once I get some time.
Agreed. I aimed for the smallest possible patch, thinking it might increase
the likelihood of acceptance. :-)
I think the complexity here stems from an uncertainty (as we prepare for the
"second" read) whether there was a first read or not. As the code reads
today, if there is data before the end, the first read is done and offset has
been set. If not, offset is NULL.
It seems like the more elegant approach would be to set offset before the
first read, and then update it if the first read takes place (in case it was
unaligned). That also gets rid of bufaddr, and seems like it might read
I'll try that and see how it goes.
I suspect we might even be able to come up with a small enough testcase
for this for xfsqa, we just need to make sure logs are aligned and then
find a good enough heuristic to reproduce log wrapping.
I don't know if it would be appropriate in that context, but maybe a tool to
process an unwrapped log and wrap it would be easier?
It ain't what you don't know that gets you into trouble.
It's what you know for sure that just ain't so. - Mark Twain