xfs
[Top] [All Lists]

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

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH 1/1] XFS: xfs_iformat realtime device target pointer check
From: Felix Blyakher <felixb@xxxxxxx>
Date: Wed, 5 Aug 2009 16:53:40 -0500
Cc: Ramon de Carvalho Valle <ramon@xxxxxxxxxxxxxxxx>, Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs mailing list <xfs@xxxxxxxxxxx>, Christoph Hellwig <hch@xxxxxx>, Linux Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>, mszeredi@xxxxxxxxxx
In-reply-to: <4A790762.3010207@xxxxxxxxxxx>
References: <1249329808.7686.26.camel@logos> <20090803214929.GB3167@xxxxxxxxxxxxx> <1249351241.7513.18.camel@logos> <4A786110.8090404@xxxxxxxxxxx> <1249411898.7576.19.camel@logos> <4A7887CE.2060300@xxxxxxxxxxx> <1249444550.7372.23.camel@logos> <4A790762.3010207@xxxxxxxxxxx>

On Aug 4, 2009, at 11:15 PM, Eric Sandeen wrote:

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@xxxxxxxxxxxxx>
Signed-off-by: Ramon de Carvalho Valle <ramon@xxxxxxxxxxxxxxxx>
Cc: stable <stable@xxxxxxxxxx>
---
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.

Agree with Eric, see the benefits of both approaches, but I think,
it'll be cleaner without shifting the numbering of all messages.
Otherwise, looks good.

Felix



Thanks,
-Eric

-Ramon

-Eric


--
To unsubscribe from this list: send the line "unsubscribe linux- kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

<Prev in Thread] Current Thread [Next in Thread>