View Incident:
http://co-op.engr.sgi.com/BugWorks/code/bwxquery.cgi?search=Search&wlong=1&view_type=Bug&wi=801066
Submitter : lord Submitter Domain : sgi.com
Assigned Engineer : dxm Assigned Domain : engr
Assigned Group : xfs-linux Category : software
Customer Reported : F Priority : 3
Project : xfs-linux Status : open
Description :
Capturing some old email state into a PV
I was digging through the code trying to figure out why the power pc is getting
zero inode numbers and I came across this code in xfs_inobt_get_rec
INT_SET(*ino, arch, INT_GET(rec->ir_startino, ARCH_CONVERT));
INT_SET(*fcnt, arch, INT_GET(rec->ir_freecount, ARCH_CONVERT));
INT_SET(*free, arch, INT_GET(rec->ir_free, ARCH_CONVERT));
Places like xfs_dialloc are calling xfs_inobt_get_rec() several times with
ARCH_CONVERT in the arguments, and the proceeding to always use statements
like INT_GET(rec.ir_freecount, ARCH_CONVERT) to extract fields from the
results. I think we could just skip all the conversion in this case, in
fact I was hard pressed to find a call to xfs_inobt_get_rec where the output
was ever used as metadata, it always appeared to be a local variable which
was always converted again before being used. xfs_difree appears to go through
even more gyrations.
Am I missing something, or are we just byte swapping for the fun of it in
these functions?
=> Yes, I think so. Daniel knows more about this than I, but I think
=> this may have been left in so we knew all the places that had been
=> touched by endian work in case there was further work to be done
=> later (i.e. if we didn't go the non-bigendian-only route).
=>
=> We should go through and remove all these (I think) - Daniel has
=> scripts for finding this sort of thing, so I'll leave it for him
=> to qualify further/fix. He's also not in this morning, so it may
=> be a little while.
Well not for fun as such - more out of force of habit. A lot of
these cases exist because the code was converted automagically,
and a lot because the sizes of the types is non-obvious.
I actually wrote another script to remove all the
INT_SET(x, ARCH_CONVERT, INT_GET(y, ARCH_CONVERT))
from the code, but this is actually quite dangerous because the
macros _are_ needed if x & y are different sized types. We
probably need is to make a new macro which copies disk-endian
values between variables, doing the conversions if the sizes
are different.
|