[Ocfs2-devel] [PATCH 1/2] Disable indexing for directory with corrupt index

[Joel Becker]

On Thu, Jul 28, 2011 at 06:43:42PM -0500, Goldwyn Rodrigues wrote:
> Hi Joel,
> 
> Thanks for the review.

	No problem.  Sorry I took so long to get back to you.

> >> +static void ocfs2_dx_disable(struct inode *dir, struct buffer_head *di_bh)
> >> +{
> >> +     struct ocfs2_inode_info *oi = OCFS2_I(dir);
> >> +     struct ocfs2_dinode *di;
> >> +     if (!di_bh) {
> >> +             mlog(ML_ERROR, "Index directory corruption but unable "
> >> +                     "to disable dx_dir for <%llu>. Please execute "
> >> +                     "fsck.ocfs2\n", OCFS2_I(dir)->ip_blkno);
> >> +             return;
> >> +     }
> >> +     di = (struct ocfs2_dinode *)di_bh->b_data;
> >> +     BUG_ON(di->i_blkno != oi->ip_blkno);
> >> +     mlog(ML_ERROR, "Disabling index for directory <%llu> due to"
> >> +             " corruption. Please execute fsck.ocfs2\n", oi->ip_blkno);
> >
> >        Please match the indentation to the open parenthesis.
> 
> You mean quotes, right?

	No, I mean parenthesis.  Both are acceptible, though.  I'd
probably do:

  mlog(ML_ERROR,
       "Disabling ..."
       " corruption...",
       oi->ip_blkno);

It's mostly a matter of taste.  The only wrong answer is an indentation
that doesn't line up with anything ;-)

> >> @@ -4364,11 +4392,17 @@ int ocfs2_prepare_dir_for_insert(struct
> >> ocfs2_super *osb,
> >>       if (ocfs2_dir_indexed(dir)) {
> >>               ret = ocfs2_prepare_dx_dir_for_insert(dir, parent_fe_bh,
> >>                                                     name, namelen, lookup);
> >> -             if (ret)
> >> +             if (ret == -EINVAL) {
> >> +                     ocfs2_dx_disable(dir, parent_fe_bh);
> >> +                     /* Reset ret */
> >> +                     ret = 0;
> >> +                     goto no_index;
> >> +             } else if (ret)
> >>                       mlog_errno(ret);
> >
> >        See here, you'll disable the index on a bad signature, but you
> > error when ECC fails.  You should disable the index on both cases.  I
> > say you return -EIO as I described above, and then disable when ret ==
> > -EIO.  Yes, we might disable the index upon an actual read error, but
> > that's OK.  We can rebuild it later.
> >
> 
> I understand why I disable index of a bad signature, but I fail to
> understand why I will err of ECC errors. In any case, I will change
> this to EIO. I chose EINVAL just to make it different from read
> errors. It does make sense to encompass read errors as well.

	Since ECC error returns -EIO, you'll trigger the mlog_errno(ret)
path and reject continuing.  Conversely, if you disable, the directory
can continue to work in non-indexed mode.  The same is true of read
errors for the index; disabling the index allows the directory to
continue to work.

Joel

-- 

 Joel's Second Law:

	If a code change requires additional user setup, it is wrong.

			http://www.jlbec.org/
			jlbec at evilplan.org