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

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

[Goldwyn Rodrigues]

Introduces ocfs2_dx_disable() to disable the index of a directory.
This function is called when a index directory is encountered.
Calling function must try and revert to regular directory handling,
instead of using indexes, in order to complete the operation.


Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.de>
---
 fs/ocfs2/dir.c |   44 +++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c
index 8582e3f..3a6f6e4 100644
--- a/fs/ocfs2/dir.c
+++ b/fs/ocfs2/dir.c
@@ -605,7 +605,7 @@ static int ocfs2_validate_dx_root(struct super_block *sb,
 	}

 	if (!OCFS2_IS_VALID_DX_ROOT(dx_root)) {
-		ocfs2_error(sb,
+		mlog(ML_ERROR,
 			    "Dir Index Root # %llu has bad signature %.*s",
 			    (unsigned long long)le64_to_cpu(dx_root->dr_blkno),
 			    7, dx_root->dr_signature);
@@ -649,9 +649,9 @@ static int ocfs2_validate_dx_leaf(struct super_block *sb,
 	}

 	if (!OCFS2_IS_VALID_DX_LEAF(dx_leaf)) {
-		ocfs2_error(sb, "Dir Index Leaf has bad signature %.*s",
-			    7, dx_leaf->dl_signature);
-		return -EROFS;
+		mlog(ML_ERROR, "Dir Index Leaf has bad signature %.*s",
+				7, dx_leaf->dl_signature);
+		return -EINVAL;
 	}

 	return 0;
@@ -1070,6 +1070,34 @@ out:
 	return ret;
 }

+/* Called only when an error occurs with reading indexed directories.
+ * The function just disables the directory indexing, and does not try
+ * delete it because it might be a corruption and the blocks useful to
+ * other structures.
+ *
+ * Must request user to run fsck.
+ */
+
+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);
+	oi->ip_dyn_features &= ~OCFS2_INDEXED_DIR_FL;
+	di->i_dyn_features = cpu_to_le16(OCFS2_I(dir)->ip_dyn_features);
+	di->i_dx_root = cpu_to_le64(0ULL);
+}
+
+
 /*
  * Try to find an entry of the provided name within 'dir'.
  *
@@ -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);
 		goto out;
 	}

+no_index:
 	if (OCFS2_I(dir)->ip_dyn_features & OCFS2_INLINE_DATA_FL) {
 		ret = ocfs2_find_dir_space_id(dir, parent_fe_bh, name,
 					      namelen, &bh, &blocks_wanted);
-- 
1.7.5.3


-- 
Goldwyn

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

[Joel Becker]

On Wed, Jun 15, 2011 at 10:57:54AM -0500, Goldwyn Rodrigues wrote:
> Introduces ocfs2_dx_disable() to disable the index of a directory.
> This function is called when a index directory is encountered.
> Calling function must try and revert to regular directory handling,
> instead of using indexes, in order to complete the operation.
> 
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.de>

Hey Goldwyn,
	I like the idea of this.  I have a few comments.

> @@ -649,9 +649,9 @@ static int ocfs2_validate_dx_leaf(struct super_block *sb,
>  	}
> 
>  	if (!OCFS2_IS_VALID_DX_LEAF(dx_leaf)) {
> -		ocfs2_error(sb, "Dir Index Leaf has bad signature %.*s",
> -			    7, dx_leaf->dl_signature);
> -		return -EROFS;
> +		mlog(ML_ERROR, "Dir Index Leaf has bad signature %.*s",
> +				7, dx_leaf->dl_signature);
> +		return -EINVAL;

	The meta_ecc validation functions return -EIO.  Perhaps we
should do the same?  -EIO signifies an attempt to continue.

> +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.

> +	oi->ip_dyn_features &= ~OCFS2_INDEXED_DIR_FL;
> +	di->i_dyn_features = cpu_to_le16(OCFS2_I(dir)->ip_dyn_features);
> +	di->i_dx_root = cpu_to_le64(0ULL);
> +}
> +
> +
>  /*
>   * Try to find an entry of the provided name within 'dir'.
>   *
> @@ -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.

Joel

-- 

"Also, all of life's big problems include the words 'indictment' or
	'inoperable.' Everything else is small stuff."
	- Alton Brown

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

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

[Goldwyn Rodrigues]

Hi Joel,

Thanks for the review.

On Thu, Jul 28, 2011 at 4:22 AM, Joel Becker <jlbec@evilplan.org> wrote:
> On Wed, Jun 15, 2011 at 10:57:54AM -0500, Goldwyn Rodrigues wrote:
>> Introduces ocfs2_dx_disable() to disable the index of a directory.
>> This function is called when a index directory is encountered.
>> Calling function must try and revert to regular directory handling,
>> instead of using indexes, in order to complete the operation.
>>
>>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.de>
>
> Hey Goldwyn,
> ? ? ? ?I like the idea of this. ?I have a few comments.
>
>> @@ -649,9 +649,9 @@ static int ocfs2_validate_dx_leaf(struct super_block *sb,
>> ? ? ? }
>>
>> ? ? ? if (!OCFS2_IS_VALID_DX_LEAF(dx_leaf)) {
>> - ? ? ? ? ? ? ocfs2_error(sb, "Dir Index Leaf has bad signature %.*s",
>> - ? ? ? ? ? ? ? ? ? ? ? ? 7, dx_leaf->dl_signature);
>> - ? ? ? ? ? ? return -EROFS;
>> + ? ? ? ? ? ? mlog(ML_ERROR, "Dir Index Leaf has bad signature %.*s",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? 7, dx_leaf->dl_signature);
>> + ? ? ? ? ? ? return -EINVAL;
>
> ? ? ? ?The meta_ecc validation functions return -EIO. ?Perhaps we
> should do the same? ?-EIO signifies an attempt to continue.
>

ok

>> +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?

>
>> + ? ? oi->ip_dyn_features &= ~OCFS2_INDEXED_DIR_FL;
>> + ? ? di->i_dyn_features = cpu_to_le16(OCFS2_I(dir)->ip_dyn_features);
>> + ? ? di->i_dx_root = cpu_to_le64(0ULL);
>> +}
>> +
>> +
>> ?/*
>> ? * Try to find an entry of the provided name within 'dir'.
>> ? *
>> @@ -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.


-- 
Goldwyn

[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