| Message ID | 20130118053947.GD13785@thunk.org |
|---|---|
| State | Rejected, archived |
| Headers | show |
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
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
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
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
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
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 +