Re: [PATCH hurd/ext2fs] Sync pager before clearing MAY_CACHE flag

[Thomas Schwinge]

Hi!

On Thu, 29 Sep 2011 11:44:40 +0200, Sergio López <slpml@sinrega.org> wrote:
> Clearing MAY_CACHE flag on a pager initiates a memory object
> termination if this one is not referenced anymore. If the object has a
> significant number of dirty pages (i.e. a file recently created was
> unlinked before diskfs periodical sync) this operation generates a lot
> of stress on the translator. This is one of the most common sources
> for thread storms.
> 
> Sync'ing the pager before clearing that flag ensures that there aren't
> dirty pages in the object before its termination.
> 
> 
> * ext2fs/pager.c (drop_pager_softrefs): Sync pager before clearing
> MAY_CACHE flag.
> 
> ---
>  ext2fs/pager.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/ext2fs/pager.c b/ext2fs/pager.c
> index 082537c..89a1b71 100644
> --- a/ext2fs/pager.c
> +++ b/ext2fs/pager.c
> @@ -851,7 +851,10 @@ drop_pager_softrefs (struct node *node)
>    spin_unlock (&node_to_page_lock);
> 
>    if (MAY_CACHE && pager)
> -    pager_change_attributes (pager, 0, MEMORY_OBJECT_COPY_DELAY, 0);
> +    {
> +      pager_sync (pager, 1);
> +      pager_change_attributes (pager, 0, MEMORY_OBJECT_COPY_DELAY, 0);
> +    }
>    if (pager)
>      ports_port_deref (pager);
>  }

First, thanks for this!  (But I guess you don't happen to have hard data
on the effectiveness?)

Shouldn't the same change be made in the other libdiskfs translators,
too: fatfs, isofs, ufs?  (Of course, fatfs and isofs are read-only at the
moment, and nobody, really nobody uses ufs, but it's still good to keep
the sources in sync.)  (Feel free to commit such a change for yourself.)

And, shouldn't the commit message in fact be a comment before the
pager_sync invocation, for the next human reading this code?  (My
rationale still is is that source code comments should describe the
existing code where it helps for understanding.  Here I as a reader would
wonder why there is an explicit synchronization, and searching for that
information in the commit log is one step too much.)  (Feel free to
commit such a change for yourself.)


Grüße,
 Thomas

pgpQUVQ5B7G8k.pgp
Description: PGP signature