| Message ID | 20230606151122.853315-1-cem@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] xfs: Comment out unreachable code within xchk_fscounters() | expand |
On Tue, Jun 06, 2023 at 05:11:22PM +0200, cem@kernel.org wrote: > From: Carlos Maiolino <cem@kernel.org> > > Comment the code out so kernel test robot stop complaining about it > every single test build. > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> Thank you, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/scrub/fscounters.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/fs/xfs/scrub/fscounters.c b/fs/xfs/scrub/fscounters.c > index e382a35e98d88..228efe0c99be8 100644 > --- a/fs/xfs/scrub/fscounters.c > +++ b/fs/xfs/scrub/fscounters.c > @@ -153,6 +153,7 @@ xchk_setup_fscounters( > return xchk_trans_alloc(sc, 0); > } > > +#if 0 > /* > * Part 1: Collecting filesystem summary counts. For each AG, we add its > * summary counts (total inodes, free inodes, free data blocks) to an incore > @@ -349,6 +350,7 @@ xchk_fscount_count_frextents( > return 0; > } > #endif /* CONFIG_XFS_RT */ > +#endif > > /* > * Part 2: Comparing filesystem summary counters. All we have to do here is > @@ -422,7 +424,10 @@ xchk_fscounters( > struct xfs_mount *mp = sc->mp; > struct xchk_fscounters *fsc = sc->buf; > int64_t icount, ifree, fdblocks, frextents; > + > +#if 0 > int error; > +#endif > > /* Snapshot the percpu counters. */ > icount = percpu_counter_sum(&mp->m_icount); > @@ -452,6 +457,7 @@ xchk_fscounters( > */ > return 0; > > +#if 0 > /* > * If ifree exceeds icount by more than the minimum variance then > * something's probably wrong with the counters. > @@ -489,4 +495,5 @@ xchk_fscounters( > xchk_set_corrupt(sc); > > return 0; > +#endif > } > -- > 2.30.2 >
On Tue, Jun 06, 2023 at 05:11:22PM +0200, cem@kernel.org wrote: > From: Carlos Maiolino <cem@kernel.org> > > Comment the code out so kernel test robot stop complaining about it > every single test build. Err, what? #if 0ing commit coe is a complete no-go. If this code is dead it should be removed.
On Tue, Jun 06, 2023 at 11:39:04PM -0700, Christoph Hellwig wrote: > On Tue, Jun 06, 2023 at 05:11:22PM +0200, cem@kernel.org wrote: > > From: Carlos Maiolino <cem@kernel.org> > > > > Comment the code out so kernel test robot stop complaining about it > > every single test build. > > Err, what? #if 0ing commit coe is a complete no-go. If this code > is dead it should be removed. The code isn't dead, it's temporarily broken. I spoke with Darrick about removing it, but by doing that, later, 'reverting' the patch that removed the broken code, will break the git history (specifically for git blame), and I didn't want to give Darrick extra work by needing to re-add back all this code later when he come back to work on this. Anyway, just an attempt to quiet built test warning alerts :) I'm totally fine ^R'ing these emails :)
On Wed, Jun 07, 2023 at 09:37:57AM +0200, Carlos Maiolino wrote: > The code isn't dead, it's temporarily broken. I spoke with Darrick about > removing it, but by doing that, later, 'reverting' the patch that removed the > broken code, will break the git history (specifically for git blame), and I > didn't want to give Darrick extra work by needing to re-add back all this code > later when he come back to work on this. > Anyway, just an attempt to quiet built test warning alerts :) > I'm totally fine ^R'ing these emails :) #if 0 is a realy bad thing. I'd much prefer to remvoe it and re-added it when needed. But even if Darrick insists on just disabling it, you need to add a comment explaining what is going on, because otherwise people will just trip over the complete undocumented #if 0 with a completely meaningless commit message in git-blame. That's how people dealt with code in the early 90s and not now.
On Wed, Jun 07, 2023 at 12:48:04AM -0700, Christoph Hellwig wrote: > On Wed, Jun 07, 2023 at 09:37:57AM +0200, Carlos Maiolino wrote: > > The code isn't dead, it's temporarily broken. I spoke with Darrick about > > removing it, but by doing that, later, 'reverting' the patch that removed the > > broken code, will break the git history (specifically for git blame), and I > > didn't want to give Darrick extra work by needing to re-add back all this code > > later when he come back to work on this. > > Anyway, just an attempt to quiet built test warning alerts :) > > I'm totally fine ^R'ing these emails :) > > #if 0 is a realy bad thing. I'd much prefer to remvoe it and re-added > it when needed. But even if Darrick insists on just disabling it, you > need to add a comment explaining what is going on, because otherwise > people will just trip over the complete undocumented #if 0 with a > completely meaningless commit message in git-blame. That's how people > dealt with code in the early 90s and not now. You are right, I should have added at least some comment on that, I'll wait for Darrick to wake up and see if we deal with it somehow or just leave it as-is.
On Wed, Jun 07, 2023 at 10:48:04AM +0200, Carlos Maiolino wrote: > On Wed, Jun 07, 2023 at 12:48:04AM -0700, Christoph Hellwig wrote: > > On Wed, Jun 07, 2023 at 09:37:57AM +0200, Carlos Maiolino wrote: > > > The code isn't dead, it's temporarily broken. I spoke with Darrick about > > > removing it, but by doing that, later, 'reverting' the patch that removed the > > > broken code, will break the git history (specifically for git blame), and I > > > didn't want to give Darrick extra work by needing to re-add back all this code > > > later when he come back to work on this. > > > Anyway, just an attempt to quiet built test warning alerts :) > > > I'm totally fine ^R'ing these emails :) > > > > #if 0 is a realy bad thing. I'd much prefer to remvoe it and re-added > > it when needed. But even if Darrick insists on just disabling it, you > > need to add a comment explaining what is going on, because otherwise > > people will just trip over the complete undocumented #if 0 with a > > completely meaningless commit message in git-blame. That's how people > > dealt with code in the early 90s and not now. > > You are right, I should have added at least some comment on that, I'll wait for > Darrick to wake up and see if we deal with it somehow or just leave it as-is. *Someone* please just review the fixes for fscounters.c that I put on the list two weeks ago. The first two patches of the patchset are sufficient to fix this problem. https://lore.kernel.org/linux-xfs/168506061483.3732954.5178462816967376906.stgit@frogsfrogsfrogs/ --D
On Wed, Jun 07, 2023 at 07:28:12AM -0700, Darrick J. Wong wrote: > *Someone* please just review the fixes for fscounters.c that I put on > the list two weeks ago. The first two patches of the patchset are > sufficient to fix this problem. > > https://lore.kernel.org/linux-xfs/168506061483.3732954.5178462816967376906.stgit@frogsfrogsfrogs/ That sounds like a way better idea indeed. I'll get to it after my meetings.
On Wed, Jun 07, 2023 at 07:30:55AM -0700, Christoph Hellwig wrote: > On Wed, Jun 07, 2023 at 07:28:12AM -0700, Darrick J. Wong wrote: > > *Someone* please just review the fixes for fscounters.c that I put on > > the list two weeks ago. The first two patches of the patchset are > > sufficient to fix this problem. > > > > https://lore.kernel.org/linux-xfs/168506061483.3732954.5178462816967376906.stgit@frogsfrogsfrogs/ > > That sounds like a way better idea indeed. I'll get to it after my > meetings. FYI there's a parallel discussion about the same patch going on fsdevel: https://lore.kernel.org/linux-fsdevel/20230522234200.GC11598@frogsfrogsfrogs/ --D
diff --git a/fs/xfs/scrub/fscounters.c b/fs/xfs/scrub/fscounters.c index e382a35e98d88..228efe0c99be8 100644 --- a/fs/xfs/scrub/fscounters.c +++ b/fs/xfs/scrub/fscounters.c @@ -153,6 +153,7 @@ xchk_setup_fscounters( return xchk_trans_alloc(sc, 0); } +#if 0 /* * Part 1: Collecting filesystem summary counts. For each AG, we add its * summary counts (total inodes, free inodes, free data blocks) to an incore @@ -349,6 +350,7 @@ xchk_fscount_count_frextents( return 0; } #endif /* CONFIG_XFS_RT */ +#endif /* * Part 2: Comparing filesystem summary counters. All we have to do here is @@ -422,7 +424,10 @@ xchk_fscounters( struct xfs_mount *mp = sc->mp; struct xchk_fscounters *fsc = sc->buf; int64_t icount, ifree, fdblocks, frextents; + +#if 0 int error; +#endif /* Snapshot the percpu counters. */ icount = percpu_counter_sum(&mp->m_icount); @@ -452,6 +457,7 @@ xchk_fscounters( */ return 0; +#if 0 /* * If ifree exceeds icount by more than the minimum variance then * something's probably wrong with the counters. @@ -489,4 +495,5 @@ xchk_fscounters( xchk_set_corrupt(sc); return 0; +#endif }