[Cluster-devel] GFS2 Patches Needing Review

[Cluster-devel] GFS2 Patches Needing Review

[Bob Peterson]

Hi,

Here is a list of recent GFS2 patches that I'd like reviewed before I push
them to the for-next branch:

Bob Peterson:

(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
(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
(3) https://www.redhat.com/archives/cluster-devel/2017-May/msg00115.html
    26 May 2017 [GFS2 PATCH] GFS2: Withdraw when directory entry inconsistencies are detected

Andreas Gruenbacher:

(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)

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] GFS2 Patches Needing Review

[Steven Whitehouse]

Hi,


On 08/06/17 21:20, Bob Peterson wrote:
> Hi,
>
> Here is a list of recent GFS2 patches that I'd like reviewed before I push
> them to the for-next branch:
>
> Bob Peterson:
>
> (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.).

> (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.

> (3) https://www.redhat.com/archives/cluster-devel/2017-May/msg00115.html
>      26 May 2017 [GFS2 PATCH] GFS2: Withdraw when directory entry inconsistencies are detected
Yes, that seems like a reasonable change, for consistency of error handling.

>
> Andreas Gruenbacher:
>
> (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)
>
> Regards,
>
> Bob Peterson
> Red Hat File Systems
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,

Steve.

[Cluster-devel] GFS2 Patches Needing Review

[Bob Peterson]

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