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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
> Sent: 23 March 2024 12:39 AM
> To: Srikanth C S <srikanth.c.s@xxxxxxxxxx>
> Cc: linux-xfs@xxxxxxxxxxxxxxx; cem@xxxxxxxxxx; Rajesh
> Sivaramasubramaniom <rajesh.sivaramasubramaniom@xxxxxxxxxx>
> Subject: [External] : Re: [PATCH] xfs_repair: Dump both inode details in
> Phase 6 duplicate file check
> 
> On Fri, Mar 22, 2024 at 04:45:12AM +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@xxxxxxxxxx>
> > ---
> >  repair/phase6.c | 51
> > +++++++++++++++++++++++++++++--------------------
> >  1 file changed, 30 insertions(+), 21 deletions(-)
> >
> > diff --git a/repair/phase6.c b/repair/phase6.c index
> > 3870c5c9..148454d0 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 = 0;
> >
> >  	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)
> 
> Hmm.  Seeing as we have a symbol (NULLFSINO) for null values, perhaps this
> should be:
> 
> 	if (ino3 != NULLFSINO)
> 		do_warn(msg, iname, ino1, ino2, ino3);
> 
> Otherwise looks fine to me....
> 
> --D

Yes, using the symbol NULLFSINO makes more sense. Will send out a V2.

Thanks,
Srikanth

> 
> > +		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, 0)) {
> >  				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, 0)) {
> >  				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, 0)) {
> >  					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) {
> >  			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, 0)) {
> >  					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, 0)) {
> >  					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) {
> >  			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
> >
> >





[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux