Re: [PATCH 1/1] XFS: xfs_iformat realtime device target pointer check

[Eric Sandeen <sandeen@sandeen.net>]

Ramon de Carvalho Valle wrote:
> On Tue, 2009-08-04 at 14:11 -0500, Eric Sandeen wrote:
>> Ramon de Carvalho Valle wrote:
>>> The xfs_iformat function does not check if the realtime device target pointer
>>> is valid when the XFS_DIFLAG_REALTIME flag is set on the ondisk inode
>>> structure.
>>>
>>> Signed-off-by: Christoph Hellwig <hch@infradead.org>
>>> Signed-off-by: Ramon de Carvalho Valle <ramon@risesecurity.org>
>>> Cc: stable <stable@kernel.org>
>>> ---
>>>  fs/xfs/xfs_inode.c |   23 +++++++++++++++++------
>>>  1 files changed, 17 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>>> index 1f22d65..37d3ee5 100644
>>> --- a/fs/xfs/xfs_inode.c
>>> +++ b/fs/xfs/xfs_inode.c
>>> @@ -343,13 +343,24 @@ xfs_iformat(
>>>  		return XFS_ERROR(EFSCORRUPTED);
>>>  	}
>>>  
>>> +	if (unlikely((ip->i_d.di_flags & XFS_DIFLAG_REALTIME) &&
>>> +			!ip->i_mount->m_rtdev_targp)) {
>>> +		xfs_fs_repair_cmn_err(CE_WARN, ip->i_mount,
>>> +			"corrupt dinode %Lu, flags = 0x%x.",
>>> +			(unsigned long long)ip->i_ino,
>>> +			ip->i_d.di_flags);
>>> +		XFS_CORRUPTION_ERROR("xfs_iformat(3)", XFS_ERRLEVEL_LOW,
>>> +				     ip->i_mount, dip);
>> I think I'd rather not change all the corruption text tag ordering;
>> it'll make it harder to track down any common occurrences of
>> "xfs_iformat(X)" corruption in the future if they get renumbered now.
>>
>> I'd either make this xfs_iformat(2.1) ;) or just leave it as Christoph
>> had.  "realtime" is a lot more informative than "3" anyway.
> 
> I don't think this is a bad decision, because the corruption errors can
> be easily identified by the output of xfs_fs_repair_cmn_err and the
> source line. I think this is a reasonable change that will keep the code
> clean and consistent.

Until you wind up looking at a problem from some old kernel, or modified
vendor kernel, and you realize that now you really don't know which
error "xfs_iformat(6)" is anymore, and you either have to go digging
through trees that aren't handy, or you just give up and don't bother to
help because now it's too much of a pain.  ;)

But I can leave it up to the folks @ sgi, I can see both sides of the
argument, and I won't care too much either way.

Thanks,
-Eric

> -Ramon
> 
>> -Eric