Patchwork [7/7,v2] ext4: reclaim extents from extent status tree

login
register
mail settings
Submitter Theodore Ts'o
Date Jan. 18, 2013, 5:39 a.m.
Message ID <20130118053947.GD13785@thunk.org>
Download mbox | patch
Permalink /patch/213455/
State Rejected
Headers show

Comments

Theodore Ts'o - Jan. 18, 2013, 5:39 a.m.
On Fri, Jan 18, 2013 at 12:19:21AM -0500, Theodore Ts'o wrote:
> I'm a bit concerned we might be too aggressive,
> because there are two ways that items can be freed from the
> extent_status tree.  One is if the inode is not used at all, and when
> we release the inode, we'll drop all of the entries in the
> extent_status_tree for that inode.  The second way is via the shrinker
> which we've registered.

If we use the sb->s_op->free_cached_objects() approach, something like
the following change to prune_super() in fs/super.c might address the
above concern:


What do folks think?

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zheng Liu - Jan. 21, 2013, 7:24 a.m.
On Fri, Jan 18, 2013 at 12:39:47AM -0500, Theodore Ts'o wrote:
> On Fri, Jan 18, 2013 at 12:19:21AM -0500, Theodore Ts'o wrote:
> > I'm a bit concerned we might be too aggressive,
> > because there are two ways that items can be freed from the
> > extent_status tree.  One is if the inode is not used at all, and when
> > we release the inode, we'll drop all of the entries in the
> > extent_status_tree for that inode.  The second way is via the shrinker
> > which we've registered.
> 
> If we use the sb->s_op->free_cached_objects() approach, something like
> the following change to prune_super() in fs/super.c might address the
> above concern:

Sorry for delay reply.  I believe that sb->s_op->free_cached_objbects()
approach is better.  So in next version I will try to implement this approach.

> 
> diff --git a/fs/super.c b/fs/super.c
> index 12f1237..fb57bd2 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -80,6 +80,7 @@ static int prune_super(struct shrinker *shrink, struct shrink_control *sc)
>  	if (sc->nr_to_scan) {
>  		int	dentries;
>  		int	inodes;
> +		int	fs_to_scan = 0;
>  
>  		/* proportion the scan between the caches */
>  		dentries = (sc->nr_to_scan * sb->s_nr_dentry_unused) /
> @@ -87,7 +88,7 @@ static int prune_super(struct shrinker *shrink, struct shrink_control *sc)
>  		inodes = (sc->nr_to_scan * sb->s_nr_inodes_unused) /
>  							total_objects;
>  		if (fs_objects)
> -			fs_objects = (sc->nr_to_scan * fs_objects) /
> +			fs_to_scan = (sc->nr_to_scan * fs_objects) /
>  							total_objects;
>  		/*
>  		 * prune the dcache first as the icache is pinned by it, then
> @@ -96,8 +97,23 @@ static int prune_super(struct shrinker *shrink, struct shrink_control *sc)
>  		prune_dcache_sb(sb, dentries);
>  		prune_icache_sb(sb, inodes);
>  
> -		if (fs_objects && sb->s_op->free_cached_objects) {
> -			sb->s_op->free_cached_objects(sb, fs_objects);
> +		/*
> +		 * If as a result of pruning the icache, we released some
> +		 * of the fs_objects, give credit to the fact and
> +		 * reduce the number of fs objects that we should try
> +		 * to release.
> +		 */
> +		if (fs_to_scan) {
> +			int fs_objects_now = sb->s_op->nr_cached_objects(sb);
> +
> +			if (fs_objects_now < fs_objects)
> +				fs_to_scan -= fs_objects - fs_objects_now;
> +			if (fs_to_scan < 0)
> +				fs_to_scan = 0;
> +		}
> +
> +		if (fs_to_scan && sb->s_op->free_cached_objects) {
> +			sb->s_op->free_cached_objects(sb, fs_to_scan);
>  			fs_objects = sb->s_op->nr_cached_objects(sb);
>  		}
>  		total_objects = sb->s_nr_dentry_unused +
> 
> What do folks think?

Do we need to CC' linux-fsdevel mailling list to let other folks review
this patch?

Thanks,
                                                - Zheng
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kara - Jan. 21, 2013, 2:43 p.m.
On Fri 18-01-13 00:39:47, Ted Tso wrote:
> On Fri, Jan 18, 2013 at 12:19:21AM -0500, Theodore Ts'o wrote:
> > I'm a bit concerned we might be too aggressive,
> > because there are two ways that items can be freed from the
> > extent_status tree.  One is if the inode is not used at all, and when
> > we release the inode, we'll drop all of the entries in the
> > extent_status_tree for that inode.  The second way is via the shrinker
> > which we've registered.
> 
> If we use the sb->s_op->free_cached_objects() approach, something like
> the following change to prune_super() in fs/super.c might address the
> above concern:
  Yeah, this would make sence to me. When you submit the final patch don't
forget to include Dave Chinner. He's the author of the shrinker framework
and XFS uses nr_cached_objects / free_cached_objects. AFAICS it uses it for
its separate inode cache so your change shouldn't affect it but better be
sure.

								Honza
> 
> diff --git a/fs/super.c b/fs/super.c
> index 12f1237..fb57bd2 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -80,6 +80,7 @@ static int prune_super(struct shrinker *shrink, struct shrink_control *sc)
>  	if (sc->nr_to_scan) {
>  		int	dentries;
>  		int	inodes;
> +		int	fs_to_scan = 0;
>  
>  		/* proportion the scan between the caches */
>  		dentries = (sc->nr_to_scan * sb->s_nr_dentry_unused) /
> @@ -87,7 +88,7 @@ static int prune_super(struct shrinker *shrink, struct shrink_control *sc)
>  		inodes = (sc->nr_to_scan * sb->s_nr_inodes_unused) /
>  							total_objects;
>  		if (fs_objects)
> -			fs_objects = (sc->nr_to_scan * fs_objects) /
> +			fs_to_scan = (sc->nr_to_scan * fs_objects) /
>  							total_objects;
>  		/*
>  		 * prune the dcache first as the icache is pinned by it, then
> @@ -96,8 +97,23 @@ static int prune_super(struct shrinker *shrink, struct shrink_control *sc)
>  		prune_dcache_sb(sb, dentries);
>  		prune_icache_sb(sb, inodes);
>  
> -		if (fs_objects && sb->s_op->free_cached_objects) {
> -			sb->s_op->free_cached_objects(sb, fs_objects);
> +		/*
> +		 * If as a result of pruning the icache, we released some
> +		 * of the fs_objects, give credit to the fact and
> +		 * reduce the number of fs objects that we should try
> +		 * to release.
> +		 */
> +		if (fs_to_scan) {
> +			int fs_objects_now = sb->s_op->nr_cached_objects(sb);
> +
> +			if (fs_objects_now < fs_objects)
> +				fs_to_scan -= fs_objects - fs_objects_now;
> +			if (fs_to_scan < 0)
> +				fs_to_scan = 0;
> +		}
> +
> +		if (fs_to_scan && sb->s_op->free_cached_objects) {
> +			sb->s_op->free_cached_objects(sb, fs_to_scan);
>  			fs_objects = sb->s_op->nr_cached_objects(sb);
>  		}
>  		total_objects = sb->s_nr_dentry_unused +
> 
> What do folks think?
> 
> 						- Ted
Theodore Ts'o - Jan. 21, 2013, 3 p.m.
On Mon, Jan 21, 2013 at 03:24:43PM +0800, Zheng Liu wrote:
> 
> Do we need to CC' linux-fsdevel mailling list to let other folks review
> this patch?

For this patch, yes.  And we'll need a commit description which
explains why it's needed.  Since I wrote the patch, I'm happy to
supply the commit description.  We can include it as a separate patch
in your patch series, and I'll cc linux-fsdevel.

As Jan mentioned, the only other user of it is Dave Chinner for the
XFS code, and I had alreday checked to make sure that it shouldn't
affect him --- but it would be good to cc him so he's in the loop.

Speaking of commit descriptions, I had made some other minor
adjustments to some of the commits in your last version of the patch
series, since I had already been assuming that I could take them until
I saw the potential problems with the memory shrinker patch.  So you
can just provide a new version of the memory shrinker patch --- or if
you have other changes you want to make to the earlier patches in that
patch series, let me know and I can extract out the various comments
and whitespace fixes that from the patches I have in my private tree,
so we can merge them with any additional updates to your patches.

Cheers, and thanks for working on this!

					- Ted

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zheng Liu - Jan. 21, 2013, 3:12 p.m.
On Mon, Jan 21, 2013 at 03:43:36PM +0100, Jan Kara wrote:
> On Fri 18-01-13 00:39:47, Ted Tso wrote:
> > On Fri, Jan 18, 2013 at 12:19:21AM -0500, Theodore Ts'o wrote:
> > > I'm a bit concerned we might be too aggressive,
> > > because there are two ways that items can be freed from the
> > > extent_status tree.  One is if the inode is not used at all, and when
> > > we release the inode, we'll drop all of the entries in the
> > > extent_status_tree for that inode.  The second way is via the shrinker
> > > which we've registered.
> > 
> > If we use the sb->s_op->free_cached_objects() approach, something like
> > the following change to prune_super() in fs/super.c might address the
> > above concern:
>   Yeah, this would make sence to me. When you submit the final patch don't
> forget to include Dave Chinner. He's the author of the shrinker framework
> and XFS uses nr_cached_objects / free_cached_objects. AFAICS it uses it for
> its separate inode cache so your change shouldn't affect it but better be
> sure.

Thanks for reminding.  This patch has been added in my patch series, and
I will CC' it to Dave.

Regards,
                                                - Zheng

> > 
> > diff --git a/fs/super.c b/fs/super.c
> > index 12f1237..fb57bd2 100644
> > --- a/fs/super.c
> > +++ b/fs/super.c
> > @@ -80,6 +80,7 @@ static int prune_super(struct shrinker *shrink, struct shrink_control *sc)
> >  	if (sc->nr_to_scan) {
> >  		int	dentries;
> >  		int	inodes;
> > +		int	fs_to_scan = 0;
> >  
> >  		/* proportion the scan between the caches */
> >  		dentries = (sc->nr_to_scan * sb->s_nr_dentry_unused) /
> > @@ -87,7 +88,7 @@ static int prune_super(struct shrinker *shrink, struct shrink_control *sc)
> >  		inodes = (sc->nr_to_scan * sb->s_nr_inodes_unused) /
> >  							total_objects;
> >  		if (fs_objects)
> > -			fs_objects = (sc->nr_to_scan * fs_objects) /
> > +			fs_to_scan = (sc->nr_to_scan * fs_objects) /
> >  							total_objects;
> >  		/*
> >  		 * prune the dcache first as the icache is pinned by it, then
> > @@ -96,8 +97,23 @@ static int prune_super(struct shrinker *shrink, struct shrink_control *sc)
> >  		prune_dcache_sb(sb, dentries);
> >  		prune_icache_sb(sb, inodes);
> >  
> > -		if (fs_objects && sb->s_op->free_cached_objects) {
> > -			sb->s_op->free_cached_objects(sb, fs_objects);
> > +		/*
> > +		 * If as a result of pruning the icache, we released some
> > +		 * of the fs_objects, give credit to the fact and
> > +		 * reduce the number of fs objects that we should try
> > +		 * to release.
> > +		 */
> > +		if (fs_to_scan) {
> > +			int fs_objects_now = sb->s_op->nr_cached_objects(sb);
> > +
> > +			if (fs_objects_now < fs_objects)
> > +				fs_to_scan -= fs_objects - fs_objects_now;
> > +			if (fs_to_scan < 0)
> > +				fs_to_scan = 0;
> > +		}
> > +
> > +		if (fs_to_scan && sb->s_op->free_cached_objects) {
> > +			sb->s_op->free_cached_objects(sb, fs_to_scan);
> >  			fs_objects = sb->s_op->nr_cached_objects(sb);
> >  		}
> >  		total_objects = sb->s_nr_dentry_unused +
> > 
> > What do folks think?
> > 
> > 						- Ted
> -- 
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zheng Liu - Jan. 21, 2013, 5:09 p.m.
On Mon, Jan 21, 2013 at 10:00:20AM -0500, Theodore Ts'o wrote:
> On Mon, Jan 21, 2013 at 03:24:43PM +0800, Zheng Liu wrote:
> > 
> > Do we need to CC' linux-fsdevel mailling list to let other folks review
> > this patch?
> 
> For this patch, yes.  And we'll need a commit description which
> explains why it's needed.  Since I wrote the patch, I'm happy to
> supply the commit description.  We can include it as a separate patch
> in your patch series, and I'll cc linux-fsdevel.

That would be great if you could give a commit description.

> 
> As Jan mentioned, the only other user of it is Dave Chinner for the
> XFS code, and I had alreday checked to make sure that it shouldn't
> affect him --- but it would be good to cc him so he's in the loop.

As I metioned at another mail, I will cc him.

> 
> Speaking of commit descriptions, I had made some other minor
> adjustments to some of the commits in your last version of the patch
> series, since I had already been assuming that I could take them until
> I saw the potential problems with the memory shrinker patch.  So you
> can just provide a new version of the memory shrinker patch --- or if
> you have other changes you want to make to the earlier patches in that
> patch series, let me know and I can extract out the various comments
> and whitespace fixes that from the patches I have in my private tree,
> so we can merge them with any additional updates to your patches.

Your meaning is that you have made some changes in my patches of extent
status tree, aren't you?  Now I have stashed es_status into es_pblk in
patch 3 (ext4: add physical block and status member into extent status
tree), and added a new patch that removes single extent cache.  I am
happy to merge your changes if you could give them to me.  Please let me
know if I could provide some helps.

Thanks,
                                                - Zheng
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/super.c b/fs/super.c
index 12f1237..fb57bd2 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -80,6 +80,7 @@  static int prune_super(struct shrinker *shrink, struct shrink_control *sc)
 	if (sc->nr_to_scan) {
 		int	dentries;
 		int	inodes;
+		int	fs_to_scan = 0;
 
 		/* proportion the scan between the caches */
 		dentries = (sc->nr_to_scan * sb->s_nr_dentry_unused) /
@@ -87,7 +88,7 @@  static int prune_super(struct shrinker *shrink, struct shrink_control *sc)
 		inodes = (sc->nr_to_scan * sb->s_nr_inodes_unused) /
 							total_objects;
 		if (fs_objects)
-			fs_objects = (sc->nr_to_scan * fs_objects) /
+			fs_to_scan = (sc->nr_to_scan * fs_objects) /
 							total_objects;
 		/*
 		 * prune the dcache first as the icache is pinned by it, then
@@ -96,8 +97,23 @@  static int prune_super(struct shrinker *shrink, struct shrink_control *sc)
 		prune_dcache_sb(sb, dentries);
 		prune_icache_sb(sb, inodes);
 
-		if (fs_objects && sb->s_op->free_cached_objects) {
-			sb->s_op->free_cached_objects(sb, fs_objects);
+		/*
+		 * If as a result of pruning the icache, we released some
+		 * of the fs_objects, give credit to the fact and
+		 * reduce the number of fs objects that we should try
+		 * to release.
+		 */
+		if (fs_to_scan) {
+			int fs_objects_now = sb->s_op->nr_cached_objects(sb);
+
+			if (fs_objects_now < fs_objects)
+				fs_to_scan -= fs_objects - fs_objects_now;
+			if (fs_to_scan < 0)
+				fs_to_scan = 0;
+		}
+
+		if (fs_to_scan && sb->s_op->free_cached_objects) {
+			sb->s_op->free_cached_objects(sb, fs_to_scan);
 			fs_objects = sb->s_op->nr_cached_objects(sb);
 		}
 		total_objects = sb->s_nr_dentry_unused +