Re: [PATCH, RFC] fix attr fit checking for filesystems which have lost their attr2

[Timothy Shimmin <tes@sgi.com>]

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@sandeen.net>
>> ---
>>
>>
>> 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