Re: [PATCH v2] xfs_repair: Dump both inode details in Phase 6 duplicate file check

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Mar 25, 2024 at 06:34:43AM +0000, Srikanth C S wrote:
> The current check for duplicate names only dumps the inode number of the
> parent directory and the inode number of the actual inode in question.
> But, the inode number of original inode is not dumped. This patch dumps
> the original inode too.
> 
> xfs_repair output before applying this patch
> Phase 6 - check inode connectivity...
>         - traversing filesystem ...
> entry "dup-name1" (ino 132) in dir 128 is a duplicate name, would junk entry
> entry "dup-name1" (ino 133) in dir 128 is a duplicate name, would junk entry
> 
> After this patch
> Phase 6 - check inode connectivity...
>         - traversing filesystem ...
> entry "dup-name1" (ino 132) in dir 128 already points to ino 131, would junk entry
> entry "dup-name1" (ino 133) in dir 128 already points to ino 131, would junk entry
> 
> The entry_junked() function takes in only 4 arguments. In order to
> print the original inode number, modifying the function to take 5 parameters
> 
> Signed-off-by: Srikanth C S <srikanth.c.s@oracle.com>

Looks good to me now,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  repair/phase6.c | 51 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index 3870c5c9..3d5af436 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -151,9 +151,10 @@ dir_read_buf(
>  }
>  
>  /*
> - * Returns 0 if the name already exists (ie. a duplicate)
> + * Returns inode number of original file if the name already exists
> + * (ie. a duplicate)
>   */
> -static int
> +static xfs_ino_t
>  dir_hash_add(
>  	struct xfs_mount	*mp,
>  	struct dir_hash_tab	*hashtab,
> @@ -166,7 +167,7 @@ dir_hash_add(
>  	xfs_dahash_t		hash = 0;
>  	int			byhash = 0;
>  	struct dir_hash_ent	*p;
> -	int			dup;
> +	xfs_ino_t		dup_inum;
>  	short			junk;
>  	struct xfs_name		xname;
>  	int			error;
> @@ -176,7 +177,7 @@ dir_hash_add(
>  	xname.type = ftype;
>  
>  	junk = name[0] == '/';
> -	dup = 0;
> +	dup_inum = NULLFSINO;
>  
>  	if (!junk) {
>  		hash = libxfs_dir2_hashname(mp, &xname);
> @@ -188,7 +189,7 @@ dir_hash_add(
>  		for (p = hashtab->byhash[byhash]; p; p = p->nextbyhash) {
>  			if (p->hashval == hash && p->name.len == namelen) {
>  				if (memcmp(p->name.name, name, namelen) == 0) {
> -					dup = 1;
> +					dup_inum = p->inum;
>  					junk = 1;
>  					break;
>  				}
> @@ -234,7 +235,7 @@ dir_hash_add(
>  	p->name.name = p->namebuf;
>  	p->name.len = namelen;
>  	p->name.type = ftype;
> -	return !dup;
> +	return dup_inum;
>  }
>  
>  /* Mark an existing directory hashtable entry as junk. */
> @@ -1173,9 +1174,13 @@ entry_junked(
>  	const char 	*msg,
>  	const char	*iname,
>  	xfs_ino_t	ino1,
> -	xfs_ino_t	ino2)
> +	xfs_ino_t	ino2,
> +	xfs_ino_t	ino3)
>  {
> -	do_warn(msg, iname, ino1, ino2);
> +	if(ino3 != NULLFSINO)
> +		do_warn(msg, iname, ino1, ino2, ino3);
> +	else
> +		do_warn(msg, iname, ino1, ino2);
>  	if (!no_modify)
>  		do_warn(_("junking entry\n"));
>  	else
> @@ -1470,6 +1475,7 @@ longform_dir2_entry_check_data(
>  	int			i;
>  	int			ino_offset;
>  	xfs_ino_t		inum;
> +	xfs_ino_t		dup_inum;
>  	ino_tree_node_t		*irec;
>  	int			junkit;
>  	int			lastfree;
> @@ -1680,7 +1686,7 @@ longform_dir2_entry_check_data(
>  			nbad++;
>  			if (entry_junked(
>  	_("entry \"%s\" in directory inode %" PRIu64 " points to non-existent inode %" PRIu64 ", "),
> -					fname, ip->i_ino, inum)) {
> +					fname, ip->i_ino, inum, NULLFSINO)) {
>  				dep->name[0] = '/';
>  				libxfs_dir2_data_log_entry(&da, bp, dep);
>  			}
> @@ -1697,7 +1703,7 @@ longform_dir2_entry_check_data(
>  			nbad++;
>  			if (entry_junked(
>  	_("entry \"%s\" in directory inode %" PRIu64 " points to free inode %" PRIu64 ", "),
> -					fname, ip->i_ino, inum)) {
> +					fname, ip->i_ino, inum, NULLFSINO)) {
>  				dep->name[0] = '/';
>  				libxfs_dir2_data_log_entry(&da, bp, dep);
>  			}
> @@ -1715,7 +1721,7 @@ longform_dir2_entry_check_data(
>  				nbad++;
>  				if (entry_junked(
>  	_("%s (ino %" PRIu64 ") in root (%" PRIu64 ") is not a directory, "),
> -						ORPHANAGE, inum, ip->i_ino)) {
> +						ORPHANAGE, inum, ip->i_ino, NULLFSINO)) {
>  					dep->name[0] = '/';
>  					libxfs_dir2_data_log_entry(&da, bp, dep);
>  				}
> @@ -1732,12 +1738,13 @@ longform_dir2_entry_check_data(
>  		/*
>  		 * check for duplicate names in directory.
>  		 */
> -		if (!dir_hash_add(mp, hashtab, addr, inum, dep->namelen,
> -				dep->name, libxfs_dir2_data_get_ftype(mp, dep))) {
> +		dup_inum = dir_hash_add(mp, hashtab, addr, inum, dep->namelen,
> +				dep->name, libxfs_dir2_data_get_ftype(mp, dep));
> +		if (dup_inum != NULLFSINO) {
>  			nbad++;
>  			if (entry_junked(
> -	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name, "),
> -					fname, inum, ip->i_ino)) {
> +	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " already points to ino %" PRIu64 ", "),
> +					fname, inum, ip->i_ino, dup_inum)) {
>  				dep->name[0] = '/';
>  				libxfs_dir2_data_log_entry(&da, bp, dep);
>  			}
> @@ -1768,7 +1775,7 @@ longform_dir2_entry_check_data(
>  				nbad++;
>  				if (entry_junked(
>  	_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is not in the the first block, "), fname,
> -						inum, ip->i_ino)) {
> +						inum, ip->i_ino, NULLFSINO)) {
>  					dir_hash_junkit(hashtab, addr);
>  					dep->name[0] = '/';
>  					libxfs_dir2_data_log_entry(&da, bp, dep);
> @@ -1801,7 +1808,7 @@ longform_dir2_entry_check_data(
>  				nbad++;
>  				if (entry_junked(
>  	_("entry \"%s\" in dir %" PRIu64 " is not the first entry, "),
> -						fname, inum, ip->i_ino)) {
> +						fname, inum, ip->i_ino, NULLFSINO)) {
>  					dir_hash_junkit(hashtab, addr);
>  					dep->name[0] = '/';
>  					libxfs_dir2_data_log_entry(&da, bp, dep);
> @@ -2456,6 +2463,7 @@ shortform_dir2_entry_check(
>  {
>  	xfs_ino_t		lino;
>  	xfs_ino_t		parent;
> +	xfs_ino_t		dup_inum;
>  	struct xfs_dir2_sf_hdr	*sfp;
>  	struct xfs_dir2_sf_entry *sfep;
>  	struct xfs_dir2_sf_entry *next_sfep;
> @@ -2639,13 +2647,14 @@ shortform_dir2_entry_check(
>  		/*
>  		 * check for duplicate names in directory.
>  		 */
> -		if (!dir_hash_add(mp, hashtab, (xfs_dir2_dataptr_t)
> +		dup_inum = dir_hash_add(mp, hashtab, (xfs_dir2_dataptr_t)
>  				(sfep - xfs_dir2_sf_firstentry(sfp)),
>  				lino, sfep->namelen, sfep->name,
> -				libxfs_dir2_sf_get_ftype(mp, sfep))) {
> +				libxfs_dir2_sf_get_ftype(mp, sfep));
> +		if (dup_inum != NULLFSINO) {
>  			do_warn(
> -_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " is a duplicate name, "),
> -				fname, lino, ino);
> +_("entry \"%s\" (ino %" PRIu64 ") in dir %" PRIu64 " already points to ino %" PRIu64 ", "),
> +				fname, lino, ino, dup_inum);
>  			next_sfep = shortform_dir2_junk(mp, sfp, sfep, lino,
>  						&max_size, &i, &bytes_deleted,
>  						ino_dirty);
> -- 
> 2.25.1
> 
>