[Cluster-devel] [PATCH] gfs2: Put bitmap buffers in put_super

[Cluster-devel] [PATCH] gfs2: Put bitmap buffers in put_super

[Andreas Gruenbacher]

gfs2_put_super calls gfs2_clear_rgrpd to destroy the gfs2_rgrpd objects
attached to the resource group glocks.  That function should release the
buffers attached to the gfs2_bitmap objects (bi_bh), but the call to
gfs2_rgrp_brelse for doing that is missing.

When gfs2_releasepage later runs across these buffers which are still
referenced, it refuses to free them.  This causes the pages the buffers
are attached to to remain referenced as well.  With enough mount/unmount
cycles, the system will eventually run out of memory.

Fix this by adding the missing call to gfs2_rgrp_brelse in
gfs2_clear_rgrpd.

(Also fix a gfs2_rgrp_relse -> gfs2_rgrp_brelse typo in a comment.)

Fixes: 39b0f1e92908 ("GFS2: Don't brelse rgrp buffer_heads every allocation")
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/rgrp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index ffe3032b1043..b08a530433ad 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -733,6 +733,7 @@ void gfs2_clear_rgrpd(struct gfs2_sbd *sdp)
 
 		if (gl) {
 			glock_clear_object(gl, rgd);
+			gfs2_rgrp_brelse(rgd);
 			gfs2_glock_put(gl);
 		}
 
@@ -1174,7 +1175,7 @@ static u32 count_unlinked(struct gfs2_rgrpd *rgd)
  * @rgd: the struct gfs2_rgrpd describing the RG to read in
  *
  * Read in all of a Resource Group's header and bitmap blocks.
- * Caller must eventually call gfs2_rgrp_relse() to free the bitmaps.
+ * Caller must eventually call gfs2_rgrp_brelse() to free the bitmaps.
  *
  * Returns: errno
  */
-- 
2.19.1.546.g028f9c799.dirty

[Cluster-devel] [PATCH] gfs2: Put bitmap buffers in put_super

[Steven Whitehouse]

Hi,

While that looks like a good fix for now, we should look at this again 
in due course. Why do we have a ref to the rgrp buffers held here in the 
first place? Unless the buffers are pinned in the journal there should 
not be a ref held, otherwise they cannot respond to memory pressure,

Steve.


On 06/11/18 09:39, Andreas Gruenbacher wrote:
> gfs2_put_super calls gfs2_clear_rgrpd to destroy the gfs2_rgrpd objects
> attached to the resource group glocks.  That function should release the
> buffers attached to the gfs2_bitmap objects (bi_bh), but the call to
> gfs2_rgrp_brelse for doing that is missing.
>
> When gfs2_releasepage later runs across these buffers which are still
> referenced, it refuses to free them.  This causes the pages the buffers
> are attached to to remain referenced as well.  With enough mount/unmount
> cycles, the system will eventually run out of memory.
>
> Fix this by adding the missing call to gfs2_rgrp_brelse in
> gfs2_clear_rgrpd.
>
> (Also fix a gfs2_rgrp_relse -> gfs2_rgrp_brelse typo in a comment.)
>
> Fixes: 39b0f1e92908 ("GFS2: Don't brelse rgrp buffer_heads every allocation")
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>   fs/gfs2/rgrp.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> index ffe3032b1043..b08a530433ad 100644
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -733,6 +733,7 @@ void gfs2_clear_rgrpd(struct gfs2_sbd *sdp)
>   
>   		if (gl) {
>   			glock_clear_object(gl, rgd);
> +			gfs2_rgrp_brelse(rgd);
>   			gfs2_glock_put(gl);
>   		}
>   
> @@ -1174,7 +1175,7 @@ static u32 count_unlinked(struct gfs2_rgrpd *rgd)
>    * @rgd: the struct gfs2_rgrpd describing the RG to read in
>    *
>    * Read in all of a Resource Group's header and bitmap blocks.
> - * Caller must eventually call gfs2_rgrp_relse() to free the bitmaps.
> + * Caller must eventually call gfs2_rgrp_brelse() to free the bitmaps.
>    *
>    * Returns: errno
>    */

[Cluster-devel] [PATCH] gfs2: Put bitmap buffers in put_super

[Andreas Gruenbacher]

Hi Steve,

On Tue, 6 Nov 2018 at 11:15, Steven Whitehouse <swhiteho@redhat.com> wrote:
> While that looks like a good fix for now, we should look at this again
> in due course. Why do we have a ref to the rgrp buffers held here in the
> first place? Unless the buffers are pinned in the journal there should
> not be a ref held, otherwise they cannot respond to memory pressure,

that should be well enough explained in the 39b0f1e92908 commit
message. Under memory pressure, the cached rgrp glocks will eventually
be forced out, which will also release those references.

Thanks,
Andreas