xfs
[Top] [All Lists]

Re: [PATCH 04/14] repair: split up scanfunc_ino

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 04/14] repair: split up scanfunc_ino
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Mon, 12 Oct 2009 15:06:40 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20090902175840.403232401@xxxxxxxxxxxxxxxxxxxxxx>
References: <20090902175531.469184575@xxxxxxxxxxxxxxxxxxxxxx> <20090902175840.403232401@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Thunderbird 2.0.0.23 (Macintosh/20090812)
Christoph Hellwig wrote:


Split out a helper to scan a single inode chunk for suspect inodes from
scanfunc_ino to make it more readable.


Signed-off-by: Barry Naujok <bnaujok@xxxxxxx>
Signed-off-by: Christoph Hellwig <hch@xxxxxx>

Index: xfsprogs-dev/repair/scan.c
===================================================================
--- xfsprogs-dev.orig/repair/scan.c     2009-08-21 19:00:15.000000000 +0000
+++ xfsprogs-dev/repair/scan.c  2009-08-21 19:03:26.000000000 +0000

...

+
+       /*
+        * set state of each block containing inodes
+        */
+       if (off == 0 && !suspect)  {
+               for (j = 0;
+                    j < XFS_INODES_PER_CHUNK;
+                    j += mp->m_sb.sb_inopblock)  {
+                       agbno = XFS_AGINO_TO_AGBNO(mp, ino + j);
+                       state = get_agbno_state(mp, agno, agbno);
+                       if (state == XR_E_UNKNOWN)  {
+                               set_agbno_state(mp, agno, agbno, XR_E_INO);
+                       } else if (state == XR_E_INUSE_FS && agno == 0 &&
+                                  ino + j >= first_prealloc_ino &&
+                                  ino + j < last_prealloc_ino)  {
+                               set_agbno_state(mp, agno, agbno, XR_E_INO);
+                       } else  {
+                               do_warn(
+_("inode chunk claims used block, inobt block - agno %d, bno %d, inopb %d\n"),
+                                       agno, agbno,
+                               mp->m_sb.sb_inopblock);

pretty weird indentation here can't you just merge w/ previous line?


Also is the change from bno to agbno intentional in the message?
I guess it's fine.

...


+               for (i = 0; i < numrecs; i++)
+                       suspect = scan_single_ino_chunk(agno, &rp[i], suspect);
if (suspect)
                        bad_ino_btree = 1;
It seems like it might be nicer to just do:


+               for (i = 0; i < numrecs; i++)
+                       suspect += scan_single_ino_chunk(agno, &rp[i]);

and let scan_single_ino_chunk return 0/1 instead of passing suspect in
and returning an incremented value?

Hm but I guess the sub-function tests it doesn't it:

+       /*
+        * set state of each block containing inodes
+        */
+       if (off == 0 && !suspect)  {

so, seems fine as-is, though I'd just fix that indentation.

Thanks,
-Eric


-Eric

<Prev in Thread] Current Thread [Next in Thread>