[lkml]   [2009]   [Feb]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    Patch in this message
    SubjectRe: [RESEND/PATCH] ext[234]: Return -EIO not -ESTALE on directory traversal missing inode
    On Sat, Feb 14, 2009 at 12:18:08AM -0500, Bryan Donlan wrote:
    > The ext[234]_iget() functions in the ext[234] family of filesystems returns
    > -ESTALE if invoked on a deleted inode, in order to report errors to NFS
    > properly. However, in ext[234]_lookup(), this -ESTALE can be propagated to
    > userspace if the filesystem is corrupted, and a inode is linked even
    > though it is marked as deleted.

    I'm not sure what you mean by "inode is linked" here. All you are
    really proposing to do here is to remap the ESTALE error to EIO, yes?

    > Apologies for the resend so quickly for those on the CC list - my mailer was
    > misconfigured and the mail rejected by vger.

    RESEND is a code word meaning --- "I sent this a few days/weeks ago
    and no one responded; please look at it again?" I was confused when I
    saw this, because I didn't remember seeing this, either in my inbox or
    in the ext4 patchwork (which is really great and helping me make sure
    I don't miss patches), so I spent a few extra minutes googling to see
    what had gotten missed until I finally got to your apologies section....

    I received two copies with the RESEND keyword; so I guess that means
    you ended up sending it three times? Anyway, apparently the first
    time you sent it your mailer was so badly misconfigured that it got
    dropped by's mailer as well. :-) In general if you end up
    resending, don't worry about flagging it as a resend; deleting
    duplicates is easy enough to do.

    > --- a/fs/ext2/namei.c
    > +++ b/fs/ext2/namei.c
    > @@ -66,8 +66,12 @@ static struct dentry *ext2_lookup(struct inode * dir, struct dentry *dentry, str
    > inode = NULL;
    > if (ino) {
    > inode = ext2_iget(dir->i_sb, ino);
    > - if (IS_ERR(inode))
    > - return ERR_CAST(inode);
    > + if (unlikely(IS_ERR(inode))) {

    I'm dubious about unlikely() here; OTOH, penalizing the error case
    seems reasonable.

    > + if (PTR_ERR(inode) == -ESTALE)

    Please add a call to ext2_error() here, and in make a similar change
    for the ext3 and ext4 patches:

    ext2_error(dir->i_sb, "ext2_lockup",
    "deleted inode referenced: %u",

    > + return ERR_PTR(-EIO);
    > + else
    > + return ERR_CAST(inode);
    > + }
    > }
    > return d_splice_alias(inode, dentry);
    > }

    - Ted

     \ /
      Last update: 2009-02-14 16:21    [W:0.022 / U:0.828 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site