[Cluster-devel] GFS2 Patches Needing Review

[Bob Peterson <rpeterso@redhat.com>]

Hi,

----- Original Message -----
| > (1) https://www.redhat.com/archives/cluster-devel/2017-May/msg00116.html
| >      26 May 2017 [GFS2 PATCH resend/revised] GFS2: Add new debug trace
| >      point and evict code path
| That one doesn't use trace points in the right way. Calling it
| gfs2_debug is far too generic a name for something that is specific to
| gfs2_evict() and we should not be using strings like that to label the
| different cases. It also doesn't seem to print out much of interest either.
| 
| What would be interesting is a generic tracepoint (or set of
| tracepoints) which can trace the inode lifecycle implemented at the VFS
| level (for race free access to i_state and i_nlink etc.).

The patch started out as a generic debug trace point we can expand later,
but then I only used it to trace the evict code path. I could clean it up,
but it's less of a priority now that the evict path has been cleaned up,
especially with Andreas's new shrinker code. I just thought it might be
useful for debugging any future problems with the GFS2 evict code. For now,
I'll just archive it and we can look at it in the future if necessary.

| > (2) https://www.redhat.com/archives/cluster-devel/2017-May/msg00114.html
| >      26 May 2017 [GFS2 PATCH V2] GFS2: Combine func meta_prep_new with its
| >      only caller
| You could do that, but what is the benefit? I doubt it makes any
| difference to the generated code, and it doesn't seem to be any cleaner
| or easier to read the code either.

Although the code may compile to the same binary, the benefit is that makes
the code easier to read. Having dozens of tiny functions that call dozens of
other tiny functions is about as bad as having long functions that have too
much code.

Since the only calling function had a mere 4 lines between { and } (one of
which was a declare, so 3 functional lines), and the called function
had 6 LOC (with 1 declare) it only served to obfuscate the code.

The patch simplifies the code by one function with a total of 8 lines
(6 functional plus 2 declares). Much more readable.

It also moves the setting of GFS2_MAGIC inside the buffer lock, so it
should be more robust as well.

(snip)
| > (4) https://www.redhat.com/archives/cluster-devel/2017-May/msg00117.html
| >      31 May 2017 [PATCH 0/8] GFS2 shrinker deadlock
| >      (series of 8, revised)
| I sent a set of replies to those patches on 1st June, so I assume that
| there should be an updated patch set in due course? You could take the
| first patch in the series anyway though I guess, and wait for updates
| for the other patches in due course,

Fair enough.

Regards,

Bob Peterson
Red Hat File Systems