On Thu 07-08-14 11:35:50, Zheng Liu wrote:
> From: Zheng Liu <wenqing.lz@xxxxxxxxxx>
>
> Currently extent status tree doesn't cache extent hole when a write
> looks up in extent tree to make sure whether a block has been allocated
> or not. In this case, we don't put extent hole in extent cache because
> later this extent might be removed and a new delayed extent might be
> added back. But it will cause a defect when we do a lot of writes.
> If we don't put extent hole in extent cache, the following writes also
> need to access extent tree to look at whether or not a block has been
> allocated. It brings a cache miss. This commit fixes this defect.
> Meanwhile, if an inode has no any extent, this extent hole also will
> be cached.
>
> Cc: "Theodore Ts'o" <tytso@xxxxxxx>
> Cc: Andreas Dilger <adilger.kernel@xxxxxxxxx>
> Cc: Jan Kara <jack@xxxxxxx>
> Signed-off-by: Zheng Liu <wenqing.lz@xxxxxxxxxx>
So I agree with this change, it certainly doesn't make things worse. But
when looking into ext4_ext_put_gap_in_cache() I have one question: That
function uses ext4_find_delalloc_range() to check that the intended range
doesn't have any delalloc blocks in it. However it doesn't make any effort
to put a smaller hole in cache - for example if we have blocks allocated
like:
5-6 = delalloc
7-10 = allocated
and ext4_ext_put_gap_in_cache() is called for block 0, the function won't
put anything into cache although it could put range 0-4 in the cache as a
hole. Why is that?
Honza
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 76c2df3..6463d34 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2284,16 +2284,15 @@ ext4_ext_put_gap_in_cache(struct inode *inode, struct ext4_ext_path *path,
> ext4_lblk_t block)
> {
> int depth = ext_depth(inode);
> - unsigned long len = 0;
> - ext4_lblk_t lblock = 0;
> + unsigned long len;
> + ext4_lblk_t lblock;
> struct ext4_extent *ex;
>
> ex = path[depth].p_ext;
> if (ex == NULL) {
> - /*
> - * there is no extent yet, so gap is [0;-] and we
> - * don't cache it
> - */
> + /* there is no extent yet, so gap is [0;-] */
> + lblock = 0;
> + len = EXT_MAX_BLOCKS;
> ext_debug("cache gap(whole file):");
> } else if (block < le32_to_cpu(ex->ee_block)) {
> lblock = block;
> @@ -2302,9 +2301,6 @@ ext4_ext_put_gap_in_cache(struct inode *inode, struct ext4_ext_path *path,
> block,
> le32_to_cpu(ex->ee_block),
> ext4_ext_get_actual_len(ex));
> - if (!ext4_find_delalloc_range(inode, lblock, lblock + len - 1))
> - ext4_es_insert_extent(inode, lblock, len, ~0,
> - EXTENT_STATUS_HOLE);
> } else if (block >= le32_to_cpu(ex->ee_block)
> + ext4_ext_get_actual_len(ex)) {
> ext4_lblk_t next;
> @@ -2318,14 +2314,14 @@ ext4_ext_put_gap_in_cache(struct inode *inode, struct ext4_ext_path *path,
> block);
> BUG_ON(next == lblock);
> len = next - lblock;
> - if (!ext4_find_delalloc_range(inode, lblock, lblock + len - 1))
> - ext4_es_insert_extent(inode, lblock, len, ~0,
> - EXTENT_STATUS_HOLE);
> } else {
> BUG();
> }
>
> ext_debug(" -> %u:%lu\n", lblock, len);
> + if (!ext4_find_delalloc_range(inode, lblock, lblock + len - 1))
> + ext4_es_insert_extent(inode, lblock, len, ~0,
> + EXTENT_STATUS_HOLE);
> }
>
> /*
> @@ -4362,8 +4358,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> * put just found gap into cache to speed up
> * subsequent requests
> */
> - if ((flags & EXT4_GET_BLOCKS_NO_PUT_HOLE) == 0)
> - ext4_ext_put_gap_in_cache(inode, path, map->m_lblk);
> + ext4_ext_put_gap_in_cache(inode, path, map->m_lblk);
> goto out2;
> }
--
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
