[15/24] xfs: eagerly free shadow buffers to reduce CIL footprint
diff mbox series

Message ID 20190801021752.4986-16-david@fromorbit.com
State New
Headers show
Series
  • mm, xfs: non-blocking inode reclaim
Related show

Commit Message

Dave Chinner Aug. 1, 2019, 2:17 a.m. UTC
From: Dave Chinner <dchinner@redhat.com>

The CIL can pin a lot of memory and effectively defines the lower
free memory boundary of operation for XFS. The way we hang onto
log item shadow buffers "just in case" effectively doubles the
memory footprint of the CIL for dubious reasons.

That is, we hang onto the old shadow buffer in case the next time
we log the item it will fit into the shadow buffer and we won't have
to allocate a new one. However, we only ever tend to grow dirty
objects in the CIL through relogging, so once we've allocated a
larger buffer the old buffer we set as a shadow buffer will never
get reused as the amount we log never decreases until the item is
clean. And then for buffer items we free the log item and the shadow
buffers, anyway. Inode items will hold onto their shadow buffer
until they are reclaimed - this could double the inode's memory
footprint for it's lifetime...

Hence we should just free the old log item buffer when we replace it
with a new shadow buffer rather than storing it for later use. It's
not useful, get rid of it as early as possible.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log_cil.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Brian Foster Aug. 5, 2019, 6:03 p.m. UTC | #1
On Thu, Aug 01, 2019 at 12:17:43PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The CIL can pin a lot of memory and effectively defines the lower
> free memory boundary of operation for XFS. The way we hang onto
> log item shadow buffers "just in case" effectively doubles the
> memory footprint of the CIL for dubious reasons.
> 
> That is, we hang onto the old shadow buffer in case the next time
> we log the item it will fit into the shadow buffer and we won't have
> to allocate a new one. However, we only ever tend to grow dirty
> objects in the CIL through relogging, so once we've allocated a
> larger buffer the old buffer we set as a shadow buffer will never
> get reused as the amount we log never decreases until the item is
> clean. And then for buffer items we free the log item and the shadow
> buffers, anyway. Inode items will hold onto their shadow buffer
> until they are reclaimed - this could double the inode's memory
> footprint for it's lifetime...
> 
> Hence we should just free the old log item buffer when we replace it
> with a new shadow buffer rather than storing it for later use. It's
> not useful, get rid of it as early as possible.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log_cil.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index fa5602d0fd7f..1863a9bdf4a9 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -238,9 +238,7 @@ xfs_cil_prepare_item(
>  	/*
>  	 * If there is no old LV, this is the first time we've seen the item in
>  	 * this CIL context and so we need to pin it. If we are replacing the
> -	 * old_lv, then remove the space it accounts for and make it the shadow
> -	 * buffer for later freeing. In both cases we are now switching to the
> -	 * shadow buffer, so update the the pointer to it appropriately.
> +	 * old_lv, then remove the space it accounts for and free it.
>  	 */

The comment above xlog_cil_alloc_shadow_bufs() needs a similar update
around how we handle the old buffer when the shadow buffer is used.

>  	if (!old_lv) {
>  		if (lv->lv_item->li_ops->iop_pin)
> @@ -251,7 +249,8 @@ xfs_cil_prepare_item(
>  
>  		*diff_len -= old_lv->lv_bytes;
>  		*diff_iovecs -= old_lv->lv_niovecs;
> -		lv->lv_item->li_lv_shadow = old_lv;
> +		kmem_free(old_lv);
> +		lv->lv_item->li_lv_shadow = NULL;
>  	}

So IIUC this is the case where we allocated a shadow buffer, the item
was already pinned (so old_lv is still around) but we ended up using the
shadow buffer for this relog. Instead of keeping the old buffer around
as a new shadow, we toss it. That makes sense, but if the objective is
to not leave dangling shadow buffers around as such, what about the case
where we allocated a shadow buffer but didn't end up using it because
old_lv was reusable? It looks like we still keep the shadow buffer
around in that scenario with a similar lifetime as the swapout scenario
this patch removes. Hm?

Brian

>  
>  	/* attach new log vector to log item */
> -- 
> 2.22.0
>
Dave Chinner Aug. 5, 2019, 11:33 p.m. UTC | #2
On Mon, Aug 05, 2019 at 02:03:01PM -0400, Brian Foster wrote:
> On Thu, Aug 01, 2019 at 12:17:43PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > The CIL can pin a lot of memory and effectively defines the lower
> > free memory boundary of operation for XFS. The way we hang onto
> > log item shadow buffers "just in case" effectively doubles the
> > memory footprint of the CIL for dubious reasons.
> > 
> > That is, we hang onto the old shadow buffer in case the next time
> > we log the item it will fit into the shadow buffer and we won't have
> > to allocate a new one. However, we only ever tend to grow dirty
> > objects in the CIL through relogging, so once we've allocated a
> > larger buffer the old buffer we set as a shadow buffer will never
> > get reused as the amount we log never decreases until the item is
> > clean. And then for buffer items we free the log item and the shadow
> > buffers, anyway. Inode items will hold onto their shadow buffer
> > until they are reclaimed - this could double the inode's memory
> > footprint for it's lifetime...
> > 
> > Hence we should just free the old log item buffer when we replace it
> > with a new shadow buffer rather than storing it for later use. It's
> > not useful, get rid of it as early as possible.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_log_cil.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> > index fa5602d0fd7f..1863a9bdf4a9 100644
> > --- a/fs/xfs/xfs_log_cil.c
> > +++ b/fs/xfs/xfs_log_cil.c
> > @@ -238,9 +238,7 @@ xfs_cil_prepare_item(
> >  	/*
> >  	 * If there is no old LV, this is the first time we've seen the item in
> >  	 * this CIL context and so we need to pin it. If we are replacing the
> > -	 * old_lv, then remove the space it accounts for and make it the shadow
> > -	 * buffer for later freeing. In both cases we are now switching to the
> > -	 * shadow buffer, so update the the pointer to it appropriately.
> > +	 * old_lv, then remove the space it accounts for and free it.
> >  	 */
> 
> The comment above xlog_cil_alloc_shadow_bufs() needs a similar update
> around how we handle the old buffer when the shadow buffer is used.

*nod*

> 
> >  	if (!old_lv) {
> >  		if (lv->lv_item->li_ops->iop_pin)
> > @@ -251,7 +249,8 @@ xfs_cil_prepare_item(
> >  
> >  		*diff_len -= old_lv->lv_bytes;
> >  		*diff_iovecs -= old_lv->lv_niovecs;
> > -		lv->lv_item->li_lv_shadow = old_lv;
> > +		kmem_free(old_lv);
> > +		lv->lv_item->li_lv_shadow = NULL;
> >  	}
> 
> So IIUC this is the case where we allocated a shadow buffer, the item
> was already pinned (so old_lv is still around) but we ended up using the
> shadow buffer for this relog. Instead of keeping the old buffer around
> as a new shadow, we toss it. That makes sense, but if the objective is
> to not leave dangling shadow buffers around as such, what about the case
> where we allocated a shadow buffer but didn't end up using it because
> old_lv was reusable? It looks like we still keep the shadow buffer
> around in that scenario with a similar lifetime as the swapout scenario
> this patch removes. Hm?

Of the top of my head, we shouldn't allocate a new shadow buffer in
that case (see xlog_cil_alloc_shadow_bufs()). i.e. we check up front
if the formatted size of the item will fit in the existing buffer,
and if it does we do not allocate a new shadow buffer as we just
reuse the existing one. SO we should only have to free a shadow
buffer when we switch them, not when we overwrite.

I'll recheck this, but I'm pretty sure overwrite won't leave a
shadow buffer around.

Cheers,

Dave.
Brian Foster Aug. 6, 2019, 12:57 p.m. UTC | #3
On Tue, Aug 06, 2019 at 09:33:26AM +1000, Dave Chinner wrote:
> On Mon, Aug 05, 2019 at 02:03:01PM -0400, Brian Foster wrote:
> > On Thu, Aug 01, 2019 at 12:17:43PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > The CIL can pin a lot of memory and effectively defines the lower
> > > free memory boundary of operation for XFS. The way we hang onto
> > > log item shadow buffers "just in case" effectively doubles the
> > > memory footprint of the CIL for dubious reasons.
> > > 
> > > That is, we hang onto the old shadow buffer in case the next time
> > > we log the item it will fit into the shadow buffer and we won't have
> > > to allocate a new one. However, we only ever tend to grow dirty
> > > objects in the CIL through relogging, so once we've allocated a
> > > larger buffer the old buffer we set as a shadow buffer will never
> > > get reused as the amount we log never decreases until the item is
> > > clean. And then for buffer items we free the log item and the shadow
> > > buffers, anyway. Inode items will hold onto their shadow buffer
> > > until they are reclaimed - this could double the inode's memory
> > > footprint for it's lifetime...
> > > 
> > > Hence we should just free the old log item buffer when we replace it
> > > with a new shadow buffer rather than storing it for later use. It's
> > > not useful, get rid of it as early as possible.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > ---
> > >  fs/xfs/xfs_log_cil.c | 7 +++----
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> > > index fa5602d0fd7f..1863a9bdf4a9 100644
> > > --- a/fs/xfs/xfs_log_cil.c
> > > +++ b/fs/xfs/xfs_log_cil.c
> > > @@ -238,9 +238,7 @@ xfs_cil_prepare_item(
> > >  	/*
> > >  	 * If there is no old LV, this is the first time we've seen the item in
> > >  	 * this CIL context and so we need to pin it. If we are replacing the
> > > -	 * old_lv, then remove the space it accounts for and make it the shadow
> > > -	 * buffer for later freeing. In both cases we are now switching to the
> > > -	 * shadow buffer, so update the the pointer to it appropriately.
> > > +	 * old_lv, then remove the space it accounts for and free it.
> > >  	 */
> > 
> > The comment above xlog_cil_alloc_shadow_bufs() needs a similar update
> > around how we handle the old buffer when the shadow buffer is used.
> 
> *nod*
> 
> > 
> > >  	if (!old_lv) {
> > >  		if (lv->lv_item->li_ops->iop_pin)
> > > @@ -251,7 +249,8 @@ xfs_cil_prepare_item(
> > >  
> > >  		*diff_len -= old_lv->lv_bytes;
> > >  		*diff_iovecs -= old_lv->lv_niovecs;
> > > -		lv->lv_item->li_lv_shadow = old_lv;
> > > +		kmem_free(old_lv);
> > > +		lv->lv_item->li_lv_shadow = NULL;
> > >  	}
> > 
> > So IIUC this is the case where we allocated a shadow buffer, the item
> > was already pinned (so old_lv is still around) but we ended up using the
> > shadow buffer for this relog. Instead of keeping the old buffer around
> > as a new shadow, we toss it. That makes sense, but if the objective is
> > to not leave dangling shadow buffers around as such, what about the case
> > where we allocated a shadow buffer but didn't end up using it because
> > old_lv was reusable? It looks like we still keep the shadow buffer
> > around in that scenario with a similar lifetime as the swapout scenario
> > this patch removes. Hm?
> 
> Of the top of my head, we shouldn't allocate a new shadow buffer in
> that case (see xlog_cil_alloc_shadow_bufs()). i.e. we check up front
> if the formatted size of the item will fit in the existing buffer,
> and if it does we do not allocate a new shadow buffer as we just
> reuse the existing one. SO we should only have to free a shadow
> buffer when we switch them, not when we overwrite.
> 

We have such a check in xlog_cil_insert_format_items(), so we'd reuse
->li_lv if it will suffice even if we have a shadow buffer available.

> I'll recheck this, but I'm pretty sure overwrite won't leave a
> shadow buffer around.
> 

But before that we have the following logic:

static void
xlog_cil_alloc_shadow_bufs(
	...

	if (!lip->li_lv_shadow ||
	    buf_size > lip->li_lv_shadow->lv_size) {
		...
		lv = kmem_alloc_large(buf_size, KM_SLEEP | KM_NOFS);
		...
		lip->li_lv_shadow = lv;
	} else {
		<reuse shadow>
	}
	...
}

... which always allocates a shadow buffer if one doesn't exist. We
don't look at the currently used (lip->li_lv) buffer at all here. IIUC,
that has to do with the TOCTOU race described in the big comment above
the function.. hm?

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
Dave Chinner Aug. 6, 2019, 9:21 p.m. UTC | #4
On Tue, Aug 06, 2019 at 08:57:27AM -0400, Brian Foster wrote:
> On Tue, Aug 06, 2019 at 09:33:26AM +1000, Dave Chinner wrote:
> > I'll recheck this, but I'm pretty sure overwrite won't leave a
> > shadow buffer around.
> > 
> 
> But before that we have the following logic:
> 
> static void
> xlog_cil_alloc_shadow_bufs(
> 	...
> 
> 	if (!lip->li_lv_shadow ||
> 	    buf_size > lip->li_lv_shadow->lv_size) {
> 		...
> 		lv = kmem_alloc_large(buf_size, KM_SLEEP | KM_NOFS);
> 		...
> 		lip->li_lv_shadow = lv;
> 	} else {
> 		<reuse shadow>
> 	}
> 	...
> }
> 
> ... which always allocates a shadow buffer if one doesn't exist. We
> don't look at the currently used (lip->li_lv) buffer at all here. IIUC,
> that has to do with the TOCTOU race described in the big comment above
> the function.. hm?

You might be right there. I haven't had a chance to follow up on
this from yesterday yet, so I'll keep this in mind when I look at it
again.

Cheers,

Dave.

Patch
diff mbox series

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index fa5602d0fd7f..1863a9bdf4a9 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -238,9 +238,7 @@  xfs_cil_prepare_item(
 	/*
 	 * If there is no old LV, this is the first time we've seen the item in
 	 * this CIL context and so we need to pin it. If we are replacing the
-	 * old_lv, then remove the space it accounts for and make it the shadow
-	 * buffer for later freeing. In both cases we are now switching to the
-	 * shadow buffer, so update the the pointer to it appropriately.
+	 * old_lv, then remove the space it accounts for and free it.
 	 */
 	if (!old_lv) {
 		if (lv->lv_item->li_ops->iop_pin)
@@ -251,7 +249,8 @@  xfs_cil_prepare_item(
 
 		*diff_len -= old_lv->lv_bytes;
 		*diff_iovecs -= old_lv->lv_niovecs;
-		lv->lv_item->li_lv_shadow = old_lv;
+		kmem_free(old_lv);
+		lv->lv_item->li_lv_shadow = NULL;
 	}
 
 	/* attach new log vector to log item */