Go through the LRU list of inode from head.
(I'm not sure whether there is any trick here I doesn't get. If yes,
any one could explain it)
Signed-off-by: Cong Meng <[email protected]>
---
fs/inode.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/inode.c b/fs/inode.c
index 775cbab..aac8449 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -704,7 +704,7 @@ void prune_icache_sb(struct super_block *sb, int nr_to_scan)
if (list_empty(&sb->s_inode_lru))
break;
- inode = list_entry(sb->s_inode_lru.prev, struct inode, i_lru);
+ inode = list_entry(sb->s_inode_lru.next, struct inode, i_lru);
/*
* we are inverting the sb->s_inode_lru_lock/inode->i_lock here,
--
1.7.5.4
On Thu, Jun 21, 2012 at 05:00:27PM +0800, Cong Meng wrote:
>Go through the LRU list of inode from head.
IIRC, prune_icache_sb is called(as a shrinker in shrink_slab) in page reclaim
path, it is used to free icache. Tail entry in LRU(Least Recently Used) list
is the least recently used inode object. When the inode cache has to shrink,
the kernel removes entries from the tail.
>
>(I'm not sure whether there is any trick here I doesn't get. If yes,
>any one could explain it)
>
>Signed-off-by: Cong Meng <[email protected]>
>---
> fs/inode.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
>diff --git a/fs/inode.c b/fs/inode.c
>index 775cbab..aac8449 100644
>--- a/fs/inode.c
>+++ b/fs/inode.c
>@@ -704,7 +704,7 @@ void prune_icache_sb(struct super_block *sb, int nr_to_scan)
> if (list_empty(&sb->s_inode_lru))
> break;
>
>- inode = list_entry(sb->s_inode_lru.prev, struct inode, i_lru);
>+ inode = list_entry(sb->s_inode_lru.next, struct inode, i_lru);
>
> /*
> * we are inverting the sb->s_inode_lru_lock/inode->i_lock here,
>--
>1.7.5.4
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to [email protected]
>More majordomo info at http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at http://www.tux.org/lkml/
On Thu 21-06-12 17:00:27, Cong Meng wrote:
> Go through the LRU list of inode from head.
>
> (I'm not sure whether there is any trick here I doesn't get. If yes,
> any one could explain it)
Look at inode_lru_list_add(). It adds at the head of the list. So you
should take from the tail to get the least recently used element...
Honza
>
> Signed-off-by: Cong Meng <[email protected]>
> ---
> fs/inode.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 775cbab..aac8449 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -704,7 +704,7 @@ void prune_icache_sb(struct super_block *sb, int nr_to_scan)
> if (list_empty(&sb->s_inode_lru))
> break;
>
> - inode = list_entry(sb->s_inode_lru.prev, struct inode, i_lru);
> + inode = list_entry(sb->s_inode_lru.next, struct inode, i_lru);
>
> /*
> * we are inverting the sb->s_inode_lru_lock/inode->i_lock here,
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jan Kara <[email protected]>
SUSE Labs, CR
Quoting Jan Kara <[email protected]>:
> On Thu 21-06-12 17:00:27, Cong Meng wrote:
>> Go through the LRU list of inode from head.
>>
>> (I'm not sure whether there is any trick here I doesn't get. If yes,
>> any one could explain it)
> Look at inode_lru_list_add(). It adds at the head of the list. So you
> should take from the tail to get the least recently used element...
I still have a quetion about the subsequent code and comment:
inode = list_entry(sb->s_inode_lru.prev, struct inode, i_lru);
/*
* we are inverting the sb->s_inode_lru_lock/inode->i_lock here,
* so use a trylock. If we fail to get the lock, just move the
* inode to the back of the list so we don't spin on it.
*/
if (!spin_trylock(&inode->i_lock)) {
list_move_tail(&inode->i_lru, &sb->s_inode_lru);
continue;
}
Shouldn't the inode be moved to the head to avoid spin on it?
I note that list_move was replaced by list_move_tail purposely in a commit.
and below piece of code (at the bottom of prune_icache_sb()):
if (inode != list_entry(sb->s_inode_lru.next,
struct inode, i_lru))
continue; /* wrong inode or list_empty */
Should the inode be compared against to the tail of the list other
than the head
after re-get the lru lock?
thanks.
cong.
>
> Honza
>
>>
>> Signed-off-by: Cong Meng <[email protected]>
>> ---
>> fs/inode.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 775cbab..aac8449 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -704,7 +704,7 @@ void prune_icache_sb(struct super_block *sb,
>> int nr_to_scan)
>> if (list_empty(&sb->s_inode_lru))
>> break;
>>
>> - inode = list_entry(sb->s_inode_lru.prev, struct inode, i_lru);
>> + inode = list_entry(sb->s_inode_lru.next, struct inode, i_lru);
>>
>> /*
>> * we are inverting the sb->s_inode_lru_lock/inode->i_lock here,
>> --
>> 1.7.5.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR
On Tue 26-06-12 03:35:46, [email protected] wrote:
> Quoting Jan Kara <[email protected]>:
>
> >On Thu 21-06-12 17:00:27, Cong Meng wrote:
> >>Go through the LRU list of inode from head.
> >>
> >>(I'm not sure whether there is any trick here I doesn't get. If yes,
> >>any one could explain it)
> > Look at inode_lru_list_add(). It adds at the head of the list. So you
> >should take from the tail to get the least recently used element...
>
> I still have a quetion about the subsequent code and comment:
>
> inode = list_entry(sb->s_inode_lru.prev, struct inode, i_lru);
> /*
> * we are inverting the sb->s_inode_lru_lock/inode->i_lock here,
> * so use a trylock. If we fail to get the lock, just move the
> * inode to the back of the list so we don't spin on it.
> */
> if (!spin_trylock(&inode->i_lock)) {
> list_move_tail(&inode->i_lru, &sb->s_inode_lru);
> continue;
> }
>
> Shouldn't the inode be moved to the head to avoid spin on it?
Yes, it should.
> I note that list_move was replaced by list_move_tail purposely in a commit.
Right, you are speaking about Christoph's commit 62a3ddef? I agree that
commit looks bogus and should be reverted AFAICT. Christoph?
> and below piece of code (at the bottom of prune_icache_sb()):
>
> if (inode != list_entry(sb->s_inode_lru.next,
> struct inode, i_lru))
> continue; /* wrong inode or list_empty */
>
> Should the inode be compared against to the tail of the list other
> than the head
> after re-get the lru lock?
And you seem to be right here as well. Thanks for having a look!
Honza
> >>Signed-off-by: Cong Meng <[email protected]>
> >>---
> >> fs/inode.c | 2 +-
> >> 1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >>diff --git a/fs/inode.c b/fs/inode.c
> >>index 775cbab..aac8449 100644
> >>--- a/fs/inode.c
> >>+++ b/fs/inode.c
> >>@@ -704,7 +704,7 @@ void prune_icache_sb(struct super_block *sb,
> >>int nr_to_scan)
> >> if (list_empty(&sb->s_inode_lru))
> >> break;
> >>
> >>- inode = list_entry(sb->s_inode_lru.prev, struct inode, i_lru);
> >>+ inode = list_entry(sb->s_inode_lru.next, struct inode, i_lru);
> >>
> >> /*
> >> * we are inverting the sb->s_inode_lru_lock/inode->i_lock here,
> >>--
> >>1.7.5.4
--
Jan Kara <[email protected]>
SUSE Labs, CR