[PATCH 2/2] xfs_repair: Properly set lost+found inode as used

[PATCH 2/2] xfs_repair: Properly set lost+found inode as used

[Carlos Maiolino]

This patch makes mk_orphanage() to properly set the inode link count of
the recently allocated inode in the AVL tree, avoiding the lost+found
directory to be bypass the link count check in phase7 and possibly leaving
lost+found directory with a wrong link count.
---
 repair/phase6.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/repair/phase6.c b/repair/phase6.c
index adad61d..0e0e294 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -823,6 +823,8 @@ mk_orphanage(xfs_mount_t *mp)
 	xfs_inode_t	*ip;
 	xfs_inode_t	*pip;
 	xfs_fsblock_t	first;
+	ino_tree_node_t	*irec;
+	int		ino_offset = 0;
 	int		i;
 	int		committed;
 	int		error;
@@ -875,6 +877,18 @@ mk_orphanage(xfs_mount_t *mp)
 			ORPHANAGE, error);
 	}
 	ip->i_d.di_nlink++;		/* account for . */
+	ino = ip->i_ino;
+
+	irec = find_inode_rec(mp,
+			XFS_INO_TO_AGNO(mp, ino),
+			XFS_INO_TO_AGINO(mp,ino));
+	ino_offset = get_inode_offset(mp, ino, irec);
+
+	/* Set the inode allocated to lost+found as used in the AVL
+ 	 * tree, so it is not bypassed in phase 7
+ 	 */
+	set_inode_used(irec,ino_offset);
+	add_inode_ref(irec,ino_offset);
 
 	/*
 	 * now that we know the transaction will stay around,
@@ -902,6 +916,8 @@ mk_orphanage(xfs_mount_t *mp)
 				XFS_INO_TO_AGINO(mp, mp->m_sb.sb_rootino)), 0);
 
 
+	add_inode_reached(irec,ino_offset);
+
 	libxfs_trans_log_inode(tp, pip, XFS_ILOG_CORE);
 	libxfs_dir_init(tp, ip, pip);
 	libxfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
@@ -912,7 +928,6 @@ mk_orphanage(xfs_mount_t *mp)
 			ORPHANAGE, error);
 	}
 
-	ino = ip->i_ino;
 
 	libxfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC);
 
-- 
1.7.6.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_repair: Properly set lost+found inode as used

[Eric Sandeen]

On 11/8/11 12:46 PM, Carlos Maiolino wrote:
> This patch makes mk_orphanage() to properly set the inode link count of
> the recently allocated inode in the AVL tree, avoiding the lost+found
> directory to be bypass the link count check in phase7 and possibly leaving
> lost+found directory with a wrong link count.

Looks pretty good to me.  I think maybe add_inode_reached shouldn't
be called until the transaction to do it is complete, though?
Otherwise that might fail and we'd still have marked it as reached;
I think that could be a problem.

Thanks,
-Eric

> ---
>  repair/phase6.c |   17 ++++++++++++++++-
>  1 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index adad61d..0e0e294 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -823,6 +823,8 @@ mk_orphanage(xfs_mount_t *mp)
>  	xfs_inode_t	*ip;
>  	xfs_inode_t	*pip;
>  	xfs_fsblock_t	first;
> +	ino_tree_node_t	*irec;
> +	int		ino_offset = 0;
>  	int		i;
>  	int		committed;
>  	int		error;
> @@ -875,6 +877,18 @@ mk_orphanage(xfs_mount_t *mp)
>  			ORPHANAGE, error);
>  	}
>  	ip->i_d.di_nlink++;		/* account for . */
> +	ino = ip->i_ino;
> +
> +	irec = find_inode_rec(mp,
> +			XFS_INO_TO_AGNO(mp, ino),
> +			XFS_INO_TO_AGINO(mp,ino));
> +	ino_offset = get_inode_offset(mp, ino, irec);
> +
> +	/* Set the inode allocated to lost+found as used in the AVL
> + 	 * tree, so it is not bypassed in phase 7
> + 	 */
> +	set_inode_used(irec,ino_offset);
> +	add_inode_ref(irec,ino_offset);
>  
>  	/*
>  	 * now that we know the transaction will stay around,
> @@ -902,6 +916,8 @@ mk_orphanage(xfs_mount_t *mp)
>  				XFS_INO_TO_AGINO(mp, mp->m_sb.sb_rootino)), 0);
>  
>  
> +	add_inode_reached(irec,ino_offset);
> +
>  	libxfs_trans_log_inode(tp, pip, XFS_ILOG_CORE);
>  	libxfs_dir_init(tp, ip, pip);
>  	libxfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> @@ -912,7 +928,6 @@ mk_orphanage(xfs_mount_t *mp)
>  			ORPHANAGE, error);
>  	}
>  
> -	ino = ip->i_ino;
>  
>  	libxfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC);
>  

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_repair: Properly set lost+found inode as used

[Christoph Hellwig]

On Tue, Nov 08, 2011 at 04:46:30PM -0200, Carlos Maiolino wrote:
> This patch makes mk_orphanage() to properly set the inode link count of
> the recently allocated inode in the AVL tree, avoiding the lost+found
> directory to be bypass the link count check in phase7 and possibly leaving
> lost+found directory with a wrong link count.

Thanks a lot, this looks very good technically.

Unfortunately I have a few style nipicks anyway:

> @@ -875,6 +877,18 @@ mk_orphanage(xfs_mount_t *mp)
>  			ORPHANAGE, error);
>  	}
>  	ip->i_d.di_nlink++;		/* account for . */
> +	ino = ip->i_ino;
> +
> +	irec = find_inode_rec(mp,
> +			XFS_INO_TO_AGNO(mp, ino),
> +			XFS_INO_TO_AGINO(mp,ino));

Please always use whitespaces after the comma.

> +	ino_offset = get_inode_offset(mp, ino, irec);
> +
> +	/* Set the inode allocated to lost+found as used in the AVL
> + 	 * tree, so it is not bypassed in phase 7
> + 	 */

The canonical comment style would be:

	/*
	 * Mark the inode allocated to lost+found as used in the AVL tree
	 * so it is not skipped in phase 7.
	 */

(also note the slight change in wording to make it easier to parse for
 me, but I'm no native speaker either)
	 *

> +	set_inode_used(irec,ino_offset);
> +	add_inode_ref(irec,ino_offset);

Same whitespace comment as above here.

Feel free to add my:

Reviewed-by: Christoph Hellwig <hch@lst.de>

after these cosmetic fixups.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 0/2] Set lost+found inode as used after its allocation

[Carlos Maiolino]

This patch set ensure the inode allocated to the lost+found directory is
set as used in AVL tree if it needs to be created (in case no lost+found
directory is already created on the disk). Also, I added a new inline
function to get AVL tree node offset which currently is being calculated
whenever the offset is needed.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/2] xfs_repair: Add inline function to get avl tree node

[Carlos Maiolino]

Add get_inode_offset() inline function, which will return the offset
of a specific node in the AVL tree avoiding the need to calculate the
the offset each time it needs to be used.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
---
 repair/incore.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/repair/incore.h b/repair/incore.h
index ee0e86a..8e311c9 100644
--- a/repair/incore.h
+++ b/repair/incore.h
@@ -311,6 +311,12 @@ void		get_inode_rec(struct xfs_mount *mp, xfs_agnumber_t agno,
 			      ino_tree_node_t *ino_rec);
 
 extern avltree_desc_t     **inode_tree_ptrs;
+
+static inline int
+get_inode_offset(struct xfs_mount *mp, xfs_ino_t ino, ino_tree_node_t *irec)
+{
+	return XFS_INO_TO_AGINO(mp, ino) - irec->ino_startnum;
+}
 static inline ino_tree_node_t *
 findfirst_inode_rec(xfs_agnumber_t agno)
 {
-- 
1.7.6.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/2] xfs_repair: Properly set lost+found inode as used

[Carlos Maiolino]

This patch makes mk_orphanage() to properly set the inode link count of
the recently allocated inode in the AVL tree, avoiding the lost+found
directory to be bypass the link count check in phase7 and possibly leaving
lost+found directory with a wrong link count.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
---
 repair/phase6.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/repair/phase6.c b/repair/phase6.c
index adad61d..d38e44f 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -823,6 +823,8 @@ mk_orphanage(xfs_mount_t *mp)
 	xfs_inode_t	*ip;
 	xfs_inode_t	*pip;
 	xfs_fsblock_t	first;
+	ino_tree_node_t	*irec;
+	int		ino_offset = 0;
 	int		i;
 	int		committed;
 	int		error;
@@ -875,6 +877,19 @@ mk_orphanage(xfs_mount_t *mp)
 			ORPHANAGE, error);
 	}
 	ip->i_d.di_nlink++;		/* account for . */
+	ino = ip->i_ino;
+
+	irec = find_inode_rec(mp,
+			XFS_INO_TO_AGNO(mp, ino),
+			XFS_INO_TO_AGINO(mp, ino));
+	ino_offset = get_inode_offset(mp, ino, irec);
+
+	/*
+	 * Mark the inode allocated to lost+found as used in the AVL tree
+ 	 * so it is not skipped in phase 7
+ 	 */
+	set_inode_used(irec, ino_offset);
+	add_inode_ref(irec, ino_offset);
 
 	/*
 	 * now that we know the transaction will stay around,
@@ -902,6 +917,7 @@ mk_orphanage(xfs_mount_t *mp)
 				XFS_INO_TO_AGINO(mp, mp->m_sb.sb_rootino)), 0);
 
 
+
 	libxfs_trans_log_inode(tp, pip, XFS_ILOG_CORE);
 	libxfs_dir_init(tp, ip, pip);
 	libxfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
@@ -912,9 +928,9 @@ mk_orphanage(xfs_mount_t *mp)
 			ORPHANAGE, error);
 	}
 
-	ino = ip->i_ino;
 
 	libxfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC);
+	add_inode_reached(irec,ino_offset);
 
 	return(ino);
 }
-- 
1.7.6.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 0/2] Set lost+found inode as used after its allocation

[Christoph Hellwig]

Thanks a lot, I've put the patches in.

I'd love to see followups to use the the get_inode_offset helper more
often, and to add a testcase for the bug if possible.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 0/2] Set lost+found inode as used after its allocation

[Carlos Maiolino]

> I'd love to see followups to use the the get_inode_offset helper more
> often, and to add a testcase for the bug if possible.

Hi Chris, I added the xfstests test case creation to my todo list, 
I should have this implemented soon.

-- 
--Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_repair: Properly set lost+found inode as used

[Ben Myers]

Hey Carlos,

On Wed, Nov 09, 2011 at 02:54:07PM -0200, Carlos Maiolino wrote:
> This patch makes mk_orphanage() to properly set the inode link count of
> the recently allocated inode in the AVL tree, avoiding the lost+found
> directory to be bypass the link count check in phase7 and possibly leaving
> lost+found directory with a wrong link count.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  repair/phase6.c |   18 +++++++++++++++++-
>  1 files changed, 17 insertions(+), 1 deletions(-)
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index adad61d..d38e44f 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -823,6 +823,8 @@ mk_orphanage(xfs_mount_t *mp)
>  	xfs_inode_t	*ip;
>  	xfs_inode_t	*pip;
>  	xfs_fsblock_t	first;
> +	ino_tree_node_t	*irec;
> +	int		ino_offset = 0;
>  	int		i;
>  	int		committed;
>  	int		error;
> @@ -875,6 +877,19 @@ mk_orphanage(xfs_mount_t *mp)
>  			ORPHANAGE, error);
>  	}
>  	ip->i_d.di_nlink++;		/* account for . */
> +	ino = ip->i_ino;
> +
> +	irec = find_inode_rec(mp,
> +			XFS_INO_TO_AGNO(mp, ino),
> +			XFS_INO_TO_AGINO(mp, ino));
> +	ino_offset = get_inode_offset(mp, ino, irec);
> +
> +	/*
> +	 * Mark the inode allocated to lost+found as used in the AVL tree
> + 	 * so it is not skipped in phase 7
> + 	 */
> +	set_inode_used(irec, ino_offset);
> +	add_inode_ref(irec, ino_offset);

Both add_inode_ref()...

>  	/*
>  	 * now that we know the transaction will stay around,
> @@ -902,6 +917,7 @@ mk_orphanage(xfs_mount_t *mp)
>  				XFS_INO_TO_AGINO(mp, mp->m_sb.sb_rootino)), 0);
>  
>  
> +
>  	libxfs_trans_log_inode(tp, pip, XFS_ILOG_CORE);
>  	libxfs_dir_init(tp, ip, pip);
>  	libxfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> @@ -912,9 +928,9 @@ mk_orphanage(xfs_mount_t *mp)
>  			ORPHANAGE, error);
>  	}
>  
> -	ino = ip->i_ino;
>  
>  	libxfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC);
> +	add_inode_reached(irec,ino_offset);

and add_inode_reached() call counted_nlink_inc.

I have to admit to being an xfs_repair newbie, so you can tell me to
take a hike if you want.  Did you intend to bump up nlink twice?

THat's not what I would have expected.

THanks,
Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs