fix attr fit checking for filesystems which have lost their attr2

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.

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:

Quoted 3 lines

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,

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...

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....

------------------

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 <[email protected]>

---

Index: linux-2.6-git/fs/xfs/xfs_attr_leaf.c
===================================================================

fs/xfs/xfs_attr_leaf.c +1 -1

@@ -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;
 	}
 

Hi Eric,

Eric Sandeen wrote:

Quoted 4 lines

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.

Hi Eric,

Quoted 8 lines · Eric Sandeen

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.

Quoted 8 lines

However, hch came along in 2.6.24 and did some endian annotation for the superblock and in the process:

Quoted 3 lines

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 ;-)

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

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....

Quoted 15 lines

------------------

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 <[email protected]>

---

fs/xfs/xfs_attr_leaf.c xfs_attr_shortform_bytesfit(xfs_inode_t · line 171

Quoted 12 lines

Index: linux-2.6-git/fs/xfs/xfs_attr_leaf.c
===================================================================

fs/xfs/xfs_attr_leaf.c +1 -1

@@ -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;
     }
 

Quoted 34 lines · Timothy Shimmin

Hi Eric,

Quoted 8 lines · Eric Sandeen

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.

Quoted 8 lines

However, hch came along in 2.6.24 and did some endian annotation for the superblock and in the process:

Quoted 3 lines

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 ;-)

Quoted 8 lines

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.

Quoted 4 lines

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...

Quoted 14 lines

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.

Quoted 6 lines

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.

Eric,

So probably need something like this for xfs_bmap_compute_maxlevels()...

--Tim

===========================================================================
Index: fs/xfs/xfs_bmap.c
===========================================================================

fs/xfs/xfs_bmap.c +10 -5

@@ -4154,16 +4154,21 @@ xfs_bmap_compute_maxlevels(
  	 * number of leaf entries, is controlled by the type of di_nextents
  	 * (a signed 32-bit number, xfs_extnum_t), or by di_anextents
  	 * (a signed 16-bit number, xfs_aextnum_t).
+	 *
+	 * Note that we can no longer assume that if we are in ATTR1 that
+	 * the fork offset of all the inodes will be (m_attroffset >> 3)
+	 * because we could have mounted with ATTR2 and then mounted back
+	 * with ATTR1, keeping the di_forkoff's fixed but probably at
+	 * various positions. Therefore, for both ATTR1 and ATTR2
+	 * we have to assume the worst case scenario of a minimum size
+	 * available.
  	 */
  	if (whichfork == XFS_DATA_FORK) {
  		maxleafents = MAXEXTNUM;
-		sz = (mp->m_flags & XFS_MOUNT_ATTR2) ?
-			XFS_BMDR_SPACE_CALC(MINDBTPTRS) : mp->m_attroffset;
+		sz = XFS_BMDR_SPACE_CALC(MINDBTPTRS);
  	} else {
  		maxleafents = MAXAEXTNUM;
-		sz = (mp->m_flags & XFS_MOUNT_ATTR2) ?
-			XFS_BMDR_SPACE_CALC(MINABTPTRS) :
-			mp->m_sb.sb_inodesize - mp->m_attroffset;
+		sz = XFS_BMDR_SPACE_CALC(MINABTPTRS);
  	}
  	maxrootrecs = (int)XFS_BTREE_BLOCK_MAXRECS(sz, xfs_bmdr, 0);
  	minleafrecs = mp->m_bmap_dmnr[0];