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@xxxxxxx>
> SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
