linux-ext4 - Re: [PATCH] ext4: call ext4_es_lru_add() after handling cache miss

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20130718124603.GA14274@gmail.com>
Date:	Thu, 18 Jul 2013 20:46:03 +0800
From:	Zheng Liu <gnehzuil.liu@...il.com>
To:	Theodore Ts'o <tytso@....edu>
Cc:	Ext4 Developers List <linux-ext4@...r.kernel.org>,
	Zheng Liu <wenqing.lz@...bao.com>, stable@...r.kernel.org
Subject: Re: [PATCH] ext4: call ext4_es_lru_add() after handling cache miss

Hi Ted,

On Tue, Jul 16, 2013 at 10:29:40AM -0400, Theodore Ts'o wrote:
> If there are no items in the extent status tree, ext4_es_lru_add() is
> a no-op.

Sorry, but I don't understand your point of view.  Now ext4_es_lru_add
is used to set i_touch_when and put this inode into the tail of lru if
this inode hasn't been no matter whether there has an item in extent
status tree.  So only we need to do is to call this function when we
think the item of extent status tree in this inode needs to be kept in
memory.  That is why I call this function in ext4_*_map_blocks() because
we always use these two functions to get a block mapping.  So that means
that this inode is accessed recently and it should be kept in memory as
much as possible than other inodes.

Ah, maybe I got your point.  You mean that if ther are no item in the
extent status tree, we don't need to reclaim the item from it, right?
IMHO, when ext4_*_map_blocks() are called, that means that we will put
some items into the extent status tree if there are no item in the tree.
So I don't think we need to worry about it.

> So it is not sufficient to call ext4_es_lru_add() before we
> try to lookup an entry in the extent status tree.  We also need to
> call it at the end of ext4_ext_map_blocks(), after items have been
> added to the extent status tree.

I don't think we only need to call ext4_es_lru_add() in
ext4_ext_map_blocks() when an item is added to the tree because extent
status tree is also useful for a indirect-based file.  That is why I
call ext4_es_insert_extent() in ext4_map_blocks().  Please correct me if
I miss something.

Thanks,
                                                - Zheng

> 
> This could lead to inodes with that have extent status trees but which
> are not in the LRU list, which means they won't get considered for
> eviction by the es_shrinker.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@....edu>
> Cc: Zheng Liu <wenqing.lz@...bao.com>
> Cc: stable@...r.kernel.org
> ---
>  fs/ext4/extents.c | 5 +++--
>  fs/ext4/inode.c   | 7 ++-----
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index cfdc51e..a618738 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4385,8 +4385,9 @@ out2:
>  	}
>  
>  out3:
> -	trace_ext4_ext_map_blocks_exit(inode, flags, map, err ? err : allocated);
> -
> +	trace_ext4_ext_map_blocks_exit(inode, flags, map,
> +				       err ? err : allocated);
> +	ext4_es_lru_add(inode);
>  	return err ? err : allocated;
>  }
>  
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 3c5edf2..ea2cbf1 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -514,10 +514,9 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
>  		  "logical block %lu\n", inode->i_ino, flags, map->m_len,
>  		  (unsigned long) map->m_lblk);
>  
> -	ext4_es_lru_add(inode);
> -
>  	/* Lookup extent status tree firstly */
>  	if (ext4_es_lookup_extent(inode, map->m_lblk, &es)) {
> +		ext4_es_lru_add(inode);
>  		if (ext4_es_is_written(&es) || ext4_es_is_unwritten(&es)) {
>  			map->m_pblk = ext4_es_pblock(&es) +
>  					map->m_lblk - es.es_lblk;
> @@ -1529,11 +1528,9 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
>  		  "logical block %lu\n", inode->i_ino, map->m_len,
>  		  (unsigned long) map->m_lblk);
>  
> -	ext4_es_lru_add(inode);
> -
>  	/* Lookup extent status tree firstly */
>  	if (ext4_es_lookup_extent(inode, iblock, &es)) {
> -
> +		ext4_es_lru_add(inode);
>  		if (ext4_es_is_hole(&es)) {
>  			retval = 0;
>  			down_read((&EXT4_I(inode)->i_data_sem));
> -- 
> 1.7.12.rc0.22.gcdd159b
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html