Timothy Shimmin wrote:
Hi Eric,
Eric Sandeen wrote:
Regarding the F8 corruption... I have a pretty narrow testcase
now, and it turns out this was a bit of a perfect storm.
First, F8 shipped 2.6.23 which had the problem with sb_features2
padding out on 64-bit boxes, but this was ok because userspace
and kernelspace both did this, and it was properly mounting &
running as attr2.
However, hch came along in 2.6.24 and did some endian annotation for
the superblock and in the process:
A new helper xfs_sb_from_disk handles the other (read)
direction and doesn't need the slightly hacky table-driven approach
because we only ever read the full sb from disk.
However, this resulted in kernelspace behaving differently,
and now *missing* the attr2 flag in sb_features2, (actually
sb_bad_features2) so we mounted as if we had attr1. Which is really
supposed to be ok, IIRC,
Yes, I remember Nathan saying that too ;-)
except in xfs_attr_shortform_bytesfit we return the default
fork offset value from the superblock, even if di_forkoff
is *already* set. In the error case I had, di_forkoff was set
to 15 (from previously being attr2...) but we returned 14
(the mp default) and I think this is where things started
going wrong; I think this caused us to write an attr on top
of the extent data.
My understanding of this is that if di_forkoff is non-zero,
we should always be using it for space calculations, regardless
of whether we are mounted with attr2 or not...
That was my understanding as well.
I'll have a look at the code soon and see if I can
see any problems with the change and the consistency
of it all.
Thanks a bunch,
Tim.
and with that, how's this look, to be honest I haven't run it
through QA yet...
I'm not certain if xfs_bmap_compute_maxlevels() may lead
to similar problems....
Yes, I think it needs to be changed too.
Looking at the code, it is needed for the log reservation stuff
where we want to know the size of the space for an EA btree root
or a data fork extents root, to aid in working out the maximum
btree level, to work out maximum log space for some transactions.
So underestimating it is dangerous and overestimating is
wasteful of logspace.
It is basing on the same assumption that if we are not mounted
attr2 then it thinks we are using m_attroffset when we could
be using di_forkoff.
However, if di_forkoff is zero then we should use m_attroffset.
The other places which referenced m_attroffset look fine to me too.
------------------
always use di_forkoff for when checking for attr space
In the case where we mount a filesystem which was previously
using the attr2 format as attr1, returning the default
mp->m_attroffset instead of the per-inode di_forkoff for
inline attribute fit calculations may result in corruption,
if the existing "attr2" formatted attribute is already taking
more space than the default.
Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxxx>
---
Index: linux-2.6-git/fs/xfs/xfs_attr_leaf.c
===================================================================
--- linux-2.6-git.orig/fs/xfs/xfs_attr_leaf.c
+++ linux-2.6-git/fs/xfs/xfs_attr_leaf.c
@@ -166,7 +166,7 @@ xfs_attr_shortform_bytesfit(xfs_inode_t
if (!(mp->m_flags & XFS_MOUNT_ATTR2)) {
if (bytes <= XFS_IFORK_ASIZE(dp))
- return mp->m_attroffset >> 3;
+ return dp->i_d.di_forkoff;
return 0;
}
Okay, and XFS_IFORK_ASIZE(dp) looks at di_forkoff and if non-zero
then returns literal-size - di_forkoff.
Right, so it will only fit if di_forkoff is operational.
Cool.
--Tim
|