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: Ramon de Carvalho Valle <ramon@xxxxxxxxxxxxxxxx>
Date: Wed, 05 Aug 2009 00:55:50 -0300
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx, hch@xxxxxx, linux-kernel@xxxxxxxxxxxxxxx, mszeredi@xxxxxxxxxx
In-reply-to: <4A7887CE.2060300@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>
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.

-Ramon

> 
> -Eric
> 
> > +           return XFS_ERROR(EFSCORRUPTED);
> > +   }
> > +
> >     switch (ip->i_d.di_mode & S_IFMT) {
> >     case S_IFIFO:
> >     case S_IFCHR:
> >     case S_IFBLK:
> >     case S_IFSOCK:
> >             if (unlikely(dip->di_format != XFS_DINODE_FMT_DEV)) {
> > -                   XFS_CORRUPTION_ERROR("xfs_iformat(3)", XFS_ERRLEVEL_LOW,
> > +                   XFS_CORRUPTION_ERROR("xfs_iformat(4)", XFS_ERRLEVEL_LOW,
> >                                           ip->i_mount, dip);
> >                     return XFS_ERROR(EFSCORRUPTED);
> >             }
> > @@ -371,7 +382,7 @@ xfs_iformat(
> >                                     "corrupt inode %Lu "
> >                                     "(local format for regular file).",
> >                                     (unsigned long long) ip->i_ino);
> > -                           XFS_CORRUPTION_ERROR("xfs_iformat(4)",
> > +                           XFS_CORRUPTION_ERROR("xfs_iformat(5)",
> >                                                  XFS_ERRLEVEL_LOW,
> >                                                  ip->i_mount, dip);
> >                             return XFS_ERROR(EFSCORRUPTED);
> > @@ -384,7 +395,7 @@ xfs_iformat(
> >                                     "(bad size %Ld for local inode).",
> >                                     (unsigned long long) ip->i_ino,
> >                                     (long long) di_size);
> > -                           XFS_CORRUPTION_ERROR("xfs_iformat(5)",
> > +                           XFS_CORRUPTION_ERROR("xfs_iformat(6)",
> >                                                  XFS_ERRLEVEL_LOW,
> >                                                  ip->i_mount, dip);
> >                             return XFS_ERROR(EFSCORRUPTED);
> > @@ -400,14 +411,14 @@ xfs_iformat(
> >                     error = xfs_iformat_btree(ip, dip, XFS_DATA_FORK);
> >                     break;
> >             default:
> > -                   XFS_ERROR_REPORT("xfs_iformat(6)", XFS_ERRLEVEL_LOW,
> > +                   XFS_ERROR_REPORT("xfs_iformat(7)", XFS_ERRLEVEL_LOW,
> >                                      ip->i_mount);
> >                     return XFS_ERROR(EFSCORRUPTED);
> >             }
> >             break;
> >  
> >     default:
> > -           XFS_ERROR_REPORT("xfs_iformat(7)", XFS_ERRLEVEL_LOW, 
> > ip->i_mount);
> > +           XFS_ERROR_REPORT("xfs_iformat(8)", XFS_ERRLEVEL_LOW, 
> > ip->i_mount);
> >             return XFS_ERROR(EFSCORRUPTED);
> >     }
> >     if (error) {
> > @@ -430,7 +441,7 @@ xfs_iformat(
> >                             "(bad attr fork size %Ld).",
> >                             (unsigned long long) ip->i_ino,
> >                             (long long) size);
> > -                   XFS_CORRUPTION_ERROR("xfs_iformat(8)",
> > +                   XFS_CORRUPTION_ERROR("xfs_iformat(9)",
> >                                          XFS_ERRLEVEL_LOW,
> >                                          ip->i_mount, dip);
> >                     return XFS_ERROR(EFSCORRUPTED);
> 

Attachment: signature.asc
Description: This is a digitally signed message part

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